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 errors with nullable typed associations #2302

Merged
merged 2 commits into from
Apr 22, 2021
Merged

Fix errors with nullable typed associations #2302

merged 2 commits into from
Apr 22, 2021

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Apr 15, 2021

Q A
Type bug
BC Break no
Fixed issues Fixed #2301

Summary

This fixes errors when loading documents with uninitialised nullable association fields (embedOne and referenceOne were affected).

While fixing this issue, I uncovered an additional bug with public typed properties: when using $dm->merge with a document that has uninitialised public properties, the properties are accessed. This may be due to a bug in the reflection code, as the TypedNoDefaultValueReflectionProperty class is only used for non-public properties. However, I believe that our workaround is correct, as the class mentioned above returns null for uninitialised properties, while we actually don't want to do anything with the property if it wasn't initialised.

@alcaeus alcaeus added the Bug label Apr 15, 2021
@alcaeus alcaeus added this to the 2.2.1 milestone Apr 15, 2021
@alcaeus alcaeus requested a review from malarzm April 15, 2021 06:49
@alcaeus alcaeus self-assigned this Apr 15, 2021
@alcaeus alcaeus mentioned this pull request Apr 15, 2021
Closed
malarzm
malarzm reviewed Apr 15, 2021
View reviewed changes
tests/Documents74/TypedDocument.php
@@ -14,50 +14,35 @@
class TypedDocument
{
/** @ODM\Id() */
private string $id;
public string $id;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's a good idea to change everything to public due to logic in https://github.com/doctrine/persistence/blob/fdef2dd7da053bc97841fb45b866a7756bfe0f2f/lib/Doctrine/Persistence/Mapping/RuntimeReflectionService.php#L78-L82. On the other hand it's our reflection mechanism's responsibility to work with both public and private properties so I'm just pointing this out as a potential future problem.

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 uncovered the problem in merge, and if there's one advantage of types properties it's that you can drop accessors if they should be publicly writable and readable. We'll see what breaks and will have to fix that 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Not denying the value of the change itself, I was more wondering if separate PublicTypedDocument would be a good idea :)

@alcaeus alcaeus merged commit 5ca00af into doctrine:2.2.x Apr 22, 2021
@alcaeus alcaeus deleted the gh-2301 branch April 22, 2021 11:54
@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 <[email protected]>
Co-authored-by: Laurens Stötzel <[email protected]>
Co-authored-by: Maciej Malarz <[email protected]>
Co-authored-by: jeeiex <[email protected]>
Co-authored-by: Claudio Zizza <[email protected]>
Co-authored-by: Andreas Braun <[email protected]>
Co-authored-by: Gocha Ossinkine <[email protected]>
Co-authored-by: Ryan RAJKOMAR <[email protected]>
Co-authored-by: wuchen90 <[email protected]>
Co-authored-by: Andreas Braun <[email protected]>
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 <[email protected]>

* Fix handling of nullable fields for upsert

Co-authored-by: wuchen90 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Laurens Stötzel <[email protected]>
Co-authored-by: Maciej Malarz <[email protected]>
Co-authored-by: jeeiex <[email protected]>
Co-authored-by: Claudio Zizza <[email protected]>
Co-authored-by: Gocha Ossinkine <[email protected]>
Co-authored-by: Ryan RAJKOMAR <[email protected]>
Co-authored-by: wuchen90 <[email protected]>
Co-authored-by: Fran Moreno <[email protected]>
Co-authored-by: Bernhard Schussek <[email protected]>
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
@malarzm malarzm mentioned this pull request Aug 15, 2022
Closed
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.

Nullable embedded documents not initialized
2 participants
@alcaeus @malarzm