[CSHARP-3531] Init properties sometimes are not serialized (not consistent with readonly properties) Created: 05/Apr/21  Updated: 20/Jan/23  Resolved: 10/Nov/22

Status: Closed
Project: C# Driver
Component/s: Serialization
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Boris Dogadov Assignee: James Kovacs
Resolution: Duplicate Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
duplicates CSHARP-4374 Add SetPropertyGetter and SetProperty... Backlog
Epic Link: Improve Serialization

 Description   

 

public class B
 {
  public int[] Data { get; private set; }
  public B(int[] data2)
  {
    Data = data2.ToArray();
  }
}

 

public class A
 {
  public int[] Data { get; }
  public A(int[] data2)
  {
    Data = data2.ToArray();
  }
}

B b = new B(new[] { 1, 20 });
A a = new A(new[] { 1, 20 });

Class B.ToJson results in "{ \"Data\" : [1, 20] }"
Class A.ToJson results in "{ }"



 Comments   
Comment by Dmitry Lukyanov (Inactive) [ 19/Apr/21 ]

After thinking more about this and looking at this line where we avoid double saving for memberMap, I can agree that it's not a bug. In case if a field corresponds to several conventions, we will save the memberMap only the first time, next attempts will be skipped.

I don't think so. The `SetMethod` property of `PropertyInfo` will be null, so there is no set method to call using reflection.

it's true, I missed this

I think we need to consider mismatches between property names and constructor parameter names as a bug in the declaration of the class

this a bit proves my original idea since in one case we don't take any steps from the resulting point of view and in another one trying to do it, but I agree that we should not skip this since we have a private setter.

Comment by Robert Stam [ 19/Apr/21 ]

> because any of them can be broken, based on naming mistmatching between ctor arguments and property names

But the mismatching is not even relevant if we're not mapping a pure immutable class (because then we won't even use constructors).

I think we need to consider mismatches between property names and constructor parameter names as a bug in the declaration of the class, and not as anything we should try to work around. If the application writer wishes to uses mismatched names then we have annotation attributes they can use to safely tell us how match them to each other.

 

Comment by Dmitry Lukyanov (Inactive) [ 19/Apr/21 ]

did not qualify as a pure immutable class, and those existing techniques include calling private setters using reflection.

it looks like in this particular case it's a coincidence , since this class is not immutable not because of property accessors, but because of mismatching in names. With the same idea, we need to remove all these conditions at all: https://github.com/mongodb/mongo-csharp-driver/blob/master/src/MongoDB.Bson/Serialization/Conventions/ReadWriteMemberFinderConvention.cs#L107, because any of them can be broken, based on naming mistmatching between ctor arguments and property names

Comment by Robert Stam [ 19/Apr/21 ]

> it's true, but the question is why this property is mapped in ReadWriteMemberFinderConvention? AFAIK this is one of the main original questions.

Because that's how we've always mapped it before, and for backward compatibility reasons we should not change it. This was how immutable classes used to be deserialized before we added support for calling constructors.

> "because we will call the private setter using reflection." we can do the same thing without a private setter as well.

I don't think so. The `SetMethod` property of `PropertyInfo` will be null, so there is no set method to call using reflection.

Comment by Dmitry Lukyanov (Inactive) [ 19/Apr/21 ]

My point is that we need to handle private set; property setter in the same way in all conventions. It should be wrong to consider this setter form as immutable in one convention(ImmutableTypeClassMapConvention) and as mutable in different(ReadWriteMemberFinderConvention).

`B` is not being mapped as an immutable class and therefore the constructor is not being used.

it's true, but the question is why this property is mapped in ReadWriteMemberFinderConvention? AFAIK this is one of the main original questions.

For `B` we map the `Data` public property (even though the setter is private) because we will call the private setter using reflection.

"because we will call the private setter using reflection." we can do the same thing without a private setter as well.

So my main point, that we need to handle `propertyInfo.CanWrite` value in the same way in all conventions.
However it probably will be a BC, so it's better to do it for 3.0 release

