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

docs(migration-guide-6x): document that strictQuery default will flip-back #14998

Merged

Conversation

markstos
Copy link
Contributor

I spent many hours migrating a large code base from 5.x to 6.x. By a large margin, more hours were lost as a result of the change to strictQuery: true. Parts of my queries had been discarded and it wasn't clear why.

Although I had read the migration guide multiple times, perhaps I glossed over this section believing that since I wasn't using the strictQuery option before, this change didn't apply to me.

To limit my scope of work, I always wasn't reading ahead to the changes in 7.x, where the default flipped back to what it was in 5.x.

So only at the very end during peer-review did someone point out that we should set strictQuery:false-- the default in both 5.x and >=7.x. Then I re-read that section of the migration guide and all my problems with parts of queries being discarded made sense.

So I recommend adding a note about this to the "Migrating to 6.x" guide because I think it could save some other people some headache, if they set strictQuery:false when upgrading from 5.x to 6.x.

This PR also fixes a closing tag issue in the same document.


Related to this, I would also suggest that there be extra logging when strictQuery:true. At least at the debug level if not info or warn, include some logging like:

foo:'zoo' was discarded from query because strictQuery:true

Considering that could be a separate issue. Really, after living with the effects of having strictQuery:true, I think it's dangerous and unintuitive option to silently modify queries, and would prefer that referencing missing schema throw (like strictQuery:'throw' or work anyway, like strictQuery:'false').

Save upgrading users the headache of `strictQuery:true` problems by
highlighting that the default is going to change back in Mongoose 7,
so they may wish to skip this default behavior change.
@vkarpov15
Copy link
Collaborator

This PR makes sense, thanks. And I'm sorry strictQuery caused you so much trouble. FWIW, that's why we undid the change in 7.x. In hindsight, we likely should have reverted that change early in the 6.x release cycle.

Re: logging, we try to avoid console.warn() in runtime code for performance. More likely, we would have to create a separate option like strictQuery: 'log' or something like that to handle the case where you want strictQuery: true and warnings.

Instead of strictQuery: 'log', what I would do would be to set strictQuery: 'throw', which will make Mongoose throw an error whenever there's unexpected fields in the query filter. At least when you're doing development, so you can catch these sorts of issues.

@vkarpov15 vkarpov15 added this to the 6.13.4 milestone Nov 4, 2024
@vkarpov15 vkarpov15 merged commit 6fbe9f0 into Automattic:6.x Nov 4, 2024
1 check passed
@markstos
Copy link
Contributor Author

markstos commented Nov 4, 2024

@vkarpov15 Thanks for the feedback and the merge!

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.

2 participants