-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-3399: Mitigate pain of using field names with dots and dollars. #568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… on field names with dots for invalid commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good though it seemed to me it could be even a bit simpler.
Take a look at the comments and let me know what you think.
{ | ||
return false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave this class alone (no changes) and instead just stop using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In FindOneAndReplaceOperation
delete the nested Validator
class and refactor GetCommandValidator
to:
protected override IElementNameValidator GetCommandValidator()
{
return NoOpElementNameValidator.Instance;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In RetryabelInsertCommandOperation
replace line 137 with:
var elementNameValidator = NoOpElementNameValidator.Instance;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In InsertMessageBinaryEncoder
replace line 147 with:
var elementNameValidator = NoOpElementNameValidator.Instance;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed via Slack. My initial concern was end users depending on CollectionElementNameValidator
throwing an ArgumentNullException
whereas NoOpElementNameValidator
does not. Upon reflection, all validators should throw an ArgumentNullException
because many will throw a NullReferenceException
if called with a null element name.
With this in mind, I was able to replace usages of CENV
with NOENV
, which simplified a few code paths.
|
||
// public static fields | ||
/// <summary> | ||
/// Gets a pre-created instance of an UpdateElementNameValidator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Gets a pre-created instance of an ReplacementElementNameValidator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We can't remove these validators completely as we still need to validate that updates contain a
$OP
at the root and that replacements do not contain$FIELD
at the root. MongoDB 5.0 allows inserts of$FIELD
at the root but not replacements.As well we have logic in
UpdateMessageBinaryEncoder.cs
line 145 where we use the validator type to check for a valid message:This is existing code and I didn't want to rewrite the logic at this point.
The spec test refreshes are required because the old tests were depending on inserting documents of the form
{ ".": 1 }
to throw a client-side BSON serialization error, which they no longer do.There are still EG test failures that I'm working through. So far they all appear to be due to test problems like the spec test refreshes mentioned above. I'll push additional commits to this PR as I fix them.