Manually serialize system types in preparation for .NET Core.

[ngbrown]

Serialize System.Type, ConstructorInfo, FieldInfo, MethodInfo, and PropertyInfo using only basic types.

I used reflection generated list of [Serializable] types in .NET Core 2.0 to find and replace fields in serialized types.

This partially addresses #889 - NH3853. While not switching away from binary serialization, it should be compatible with .NET Core (to be verified).

If an application is serializing NHibernate internals (like ISession or some of the exceptions that are now completely serialized like PropertyNotFoundException) into permanent storage and expects them to be rehydrated with any future version of NHibernate, then this will break that. Applications should only expect binary serialization compatibility within a very limited version range.

[hazzik]

Too many changes (some of them are breaking) for a minor version.

[hazzik]

This should be set on deserialization

[fredericDelaporte]

This should be set on deserialization

Maybe with a method like private void SetValuesOnDeserialized(StreamingContext context) marked with [OnDeserialized] for avoiding implementing the ISerializable interface.

Introducing this property would then be revert-able.

[hazzik]

@ngbrown please review these documents:

• https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-changes.md
• https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-change-rules.md

[ngbrown]

I am planning on cleaning up the loading of types in the property getters. I didn't initially have the type serialization helper classes written and now that I do, I will spread those around.

@hazzik can you clarify what you mean by breaking changes? I mentioned that the binary serialization ISession is not backward compatible with rehydrating from a previous version. This is considered a gray area in the linked documents.

The only "break" (bucket 1, public contract) I know of is that I marked some classes sealed to avoid making GetObjectData virtual, but they are only new'd by sealed classes, so I would argue that is simply fixing a bug that couldn't have been exploited.

This is what it will take to avoid disabling lots of serialization for the .NET Core version.

[fredericDelaporte]

It looks to me we have two main subjects here:

• Adjusting serialization for .Net Standard compatibility.
• Cleaning serialization.

Some changes for compatibility require adjustments for being mergeable as a minor version.

Most changes for cleaning are potential breaking changes and should be moved to a PR for 6.0 I think.

There are also some serialization fixes about types declared serializable but having serialized members or ancestor which are not. Are those fixes required for .Net Standard compatibility? If no, better do them within the cleanup PR.

[fredericDelaporte]

Missing attribute [SecurityCritical].

[ngbrown]

Done in #1469

[fredericDelaporte]

For avoiding a breaking change, let it public but add a comment like:

// 6.0 TODO: switch to protected.

[ngbrown]

Moved to #1469 as a potentially breaking change.

[fredericDelaporte]

Is it removed because of its dialect field which is not serializable and is there since 2008?

[ngbrown]

Moved to #1469 as a potentially breaking change.

[fredericDelaporte]

Should be kept and instead mark the _session field as non serializable.

[ngbrown]

Done in #1469

[fredericDelaporte]

I think we should instead switch SqlStringBuilder to Serializable. It should only take adding to it the attribute and adding it to its inner class AddingSqlStringVisitor too.

[ngbrown]

Done in #1469

[fredericDelaporte]

Avoid this.

[fredericDelaporte]

Should be moved to deserialization constructor, especially if it is confirmed we do not need the TryGetType.

[fredericDelaporte]

Do we need this? It seems unused currently. If needed, it looks better to use a Lazy<Type> for not having it trying to resolve the type at each call when it fails.

[fredericDelaporte]

Missing SecurityCritical attribute.

[fredericDelaporte]

AddValue(string name, object value) almost does that. Two differences here:

• The native one will use the actual type of the instance, while the extension method will use the compile time type.
• The native one will default to object in case of null, which is supported on deserialization (it has a special handling for null).

Looking the AddValueWithType usages, it does not look to me it needs these differences. In such case I would favor using the native AddValue(string name, object value) instead and remove this extension.

[hazzik]

This needs to be implemented using IObjectReference instead in conjunction with ISerializable.

[fredericDelaporte]

I do not find any example about IObjectReference for something else than a custom singleton and I do not really see how it can be used for a type we do not define (System.Type in this case).
If we try SerializationInfo.AddValue("someTypeField", someTypeField, typeof(SerializableSystemType)):

• This violate AddValue documentation which states:

This parameter must always be the type of the object itself or of one of its base classes.

• This will style serialize the type fields I think (if it works).

Then on deserialization (if it indeed works for serializing despite what the documentation writes), how do we get information on what was actually serialized? By defining fields matching what a System.Type serialize? If it is that, it seems quite brittle compared to current implementation.

