Skip to content
This repository was archived by the owner on Feb 28, 2025. It is now read-only.

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Oct 18, 2023

  • In generator config, create files <operator>.tests.js to store a list of expected pipelines.

  • Use a Node script to convert the JS to PHP array

  • Add tests: item operators config Yaml files.

  • Generate a PHPUnit test class for each operator.

@GromNaN GromNaN requested review from jmikola and alcaeus October 18, 2023 20:08
"mongodb/builder": "@dev",
"mongodb/mongodb": "^1.17.0@dev",
"nette/php-generator": "^4",
"nikic/php-parser": "^4.17",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required to modify existing files with the generator.

/** @see getExpectedUsingTwoAaddFieldsStages */
public function testUsingTwoAaddFieldsStages(): void
{
$this->markTestSkipped('$sum must accept arrayFieldPath and render it as a single value: https://jira.mongodb.org/browse/PHPLIB-1287');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pipeline can't be implemented. Work on $sum operator is required. PHPLIB-1287

@GromNaN GromNaN changed the title PHPLIB-1271 Add tests from the documentation in JS PHPLIB-1271 Add tests from the documentation Oct 19, 2023
),
);

$expected = $this->getExpectedUseAccumulatorToImplementTheAvgOperator();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, only the get methods are generated and you still need to write the test yourself. Rather than embed generated methods within non-generated files, why not just generate JSON files which are then referenced from these tests?

Alternatively, you can generate actual PHP files that return the value to compare against. I suppose that could be either an array or MongoDB\BSON\PackedArray instance.


class PipelineTestCase extends TestCase
{
final public static function assertSamePipeline(string $expected, Pipeline $pipeline): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is the first use of this function. If we keep it as-is, renaming $expected to $expectedJson may be clearer. I do think it may be preferable to avoid JSON entirely and just generate PHP files that return a value that can be included here.

Those generated files could then have the appropriate "auto-generated" header and be ignored via .gitattributes.

Comment on lines +23 to +26
# Should be nested in $regex
$regularExpression:
pattern: '789$'
options: ''
Copy link
Member Author

@GromNaN GromNaN Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$regex is a special key. I can't use it for a regular object.

$json = <<<'JSON'
{
    "foo": {
        "$regex": {
            "$regularExpression": {
                "pattern": "789$",
                "options": "i"
            }
        }
    }
}
JSON;

$document = fromJSON($json);

// Fatal error: Uncaught MongoDB\Driver\Exception\UnexpectedValueException: Unexpected nested object value for "$regex" key 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That exception originates from _bson_json_read_start_map()

Also discussed in the Extended JSON spec:

If you're using $regex as a query operator, the "Parsers" section above says:

A parser that accepts Legacy Extended JSON MUST be configurable such that a JSON text of a MongoDB query filter containing the regex query operator can be parsed.

And then it proceeds to give two examples. The first example resembles what you're trying to do here, where the $regex value is the extended JSON representation of a regex object (using the $regularExpression key). I'm not sure why that's prohibited in libbson, but it may be worth asking the C team about.

That aside, is anything preventing you from using the following?

{ $regex: 'pattern', $options: '<options>' }

Copy link
Member Author

@GromNaN GromNaN Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That aside, is anything preventing you from using the following?
{ $regex: 'pattern', $options: '<options>' }

This will result in new MongoDB\BSON\Regex('pattern', '<options'>).

The codec wraps the regex in the $regex operator for consistency with other operators.

The expected query that I need to encode in Yaml/Json is:

(object) ['sku' => (object) ['$regex' => new MongoDB\BSON\Regex('789$', '')]]

Copy link
Member Author

@GromNaN GromNaN Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$query = (object) ['sku' => (object) ['$regex' => new MongoDB\BSON\Regex('789$', '')]];

$json = MongoDB\BSON\toJSON(MongoDB\BSON\fromPHP($x));

// { "sku" : { "$regex" : { "$regex" : "789$", "$options" : "" } } }

MongoDB\BSON\fromJSON($json);

// Warning: Uncaught MongoDB\Driver\Exception\UnexpectedValueException: Unexpected nested object value for "$regex" key in php shell code:1

Copy link
Member Author

@GromNaN GromNaN Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue solved by comparing JSON: #13

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM. No objection to using JSON for pipelines until we're able to read JSON pipelines into BSON without encountering parsing issues. 👍

@GromNaN GromNaN merged commit 49e266d into mongodb:0.1 Oct 31, 2023
@GromNaN GromNaN deleted the PHPLIB-1271 branch October 31, 2023 17:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants