PHPORM-53 Fix and test `like` and `regex` operators #17

GromNaN · 2023-07-19T21:19:00Z

Fix PHPORM-53

Fix support for % and _ in like expression and escaped \% and \_
Keep ilike and regexp operators as aliases for like and regex
Allow /, # and ~ as regex delimiters
Add functional tests on regexp and not regexp
Add support for not regex

jmikola

I have a lot of thoughts about this, so read through the comments at your leisure.

I'm mostly concerned about the changes in QueryTest.php and how that could affect existing applications.

CHANGELOG.md

src/Query/Builder.php

tests/Query/BuilderTest.php

jmikola · 2023-07-20T18:17:27Z

src/Query/Builder.php

+                    throw new \InvalidArgumentException(sprintf('Regular expressions must be surrounded by slashes "/". Got "%s"', $value));
+                }
+                $flags = end($e);
+                $regstr = substr($value, 1, -1 - strlen($flags));


I just want to confirm that this handles cases where the pattern includes escaped forward slashes. If we don't currently test that perhaps we should.

I think it does, you only assert that explode() returns at least three components and only the final component is used to extra flags (which will be an empty string if something like /acme/ is passed in).

I think there may be some edge cases where parsing might accept invalid input. Given '/acme/bar/foo', this will construct a Regex with the pattern acme/bar, which seems incorrect? I'm not sure that the Laravel library should care about that, though. Probably not.

If it helps, here are some local tests using a BSON regex object where the pattern string contains slash characters.

<?php require __DIR__ . '/vendor/autoload.php'; $client = new MongoDB\Client('mongodb://localhost:27070/?replicaSet=rs0'); $collection = $client->test->foo; $collection->drop(); $collection->insertOne(['x' => 'foo']); $collection->insertOne(['x' => 'foo/']); foreach (['foo', 'foo/', 'foo\/', 'foo\\/', 'foo\\\/'] as $pattern) { $regex = new MongoDB\BSON\Regex($pattern); $cursor = $collection->find(['x' => $regex]); $cursor->setTypeMap(['root' => 'bson']); printf("Querying with: %s\n", json_encode($regex)); foreach ($cursor as $document) { echo $document->toRelaxedExtendedJSON(), "\n"; } echo "\n"; }

Output:

Querying with: {"$regex":"foo","$options":""} { "_id" : { "$oid" : "64b97a01285f15fae9083a62" }, "x" : "foo" } { "_id" : { "$oid" : "64b97a01285f15fae9083a63" }, "x" : "foo/" } Querying with: {"$regex":"foo\/","$options":""} { "_id" : { "$oid" : "64b97a01285f15fae9083a63" }, "x" : "foo/" } Querying with: {"$regex":"foo\\\/","$options":""} { "_id" : { "$oid" : "64b97a01285f15fae9083a63" }, "x" : "foo/" } Querying with: {"$regex":"foo\\\/","$options":""} { "_id" : { "$oid" : "64b97a01285f15fae9083a63" }, "x" : "foo/" } Querying with: {"$regex":"foo\\\\\/","$options":""}

@GromNaN: I think the only possible action item from the above is:

I just want to confirm that this handles cases where the pattern includes escaped forward slashes. If we don't currently test that perhaps we should.

I'll defer to you. If you don't think any such test is necessary, feel free to resolve this thread after reading.

I believe any more regex parsing than absolutely necessary should be avoided. Looking at the example with /acme/bar/foo, plugging that into preg_match reveals that the first unescaped forward slash is treated as the end delimiter, resulting in b being reported as an unknown modified: https://3v4l.org/sk2fQ

Anything more than that would require us to specifically explode on unescaped delimiters. This means that simply matching for '[^\\\]' . $delimiter won't work, as the backslash itself could've been escaped, resulting once again in an unescaped delimiter. There's only so much polishing one can do on crappy inputs to make them work.

plugging that into preg_match reveals that the first unescaped forward slash is treated as the end delimiter

