PHPLIB-1249 Init code generator project for aggregation builder #1174

GromNaN · 2023-09-25T18:51:12Z

Reimplement aggregation builder prototype.

The branch feature/agg-builder will be the target of all the PRs of the aggregation builder feature.

Use nette/php-generator to generate operator code.
Inspired by AsyncAWS Code Generator.

GromNaN · 2023-09-26T09:03:03Z

generator/config/config.php

+    [
+        'configFile' => __DIR__ . '/stages.yaml',
+        'generatorClass' => ValueClassGenerator::class,
+        'namespace' => MongoDB\Aggregation\Stage::class,


About Aggregation Builder namespacing. I'd like to put all the classes in the same MongoDB\Operator namespace. They are grouped under this name this in the documentation: https://www.mongodb.com/docs/manual/reference/operator/

MongoDB\Operator\Query MongoDB\Operator\Project MongoDB\Operator\Update MongoDB\Operator\Stage MongoDB\Operator\Pipeline

(update operator example is for future, not implemented in this project).

Each of this would be classes with static factory methods for value-object classes in their sub-namespace. So we will have:

MongoDB\Operator\Query\AndQuery MongoDB\Operator\Query\EqQuery MongoDB\Operator\Query\ElemMatchQuery ... MongoDB\Operator\Project\ElemMatchProjection ... MongoDB\Operator\Update\SetUpdate MongoDB\Operator\Update\PopUpdate ... MongoDB\Operator\Stage\BucketStage MongoDB\Operator\Stage\ProjectStage MongoDB\Operator\Stage\SetStage ... MongoDB\Operator\Aggregation\AbsAggregation MongoDB\Operator\Aggregation\BinarySizeAggregation MongoDB\Operator\Aggregation\EqAggregation MongoDB\Operator\Aggregation\BinarySizeAggregation MongoDB\Operator\Aggregation\FirstNAggregation MongoDB\Operator\Aggregation\MaxNAggregation

@jmikola what do you think?

Note that we already have the MongoDB\Operation namespace and there could be confusion between MongoDB\Operation\Aggregate and MongoDB\Operator\Aggregation.

We can choose MongoDB\Builder as namespace. Like "query builder", "aggregation builder", "update builder" ...

What is the intention of repeating the namespace component as a class name suffix? Is that to avoid requiring use as if multiple operators are included in the same file?

Avoid name restrictions (And and Match are not allowed as class names) and avoid use as when needing to use operators with the same name in the same file (Query\Eq and Aggregation\Eq).

But this classes will not be used outside of this library or ORMs.

If you have alternative suggestions for the main namespace MongoDB\Builder\ and MongoDB\Builder\Aggregation for aggregation pipeline operators, I'm all ears.

I think the current namespaces are fine, especially if users are unlikely to interface with these directly (assuming they use factory methods). We can always revisit later if needed.

generator/config/config.php

generator/config/query-operators.yaml

generator/README.md

generator/src/AbstractGenerator.php

alcaeus · 2023-09-28T11:12:42Z

tests/Builder/BuiderCodecTest.php

+    public function testPipeline(): void
+    {
+        $pipeline = new Pipeline(
+            Stage::match(Aggregation::eq('$foo', 1)),


Note that the $match stage shouldn't accept the aggregation pipeline operators, except within $expr.

tests/Builder/BuiderCodecTest.php

alcaeus · 2023-09-28T11:14:06Z

tests/Builder/BuiderCodecTest.php

+            Stage::project([
+                'items' => Aggregation::filter(
+                    input: Expression::fieldPath('items'),
+                    cond: Aggregation::gte(Expression::variable('item.price'), 100),
+                    as: 'item',
+                    limit: $limit,
+                ),
+            ]),


Consider the named argument form:

Suggested change

Stage::project([

'items' => Aggregation::filter(

input: Expression::fieldPath('items'),

cond: Aggregation::gte(Expression::variable('item.price'), 100),

as: 'item',

limit: $limit,

),

]),

Stage::project(

items: Aggregation::filter(

input: Expression::fieldPath('items'),

cond: Aggregation::gte(Expression::variable('item.price'), 100),

as: 'item',

limit: $limit,

),

),

Tracked in PHPLIB-1267

I made the change in this branch.

alcaeus · 2023-09-28T11:17:16Z

src/Builder/BuilderCodec.php

+            if ($val !== null) {
+                $result->{$key} = $val;
+            }


The assumption that fields with a null value can be skipped may not always be true, just to keep in mind.

We will need a special "not specified" object to mark unspecified values...

alcaeus · 2023-09-28T11:17:46Z

src/Builder/BuilderCodec.php

+        }
+
+        // A pipeline is encoded as a list of stages
+        if ($value instanceof Pipeline) {


This check and all subsequent checks should be handled by their own encoder; we can then leverage an encoder map to encode values.

This is a draft implementation to get an overview of how operators and stages are dumped. I'll work on specificities on this tickets:

Tracked by PHPLIB-1257

src/Builder/Pipeline.php

Exclude generated files from phpcs Add Interface suffix to interfaces to prevent confusion with factories Update codec to be encoder only

…rator

GromNaN · 2023-09-28T19:03:07Z

generator/config/expressions.php

+        'implements' => [ExpressionInterface::class],
+        'types' => ['mixed'],
+    ],
+    // @todo check for use-case


Tracked by PHPLIB-1251

GromNaN · 2023-09-28T19:08:16Z

generator/config/expressions.php

+        'implements' => [ExpressionInterface::class],
+        'types' => ['string'],
+    ],
+    // @todo if replaced by a string, it must start with $$


Tracked by PHPLIB-1265

GromNaN · 2023-09-28T21:15:19Z

psalm.xml.dist

@@ -1,5 +1,6 @@
 <?xml version="1.0"?>
 <psalm
+    phpVersion="8.0"


This needs to be configured per directory.
Tracked by PHPLIB-1268

GromNaN · 2023-09-29T09:01:38Z

psalm-baseline.xml

@@ -59,6 +82,11 @@
      <code><![CDATA[$mergedDriver['platform']]]></code>
    </MixedAssignment>
  </file>
+  <file src="src/Codec/EncodeIfSupported.php">
+    <InvalidReturnType>
+      <code>($value is NativeType ? BSONType : $value)</code>


@alcaeus error details:

ERROR: InvalidReturnType at src/Codec/EncodeIfSupported.php:46:22 The declared return type '(BSONType:MongoDB\Codec\EncodeIfSupported as mixed)|(TGeneratedFromParam0:fn-mongodb\codec\encodeifsupported::encodeifsupported as mixed)' for MongoDB\Codec\EncodeIfSupported::encodeIfSupported is incorrect, got '(TGeneratedFromParam0:fn-mongodb\codec\encodeifsupported::encodeifsupported as mixed)|array<array-key, mixed>|stdClass|string' (see https://psalm.dev/011) * @psalm-return ($value is NativeType ? BSONType : $value)

I'm having trouble using the template annotations you've defined.

generator/README.md

generator/composer.json

generator/config/expressions.php

generator/config/operators.php

src/Builder/BuilderEncoder.php

src/Builder/Expression.php

jmikola · 2023-09-30T03:28:03Z

tests/Builder/BuilderCodecTest.php

+    {
+        $pipeline = new Pipeline(
+            // @todo array is accepted by the stage class, but we expect an object. The driver accepts both.
+            Stage::match((object) ['author' => 'dave']),


Is the object-cast necessary here? The match() factory method and Match constructor only say they accept mixed $query, so I don't see anything that enforces the argument type.

Does that mean the implementation for Match is incomplete, or is that intentional? I'm curious what would happen if you passed a scalar value here. Would it just encode to an invalid stage like { "$match": 1 }?

It's a perverse effect to convert all associative arrays into object with objectify to improve readability of expected result. The value could be an associative array here.

Are you saying that the (object) cast was just done to make the test assertion more readable (presumably in the event it fails)? I wasn't thinking of objectify() when I wrote the above comment and was more just generally curious about how Stage::match() reacts to an invalid value being passed in since it only uses mixed.

Confirmed this was a temporary issue, and the type system will be improved down the line.

generator/src/Definition/ArgumentDefinition.php

alcaeus · 2023-10-04T09:22:40Z

generator/config/expressions.php

+    'int' => ['scalar' => true, 'types' => ['int', BSON\Int64::class]],
+    'number' => ['scalar' => true, 'types' => ['int', BSON\Int64::class]],
+    'decimal' => ['scalar' => true, 'types' => ['float', BSON\Decimal128::class]],


A couple of things here:

double is missing from the list; it would correspond to PHP's float type

number can be any of the numeric types (i.e. int, Int64, float, Decimal128)

An int is a valid value to be given whenever a decimal type is accepted.

Given that, I think this should look as follows:

Suggested change

'int' => ['scalar' => true, 'types' => ['int', BSON\Int64::class]],

'number' => ['scalar' => true, 'types' => ['int', BSON\Int64::class]],

'decimal' => ['scalar' => true, 'types' => ['float', BSON\Decimal128::class]],

'int' => ['scalar' => true, 'types' => ['int', BSON\Int64::class]],

'double' => ['scalar' => true, 'types' => ['int', BSON\Int64::class, 'float']],

'decimal' => ['scalar' => true, 'types' => ['int', BSON\Int64::class, 'float', BSON\Decimal128::class]],

'number' => ['scalar' => true, 'types' => ['int', BSON\Int64::class, 'float', BSON\Decimal128::class]],

I'll note that a 64-bit integer value may be truncated when converted to a double, but I believe that shouldn't be an issue in this case.

alcaeus · 2023-10-04T09:34:48Z

generator/config/pipeline-operators.yaml

+      type: expression
+      isVariadic: true
+      variadicMin: 1
+- name: eq


Not an issue here, but we should consider splitting this file into multiple smaller files in the future.

generator/config/stages.yaml

GromNaN · 2023-10-04T10:49:02Z

Tests failing due to PHP version incompatibility. This will be fixed by PHPLIB-1268

GromNaN commented Sep 26, 2023

View reviewed changes

alcaeus reviewed Sep 26, 2023

View reviewed changes

generator/README.md Outdated Show resolved Hide resolved

generator/src/AbstractGenerator.php Outdated Show resolved Hide resolved

generator/src/AbstractGenerator.php Outdated Show resolved Hide resolved

GromNaN force-pushed the PHPLIB-1249 branch from 11e4854 to 8b34f89 Compare September 27, 2023 13:49

GromNaN added 9 commits September 27, 2023 16:34

PHPLIB-1249 Init code generator project

96e03ea

Use promoted properties for config validation

42337fd

Remove one useless config level

e269731

Implement FactoryClassGenerator

725c5ef

Move to MongoDB\builder namespace

32d768f

Add Expression interfaces

9f29f28

Commit generated classes

194f544

Add minimum for variadic arguments

900c748

Add expression types

1ad50f8

GromNaN force-pushed the PHPLIB-1249 branch from 8b34f89 to 1ad50f8 Compare September 27, 2023 14:34

GromNaN added 3 commits September 27, 2023 20:04

Init BuilderCodec with a unit test

2b60cf7

Cleanup

02ae084

Merge multiple arg types

d8c3776

GromNaN marked this pull request as ready for review September 28, 2023 08:23

GromNaN requested review from jmikola and alcaeus September 28, 2023 08:23

alcaeus reviewed Sep 28, 2023

View reviewed changes

GromNaN commented Sep 28, 2023

View reviewed changes

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

GromNaN added 2 commits September 28, 2023 19:27

Add typed FieldPath classes

d0ef3bb

Exclude generated files from phpcs Add Interface suffix to interfaces to prevent confusion with factories Update codec to be encoder only

Pedantry

c1e2cb4

GromNaN changed the base branch from agg-builder to feature/agg-builder September 28, 2023 20:06

Simplify expected pipeline in tests by converting assoc array to objects

3fbb7da

GromNaN force-pushed the PHPLIB-1249 branch from 19b4755 to 3fbb7da Compare September 28, 2023 20:07

GromNaN added 3 commits September 28, 2023 22:14

Fix CS

eed8ea5

Remove useless ACCEPTED_TYPES constant. Pass types config to the gene…

c137337

…rator

Fix psalm issues

1084b1a

GromNaN commented Sep 28, 2023

View reviewed changes

GromNaN commented Sep 29, 2023

View reviewed changes

GromNaN force-pushed the PHPLIB-1249 branch from 100f1e9 to 8b03499 Compare September 29, 2023 09:02

Static analysis

a3eef39

GromNaN force-pushed the PHPLIB-1249 branch from 8b03499 to a3eef39 Compare September 29, 2023 09:37

jmikola reviewed Sep 30, 2023

View reviewed changes

GromNaN added 6 commits October 2, 2023 12:59

Add FieldName type

63aa368

Reractor operators config to use avoid redondancy

078d679

Use assert instead of if...throw to validate config

97e0329

Add Aggregation Builder example

e62d243

Implement variadic options

fdf8f5a

Typo namespace

55f8cce

jmikola reviewed Oct 2, 2023

View reviewed changes

generator/src/Definition/ArgumentDefinition.php Outdated Show resolved Hide resolved

GromNaN added 2 commits October 2, 2023 15:42

Assert types is a list of strings

ace7f60

Make generator a single-command app

cae32c0

GromNaN force-pushed the PHPLIB-1249 branch from 6728f0a to d3e48cc Compare October 2, 2023 14:10

Add Optional::Undefined for optional values

0e80bb6

GromNaN force-pushed the PHPLIB-1249 branch from d3e48cc to 0e80bb6 Compare October 2, 2023 14:22

jmikola approved these changes Oct 2, 2023

View reviewed changes

GromNaN added 2 commits October 2, 2023 17:57

Partial implementation of

d4e53d4

Fix type for $limit

c789074

alcaeus reviewed Oct 4, 2023

View reviewed changes

Add object and double to type system

5bf4753

alcaeus approved these changes Oct 4, 2023

View reviewed changes

GromNaN merged commit 60908f0 into mongodb:feature/agg-builder Oct 4, 2023
22 of 39 checks passed

GromNaN deleted the PHPLIB-1249 branch October 4, 2023 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1249 Init code generator project for aggregation builder #1174

PHPLIB-1249 Init code generator project for aggregation builder #1174

GromNaN commented Sep 25, 2023 •

edited

GromNaN Sep 26, 2023

GromNaN Sep 26, 2023

jmikola Sep 26, 2023

GromNaN Sep 26, 2023

GromNaN Sep 29, 2023

jmikola Oct 2, 2023

alcaeus Sep 28, 2023

alcaeus Sep 28, 2023

GromNaN Sep 29, 2023

GromNaN Oct 2, 2023

alcaeus Sep 28, 2023

GromNaN Sep 28, 2023

alcaeus Sep 28, 2023

GromNaN Sep 28, 2023 •

edited

GromNaN Sep 28, 2023

GromNaN Sep 28, 2023

GromNaN Sep 28, 2023 •

edited

GromNaN Sep 29, 2023

jmikola Sep 30, 2023

GromNaN Oct 2, 2023

jmikola Oct 2, 2023

jmikola Oct 2, 2023

alcaeus Oct 4, 2023

GromNaN Oct 4, 2023

alcaeus Oct 4, 2023

GromNaN commented Oct 4, 2023

PHPLIB-1249 Init code generator project for aggregation builder #1174

PHPLIB-1249 Init code generator project for aggregation builder #1174

Conversation

GromNaN commented Sep 25, 2023 • edited

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

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

GromNaN Sep 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Sep 28, 2023 • edited

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

GromNaN commented Oct 4, 2023

GromNaN commented Sep 25, 2023 •

edited

GromNaN Sep 28, 2023 •

edited

GromNaN Sep 28, 2023 •

edited