Skip to content

Conversation

jogibear9988
Copy link
Contributor

fix for #183

what is still a problem:

if we have two classes wich reference one another, the known type is not found on one, cause one of them is added later.

also the order of the adding of the classes is important:

for example, this will work:

    class Temp
    {
        public object[] SubArray { get; set; }
        public string aa { get; set; }
        public Dictionary<string,string> dc { get; set; }
    }

    [Fact]
    public void WritesManifestEvenIfKnown()
    {
        var stream = new MemoryStream();
        var msg = new Temp() { aa = "huhu", dc = new Dictionary<string, string>() { { "a", "b" } }, SubArray = new object[] { 1, (byte)2, new object[] { 3 } } };

        var serializer = new Serializer(new SerializerOptions(knownTypes: new[] { typeof(object[]), typeof(DictionaryEntry), typeof(Dictionary<string, string>), typeof(Temp),  }));


        serializer.Serialize(msg, stream);
        stream.Position = 0;
        var a = stream.ToArray();
        var q = string.Join(", ", a.Select(x => x.ToString()));
        var q2 = string.Join(", ", a.Select(x => ((char)x).ToString()));
        var res = serializer.Deserialize(stream);
    }

this not:

    [Fact]
    public void WritesManifestEvenIfKnown()
    {
        var stream = new MemoryStream();
        var msg = new Temp() { aa = "huhu", dc = new Dictionary<string, string>() { { "a", "b" } }, SubArray = new object[] { 1, (byte)2, new object[] { 3 } } };

        var serializer = new Serializer(new SerializerOptions(knownTypes: new[] { typeof(DictionaryEntry), typeof(Dictionary<string, string>), typeof(Temp), typeof(object[]),  }));


        serializer.Serialize(msg, stream);
        stream.Position = 0;
        var a = stream.ToArray();
        var q = string.Join(", ", a.Select(x => x.ToString()));
        var q2 = string.Join(", ", a.Select(x => ((char)x).ToString()));
        var res = serializer.Deserialize(stream);
    }

see changed order of types in the Serializer constructor

@jogibear9988
Copy link
Contributor Author

@Aaronontheweb @Arkatufus
could you review?

@Aaronontheweb
Copy link
Member

@jogibear9988 yessir, we will

@Aaronontheweb
Copy link
Member

Yep, added @Arkatufus to this issue

@Arkatufus
Copy link
Contributor

@jogibear9988
I'm sorry, but I couldn't understand the problem this PR is trying to solve, I tried both your code and the original code, both outputs the exact same binary output.

@Aaronontheweb
Copy link
Member

@jogibear9988 if you could help explain the issue more clearly in terms of "expected" vs. "actual" behavior, we would appreciate it. Can't merge this change without fully grasping what it was trying to solve and how this works.

@jogibear9988
Copy link
Contributor Author

Surely,

Look at my sample, where I said it is not workin.
Run it without my changes. The size is much bigger, cause the Serialized Data does contain the type information even if it should not, cause the types are in the knownTypes.

The apply my patch and the size is much smaller, cause the type info is now not serialized

@jogibear9988
Copy link
Contributor Author

Any if you have circular references you could not fix this problem by changeing order of the knowntypes

@Arkatufus
Copy link
Contributor

Arkatufus commented Oct 28, 2020

@jogibear9988
To be honest, I still can't see the difference.
Here are the resulting byte stream from both net461 and netcoreapp3.0 output, original and modified:

net461
Ori: fe 00 00 ff 20 00 00 00 53 79 73 74 65 6d 2e 4f 62 6a 65 63 74 5b 5d 2c 20 6d 73 63 6f 72 6c 69 62 2c 25 63 6f 72 65 25 03 00 00 00 08 01 00 00 00 08 02 00 00 00 08 03 00 00 00
Mod: fe 00 00 ff 20 00 00 00 53 79 73 74 65 6d 2e 4f 62 6a 65 63 74 5b 5d 2c 20 6d 73 63 6f 72 6c 69 62 2c 25 63 6f 72 65 25 03 00 00 00 08 01 00 00 00 08 02 00 00 00 08 03 00 00 00

netcoreapp3.0
Ori: fe 00 00 ff 2e 00 00 00 53 79 73 74 65 6d 2e 4f 62 6a 65 63 74 5b 5d 2c 20 53 79 73 74 65 6d 2e 50 72 69 76 61 74 65 2e 43 6f 72 65 4c 69 62 2c 25 63 6f 72 65 25 03 00 00 00 08 01 00 00 00 08 02 00 00 00 08 03 00 00 00
Mod: fe 00 00 ff 2e 00 00 00 53 79 73 74 65 6d 2e 4f 62 6a 65 63 74 5b 5d 2c 20 53 79 73 74 65 6d 2e 50 72 69 76 61 74 65 2e 43 6f 72 65 4c 69 62 2c 25 63 6f 72 65 25 03 00 00 00 08 01 00 00 00 08 02 00 00 00 08 03 00 00 00

Made using your original code as a reference:

[Fact]
public void WritesManifestEvenIfKnown()
{
    var stream = new MemoryStream();
    var msg = new Temp() { SubArray = new object[] { 1, 2, 3 } };
    var serializer = new Serializer(new SerializerOptions(knownTypes: new[] { typeof(Temp), typeof(object[]) }));
    serializer.Serialize(msg, stream);
    var bytes = stream.ToArray();
    stream.Position = 0;
    var res = serializer.Deserialize(stream);

    var sb = new StringBuilder();
    foreach (var b in bytes) {
        sb.Append($"{b:x2} ");
    }
    _output.WriteLine(sb.ToString());
}

As you can see, they are identical, so I can't really see the original problem

@jogibear9988
Copy link
Contributor Author

@Arkatufus I've now attached a test wich fails if you revert the changes from my commit 6bafc4c

@Aaronontheweb
Copy link
Member

Thanks @jogibear9988 !

@Arkatufus
Copy link
Contributor

@jogibear9988
Can you merge this PR into your changes?
jogibear9988#1

I feel a lot safer if we remove all modified codes that can't be reached by unit testing.

jogibear9988 and others added 2 commits October 29, 2020 22:00
@Arkatufus Arkatufus merged commit bc41932 into akkadotnet:dev Oct 30, 2020
@Aaronontheweb Aaronontheweb mentioned this pull request Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants