CSHARP-4040: Fix bug when using field with same element name as discriminator #1684
Conversation
|
|
||
| private class DerivedDocument : BaseDocument {} | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
These tests can eventually be moved to somewhere more appropriate.
tests/MongoDB.Bson.Tests/Serialization/Conventions/StandardDiscriminatorConventionTests.cs
Outdated
Show resolved
Hide resolved
| /// </summary> | ||
| /// <returns>The discriminator convention for the class.</returns> | ||
| internal IDiscriminatorConvention GetDiscriminatorConvention() | ||
| internal IDiscriminatorConvention GetDiscriminatorConvention(bool checkConflicts = false) |
There was a problem hiding this comment.
I tried to find a place where to check that there are no conflicts with the other fields element names. I decided to do it here so it's done once and not continuously when serializing objects.
For now checkConflicts is true only when GetDiscriminatorConvention is called in BsonClassMapSerializer.SerializeDiscriminator (that is used only when serializing classes).
I'm not sure this is the optimal place for this, and if it would be worth throwing also when deserializing, for instance.
There was a problem hiding this comment.
Why would we ever NOT check for conflicts? Why do we even need the checkConflicts parameter?
There was a problem hiding this comment.
We discussed on slack, but I've removed the parameter and moved the conflict checking inside so that it is run once and not all the time the method is called.
| /// </summary> | ||
| /// <returns>The discriminator convention for the class.</returns> | ||
| internal IDiscriminatorConvention GetDiscriminatorConvention() | ||
| internal IDiscriminatorConvention GetDiscriminatorConvention(bool checkConflicts = false) |
There was a problem hiding this comment.
Why would we ever NOT check for conflicts? Why do we even need the checkConflicts parameter?
src/MongoDB.Bson/Serialization/Conventions/StandardDiscriminatorConvention.cs
Outdated
Show resolved
Hide resolved
| if (elementName.IndexOf('\0') != -1) | ||
| { | ||
| throw new ArgumentException("Element names cannot contain nulls.", "elementName"); | ||
| throw new ArgumentException("Element names cannot be null or empty.", nameof(elementName)); |
There was a problem hiding this comment.
throw new ArgumentException("Element names cannot contain nulls.", "elementName");
Should not have been changed.
There was a problem hiding this comment.
Yes, this should have gone for the previous condition.
There was a problem hiding this comment.
Would you consider slightly more precise wording?
throw new ArgumentException("Discriminator element name cannot be null or empty.", nameof(elementName));
I think element name (singular) is better than plural.
Also, I prefer Discriminator element name to Element name because element names CAN be empty. It is only discriminator element names that cannot be empty (but that's an arbitrary limitation that we are imposing, in theory they could be empty because "" is an element name just as much as "_t" is).
There was a problem hiding this comment.
I also think it's more appropriate, I will change the wording also on the other exception.
src/MongoDB.Bson/Serialization/Conventions/StandardDiscriminatorConvention.cs
Outdated
Show resolved
Hide resolved
| if (elementName.IndexOf('\0') != -1) | ||
| { | ||
| throw new ArgumentException("Element names cannot contain nulls.", "elementName"); | ||
| throw new ArgumentException("Element names cannot be null or empty.", nameof(elementName)); |
There was a problem hiding this comment.
Would you consider slightly more precise wording?
throw new ArgumentException("Discriminator element name cannot be null or empty.", nameof(elementName));
I think element name (singular) is better than plural.
Also, I prefer Discriminator element name to Element name because element names CAN be empty. It is only discriminator element names that cannot be empty (but that's an arbitrary limitation that we are imposing, in theory they could be empty because "" is an element name just as much as "_t" is).
| if (elementName.IndexOf('\0') != -1) | ||
| { | ||
| throw new ArgumentException("Element names cannot contain nulls.", "elementName"); | ||
| throw new ArgumentException("Element names cannot contain nulls.", nameof(elementName)); |
There was a problem hiding this comment.
Consider Element name (singular)?
There was a problem hiding this comment.
I think it makes sense, the wording was to be uniform with the other exception. I've changed that too, I think the singular is more appropriate here.
sanych-sun
changed the title
|
hi @papafe and @sanych-sun , this change introduced a breaking behavior in our codebase and is blocking us from moving to .NET 10 (the fix for .NET 10 is we set the discriminator to would really appreciate a quick response — we’ve already been blocked waiting for 3.5.1. |
|
Hi @dorki, sorry to hear about that. Could you possibly create a ticket following the description here? |
|
will do, but may i ask what was the idea behind this change? since the element selected for the discriminator, is serialized, i should be able to query it, and therefore it must exists in the class map ? |
|
Our current discriminator implementation assumes that the discriminator is an hidden field that only the driver can set. Because of this assumption, having a field with the same element name of the discriminator is something we never intended to support, and could lead to some unexpected consequences if done involuntarily. |
3 participants
No description provided.