I guess that in fact I do not understand at all how to use IObjectReference in this context.

[ngbrown]

Missing SecurityCritical attribute.

Is there an analyzer or something I should be using to find these?

This needs to be implemented using IObjectReference instead.

I think I do see how @hazzik thought IObjectReference could be used to to allow the deserialized object to be substituted on the way out. So a System.Type would be wrapped at serialization, and then de-wrapped via the GetRealObject method. But that would probably create a issue with ever directly using SerializableSystemType as a property or even in the deserialization constructor. I think it's only meant to be substituted from within the target type. In this case we have no control over System.Type.

[fredericDelaporte]

I guess that in fact I do not understand at all how to use IObjectReference in this context.

So in fact I am asking you @hazzik, may you give some pointers to explain how it should be used?

[fredericDelaporte]

Is there an analyzer or something I should be using to find these?

I had checked the master branch by searching all "void GetObjectData" and checking they have the attribute (this is the case). Only those here were missing it. I do not know of a way to detect them excepted having a serialization/deserialization test for all serializable types.

[hazzik]

[Serializable]
public class TestClass : ISerializable
{
    [NonSerialized] private Type _type;

    public Type Type
    {
        get => _type;
        set => _type = value;
    }

    public TestClass() {}

    protected TestClass(SerializationInfo info, StreamingContext context)
        => _type = (Type) info.GetValue("Type", typeof(Type));

    public void GetObjectData(SerializationInfo info, StreamingContext context)
        => info.AddValue("Type", new TypeObjectReference(_type));
}

[Serializable]
public class TypeObjectReference : IObjectReference, ISerializable
{
    private readonly Type _type;

    public TypeObjectReference(Type type) 
        => _type = type;

    protected TypeObjectReference(SerializationInfo info, StreamingContext context)
        => _type = Type.GetType(info.GetString("AssemblyQualifiedName"));

    public object GetRealObject(StreamingContext context) 
        => _type;

    public void GetObjectData(SerializationInfo info, StreamingContext context) 
        => info.AddValue("AssemblyQualifiedName", _type.AssemblyQualifiedName);
}

public class TestCase
{
    [Test]
    public void TestSerializingType()
    {
        BinaryFormatter bf = new BinaryFormatter();
        var stream = new MemoryStream();
        bf.Serialize(stream, new TestClass
        {
            Type = typeof(TestCase)
        });
        stream.Position = 0;

        var deserialize = (TestClass) bf.Deserialize(stream);

        Assert.That(deserialize.Type, Is.EqualTo(typeof(TestCase)));
    }
}

[ngbrown]

@hazzik I did not discount your suggestion and looked at how IObjectReference was documented and found the same thing.

Like you show, IObjectReference allows the deserializer to return a specific instance, even of a different type, but that isn't exactly what I'm trying to do here. It would have worked in certain cases, but I'm not using SerializableSystemType exclusively in GetObjectData, so can't deal with the deserialized type being swapped (or "fixup"ed) in every case. See BasicLazyInitializer.cs.

Running the following in LINQPad throws an exception on Deserialize (ArgumentException: Object of type 'System.RuntimeType' cannot be converted to type 'UserQuery+ProxyType'):

void Main()
{
    var stream = new MemoryStream();
    BinaryFormatter formatter = new BinaryFormatter();
    
    var a1 = new SerializedObject();
    formatter.Serialize(stream, a1);
    
    stream.Position = 0;
    var a2 = (SerializedObject)formatter.Deserialize(stream);
}

// Define other methods and classes here
[Serializable]
public class SerializedObject
{
    private ProxyType someType = new ProxyType();
}

[Serializable]
public class ProxyType : IObjectReference
{
    public Object GetRealObject(StreamingContext context) 
    {
        // When deserializing this object, return a reference to 
        // the Type object instead.
        return typeof(Object);
    }
}

[hazzik]

Ok, it's weird. And also I did not see that use cases (as the PR is too big for review)

[fredericDelaporte]

Thanks @hazzik for the example, I was not finding this case and was not understanding it could be used that way.
(Upd: NHibernate code base has example of usage too, but a bit obfuscated by being coded partly through IL emit.)

[hazzik]

@ngbrown I would replace wrap/unwrap with conversion operators (probably implicit). This will allow making a lot fewer changes.

UPD: I will make this change now. UPD2: I will not.

[ngbrown]