Would it make sense to use consistent parsing here? So we pick up the first delimiter character and then find the next unescaped occurrence? It's unfortunate there's no preg_unquote() implementation, but perhaps stripslashes() might suffice? (see: https://stackoverflow.com/a/9172908/162228).

If we rely on that, we're more likely to produce an exception by yielding an invalid flag string, instead of a pattern that won't match anything and could be more confusing to debug.

Quick follow-up as I mentioned in Slack while thinking about this some more:

Although to be honest I'm not confident stripslashes() would be a fool-proof solution, as stripslashes('/foo\/bar/i') yields "/foo/bar/i" and it's still ambiguous what the user intended because we eliminated the escaping. So maybe this problem is closer to the parsing you had to do to deal with % and _ escaping for like.

Given that, I now understand @alcaeus' previous comment:

Anything more than that would require us to specifically explode on unescaped delimiters.

I think we're OK to leave this as-is, but I'd propose adding a comment somewhere around this code to note the edge cases we're choosing not to handle.

tests/Query/BuilderTest.php

jmikola · 2023-07-20T18:22:35Z

tests/Query/BuilderTest.php

@@ -433,6 +434,62 @@ function (Builder $builder) {
                ->orWhereNotBetween('id', collect([3, 4])),
        ];

+        yield 'where like' => [
+            ['find' => [['name' => new Regex('^acme$', '')], []]],


If the like pattern is not using any special operators, does it make sense to produce a regex instead of a direct value match? I realize ilike would still need a regex for case-insensitivity.

Both this and the where like escape test below could avoid using a regex object.

If you agree, feel free to defer this to another JIRA ticket, since it's probably just an optimization.

Since like has become case-insensitive again, this optimization is no longer possible.

Agreed. I was writing that suggestion alongside my other suggestion that we not break BC in case we went in either direction. Feel free to resolve after reading.

src/Query/Builder.php

tests/Query/BuilderTest.php

src/Query/Builder.php

tests/Query/BuilderTest.php

jmikola · 2023-07-21T16:00:17Z

src/Query/Builder.php

-                    throw new \InvalidArgumentException(sprintf('Regular expressions must be surrounded by slashes "/". Got "%s"', $value));
+            if (is_string($value)) {
+                $delimiter = substr($value, 0, 1);
+                if (! in_array($delimiter, ['/', '#', '~'])) {


This looks like a good compromise to allow users some flexibility. Thanks 👍

I suggest storing this array in a constant property and then referencing it in the exception message below, though, which suggests that only "/" is permitted.

src/Query/Builder.php

jmikola · 2023-07-21T16:13:37Z

src/Query/Builder.php

+                }
+                $e = explode($delimiter, $value);
+                if (count($e) < 3) {
+                    throw new \InvalidArgumentException(sprintf('Regular expressions must be surrounded by delimiter "%s". Got "%s"', $delimiter, $value));


I understand why you printed $value here. By this point, we failed to find the second delimiter and cannot say with any certainty where it should have been expected. Consider:

> $value = '/foo#bar' = "/foo#bar" > $delimiter = substr($value, 0, 1); = "/" > $e = explode($delimiter, $value); = [ "", "foo#bar", ]

What do you think about the following error message?

Missing expected ending delimiter "/" in "/foo#bar"

Just a suggestion to avoid must be surrounded by "%s" since the code above was changed to be more flexible about what delimiters are supported.

jmikola

cc: @alcaeus in case he'd like to take a quick pass through this.

src/Query/Builder.php

GromNaN · 2023-07-25T07:08:27Z

src/Query/Builder.php

@@ -122,6 +120,13 @@ class Builder extends BaseBuilder
        '>=' => '$gte',
    ];

+    private $replacementOperators = [
+        'regexp' => 'regex',


The regexp and ilike operators are provided by Laravel and documented in the readme of this project.

NOTE: you can also use the Laravel regexp operations.

Keeping compatibility with Laravel Query Builder and with previous versions of this package is cheap. I'd like to step back and simply alias the operators.

I decided to keep the Laravel operators as aliases.

I'm OK with this. Being pedantic about what operators we support is likely just going to cause an inconvenience to users, especially if there's no harm in these aliases. The biggest mistake I can see is the fact that like is case-insensitive, but that's a default we cannot easily/safely change without harming users.

regex is probably the best migration path for folks using like today, and we can always revisit this down the line and carefully consider how we might deprecate like with its case-insensitivity (if ever, but probably not).

GromNaN

A lot of changes to keep supporting ilike and regexp.
Also fixes on double %% and __ in like value.

src/Query/Builder.php

GromNaN · 2023-07-26T12:48:12Z

src/Query/Builder.php

@@ -122,6 +120,13 @@ class Builder extends BaseBuilder
        '>=' => '$gte',
    ];

+    private $replacementOperators = [
+        'regexp' => 'regex',


I decided to keep the Laravel operators as aliases.

GromNaN · 2023-07-26T12:50:48Z

src/Query/Builder.php

-                    $where['operator'] = $convert[$where['operator']];
+                // Convert aliased operators
+                if (isset($this->conversion[$where['operator']])) {
+                    $where['operator'] = $this->conversion[$where['operator']];


We had 2 lists of converted operators. I merged them into a single in the $conversion property.

Just to confirm, does this intentionally use isset() to ensure that an operator without an alias is left as-is and yields some error down the line when another query builder method realizes it isn't supported?

I'm OK with the error being reported later, but wanted to ensure that actually happens without us having to explicitly handle it. I think the explicit handling was only for actual builder methods ("integerRaw" was it?), which I don't think applies here since we're using where().

Operators like all are supported and not aliases. The isset is here to not work on them.
Checking if the operator is supported is done before in invalidOperator called by Laravel's Illuminate\Database\Query\Builder

GromNaN · 2023-07-26T12:51:46Z

src/Query/Builder.php

        }

        if (! isset($operator) || $operator == '=') {
            $query = [$column => $value];
-        } elseif (array_key_exists($operator, $this->conversion)) {
-            $query = [$column => [$this->conversion[$operator] => $value]];


The conversion is now done earlier in compileWheres.

alcaeus

LGTM with a small comment about escaped escape characters in front of wildcards.

src/Query/Builder.php

jmikola · 2023-07-27T16:09:03Z

src/Query/Builder.php

@@ -122,6 +120,13 @@ class Builder extends BaseBuilder
        '>=' => '$gte',
    ];

+    private $replacementOperators = [
+        'regexp' => 'regex',


I'm OK with this. Being pedantic about what operators we support is likely just going to cause an inconvenience to users, especially if there's no harm in these aliases. The biggest mistake I can see is the fact that like is case-insensitive, but that's a default we cannot easily/safely change without harming users.

regex is probably the best migration path for folks using like today, and we can always revisit this down the line and carefully consider how we might deprecate like with its case-insensitivity (if ever, but probably not).

jmikola · 2023-07-27T16:10:01Z

src/Query/Builder.php

-                    $where['operator'] = $convert[$where['operator']];
+                // Convert aliased operators
+                if (isset($this->conversion[$where['operator']])) {
+                    $where['operator'] = $this->conversion[$where['operator']];


Just to confirm, does this intentionally use isset() to ensure that an operator without an alias is left as-is and yields some error down the line when another query builder method realizes it isn't supported?

I'm OK with the error being reported later, but wanted to ensure that actually happens without us having to explicitly handle it. I think the explicit handling was only for actual builder methods ("integerRaw" was it?), which I don't think applies here since we're using where().

src/Query/Builder.php

jmikola · 2023-07-27T16:14:43Z

src/Query/Builder.php

+
+        $operator = strtolower($operator);
+
+        if (isset($this->conversion[$operator])) {


I noticed this PR didn't introduce the $conversion property, it just added to it. How is this used in the base Eloquent library?

I'm also curious because you basically recreated the entire $conversion map in this PR (assuming '=' => '=' is redundant and should be removed), so I'm unsure if it's still relevant to any parent context.

$conversion is specific to mongodb and doesn't exist in Eloquent.

Yes '=' => '=' is useless. I removed it and restored the complete operators list in $operators because the property is public. So I removed this method overload.

- Fix support for % and _ in like expression and escaped \% and \_ - Keep ilike and regexp operators as aliases for like and regex - Allow /, # and ~ as regex delimiters - Add functional tests on regexp and not regexp - Add support for not regex

GromNaN requested review from jmikola and alcaeus July 19, 2023 21:19

GromNaN changed the title ~~PHPORM-53 Fix and test like and regex operators~~ PHPORM-53 Fix and test like and regex operators Jul 19, 2023

GromNaN force-pushed the PHPORM-53 branch from b107b8b to 9d54e0f Compare July 19, 2023 21:26

jmikola reviewed Jul 20, 2023

View reviewed changes

GromNaN force-pushed the PHPORM-53 branch 2 times, most recently from 654b04e to 4a491f6 Compare July 20, 2023 22:52

GromNaN requested a review from jmikola July 20, 2023 23:01

jmikola reviewed Jul 21, 2023

View reviewed changes

src/Query/Builder.php Outdated Show resolved Hide resolved

tests/Query/BuilderTest.php Show resolved Hide resolved

jmikola reviewed Jul 21, 2023

View reviewed changes

jmikola approved these changes Jul 24, 2023

View reviewed changes

src/Query/Builder.php Show resolved Hide resolved

GromNaN force-pushed the PHPORM-53 branch from fcc61a6 to 40d56c6 Compare July 26, 2023 09:20

GromNaN commented Jul 26, 2023

View reviewed changes

GromNaN force-pushed the PHPORM-53 branch 2 times, most recently from 132a709 to 22a8bce Compare July 26, 2023 12:50

GromNaN commented Jul 26, 2023

View reviewed changes

GromNaN force-pushed the PHPORM-53 branch from e7b0fad to 8e1b602 Compare July 26, 2023 13:16

GromNaN mentioned this pull request Jul 26, 2023

PHPORM-68 Fix unique validator when the validated value is part of an existing value #21

Merged

GromNaN force-pushed the PHPORM-53 branch from 8e1b602 to 2025405 Compare July 26, 2023 13:37

alcaeus approved these changes Jul 26, 2023

View reviewed changes

src/Query/Builder.php Show resolved Hide resolved

GromNaN added 5 commits July 26, 2023 15:50

PHPORM-53 Improve like and regex operators

c18e814

Restore ilike & regexp for compatibility with Eloquent

491e96d

Unescape forward slash from regex

4bb2b24

Fix consecutive _ in like

4d9d400

Optimize DatabasePresenceVerifier

08330fd

GromNaN force-pushed the PHPORM-53 branch from 2025405 to 08330fd Compare July 26, 2023 14:22

jmikola reviewed Jul 27, 2023

View reviewed changes

GromNaN force-pushed the PHPORM-53 branch 2 times, most recently from 56d9da8 to 47304e9 Compare July 27, 2023 16:59

Keep all operators in Builder::$operators public variable

6ddd29d

GromNaN force-pushed the PHPORM-53 branch from 47304e9 to 6ddd29d Compare July 27, 2023 17:00

GromNaN merged commit 2319d53 into master Jul 27, 2023

GromNaN deleted the PHPORM-53 branch July 27, 2023 17:03

GromNaN mentioned this pull request Aug 21, 2023

Cleanup and tests mongodb/laravel-mongodb#2567

Merged


		$operator = strtolower($operator);

		if (isset($this->conversion[$operator])) {

PHPORM-53 Fix and test like and regex operators #17

PHPORM-53 Fix and test like and regex operators #17

Uh oh!

Conversation

GromNaN commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmikola Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PHPORM-53 Fix and test `like` and `regex` operators #17

PHPORM-53 Fix and test `like` and `regex` operators #17

GromNaN commented Jul 19, 2023 •

edited

Loading

jmikola Jul 20, 2023 •

edited

Loading

GromNaN Jul 20, 2023 •

edited

Loading

GromNaN Jul 26, 2023 •

edited

Loading