PHPLIB-1363 PHPLIB-1355 Add enum for `$type` query and `$meta` expression #38

GromNaN · 2024-01-18T18:52:43Z

The enum implements FieldQueryInterface so it can be used anywhere a field query is expected. Deprecated types have been skipped.

List imported from the doc, including the alias number.

Usage:

        $pipeline = new Pipeline(
            Stage::match(
                zipCode: QueryType::Int,
            ),
        );

Limitation, it's not possible to use the enum in a list.

        $pipeline = new Pipeline(
            Stage::match(
                zipCode: [QueryType::Int, QueryType::Long],
            ),
        );

src/Builder/BuilderEncoder.php

jmikola · 2024-01-18T19:32:24Z

src/Builder/QueryType.php

+ */
+enum QueryType: string implements OperatorInterface, FieldQueryInterface
+{
+ public const ENCODE = Encode::Array;


Is this the first time a BackedEnum is being used for an encoding type? If so, perhaps it'd be prudent to have BuilderEncoder::encode() assert that that $value::ENCODE is Encode::Array (and you can revise that as needed should another use case come up).

That would address my question about how a BackedEnum might be handled for other switch cases.

We need to rework the whole encoder class. The "ENCODE" constant will not stay, it may become a method of the Operator interface.

If this isn't already ticketed please do so and drop a reference to the issue.

alcaeus · 2024-01-19T08:05:36Z

tests/Builder/Query/TypeOperatorTest.php

@@ -32,7 +33,7 @@ public function testQueryingByDataType(): void
 zipCode: Query::type(2),
 ),
 Stage::match(
- zipCode: Query::type('string'),
+ zipCode: QueryType::String,


Huh, this is interesting. I had initially considered using an enum case as the argument for Query::type, but this is interesting as well.

I can customize the Query::type factory to accept enum values and convert them to string if necessary.

Not sure if that is necessary. This allows people to do whatever they want with the $type operator, but provides a nice shorthand for all known types.

I assume this only works because QueryType is both an enum and an OperatorInterface (and FieldQueryInterface in that it's associated with a field path). I assume this convenience only works for a single type check, though. Trying to match zipCode with [QueryType::String, ...] in order to match multiple types would produce invalid syntax, correct?

And that's where Query::type(...) comes in.

jmikola

Per the Psalm error below, what enforces that OperatorInterface instances define an ENCODE constant?

LGTM, but I think the various Psalm errors need correction.

jmikola · 2024-01-19T19:24:29Z

src/Builder/BuilderEncoder.php

@@ -129,6 +130,10 @@ public function encode($value): stdClass|array|string
 */
 private function encodeAsArray(OperatorInterface $value): stdClass
 {
+ if ($value instanceof BackedEnum) {
+ return $this->wrap($value, [$value->value]);


Is the NoInterfaceProperties error below the result of Psalm failing to infer that BackedEnum has an implicit $value property?

Yes it looks like an issue in palm: vimeo/psalm#10585

phpstan don't have this issue: https://phpstan.org/r/e7094884-1adf-4369-b255-6c7e45367865

How is this related to the NoInterfaceProperties warning earlier in encode()? There, you're only checking for an instance of FieldPathInterface and then accessing a $value property. That seems incorrect for an entirely different reason.

src/Builder/BuilderEncoder.php

jmikola · 2024-01-19T19:32:04Z

tests/Builder/Query/TypeOperatorTest.php

@@ -32,7 +33,7 @@ public function testQueryingByDataType(): void
 zipCode: Query::type(2),
 ),
 Stage::match(
- zipCode: Query::type('string'),
+ zipCode: QueryType::String,


I assume this only works because QueryType is both an enum and an OperatorInterface (and FieldQueryInterface in that it's associated with a field path). I assume this convenience only works for a single type check, though. Trying to match zipCode with [QueryType::String, ...] in order to match multiple types would produce invalid syntax, correct?

And that's where Query::type(...) comes in.

GromNaN · 2024-01-26T14:19:56Z

src/Builder/Meta.php

+ *
+ * @see https://www.mongodb.com/docs/manual/reference/operator/aggregation/meta/
+ */
+enum Meta: string implements OperatorInterface, ExpressionInterface


@alcaeus suggested to name it SortMeta. #56 (comment)

$meta: Behavior suggests the operator can be used in context beyond sorting, such as projections.

The ExpressionInterface allows to use it anywhere an expression is expected.

jmikola

Just realized I had some pending comments that were never posted. Doing so now.

jmikola · 2024-01-22T14:38:41Z

src/Builder/BuilderEncoder.php

@@ -129,6 +130,10 @@ public function encode($value): stdClass|array|string
 */
 private function encodeAsArray(OperatorInterface $value): stdClass
 {
+ if ($value instanceof BackedEnum) {
+ return $this->wrap($value, [$value->value]);


How is this related to the NoInterfaceProperties warning earlier in encode()? There, you're only checking for an instance of FieldPathInterface and then accessing a $value property. That seems incorrect for an entirely different reason.

jmikola · 2024-01-22T14:39:21Z

src/Builder/QueryType.php

+ */
+enum QueryType: string implements OperatorInterface, FieldQueryInterface
+{
+ public const ENCODE = Encode::Array;


If this isn't already ticketed please do so and drop a reference to the issue.

jmikola · 2024-01-27T01:29:08Z

src/Builder/Meta.php

+ *
+ * @see https://www.mongodb.com/docs/manual/reference/operator/aggregation/meta/
+ */
+enum Meta: string implements OperatorInterface, ExpressionInterface


$meta: Behavior suggests the operator can be used in context beyond sorting, such as projections.

jmikola · 2024-01-27T01:30:49Z

src/Builder/Meta.php

+ * { $meta: "indexKey" } expression is for debugging purposes only, and
+ * not for application logic, and is preferred over cursor.returnKey().
+ */
+ case IndexKey = 'indexKey';


Do the cases here preclude users from using this with additional modes? $meta: Definition mentions searchScore and searchHighlights for Atlas.

GromNaN · 2024-01-29T10:00:26Z

Rebased on top of #46

jmikola

Left some additional comments on the enum cases, and there are already a number of open comment threads from last week.

src/Builder/QueryType.php

jmikola · 2024-01-29T15:58:14Z

src/Builder/QueryType.php

+ case JavaScript = 'javascript';
+ case Int = 'int';
+ case Timestamp = 'timestamp';
+ case Long = 'long';


Suggested change

case Long = 'long';

case Int64 = 'long';

This also leaves nothing to the imagination.

It just occurred to me that you might have chosen these names to be consistent with the ExpressionFactoryTrait field path methods. Unless it was just the first column from the table in $type: Available Types.

In any event, you may want to keep the factory methods consistent with the enums here. And if factory methods will be distinguishing 32-bit and 64-bit integers, I do think explicit names with the bit size are more clear to users than "int" and "long".

I updated all the type name to take more descriptive ones, from the BSON classes when they exist.

src/Builder/QueryType.php

jmikola · 2024-01-29T16:00:01Z

src/Builder/QueryType.php

+ case Int = 'int';
+ case Timestamp = 'timestamp';
+ case Long = 'long';
+ case Decimal128 = 'decimal';


I do think using the BSON class name was a good idea here, as users could confuse "decimal" with "double" if they didn't realize there was a specific type.

src/Builder/QueryType.php

GromNaN · 2024-02-16T13:59:32Z

Closing for now. Lets revisit later. Ticket still open.

GromNaN requested review from jmikola and alcaeus January 18, 2024 18:52

GromNaN force-pushed the PHPLIB-1363 branch from 18a4092 to 3a64f81 Compare January 18, 2024 18:53

GromNaN changed the title ~~PHPLIB-1363 Add enum for list~~ PHPLIB-1363 Add enum for $type list Jan 18, 2024

jmikola reviewed Jan 18, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1363 branch 2 times, most recently from 9443998 to 2134961 Compare January 18, 2024 20:21

alcaeus reviewed Jan 19, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1363 branch from 2134961 to 4401d39 Compare January 19, 2024 09:39

jmikola reviewed Jan 19, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1363 branch from 4401d39 to 985f78f Compare January 22, 2024 09:21

GromNaN changed the title ~~PHPLIB-1363 Add enum for $type list~~ PHPLIB-1363 Add enum for $type query and $meta expression Jan 22, 2024

GromNaN changed the title ~~PHPLIB-1363 Add enum for $type query and $meta expression~~ PHPLIB-1363 PHPLIB-1355 Add enum for $type query and $meta expression Jan 23, 2024

This was referenced Jan 24, 2024

PHPLIB-1340 Remove Projection Operators #54

Merged

PHPLIB-1374 Add tests on all stages #56

Merged

GromNaN commented Jan 26, 2024

View reviewed changes

jmikola reviewed Jan 27, 2024

View reviewed changes

jmikola self-requested a review January 27, 2024 01:36

GromNaN force-pushed the PHPLIB-1363 branch from 985f78f to ce1fbdc Compare January 29, 2024 09:59

jmikola reviewed Jan 29, 2024

View reviewed changes

GromNaN added 3 commits January 31, 2024 15:20

PHPLIB-1363 PHPLIB-1355 Add enum for $type query and $meta expression

680659f

Add searchHighlights

141874e

Add wrong error to psalm baseline

9f14536

GromNaN force-pushed the PHPLIB-1363 branch from 9d7be9a to 9f14536 Compare January 31, 2024 14:23

Use BSON class names for $type query

a0c79c0

GromNaN mentioned this pull request Feb 1, 2024

PHPLIB-1355 Add tests to $meta expression #59

Merged

alcaeus self-requested a review February 13, 2024 14:58

GromNaN closed this Feb 16, 2024

GromNaN deleted the PHPLIB-1363 branch February 16, 2024 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1363 PHPLIB-1355 Add enum for `$type` query and `$meta` expression #38

PHPLIB-1363 PHPLIB-1355 Add enum for `$type` query and `$meta` expression #38

GromNaN commented Jan 18, 2024 •

edited

Loading

jmikola Jan 18, 2024

GromNaN Jan 22, 2024 •

edited

Loading

jmikola Jan 22, 2024

alcaeus Jan 19, 2024

GromNaN Jan 19, 2024

alcaeus Jan 19, 2024

jmikola Jan 19, 2024

jmikola left a comment

jmikola Jan 19, 2024

GromNaN Jan 22, 2024 •

edited

Loading

jmikola Jan 22, 2024

jmikola Jan 19, 2024

GromNaN Jan 26, 2024

jmikola Jan 27, 2024

GromNaN Jan 31, 2024

jmikola left a comment

jmikola Jan 22, 2024

jmikola Jan 22, 2024

jmikola Jan 27, 2024

jmikola Jan 27, 2024

GromNaN commented Jan 29, 2024

jmikola left a comment

jmikola Jan 29, 2024

jmikola Jan 30, 2024

GromNaN Jan 31, 2024

jmikola Jan 29, 2024

GromNaN commented Feb 16, 2024

PHPLIB-1363 PHPLIB-1355 Add enum for $type query and $meta expression #38

PHPLIB-1363 PHPLIB-1355 Add enum for $type query and $meta expression #38

Conversation

GromNaN commented Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

GromNaN Jan 22, 2024 • edited Loading

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

GromNaN Jan 22, 2024 • edited Loading

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

GromNaN commented Jan 29, 2024

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

GromNaN commented Feb 16, 2024

PHPLIB-1363 PHPLIB-1355 Add enum for `$type` query and `$meta` expression #38

PHPLIB-1363 PHPLIB-1355 Add enum for `$type` query and `$meta` expression #38

GromNaN commented Jan 18, 2024 •

edited

Loading

GromNaN Jan 22, 2024 •

edited

Loading

GromNaN Jan 22, 2024 •

edited

Loading