The commits are in order of clean-up first, then .NET Core required changes last, so splitting isn't difficult.

My question is; is the serialization stream change also a 6.0 change?

Splitting this pull request will probably result in three separate ones, one with a bunch of "6.0 TODO", then a 6.0 one to actually make the change, and then finally a pull request for .NET Core compatible serialization, which I'm guessing is 6.0 also.

Serialization isn't strictly required for .NET Core, but lots of tests have to be skipped without it.

[fredericDelaporte]

My question is; is the serialization stream change also a 6.0 change?

For me this gray area is ok as a minor (5.1) change. (It would not as a patch (5.0.2).)

one with a bunch of "6.0 TODO"

Not useful in my opinion if a PR for actually doing those changes is already here.

[ngbrown]

I'm about half-way through re-working this pull request.

I'm now using SerializableSystemType.TryGetType() in the exceptions as it's possible those would be stored and retrieved in different contexts (where the referred to type doesn't exist, but the string value is still desired for the Message). More care is also taken in the exception to deserialize the previous version.

The Serializeable... types now have implicit or explicit conversion operators. The explicit is because they may throw an exception depending on the circumstances.

[ngbrown]

I've finished the re-working of this pull request.

I will be using this as a basis for the .NET Core port to not disable so many unit tests in that pull request.

[hazzik]

@ngbrown I don't understand why you use SerializableSystemType as fields, could you please explain?

May receive more changes

[ngbrown]

This can't help with serializing delegates (AfterTransactionCompletionProcessDelegate) so tests that depend on serializing them will have to be excluded for .net core.

I did rebase to head, but made no other changes. The needs work tag can be removed.

[hazzik]

Please do not merge yet

[fredericDelaporte]

Please do not merge yet

Can you please elaborate?
I know you have another PR (#1523) ongoing tackling multiple subjects at once, including choosing another way for the serialization:

Assume that serialization of System.Type and co is a foreign concept to .NET Core 2.0 so the users required to write serialization surrogates. We might provide these in the future if we want to.

This PR looks to me as matching "We might provide these in the future if we want to.". And it would remove that point from your PR, allowing it to be more lightweight.

[hazzik]

Can you please elaborate?

First, this PR has a lot of unrelated changes (randomly renamed properties, method changes, etc). That makes the review is almost impossible as it is not clear what is the intended change and what is changed BTW.

Second, it’s not clear what level of serialization compatibility is intended. It’s clearly not [backward] compatible with 5.0.

I know you have another PR (#1523) ongoing tackling multiple subjects at once, including choosing another way for the serialization

The #1523 has nothing to do with this PR or with the serialization in general. That PR is only about providing a .net core version. In #1523 I have chosen not to provide any serialization means.

In regards serialization we have these options:

1. This PR - a breaking change (“grayish area”) in the serialization format.

2. Do not provide any serialization (the “other” PR) - leaving the responsibility of serializing system types (and friends) to the users.

3. Implement custom serialization in .NET Core build. This would mean that the serialization format will be not compatible across platforms (eg. unable to deserialize a session serialized in Full .NET) - this is the approach chosen by the .NET Core team.

My preferred option is 2 with probability to implement 3 in the future.

it would remove that point from your PR, allowing it to be more lightweight.

I had no intention to implement the serialization in my PR.

[fredericDelaporte]

First, this PR has a lot of unrelated changes (randomly renamed properties, method changes, etc). That makes the review is almost impossible as it is not clear what is the intended change and what is changed BTW.

I have thoroughly reviewed them all, and I have done this multiple times, due to long life time of this PR. Yes it is a bit more time consuming, but I have made the effort each time I have reviewed it.

Second, it’s not clear what level of serialization compatibility is intended. It’s clearly not [backward] compatible with 5.0.

I think the ability to deserialize in 5.1 something serialized in 5.0 is off target. I am quite sure it is already not guaranteed due to currently merged PRs in 5.1, and it should not be considered for a minor release anyway in my view. This would be limiting for little benefits. I think serialization compatibility should be a target only for release increments (5.0.x series currently). That is not for no reasons this is a grayish area.

I still think this PR is a better option for handling the subject than the two other options. It is ready, reviewed many times, already well polished, and offer a better support of the feature for all cases than the two other options.

[ngbrown]

Close in lieu of #1652.

[jrgcubano]

@ngbrown Hi. I do not know if it will give you any value, but on the FluentNHibernate side we have finished using serialization surrogates for our cloner

Thanks in advance for the amazing work guys!