CSHARP4040: 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
Show resolved
Hide resolved
| @@ -1317,16 +1317,31 @@ public void UnmapProperty(string propertyName) | |||
| /// Gets the discriminator convention for the class. | |||
| /// </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.
| @@ -1317,16 +1317,31 @@ public void UnmapProperty(string propertyName) | |||
| /// Gets the discriminator convention for the class. | |||
| /// </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.
No description provided.