[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: |
|
||||||||
| Epic Link: | Improve Serialization | ||||||||
| Description |
|
Class B.ToJson results in "{ \"Data\" : [1, 20] }" |
| 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.
it's true, I missed this
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 ] | |||||||||||
it looks like in this particular case it's a coincidence | |||||||||||
| 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).
it's true, but the question is why this property is mapped in ReadWriteMemberFinderConvention? AFAIK this is one of the main original questions.
"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. | |||||||||||
| 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:
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:
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:
The two annotations are needed because:
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. |