Remove specific encoder for `Group` and `DollarGroup`, use property name map #1540

GromNaN · 2024-11-27T16:57:45Z

For every Operator class, we add a PROPERTIES constant that contains a map of PHP property name to the operator name.
This allows to remove 2 specific encoders:

$group stage is now [ '_id' => '_id', 'field' => null ]. This means every property for the field object are encoded as properties of the operator object.
$near operator is now ['geometry' => null, 'maxDistance' => '$maxDistance', 'minDistance' => '$minDistance']. The property $geometry of the geometry object is set to the operator object properties. And the name of the other properties get the $ prefix.

GromNaN · 2024-11-27T17:05:37Z

generator/config/query/geoIntersects.yaml

+encode: object
 description: |
    Selects geometries that intersect with a GeoJSON geometry. The 2dsphere index supports $geoIntersects.
 arguments:
    -
        name: geometry
+        noName: true


This change was not required. But this is for consistency with other geo operators.

GromNaN · 2024-11-27T17:07:07Z

src/Builder/Encoder/OperatorEncoder.php

@@ -55,7 +46,8 @@ private function encodeAsArray(OperatorInterface $value): stdClass
    {
        $result = [];
        /** @var mixed $val */
-        foreach (get_object_vars($value) as $val) {
+        foreach ($value::PROPERTIES as $prop => $name) {


Loops over existing array instead of converting object to array with get_object_vars. Must be more performant.

src/Builder/Encoder/OperatorEncoder.php

alcaeus

Nice way of cleaning up some more confusing encoding types, I like it. I have a few smaller suggestions around wording and logic encapsulation.

alcaeus · 2024-11-28T08:44:03Z

generator/README.md

+| `variadic` | `string` | If sent, the argument is variadic. Defines the format `array` for a list or `object` for a map |
+| `variadicMin` | `integer` | The minimum number of arguments for a variadic parameter. |
+| `default` | `scalar` or `array` | The default value for the argument. |
+| `noName` | `bool` | Default `false`. If `true`, the value must be an object and the properties of the value object are merged into the parent operator. `$group` stage uses it for the fields. |


I'll let @jmikola add his thoughts as well, but "pulling to the top" is usually called "hoisting". An example of this would be JavaScript Hoisting, where variable declarations are hoisted to the top of the block they appear in. Both noName and hoist would need explanations here, but I feel like noName is more ambiguous than hoist.

A different alternative is mergeToTopLevel, referring to the "merge" wording you used in the field description in the JSON schema.

hoist is too unfamiliar to me. I'll change to mergeObject.

I'm fine with mergeObject or mergeUp. I read the "JavaScript Hoisting" article, putting aside my historic disdain for W3Fools, but it didn't seem entirely relevant as it was talking about variable declarations and scopes.

alcaeus · 2024-11-28T08:45:27Z

generator/README.md

+### Arguments
+
+| Field | Type | Description |
+| `name` | `string` | The name of the argument. It can start with `$`. |


I suppose a leading $ is used when the parameter in the aggregation operator needs it, but it is not part of the name used in the generated factory? In that case, I would recommend specifying the last part, as it is important to know for people re-using these files.

alcaeus · 2024-11-28T08:48:58Z

generator/src/OperatorClassGenerator.php

+            $argName = ltrim($argument->name, '$');
+            $encodeNames[$argName] = $argument->noName ? null : $argument->name;


How about adding methods to the argument class for these:

name would return the name as-is

getParameterName would return the name as used in factories, properties, etc.

getEncodedName would return the name, or null if noName has been set.

This would avoid duplicating the logic in various generator classes.

Added $propName property.

generator/src/OperatorClassGenerator.php

alcaeus · 2024-11-28T08:55:55Z

src/Builder/Type/OperatorInterface.php

@@ -9,8 +9,11 @@
 */
 interface OperatorInterface
 {
-    /** To be overridden by implementing classes */
+    /** @var Encode */
    public const ENCODE = Encode::Undefined;


I was wondering why we didn't use a typed class constant, until I saw that they were only added in PHP 8.3.

jmikola

Two small comments, but defer to you to address.

jmikola · 2024-12-02T19:26:52Z

generator/README.md

+| Field | Type | Description |
+| `name` | `string` | The name of the argument. It can start with `$` when the aggregation operator needs it, but it will be trimmed from the class property name. |
+| `type` | list of `string` | The list of accepted types |
+| `description` | `string` | The description of the argument from MongoDB's documentation. |


For single-phrase descriptions, I think you can omit the trailing period. Periods make sense when there are multiple sentences in a description.

I'll refrain from commenting on the other lines here, but there are some inconsistencies.

Also, this is a very minor nit.

jmikola · 2024-12-02T19:33:59Z

src/Builder/Encoder/OperatorEncoder.php

            // Skip optional arguments. If they have a default value, it is resolved by the server.
            if ($val === Optional::Undefined) {
                continue;
            }

-            $result->{$key} = $this->recursiveEncode($val);
+            if ($name === null) {


Assuming this is the place where mergeObject: true is handled, a comment here would be helpful.

👍🏻 Comment added.

GromNaN requested a review from a team as a code owner November 27, 2024 16:57

GromNaN requested a review from alcaeus November 27, 2024 16:57

GromNaN changed the title ~~Remove specific encoder for Group and DollarGroup, use property name map~~ Remove specific encoder for Group and DollarGroup, use property name map Nov 27, 2024

GromNaN commented Nov 27, 2024

View reviewed changes

GromNaN mentioned this pull request Nov 27, 2024

PHPLIB-1596 Fully describe $search, $searchMeta and $vectorSearch stages #1537

Merged

2 tasks

GromNaN force-pushed the builder-noName branch 2 times, most recently from 5f53770 to fa8fbab Compare November 27, 2024 17:52

github-advanced-security bot found potential problems Nov 27, 2024

View reviewed changes

alcaeus reviewed Nov 28, 2024

View reviewed changes

GromNaN force-pushed the builder-noName branch from 634cea7 to 63333c5 Compare November 28, 2024 14:03

GromNaN requested a review from alcaeus November 28, 2024 14:18

jmikola approved these changes Dec 2, 2024

View reviewed changes

jmikola mentioned this pull request Dec 2, 2024

Replace OperatorInterface::getOperator() with OperatorInterface::NAME constant #1541

Merged

GromNaN added 2 commits December 2, 2024 21:27

Remove specific encoder for Group and DollarGroup, use property name map

452bf50

Rename noName to mergeObject

acb5520

GromNaN force-pushed the builder-noName branch from 63333c5 to 0d6ba87 Compare December 2, 2024 20:41

GromNaN enabled auto-merge (squash) December 2, 2024 20:42

GromNaN requested review from alcaeus and removed request for alcaeus December 2, 2024 21:01

GromNaN disabled auto-merge December 2, 2024 21:02

GromNaN enabled auto-merge (squash) December 2, 2024 21:03

Wording

2f66b76

GromNaN force-pushed the builder-noName branch from 0d6ba87 to 2f66b76 Compare December 2, 2024 21:15

GromNaN merged commit 2106092 into mongodb:v1.x Dec 2, 2024
29 checks passed

This was referenced Dec 2, 2024

Merge v1.x into v2.x #1545

Closed

Merge v1.x into v2.x #1546

Merged

Merge v1.x into v2.x #1548

Merged

Merge v1.x into v2.x #1550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove specific encoder for `Group` and `DollarGroup`, use property name map #1540

Remove specific encoder for `Group` and `DollarGroup`, use property name map #1540

GromNaN commented Nov 27, 2024 •

edited by jmikola

Loading

GromNaN Nov 27, 2024

GromNaN Nov 27, 2024

alcaeus left a comment

alcaeus Nov 28, 2024

GromNaN Nov 28, 2024

jmikola Dec 2, 2024

alcaeus Nov 28, 2024

GromNaN Nov 28, 2024

alcaeus Nov 28, 2024

GromNaN Nov 28, 2024

alcaeus Nov 28, 2024

jmikola left a comment

jmikola Dec 2, 2024

GromNaN Dec 2, 2024

jmikola Dec 2, 2024

GromNaN Dec 2, 2024

		$argName = ltrim($argument->name, '$');
		$encodeNames[$argName] = $argument->noName ? null : $argument->name;

Remove specific encoder for Group and DollarGroup, use property name map #1540

Remove specific encoder for Group and DollarGroup, use property name map #1540

Conversation

GromNaN commented Nov 27, 2024 • edited by jmikola Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove specific encoder for `Group` and `DollarGroup`, use property name map #1540

Remove specific encoder for `Group` and `DollarGroup`, use property name map #1540

GromNaN commented Nov 27, 2024 •

edited by jmikola

Loading