Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix query preparation when in elemMatch #2299

Merged
merged 1 commit into from
Apr 13, 2021
Merged

Fix query preparation when in elemMatch #2299

merged 1 commit into from
Apr 13, 2021

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Mar 31, 2021

Q A
Type bug
BC Break no
Fixed issues Fixes #1674

Summary

As outlined previously, fixing the issue requires a little bit of work in the Expr class. This PR changes the logic to no longer evaluate Expr instances right away, but defer until they are built themselves. This allows us to look at the field mapping and update the ClassMetadata instance of Expr objects with the necessary information. Despite my previous suggestion to refactor the persister, I decided against it due to the complexity involved.

Note that this logic only applies to associations with a targetDocument. If no association of that kind exists, the behaviour is as before and the "parent" ClassMetadata instance is used.

There is a separate issue where you can't use references or includesReferenceTo on an Expr instance that will later be used. I'll fix that in a separate PR as that may have to wait until the next minor release due to API changes.

@alcaeus alcaeus added the Bug label Mar 31, 2021
@alcaeus alcaeus added this to the 2.2.1 milestone Mar 31, 2021
@alcaeus alcaeus requested a review from malarzm March 31, 2021 11:54
@alcaeus alcaeus self-assigned this Mar 31, 2021
@alcaeus alcaeus mentioned this pull request Mar 31, 2021
Closed
malarzm
malarzm approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

Good idea with deferring the conversion 👍

lib/Doctrine/ODM/MongoDB/Query/Expr.php
*
* @param Expr|mixed $expression
*/
private static function convertExpression($expression, ClassMetadata $classMetadata)
Copy link
Member

Choose a reason for hiding this comment

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

I might be tired but it's confusing we have a non-static convertExpressions and static convertExpression. Don't have any better idea or objections, just pointing this out :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it static to not be tempted to use $this->class for the conversion and instead relying on what the previous logic decided was the right class metadata instance.

lib/Doctrine/ODM/MongoDB/Query/Expr.php

