Skip to content
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

allow $ in field name #13202

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from
Open

allow $ in field name #13202

wants to merge 5 commits into from

Conversation

minhna
Copy link
Contributor

@minhna minhna commented Jun 22, 2024

@@ -1910,10 +1910,9 @@ const NO_CREATE_MODIFIERS = {
};

// Make sure field names do not contain Mongo restricted
// characters ('.', '$', '\0').
// characters ('.', '\0').
// https://docs.mongodb.com/manual/reference/limits/#Restrictions-on-Field-Names
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me. However, I'm curious about the explicit prohibition of the dollar sign in minimongo. The documentation attached to the code mentions field name restrictions in Mongo, so it might just be an implementation based on those rules on the past, without other reasons behind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before version 5 of mongodb, it didn't accept dot and dollar sign in field name, but now it does.
I think @hwillson added these verification to minimongo 8 years ago because Meteor allows us to insert data to minimongo and it will auto sync with mongodb in server (insecure package).
Btw, I'm thinking adding commits to also allow dot in field name but maybe do it in other PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the context provided. Now, I remember some use cases faced on the past with "$" and ".". It is ok to have these unlocked.

@zodern
Copy link
Member

zodern commented Jun 26, 2024

It seems like there are still quite a few limitations on $ in field names: https://www.mongodb.com/docs/manual/core/dot-dollar-considerations/dollar-prefix/#dollar-prefixed-field-names

For the tests you removed of cases minimongo did not allow, did you try them in Mongo to verify they should be allowed?

It seems to fully support $ in field names, minimongo would need basic support for the $expr operator.

@minhna
Copy link
Contributor Author

minhna commented Jun 26, 2024

minimongo would need basic support for the $expr operator.

I agreed that if you want to easily work with data in minimongo, you will need that.
But let's think about the most use cases we have: We have data on server, we use publication to sync it to minimongo. Now our server accept $ sign but minimongo doesn't. Your pub/sub error.
Now with this PR, you can have that data on your minimongo. You can work with that data by using normal javascript. It supports but doesn't mean you should use that. Minimongo is just a subset of mongodb features, it hasn't had all features and will never be.

@nachocodoner
Copy link
Member

But let's think about the most use cases we have: We have data on server, we use publication to sync it to minimongo. Now our server accept $ sign but minimongo doesn't. Your pub/sub error.

Anyway, it would be good to have test coverage with the new case, to also verify the server side, and all this flow you describe. This will verify it is usable in this way, and we have both mongo driver on server and minimingo supporting the use case.

@minhna
Copy link
Contributor Author

minhna commented Jun 28, 2024

it would be good to have test coverage with the new case

It has, I added test to make sure we can insert object which has $ sign in key.
As @zodern said, minimongo needs to support $expr operator to query on field which has $ sign. So basically you can't put it in query to find, update but we can read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants