PHPLIB-1339 Add tests on Bitwise Query Operators #37

GromNaN · 2024-01-18T18:00:03Z

https://www.mongodb.com/docs/manual/reference/operator/query/#bitwise

$bitsAllClear
$bitsAllSet
$bitsAnyClear
$bitsAnySet

I made all this operators variadics, as suggested previously for $type.

GromNaN · 2024-01-18T18:01:23Z

generator/config/query/bitsAllClear.yaml

+            - $match:
+                  a:
+                      $bitsAllClear:
+                          - { $binary: { base64: 'TUM9PQ==', subType: '00' } }


I've no idea what I'm doing with this binary data. This is the Extended JSON representation of the PHP new Binary('MC=='), which I want to be the equivalent of the JS BinData(0, "ID==")

Note: Binary::__construct() takes a string argument representing binary data. So if you're constructing this in PHP, you should be doing new Binary(base64_decode(...)).

I have no idea why the docs use base64 ID== as the representation of binary 00100000. Even the shell seems to disagree:

> BinData(0, "ID==") Binary.createFromBase64("IA==", 0)

Using a tool like aaronluna.dev/base64, you can see that ID== has the following representation:

ASCII HEX 2 0 8-BIT 00100000 6-BIT 001000 000011 DECIMAL 8 3 BASE64 I D = =

IA== is:

ASCII HEX 2 0 8-BIT 00100000 6-BIT 001000 000000 DECIMAL 8 0 BASE64 I D = =

That suggests ID== is non-canonical, since it denotes 12 bits of data. Only by truncating the last four bits do you end up with the same value as IA==. I think this is worth a DOCSP ticket to suggest a change to the server docs.

libbson actually raises an error from _bson_json_parse_binary_elem() when parsing ID== (perhaps rightfully so):

Invalid input string "ID==", looking for base64-encoded binary

I'd suggest switching over to IA== in this example and leaving a comment in the YAML explaining that you modified the value from the docs to use the more canonical representation.

Thanks, I'm fixing the BSON binary type in #34 using a Yaml tag.

The tag !binary 'IA==' will be converted to new MongoDB\BSON\Binary(base64_decode('IA=='))

See DOCSP-35733

jmikola · 2024-01-18T21:42:06Z

generator/config/query/bitsAllClear.yaml

+            - $match:
+                  a:
+                      $bitsAllClear:
+                          - { $binary: { base64: 'TUM9PQ==', subType: '00' } }


Note: Binary::__construct() takes a string argument representing binary data. So if you're constructing this in PHP, you should be doing new Binary(base64_decode(...)).

I have no idea why the docs use base64 ID== as the representation of binary 00100000. Even the shell seems to disagree:

> BinData(0, "ID==") Binary.createFromBase64("IA==", 0)

Using a tool like aaronluna.dev/base64, you can see that ID== has the following representation:

ASCII HEX 2 0 8-BIT 00100000 6-BIT 001000 000011 DECIMAL 8 3 BASE64 I D = =

IA== is:

ASCII HEX 2 0 8-BIT 00100000 6-BIT 001000 000000 DECIMAL 8 0 BASE64 I D = =

That suggests ID== is non-canonical, since it denotes 12 bits of data. Only by truncating the last four bits do you end up with the same value as IA==. I think this is worth a DOCSP ticket to suggest a change to the server docs.

libbson actually raises an error from _bson_json_parse_binary_elem() when parsing ID== (perhaps rightfully so):

Invalid input string "ID==", looking for base64-encoded binary

I'd suggest switching over to IA== in this example and leaving a comment in the YAML explaining that you modified the value from the docs to use the more canonical representation.

jmikola · 2024-01-18T21:45:00Z

generator/config/query/bitsAllSet.yaml

+            - $match:
+                  a:
+                      $bitsAllSet:
+                          - { $binary: { base64: 'TUM9PQ==', subType: '00' } }


https://www.mongodb.com/docs/manual/reference/operator/query/bitsAllSet/#bindata-bitmask uses MC==, which also isn't canonical:

> BinData(0, "MC==") Binary.createFromBase64("MA==", 0)

I'd suggest including this in your DOCSP ticket as well. I've no idea where the author(s) got these values from.

jmikola · 2024-01-18T21:45:25Z

generator/config/query/bitsAnyClear.yaml

+            - $match:
+                  a:
+                      $bitsAnyClear:
+                          - { $binary: { base64: 'TUM9PQ==', subType: '00' } }


Same issue:

> BinData(0, "MC==") Binary.createFromBase64("MA==", 0)

jmikola · 2024-01-18T21:45:46Z

generator/config/query/bitsAnySet.yaml

+            - $match:
+                  a:
+                      $bitsAnySet:
+                          - { $binary: { base64: 'TUM9PQ==', subType: '00' } }


Same issue:

> BinData(0, "MC==") Binary.createFromBase64("MA==", 0)

tests/Builder/Query/BitsAllClearOperatorTest.php

jmikola · 2024-01-19T16:43:17Z

generator/config/query/bitsAllClear.yaml

+            - $match:
+                  a:
+                      $bitsAllClear:
+                          - !binary 'IA=='


TIL about YAML binary strings. Since this is a single line and we're not dealing with whitespace, I think it'd be reasonable to use the canonical !!binary notation.

But I'm also curious if a binary string is even appropriate here. The corresponding example uses a BSON binary type, so shouldn't you be using extended JSON notation here? I think that was in an earlier draft of the PR.

Same question applies to other files.

Should I rename the !binary tag to !bson_binary, and the same for other tags?

If these tags are custom on your part (wasn't clear to me if you were re-using existing YAML syntax), I think that'd be a good idea.

jmikola · 2024-01-19T16:44:43Z

generator/config/query/bitsAllClear.yaml

+            - $match:
+                    a:
+                        # $bitsAllClear: 35
+                        $bitsAllClear: [35]


What's the purpose of the array? The original Integer Example uses 35, which seems to be what you have in the comment above.

Same question applies to other files.

I added a comment.

Example uses the short form, the builder always generates the array form

Apologies if I'm being obtuse, but $bitsAllClear: [35] seems entirely different than $bitsAllClear: 35. The former would be checking for the 35th bit and the latter checks for the bits according to the binary representation of int(35). These are functionally different APIs (for all of these operators).

Thanks for insisting, it's fixed.

jmikola · 2024-01-19T18:43:13Z

generator/config/query/bitsAllSet.yaml

+            - $match:
+                  a:
+                      $bitsAllSet:
+                          - !binary 'IA=='


Note: apart from bitsAllClear, all of the other examples in the server docs used "MC==", which should be "MA==".

That's not very relevant for what we are testing, but I fixed. Thanks.

Aye, I don't imagine it affects the test, but thought it may be helpful to keep things in sync with the doc examples.

jmikola · 2024-01-19T18:52:04Z

tests/Builder/Query/Pipelines.php

+                "a": {
+                    "$bitsAllClear": [
+                        {
+                            "$binary": {


I see the !binary syntax in YAML is producing extended JSON for a BSON binary type here. Where does that conversion happen? I'd have expected the YAML to just produce a binary string, which would still be parsed as a string type.

Do any of the YAML templates include extended JSON?

Edit: I just saw https://github.com/mongodb/mongo-php-builder/pull/34/files#diff-a715bdcf88cd771d7ae09bd01863bab314ff194ba28e984689e9040d87846db9R20 and https://github.com/mongodb/mongo-php-builder/pull/34/files#diff-41189b0703bfb03f394530009a7d9c88c508f36a39585abd9f73017d6f77745eR135 from PHPLIB-1350 -- so this appears intentional.

tests/Builder/Query/BitsAllClearOperatorTest.php

tests/Builder/Query/BitsAllSetOperatorTest.php

tests/Builder/Query/BitsAnyClearOperatorTest.php

tests/Builder/Query/BitsAllClearOperatorTest.php

jmikola · 2024-01-22T14:51:33Z

generator/config/query/bitsAllClear.yaml

+            - $match:
+                    a:
+                        # $bitsAllClear: 35
+                        $bitsAllClear: [35]


Apologies if I'm being obtuse, but $bitsAllClear: [35] seems entirely different than $bitsAllClear: 35. The former would be checking for the 35th bit and the latter checks for the bits according to the binary representation of int(35). These are functionally different APIs (for all of these operators).

jmikola · 2024-01-22T14:57:33Z

src/Builder/Query/BitsAllClearOperator.php

     */
-    public function __construct(Binary|PackedArray|BSONArray|array|int|string $bitmask)
+    public function __construct(Binary|int|string ...$bitmask)


Re:

https://github.com/mongodb/mongo-php-builder/pull/37/files#r1459574672

https://github.com/mongodb/mongo-php-builder/pull/37/files#r1459323536

I think the problem I'm pointing out in the above threads is due to ambiguity when a single integer is passed here. If 5 is passed to this method, it is interpreted as [5], which is not the same.

I think this needs to be reworked to accept either:

An array of ints

A single int

A single BSON Binary

You're right, there are 3 distinct behaviors. I'll remove the variadic and change the type to int|binary|list<int>.

GromNaN requested a review from alcaeus January 18, 2024 18:00

GromNaN commented Jan 18, 2024

View reviewed changes

GromNaN requested a review from jmikola January 18, 2024 19:51

jmikola reviewed Jan 18, 2024

View reviewed changes

tests/Builder/Query/BitsAllClearOperatorTest.php Outdated Show resolved Hide resolved

alcaeus removed their request for review January 19, 2024 08:07

GromNaN mentioned this pull request Jan 19, 2024

PHPLIB-1350 PHPLIB-1350 Add tests on Date Expression Operators and add Yaml tags for BSON types #34

Merged

GromNaN added 2 commits January 19, 2024 14:47

PHPLIB-1339 Add tests on Bitwise Query Operators

3935a2a

Use the !binary yaml tag

06658a0

GromNaN force-pushed the PHPLIB-1339 branch from 58441fd to 06658a0 Compare January 19, 2024 14:16

GromNaN requested a review from jmikola January 19, 2024 14:33

jmikola reviewed Jan 19, 2024

View reviewed changes

Add comment why we use array for bit operators

540cc0c

GromNaN force-pushed the PHPLIB-1339 branch from d03cfc5 to 540cc0c Compare January 22, 2024 10:12

Fix consistency with the doc for binary value

d4fcb48

GromNaN force-pushed the PHPLIB-1339 branch from ea7ec88 to d4fcb48 Compare January 22, 2024 10:15

GromNaN requested a review from jmikola January 22, 2024 10:16

jmikola reviewed Jan 22, 2024

View reviewed changes

Fix bit query endoding to accept single int or single bin

bc3cb31

GromNaN requested a review from jmikola January 22, 2024 15:08

GromNaN mentioned this pull request Jan 22, 2024

Prefix Yaml tags with !bson_ #43

Merged

jmikola approved these changes Jan 22, 2024

View reviewed changes

GromNaN merged commit 7c9a36a into mongodb:0.1 Jan 22, 2024

PHPLIB-1339 Add tests on Bitwise Query Operators #37

PHPLIB-1339 Add tests on Bitwise Query Operators #37

Uh oh!

Conversation

GromNaN commented Jan 18, 2024

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!

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!

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 Jan 22, 2024 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GromNaN Jan 22, 2024 •

edited

Loading