Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialized proto not matching between Attribute declaration vs RuntimeTypeModel with missing set on property #964

Closed
Turnerj opened this issue Oct 13, 2022 · 3 comments · Fixed by #965

Comments

@Turnerj
Copy link

Turnerj commented Oct 13, 2022

Not the most succinct title but it does more-or-less say what I mean. Basically I think I've discovered a problem between how declaring proto members via attributes vs via the RuntimeTypeModel works, particularly around properties that have setters or not.

This is a sample of what I put together in LINQPad which triggers the problem:

RuntimeTypeModel.Default.Add(typeof(TestClass))
	.Add(1, nameof(TestClass.Expiry))
	.Add(2, nameof(TestClass.Value));

var expiry = new DateTime(2022, 1, 1, 0, 0, 0, 0, DateTimeKind.Utc);
var memoryStreamA = new MemoryStream();
Serializer.Serialize(memoryStreamA, new TestClass("some data", expiry));
var memoryStreamB = new MemoryStream();
Serializer.Serialize(memoryStreamB, new ContractClass("some data", expiry));

memoryStreamA.Seek(0, SeekOrigin.Begin);
memoryStreamB.Seek(0, SeekOrigin.Begin);

var a = memoryStreamA.ToArray();
var b = memoryStreamB.ToArray();

new byte[][]
{
	a,
	b
}.Dump();
a.SequenceEqual(b).Dump();

class TestClass
{
	public DateTime Expiry { get; }
	public string Value { get; }

	public TestClass(string value, DateTime expiry)
	{
		Value = value;
		Expiry = expiry;
	}
}

[ProtoContract]
class ContractClass
{
	[ProtoMember(1)]
	public DateTime Expiry { get; }
	[ProtoMember(2)]
	public string Value { get; }
	
	public ContractClass(string value, DateTime expiry)
	{
		Value = value;
		Expiry = expiry;
	}
}

Now for TestClass, make either of the properties have a set; on it and run the code again - you'll see that it previously was not outputting byte-for-byte the same representation but actually applying a different order. The second attempt with the set; will match but the first attempt without it won't.

Am I missing some behavioural quirk with how this is meant to work? I've looked over my code a bunch of times but don't see anything I'm doing wrong.

@mgravell
Copy link
Member

mgravell commented Oct 13, 2022

OK; running this through https://protogen.marcgravell.com/decode, we see:

Field #1: 0A String Length = 9, Hex = 09, UTF8 = "some data"
Field #2: 12 String Length = 4, Hex = 04, UTF8 = "���"
  As sub-object :
  Field #1: 08 Varint Value = 37986, Hex = E2-A8-02

vs

Field #1: 0A String Length = 4, Hex = 04, UTF8 = "���"
    As sub-object :
    Field #1: 08 Varint Value = 37986, Hex = E2-A8-02
Field #2: 12 String Length = 9, Hex = 09, UTF8 = "some data"

so: the order of fields has inverted, we expect the order to be Expiry, Value, so the first version is wrong.

The immediate problem here is that it is inferring the member order as a tuple-like type - meaning: when it finds no attributes (in RuntimeTypeModel.Add), it is looking for a constructor that sets everything, finding the TestClass(string value, DateTime expiry) constructor, and deciding "value", "expiry". An immediate workaround here is to pass the applyDefaultBehaviour: false parameter to Add, which disables all type inspections; with this change alone: everything works as intended.

A library failure is that the Add(1, nameof(TestClass.Expiry)) failed to throw; I'm .... confused by this, quite honestly. I suspect that the "I'm a tuple-like type" flag is set, and we should make Add throw if that is the case. It looks like the "I'm a tuple-like type" scenario does not eagerly populate the fields. I will check on this and fix.


As a side note - interestingly, if we have the protobuf-net.BuildTools package included, we see (against ContractClass):

Error PBN0015 There is no suitable (parameterless) constructor available for the proto-contract, and the SkipConstructor flag is not set.

However, the serializer does work, so: I would consider this a tertiary error in the analyzer package.

@mgravell
Copy link
Member

PR added; thoughts welcome

@Turnerj
Copy link
Author

Turnerj commented Oct 13, 2022

Thanks for getting back to me so quickly - really appreciate it! Makes sense what you've said and your PR looks good.

For the project I'm using this in, I didn't catch this problem in time so now I've got users with both types of generated proto data (which is my own problem to resolve). As much as I'd rather not have hit this problem, it is great to know it isn't some major problem in protobuf-net and I can safely get around it by disabling that applyDefaultBehaviour option you mentioned.

Keep building awesome things! 🙂

mgravell added a commit that referenced this issue Oct 13, 2022
Turnerj added a commit to TurnerSoftware/CacheTower that referenced this issue Apr 7, 2023
Turnerj added a commit to TurnerSoftware/CacheTower that referenced this issue Apr 8, 2023
Turnerj added a commit to TurnerSoftware/CacheTower that referenced this issue Apr 9, 2023
* Disable default behaviour of protobuf-net

See issue: protobuf-net/protobuf-net#964

* Catch and log serialization exceptions

* Remove obsolete service collection methods

* Allow logging to be optional

* Fix test compilation errors

* Fix build errors in benchmarks

* Fix service collection tests

* Allow init properties for CacheEntry

Makes Protobuf happy and is probably a better solution than making Protobuf skip any constructors
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 a pull request may close this issue.

2 participants