Skip to content

Query builder considerations for inadvertent use of $where operator #2203

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

Closed
jmikola opened this issue Feb 19, 2021 · 1 comment · Fixed by #2204
Closed

Query builder considerations for inadvertent use of $where operator #2203

jmikola opened this issue Feb 19, 2021 · 1 comment · Fixed by #2204

Comments

@jmikola
Copy link
Member

jmikola commented Feb 19, 2021

Apologies for venturing outside of the bug/feature templates. I'm currently investigating a MongoDB support issue related to $where queries being generated by this library.

Looking at the git-blame of the most recent v3.8.2 release lead me to a single commit: a3bcbe9 from #2127 (released in v3.8.1. Trying to navigate the git-blame before that commit doesn't turn up anything, which I can't explain; however, digging through this project's issues revealed that $where was actually introduced in 336fd3a from #2020 (released in v3.6.4). The original feature request dates back to #2016.

I'm not sure if any of the folks involved in these PRs were aware of the performance implications for using $where, but it can add significant overhead due to its reliance on server-side Javascript and its inability to utilize indexes. Aside from the performance implications, it bears mentioning that server-side Javascript is configurable (permanently so free/shared-tier Atlas clusters. For such deployments, these queries would fail outright.

Allowing pattern-matching on numeric fields

I may be reading too much into @Smolevich's response in #2016, but I got the impression that there was originally no intention of having like support non-string fields. If so, I think that's entirely sensible for a query builder that is abstracting the raw query language. MongoDB's regex queries (i.e. $regex operators and/or BSON regex types) are not dependent on Javascript and can utilize indexes. I don't know if Laravel's SQL query builder allows pattern matching numeric fields and whether that motivated the request in #2016, but the docs for [MySQL's LIKE operator] suggest that it's a non-standard feature:

As an extension to standard SQL, MySQL permits LIKE on numeric expressions.

Based on this Stack Overflow thread, it looks like PostgreSQL requires explicit casting to pattern-match integer fields.

Regression for numeric pattern matching on string fields

Another unfortunate side effect of #2020 is that the decision to use $where seems based purely on whether the pattern itself is numeric. The type of the field is not considered. This is problematic for users that are attempting to pattern-match string fields, as they're now unable to utilize regex queries despite there being no technical limitation to do so. It's possible this is what's affecting the folks in #2138.

Since $where was only introduced for like operations, a viable work-around may be to use regex(p). Its code path was never modified to detect numeric patterns (fortunate oversight?), so it should always allow a BSON regex type to be used directly.

Allowing informed use of $where

IMO, the library would do well to protect users from inadvertently introducing $where into their queries. I don't see any problem with providing a facility (or escape hatch) to use $where explicitly. Looking at the docs, that may be what whereRaw() was intended to allow. If so, I'd encourage you to consider reverting #2020 and #2127 in favor of a blurb demonstrating how to use whereRaw() to pattern-match a numeric field. For example:

::whereRaw(['$where' => '/.*123.*/.test(this.field)'])
::whereRaw(['$where' => '/.*123.*/.test(this["hyphenated-field"])'])

An alternative idea is adding a builder method for the $where operator. That may be helpful for folks that want to nest it in $and or $or operators; however, that may also be an unwelcome API addition if the goal is to resemble the SQL builder as much as possible.

@divine
Copy link
Contributor

divine commented Feb 19, 2021

Hello @jmikola,

We will definitely revert it back as you've suggested.

Very much thank you for your investigation and it's really nice to see the official MongoDB representative here.

Maybe one day MongoDB will have an official library for Laravel too 🙏

Thank you!

divine added a commit that referenced this issue Feb 19, 2021
This reverts back #2020 and #2127. See explanation here: #2203
divine added a commit that referenced this issue Feb 19, 2021
This reverts back #2020 and #2127. See explanation here: #2203
divine added a commit that referenced this issue Feb 20, 2021
This reverts back #2020 and #2127. See explanation here: #2203
softdevee added a commit to softdevee/laravel-mongodb that referenced this issue Sep 5, 2022
This reverts back #2020 and #2127. See explanation here: mongodb/laravel-mongodb#2203
lisadeloach63 added a commit to lisadeloach63/mongodb-laravel that referenced this issue Oct 7, 2022
This reverts back #2020 and #2127. See explanation here: mongodb/laravel-mongodb#2203
PermitinYury pushed a commit to PermitinYury/laravel-mongodb that referenced this issue Feb 17, 2023
This reverts back #2020 and #2127. See explanation here: mongodb/laravel-mongodb#2203
KarenEtheridg added a commit to KarenEtheridg/laravel-mongodb that referenced this issue Feb 17, 2023
This reverts back #2020 and #2127. See explanation here: mongodb/laravel-mongodb#2203
mnphpexpert added a commit to mnphpexpert/laravel-mongodb that referenced this issue Sep 2, 2024
This reverts back mongodb#2020 and mongodb#2127. See explanation here: mongodb#2203
Giant775 added a commit to Giant775/laravel_MongoDB that referenced this issue Nov 15, 2024
This reverts back #2020 and #2127. See explanation here: mongodb/laravel-mongodb#2203
dev-arrow added a commit to dev-arrow/laravel-mongodb that referenced this issue Nov 26, 2024
This reverts back #2020 and #2127. See explanation here: mongodb/laravel-mongodb#2203
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 a pull request may close this issue.

2 participants