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

Add readConcern and collation support #105

Merged
merged 7 commits into from
Jul 29, 2018

Conversation

Fonger
Copy link
Contributor

@Fonger Fonger commented Jul 29, 2018

readConcern

The readConcern options is not intuitive and have to be an object containing level property.
If we just pass string to it, the native driver will throw error.
See Automattic/mongoose#6777 for the motivation

Before this pull request

Model.find().setOptions({ readConcern: { level: 'linearizable' } })
Model.find().maxTime(5000)

After this pull request

Model.find().readConcern('linearizable')
Model.find().readConcern('lz')

maxTimeMS

Also, this pull request make maxTimeMS alias of maxTime.
So the syntax is now closer to mongo shell.
See Automattic/mongoose#6777 for the motivation

Before this pull request

Model.find().maxTime(5000)

After this pull request

Model.find().maxTime(5000) // work
Model.find().maxTimeMS(5000) // also work. same syntax with mongo shell

collation

Add collation support

Before this pull request

Model.find({ caseInsensitive: 'abc' }).setOptions({ collation: { locale: "en_US", strength: 1} })

After this pull request

Model.find({ caseInsensitive: 'abc' }).collation({ locale: "en_US", strength: 1} })

mongoose currently inherit mquery and extend collation support.
mongoose may remove the implementation if using this version of mquery

@Fonger Fonger changed the title Add readConcern support Add readConcern and collation support Jul 29, 2018
Copy link
Member

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

This is excellent work, thanks! We'll make sure to add this to mongoose as well.

@vkarpov15 vkarpov15 merged commit b19860a into mongoosejs:master Jul 29, 2018
@vkarpov15
Copy link
Member

@Fonger quick question: do you use mquery directly or do you go through mongoose?

@Fonger
Copy link
Contributor Author

Fonger commented Jul 30, 2018

@vkarpov15 Thanks for the merge. #106 fix some documentation errors.

I use both. I'm currently working on a project that use mquery as template in browser and convert it to pass all of the options, conditions... etc to backend to process the query in node

I would like to add eslint for this project to keep consistent coding style. Any idea on this?

@aheckmann
Copy link
Collaborator

@Fonger +1 for the addition of eslint

@vkarpov15
Copy link
Member

@Fonger thanks for eslint! That's an interesting approach. Mongoose has been slowly but surely moving away from using mquery internally because its proven to be a tangled abstraction and there are several features in Mongoose queries that aren't in mquery. But I've seen several projects popping up recently that use mquery so I'm thinking about whether we should focus on mquery more or focus on exporting something more like mquery from mongoose. I have a couple questions:

  1. What do you use to serialize mquery for HTTP?
  2. If Mongoose's browser lib included a serializable query builder, would you use that instead of mquery?
  3. If Mongoose's browser lib supported connecting to the browser and executing queries via MongoDB Stitch, would you use that instead or would you still prefer to send queries to a server for validation?

@Fonger
Copy link
Contributor Author

Fonger commented Jul 30, 2018

@vkarpov15

I prefer to support more query helpers in mquery and working on it. #108 - support write concern.
The best is to make the syntax more compatible with mongo shell to avoid confusion. As there are 2 goals shown in README: browser compatibility #4 and mongo-shell compatibility #5

What do you use to serialize mquery for HTTP?

I use mongodb-extjson to preserve data types such as ObjectId, Date, NumberLong, ...etc. JSON only support string, number, boolean and plain object.

mongodb-extjson with output like '{"int32":{"$numberInt":"10"}}' as pass it to HTTP server with conditions=xxxx parameter. And also pass options=xxxx parameter.

I have another version using socket.io and transfer with binary. To enhance the performance, in this websocket version, I serialize and encode it via bson which is compatible with browser, sending with websocket binary data and then deserializing via bson-ext in node.js backend. We can use the same bson package in backend but bson-ext is better because it has better performance. bson is built with native C++ and is fully compatible with bson.

I'm not using mquery directly with browser. I just copy part of the code. Maybe I can add the implementation back to mquery if the spec for browser is well-defined.

If Mongoose's browser lib included a serializable query builder, would you use that instead of mquery?

I'm not sure at this moment. In fact in my project i'm using mongoose for developer accounts only because I need custom validation. I have implemented a rule script language like Firebase to validate the operation and data. The actual mongodb operations are done by calling native mongodb driver.

If Mongoose's browser lib supported connecting to the browser and executing queries via MongoDB Stitch, would you use that instead or would you still prefer to send queries to a server for validation?

I have never heard of MongoDB Stitch before. I google it and find it fantastic!
I'm developing similar service like MongoDB Stich. It's more like firebase. When I finish this project I'll open source it.
It's great to support MongoDB Stitch but I think it should be created in another npm package because it is not really related to mongoose.

@aheckmann
Copy link
Collaborator

aheckmann commented Jul 31, 2018 via email

@vkarpov15
Copy link
Member

Yeah, I meant winding mquery back into mongoose, because adding new functionality to both mongoose and mquery is cumbersome. Not actively pushing on that, just an idea to think about.

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

Successfully merging this pull request may close these issues.

3 participants