Comment by Robert Stam [ 19/Apr/21 ]

I don't fully understand your point about `B`.

`B` is not being mapped as an immutable class and therefore the constructor is not being used.

For `B` we map the `Data` public property (even though the setter is private) because we will call the private setter using reflection.

Keep in mind that historically there was a point in time when we didn't support calling constructors at all. When we added full support for deserializing pure immutable classes using constructors, for backward compatibility we specifically chose to fall back to the existing techniques for any class which did not qualify as a pure immutable class, and those existing techniques include calling private setters using reflection.

Comment by Dmitry Lukyanov (Inactive) [ 19/Apr/21 ]

I agreed about A, but we probably still need to handle B in the same way as A, and I would consider this as a bug (or not fully implemented the previous feature) since from the user's point of view these two classes are the same.

It's a bit strange that in ImmutableConvention we consider "Data { get; private set; }" as a field from Immutable class, but in ReadWrite convention, we consider this class as mutable because of this line https://github.com/mongodb/mongo-csharp-driver/blob/master/src/MongoDB.Bson/Serialization/Conventions/ReadWriteMemberFinderConvention.cs#L107 (which actually should be the same as (CanWrite part) https://github.com/mongodb/mongo-csharp-driver/blob/master/src/MongoDB.Bson/Serialization/Conventions/ImmutableTypeClassMapConvention.cs#L106)

Comment by Robert Stam [ 19/Apr/21 ]

Here's a fuller analysis of why class `B` maps and serializes as it does:

  1. `B` is an immutable class because all properties are read-only (from a `public` point of view, because the setter is private)
  2. Only constructors with parameters that match properties (by name) are mapped, so the single constructor is not mapped
  3. Since no constructors were mapped we fall back to map the class as a mutable class
  4. The `Data` property is mapped because we will call the private setter using reflection

An important point here: the constructor for `B` is NOT called during deserialization (contrary to what you might expect).

So class `B` does round trip correctly without data loss, but if you intended for the constructor to be called during deserialization you would have to annotate class `B` in the same was as class `A` was annotated above.

In summary, I think class `B` is already being correctly mapped and no changes are needed.

Comment by Robert Stam [ 19/Apr/21 ]

Here's a fuller analysis of why class `A` maps and serializes as it does:

  1. `A` is an immutable class because all properties are read-only
  2. Only constructors with parameters that match properties (by name) are mapped, so the single constructor is not mapped
  3. Since no constructors were mapped we fall back to map the class as a mutable class
  4. Since the class has no writable properties the class map is empty

Class `A` is written in such a way that I do not think it is possible to map correctly without annotations. It is not safe to match constructor parameters to properties by anything other than name, and class `A` was written in such a way that there is no way to safely algorithmically match the `data2` constructor parameter to the `Data` read-only property.

If we want class `A` to be mapped correctly we MUST annotate the class to manually tell the driver how to match constructor parameters to properties:

 public class A
{
    [BsonElement("Data")]
    public int[] Data { get; }
 
    [BsonConstructor("Data")]
    public A(int[] data2)
    {
        Data = data2.ToArray();
    }
}

The two annotations are needed because:

  1. The `Data` property would not be automatically mapped because there is no constructor parameter called `data`
  2. The constructor would not be automatically mapped because there is no property called `Data2`

The annotations tell the mapper what to do in the presence of constructor parameter names and property names that don't match.

An argument can be made that the annotation on the property should not be needed, since the annotation on the constructor has already established a mapping between the `data2` constructor parameter and the `Data` property.

In summary, I think class `A` is already being correctly mapped and no changes are needed. The only change that might be considered is to throw an exception in cases like this, but that would probably be a breaking change.

Comment by Robert Stam [ 19/Apr/21 ]

We match constructor parameters to properties by *name* not *type* so for type A the property named `Data` and the constructor parameter named `data2` don't match.

Either make the names match or annotate the class to indicate the match manually.

Generated at Wed Feb 07 21:45:33 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.