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.

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

doc: Review and test validation cookbook #2662

Merged
merged 4 commits into from
Jul 1, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jun 28, 2024

Q A
Type improvement
BC Break no
Fixed issues -

Comment on lines 86 to 95
Now validation is performed when you call ``DocumentManager#flush()`` and an
order is about to be inserted or updated. Any Exception that happens in the
lifecycle callbacks will stop the flush operation and the exception will be
propagated.

You might want to use the ``PrePersist`` instead of ``PreFlush`` to validate
the document sooner, when you call ``DocumentManager#persist()``. This way you
can catch validation errors earlier in your application flow. But be aware that
if the document is modified after the ``PrePersist`` event, the validation
might not be triggered again and an invalid document can be persisted.
Copy link
Member Author

@GromNaN GromNaN Jun 28, 2024

Choose a reason for hiding this comment

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

I replaced PrePersist and PreUpdate by PreFlush, because the previous events were not equivalent (one is on persist, the other on flush): using preFlush, the validation is always done in the same step. It also removes a vulnerability in the validation if the document is updated after being persisted.

@@ -181,44 +181,44 @@ the ``odm:schema:create`` or ``odm:schema:update`` command.
#[ODM\Document]
#[ODM\Validation(
validator: self::VALIDATOR,
action: ClassMetadata::SCHEMA_VALIDATION_ACTION_WARN,
level: ClassMetadata::SCHEMA_VALIDATION_LEVEL_MODERATE,
action: ClassMetadata::SCHEMA_VALIDATION_ACTION_ERROR,
Copy link
Member Author

Choose a reason for hiding this comment

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

Changing to throw an error. Reading the server logs is totally disconnected from the application code, a developer will prefer having an exception with the context and the stacktrace.

@@ -1229,7 +1229,7 @@ for the related collection.
)]
class SchemaValidated
{
public const VALIDATOR = <<<'EOT'
private const VALIDATOR = <<<'EOT'
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to use a public constant when it's read by the attribute on the class, that was required for the doctrine annotation.

*
* @see https://www.mongodb.com/docs/manual/core/schema-validation/handle-invalid-documents/#option-2--allow-invalid-documents--but-record-them-in-the-log
*/
public const SCHEMA_VALIDATION_ACTION_WARN = 'warn';
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 only updated comments to have a better tooltip in the IDE. The constant definition haven't been modified.

@GromNaN GromNaN marked this pull request as ready for review June 28, 2024 13:39

try {
$this->dm->persist($order1);
$this->dm->flush();
Copy link
Member Author

Choose a reason for hiding this comment

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

The phpdoc for DocumentManager::flush() is incorrect as any exception can be thrown by lifecycle events.

* @throws MongoDBException
*/
public function flush(array $options = [])

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 updated the @throws annotation of DocumentManager::flush.

@GromNaN GromNaN requested a review from alcaeus June 28, 2024 14:07
@@ -128,11 +131,8 @@ can register multiple methods for validation in "PrePersist" or
"PreUpdate" or mix and share them in any combinations between those
two events.

There is no limit to what you can and can't validate in
"PrePersist" and "PreUpdate" as long as you don't create new document
instances. This was already discussed in the previous blog post on
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that this cookbook was copied from a blog post.

docs/en/cookbook/validation-of-documents.rst Outdated Show resolved Hide resolved
docs/en/cookbook/validation-of-documents.rst Outdated Show resolved Hide resolved
docs/en/cookbook/validation-of-documents.rst Outdated Show resolved Hide resolved
@GromNaN GromNaN merged commit af0f6e3 into doctrine:2.9.x Jul 1, 2024
17 of 18 checks passed
@GromNaN GromNaN deleted the doc-validation branch July 1, 2024 08:41
@GromNaN GromNaN added this to the 2.9.0 milestone Jul 1, 2024
alcaeus added a commit that referenced this pull request Sep 6, 2024
* 2.9.x: (24 commits)
  Fix typo in code example (#2670)
  Label PRs about GH actions with "CI" (#2632)
  Review basic mapping (#2668)
  Fix wording (#2667)
  Add native type to private properties and final classes (#2666)
  Review and add tests on `ResolveTargetDocumentListener` (#2660)
  Remove soft-delete-cookbook (#2657)
  doc: Remove wakeup and clone cookbook (#2663)
  Modernize generated code for Hydrators (#2665)
  Add tests for introduction (#2664)
  doc: Review mapping ORM and ODM cookbook (#2658)
  doc: Review cookbook on blending ORM and ODM (#2656)
  doc: Review and test validation cookbook (#2662)
  Update custom mapping example (#2654)
  doc: Review Simple Search Engine Cookbook (#2659)
  doc: Add cookbook about embedding referenced documents using $lookup (#2655)
  doc: Add type to properties (#2652)
  doc: Review custom collections and repository docs (#2653)
  doc: Review Getting Started (#2650)
  Move annotations-reference to attributes-reference (#2651)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants