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

Correctly handle write concern specified in defaultCommitOptions #2294

Merged
merged 1 commit into from
Mar 10, 2021
Merged

Correctly handle write concern specified in defaultCommitOptions #2294

merged 1 commit into from
Mar 10, 2021

Conversation

ossinkine
Copy link
Contributor

@ossinkine ossinkine commented Mar 9, 2021 •
edited
Loading

Q A
Type bug
BC Break no
Fixed issues #2292

Summary

Fix passing w option to DocumentManager::flush or default commit options

  • Bugfix
  • Tests

@ossinkine ossinkine force-pushed the issue-2292 branch 4 times, most recently from 116f6ac to e389c7a Compare March 10, 2021 09:30
alcaeus
alcaeus reviewed Mar 10, 2021
View reviewed changes
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.

Thanks for tackling this. I've added some suggestions on how to improve the changes you've made. The tests look fine. I think we could profit from some more functional tests (e.g. insert a duplicate document with an unacknowledged write, run into a timeout waiting for the write to be acknowledged, etc.)

lib/Doctrine/ODM/MongoDB/Configuration.php Outdated Show resolved Hide resolved
lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -1493,10 +1498,17 @@ private function getWriteOptions(array $options = []): array
$defaultOptions = $this->dm->getConfiguration()->getDefaultCommitOptions();
$documentOptions = [];
if ($this->class->hasWriteConcern()) {
$documentOptions['w'] = $this->class->getWriteConcern();
$documentOptions['writeConcern'] = new WriteConcern($this->class->getWriteConcern());
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should cover us for now since the write concern option is documented as string|int|null. We'll have to refactor the metadata handling to no longer accept a w option alone but rather find a way to specify actual write concern objects in metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't get what you mean

Copy link
Member

Choose a reason for hiding this comment

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

You are able to specify a write concern in metadata:

@ODM\Document(writeConcern="majority")

However, there's more to a write concern than just the w, but it also includes a wtimeout option to specify how long you're willing to wait for the write concern to be achieved. At this time, it's not possible to specify this option, which means that we'll have to refactor this handling in general, at which point we'll no longer have to deal with w.

lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php Outdated Show resolved Hide resolved
@ossinkine
Copy link
Contributor Author

What about some more tests, undoubtedly it will be much better with them, but I'm not sure this bugfix requires them.

@alcaeus
Copy link
Member

alcaeus commented Mar 10, 2021

I believe we are good on tests for this change. I'll circle back to add more tests when the handling of write concern in metadata is fixed.

@alcaeus alcaeus added the Bug label Mar 10, 2021
@alcaeus alcaeus added this to the 2.2.1 milestone Mar 10, 2021
@alcaeus alcaeus merged commit 8069ded into doctrine:2.2.x Mar 10, 2021
@alcaeus alcaeus changed the title Fix "w" option Correctly handle write concern specified in defaultCommitOptions Mar 10, 2021
@alcaeus
Copy link
Member

alcaeus commented Mar 10, 2021

Thanks @ossinkine!

@alcaeus alcaeus mentioned this pull request Mar 10, 2021
Closed
@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

@Steveb-p Steveb-p Steveb-p left review comments

@alcaeus alcaeus alcaeus approved these changes

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
2.2.1
Development

Successfully merging this pull request may close these issues.
3 participants
@ossinkine @alcaeus @Steveb-p