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.
| @@ -28,6 +28,12 @@ public void TestConstructorThrowsWhenElementNameContainsNulls() | |||
| Assert.Throws<ArgumentException>(() => new ScalarDiscriminatorConvention("a\0b")); | |||
| } | |||
|
|
|||
| [Fact] | |||
There was a problem hiding this comment.
This seems to be a reasonable limitation, doesn't it?
| @@ -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?
| @@ -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?
| { | ||
| throw new ArgumentNullException("elementName"); | ||
| throw new ArgumentNullException(nameof(elementName)); |
There was a problem hiding this comment.
throw new ArgumentException("Element names cannot be null or empty.", nameof(elementName));
| } | ||
| 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.
| } | ||
| if (elementName == "_id") | ||
| { | ||
| throw new ArgumentException("Element names cannot be '_id'.", nameof(elementName)); |
There was a problem hiding this comment.
Of course element names can be _id. I think what you wanted to say was that the discriminator element name could not be _id.
throw new ArgumentException("The discriminator element name cannot be '_id'.", nameof(elementName));
There was a problem hiding this comment.
But why? The discriminator element name can be anything that doesn't conflict with an element name that is already used.
If the class doesn't have an _id I don't see any reason why it couldn't be used as the discriminator element name.
I wouldn't recommend anyone doing that, but it seems legal.
There was a problem hiding this comment.
Also, we should probably consider that even if the discriminator element name doesn't conflict that the base class level, what if a derived class introduces a conflicting element name?
|
|
||
| throw new BsonSerializationException( | ||
| $"The {fieldOrProperty} {conflictingMemberMap.MemberName} of type '{_classType.FullName}' cannot use element name '{discriminatorConvention.ElementName}' " + | ||
| $"because it is already being used by the discriminator convention '{discriminatorConvention.GetType().Name}'."); |
There was a problem hiding this comment.
I would turn the error message around:
throw new BsonSerializationException(
$"The discriminator element name cannot be {discriminatorConvention.ElementName} " +
$"because it is already being used by the {fieldOrProperty} {conflictingMemberMap.MemberName} of type {_classType.FullName}");
|
|
||
| namespace MongoDB.Driver.Tests.Jira | ||
| { | ||
| public class Csharp4040Tests |
There was a problem hiding this comment.
Fix file name and class name:
Csharp4040Tests => CSharp4040Tests
No description provided.