$convertedQuery = [];
foreach ($query as $key => $value) {
if (is_string($key) && $classMetadata->hasAssociation($key)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can a mapped field be named 9 (for whatever reasons) or will it end as a string in the array anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, that's something PHP doesn't like at all anyways: https://3v4l.org/GoKLK. Reason for this is to reduce the performance impact of the logic so we don't have to check for associations with numeric indexes. Since PHP converts numeric strings to numbers I'm not sure what would happen, but I'm willing to take my chances on this and wait for someone's stuff to break.

@alcaeus alcaeus merged commit 05c9a25 into doctrine:2.2.x Apr 13, 2021
@alcaeus alcaeus deleted the gh-1674 branch April 13, 2021 08:51
@github-actions github-actions bot mentioned this pull request May 15, 2021
Merged
alcaeus added a commit that referenced this pull request May 21, 2021
@github-actions @buffcode
@malarzm @SenseException @alcaeus @ossinkine @rrajkomar @wuchen90
Merge release 2.2.1 into 2.3.x (#2305)
52c2c57 
* Fix locking when ClassMetadata is unserialized

Caching / unserializing ClassMetadata broke locking functionality

Fixes #2278

* Test serialization of lock/version fields

* Update working-with-objects.rst

Detach doc text from code block

* Update storage-strategies.rst

* Fix invalid strict comparison when validating mappings

* Correctly handle write concern specified in defaultCommitOptions (#2294)

* Fix documentation for uploadFromFile

* Fix mapping of the nullable option for XML driver

* Fix query preparation when in elemMatch (#2299)

* Fix preparation of $elemMatch operators in queries (#2298)

* Fix using null values in partial filter expressions (#2300)

* Fix errors with nullable typed associations (#2302)

* Fix initialising nullable associations

* Fix error when merging documents with uninitialised typed properties

Co-authored-by: buffcode <buffcode@users.noreply.github.com>
Co-authored-by: Laurens Stötzel <l.stoetzel@meeva.de>
Co-authored-by: Maciej Malarz <malarzm@gmail.com>
Co-authored-by: jeeiex <78592605+jeeiex@users.noreply.github.com>
Co-authored-by: Claudio Zizza <859964+SenseException@users.noreply.github.com>
Co-authored-by: Andreas Braun <git@alcaeus.org>
Co-authored-by: Gocha Ossinkine <ossinkine@ya.ru>
Co-authored-by: Ryan RAJKOMAR <rrajkomar@users.noreply.github.com>
Co-authored-by: wuchen90 <wu.chen@agriconomie.com>
Co-authored-by: Andreas Braun <alcaeus@users.noreply.github.com>
alcaeus added a commit to alcaeus/mongodb-odm that referenced this pull request Jun 29, 2021
@alcaeus
Merge branch '2.2.x' into 2.3.x
0c4407c 
* 2.2.x:
  Fix handling of upserts during scheduling for deletion (doctrine#2334)
  Fix wrong handling for nullable fields in upsert and update (doctrine#2318)
  [2.2] Fix builds (doctrine#2319)
  Allow mixed value in $not operator (doctrine#2307)
  Fix errors with nullable typed associations (doctrine#2302)
  Fix using null values in partial filter expressions (doctrine#2300)
  Fix preparation of $elemMatch operators in queries (doctrine#2298)
  Fix query preparation when in elemMatch (doctrine#2299)
  Fix mapping of the nullable option for XML driver
  Fix documentation for uploadFromFile
  Correctly handle write concern specified in defaultCommitOptions (doctrine#2294)
  Fix invalid strict comparison when validating mappings
  Update storage-strategies.rst
  Update working-with-objects.rst
  Test serialization of lock/version fields
  Fix locking when ClassMetadata is unserialized
alcaeus added a commit to alcaeus/mongodb-odm that referenced this pull request Jun 29, 2021
@alcaeus
Merge branch '2.2.x' into 2.3.x
7919638 
* 2.2.x:
  Fix handling of upserts during scheduling for deletion (doctrine#2334)
  Fix wrong handling for nullable fields in upsert and update (doctrine#2318)
  [2.2] Fix builds (doctrine#2319)
  Allow mixed value in $not operator (doctrine#2307)
  Fix errors with nullable typed associations (doctrine#2302)
  Fix using null values in partial filter expressions (doctrine#2300)
  Fix preparation of $elemMatch operators in queries (doctrine#2298)
  Fix query preparation when in elemMatch (doctrine#2299)
  Fix mapping of the nullable option for XML driver
  Fix documentation for uploadFromFile
  Correctly handle write concern specified in defaultCommitOptions (doctrine#2294)
  Fix invalid strict comparison when validating mappings
  Update storage-strategies.rst
  Update working-with-objects.rst
  Test serialization of lock/version fields
  Fix locking when ClassMetadata is unserialized
alcaeus added a commit to alcaeus/mongodb-odm that referenced this pull request Jun 29, 2021
@alcaeus
Merge branch '2.2.x' into 2.3.x
b6f3f32 
* 2.2.x:
  Fix handling of upserts during scheduling for deletion (doctrine#2334)
  Fix wrong handling for nullable fields in upsert and update (doctrine#2318)
  [2.2] Fix builds (doctrine#2319)
  Allow mixed value in $not operator (doctrine#2307)
  Fix errors with nullable typed associations (doctrine#2302)
  Fix using null values in partial filter expressions (doctrine#2300)
  Fix preparation of $elemMatch operators in queries (doctrine#2298)
  Fix query preparation when in elemMatch (doctrine#2299)
  Fix mapping of the nullable option for XML driver
  Fix documentation for uploadFromFile
  Correctly handle write concern specified in defaultCommitOptions (doctrine#2294)
  Fix invalid strict comparison when validating mappings
  Update storage-strategies.rst
  Update working-with-objects.rst
  Test serialization of lock/version fields
  Fix locking when ClassMetadata is unserialized
alcaeus added a commit to alcaeus/mongodb-odm that referenced this pull request Jun 29, 2021
@alcaeus
Merge branch '2.2.x' into 2.3.x
c186b46 
* 2.2.x:
  Fix handling of upserts during scheduling for deletion (doctrine#2334)
  Fix wrong handling for nullable fields in upsert and update (doctrine#2318)
  [2.2] Fix builds (doctrine#2319)
  Allow mixed value in $not operator (doctrine#2307)
  Fix errors with nullable typed associations (doctrine#2302)
  Fix using null values in partial filter expressions (doctrine#2300)
  Fix preparation of $elemMatch operators in queries (doctrine#2298)
  Fix query preparation when in elemMatch (doctrine#2299)
  Fix mapping of the nullable option for XML driver
  Fix documentation for uploadFromFile
  Correctly handle write concern specified in defaultCommitOptions (doctrine#2294)
  Fix invalid strict comparison when validating mappings
  Update storage-strategies.rst
  Update working-with-objects.rst
  Test serialization of lock/version fields
  Fix locking when ClassMetadata is unserialized
alcaeus added a commit to alcaeus/mongodb-odm that referenced this pull request Aug 2, 2021
@alcaeus
Merge branch '2.2.x' into merge-2.2.x-into-2.3.x
f013d11 
* 2.2.x:
  Fix wrong assertion (doctrine#2335)
  Fix handling of upserts during scheduling for deletion (doctrine#2334)
  Fix wrong handling for nullable fields in upsert and update (doctrine#2318)
  [2.2] Fix builds (doctrine#2319)
  Allow mixed value in $not operator (doctrine#2307)
  Fix errors with nullable typed associations (doctrine#2302)
  Fix using null values in partial filter expressions (doctrine#2300)
  Fix preparation of $elemMatch operators in queries (doctrine#2298)
  Fix query preparation when in elemMatch (doctrine#2299)
  Fix mapping of the nullable option for XML driver
  Fix documentation for uploadFromFile
  Correctly handle write concern specified in defaultCommitOptions (doctrine#2294)
  Fix invalid strict comparison when validating mappings
  Update storage-strategies.rst
  Update working-with-objects.rst
  Test serialization of lock/version fields
  Fix locking when ClassMetadata is unserialized
alcaeus added a commit that referenced this pull request Aug 5, 2021
@alcaeus @buffcode
@malarzm @SenseException @ossinkine @rrajkomar @wuchen90 @franmomu @webmozart
Merge 2.2.x into 2.3.x (#2350)
08b3799 
* Fix locking when ClassMetadata is unserialized

Caching / unserializing ClassMetadata broke locking functionality

Fixes #2278

* Test serialization of lock/version fields

* Update working-with-objects.rst

Detach doc text from code block

* Update storage-strategies.rst

* Fix invalid strict comparison when validating mappings

* Correctly handle write concern specified in defaultCommitOptions (#2294)

* Fix documentation for uploadFromFile

* Fix mapping of the nullable option for XML driver

* Fix query preparation when in elemMatch (#2299)

* Fix preparation of $elemMatch operators in queries (#2298)

* Fix using null values in partial filter expressions (#2300)

* Fix errors with nullable typed associations (#2302)

* Fix initialising nullable associations

* Fix error when merging documents with uninitialised typed properties

* Allow mixed value in $not operator (#2307)

* [2.2] Fix builds (#2319)

* Fix wrong handling for nullable fields in upsert and update (#2318)

* Comprehensively test nullable behaviour for embedOne

Co-authored-by: wuchen90 <wu.chen@agriconomie.com>

* Fix handling of nullable fields for upsert

Co-authored-by: wuchen90 <wu.chen@agriconomie.com>

* Fix handling of upserts during scheduling for deletion (#2334)

* Fix handling of upserts during scheduling for deletion

* Added test

* Fix wrong assertion (#2335)

This was uncovered by Psalm testing when merging 2.2.x up into 2.3.x.

* Remove psalm-baseline.xml

Co-authored-by: buffcode <buffcode@users.noreply.github.com>
Co-authored-by: Laurens Stötzel <l.stoetzel@meeva.de>
Co-authored-by: Maciej Malarz <malarzm@gmail.com>
Co-authored-by: jeeiex <78592605+jeeiex@users.noreply.github.com>
Co-authored-by: Claudio Zizza <859964+SenseException@users.noreply.github.com>
Co-authored-by: Gocha Ossinkine <ossinkine@ya.ru>
Co-authored-by: Ryan RAJKOMAR <rrajkomar@users.noreply.github.com>
Co-authored-by: wuchen90 <wu.chen@agriconomie.com>
Co-authored-by: Fran Moreno <franmomu@gmail.com>
Co-authored-by: Bernhard Schussek <bschussek@gmail.com>
alcaeus added a commit that referenced this pull request Aug 5, 2021
@alcaeus
Merge branch '2.2.x' into 2.2.x-merge-up-into-2.3.x_iA5R4YuE
81b0647 
* 2.2.x:
  Fix wrong assertion (#2335)
  Fix handling of upserts during scheduling for deletion (#2334)
  Fix wrong handling for nullable fields in upsert and update (#2318)
  [2.2] Fix builds (#2319)
  Allow mixed value in $not operator (#2307)
  Fix errors with nullable typed associations (#2302)
  Fix using null values in partial filter expressions (#2300)
  Fix preparation of $elemMatch operators in queries (#2298)
  Fix query preparation when in elemMatch (#2299)
  Fix mapping of the nullable option for XML driver
  Fix documentation for uploadFromFile
  Correctly handle write concern specified in defaultCommitOptions (#2294)
  Fix invalid strict comparison when validating mappings
  Update storage-strategies.rst
  Update working-with-objects.rst
  Test serialization of lock/version fields
  Fix locking when ClassMetadata is unserialized
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malarzm malarzm malarzm approved these changes

Assignees

@alcaeus alcaeus

Labels
Bug
Projects
None yet
Milestone
2.2.1
Development

Successfully merging this pull request may close these issues.

Wrong mapping of $id field.
2 participants
@alcaeus @malarzm