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

[Serializer] Fix and improve constraintViolationListNormalizer's RFC7807 compliance #27292

Merged
merged 1 commit into from May 21, 2018

Conversation

@dunglas
Member

dunglas commented May 17, 2018

Q A
Branch? 4.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? yes
Fixed tickets #22150 (comment)
License MIT
Doc PR symfony/symfony-docs#9855

This PR fixes and improves RFC 7807 compliance of ConstraintViolationListNormalizer (introduced in 4.1):

  • As recommended, use a specific namespace for Symfony validation error (http://symfony.com/doc/current/validation.html, because it already exists and gives information about the error.
  • Allow to set all properties defined in the RFC using the serialization context
  • Remove the detail key if no detail is provided (according to the spec)
  • Change the Symfony specific extension to use the same terminology than the RFC itself (type and title)
  • Use the proper urn:uuid scheme (RFC 4122) for the UUID code (more standard, and improve hypermedia capabilities).

ping @teohhanhui

@dunglas dunglas added this to the 4.1 milestone May 17, 2018

@dunglas dunglas requested a review from lyrixx May 17, 2018

'detail' => $messages ? implode("\n", $messages) : '',
'violations' => $violations,
$result = array(
'type' => $context['type'] ?? 'https://symfony.com/doc/current/validation.html',

This comment has been minimized.

@fabpot

fabpot May 17, 2018

Member

I'd prefer a more generic URL (that could redirect to that page if we want to). Having a redirection also means we would be able to change the target very easily without breaking anything.

This comment has been minimized.

@dunglas

dunglas May 17, 2018

Member

https://symfony.com/validation-error?

This comment has been minimized.

@dunglas

dunglas May 17, 2018

Member

(Actually it's like a XML namespace, it doesn't even have to be dereferencable, but it's better from a DX POV)

This comment has been minimized.

@fabpot

fabpot May 18, 2018

Member

Let's use https://symfony.com/validation-error

This comment has been minimized.

@dunglas
@fabpot

fabpot approved these changes May 18, 2018

@@ -34,19 +34,29 @@ public function normalize($object, $format = null, array $context = array())
foreach ($object as $violation) {
$violations[] = array(
'propertyPath' => $violation->getPropertyPath(),
'message' => $violation->getMessage(),
'code' => $violation->getCode(),
'type' => sprintf('urn:uuid:%s', $violation->getCode()),

This comment has been minimized.

@teohhanhui

teohhanhui May 18, 2018

Contributor

I don't think we should change it from "message" and "code" here, especially because:

  "title" (string) - ... It SHOULD NOT change from occurrence to occurrence of the
  problem, except for purposes of localization (e.g., using
  proactive content negotiation

This comment has been minimized.

@dunglas

dunglas May 18, 2018

Member

It's ok. For the same problem, the same message and the same code will always be the same. They share the same semantic at a different level (for a given constraint violation, the code is always the same).

(Btw, the spec applies only to the root keys, not to our extension).

This comment has been minimized.

@teohhanhui

teohhanhui May 18, 2018

Contributor

For the same problem, the same message and the same code will always be the same.

The message is customizable, so it might not be the same.

(Btw, the spec applies only to the root keys, not to our extension).

Yes, that's also why I don't see why we should try to mimic the spec here... It only creates more confusion.

This comment has been minimized.

@dunglas

dunglas May 18, 2018

Member

The message is customizable, so it might not be the same.

The good practice will be to change the code if you change the message. It's not very easy right now, but we can improve this in the validator later.

Anyway it's just a SHOULD NOT:

This phrase, or the phrase "NOT RECOMMENDED" mean that
there may exist valid reasons in particular circumstances when the
particular behavior is acceptable or even useful, but the full
implications should be understood and the case carefully weighed
before implementing any behavior described with this label.

It's up to the developper to change the UUID if it uses this normalizer and change the default message and want to follow the spec's best practice (not very common).

Yes, that's also why I don't see why we should try to mimic the spec here... It only creates more confusion.

Consistency. It's very confusing to use different terms for the same usage.

This comment has been minimized.

@teohhanhui

teohhanhui May 18, 2018

Contributor

Consistency. It's very confusing to use different terms for the same usage.

It's not the same usage... We don't have to think of it like that.

This comment has been minimized.

@teohhanhui

teohhanhui May 18, 2018

Contributor

In the context of constraint violations, it's very obvious what code and message means. It's instantly familiar to Symfony users. Adopting the RFC 7807 semantics for this makes no sense.

@sroze

sroze approved these changes May 18, 2018

Lucky it has been introduced in 4.1 so BC doesn't count here.

@@ -34,19 +34,29 @@ public function normalize($object, $format = null, array $context = array())
foreach ($object as $violation) {
$violations[] = array(
'propertyPath' => $violation->getPropertyPath(),

This comment has been minimized.

@deguif

deguif May 18, 2018

Contributor

A $propertyPath variable is declared just after, this could probably be used here after moving the declaration up.

This comment has been minimized.

@dunglas

dunglas May 21, 2018

Member

good catch, done

@fabpot

This comment has been minimized.

Member

fabpot commented May 21, 2018

Last call before beta 2. AFAIU, we should either come up with something here and merge or revert the PR that introduces this. Please advise.

'detail' => $messages ? implode("\n", $messages) : '',
'violations' => $violations,
$result = array(
'type' => $context['type'] ?? 'https://symfony.com/validation-error',

This comment has been minimized.

@javiereguiluz

javiereguiluz May 21, 2018

Member

Just asking: should we make this URL future-proof? Will in the future be other errors? For example: serializer errors. If that's the case, maybe we can change it to https://symfony.com/error/validation (and https://symfony.com/error/serialization, etc.)

This comment has been minimized.

@dunglas

dunglas May 21, 2018

Member

Good idea, done.

@@ -34,19 +34,29 @@ public function normalize($object, $format = null, array $context = array())
foreach ($object as $violation) {
$violations[] = array(
'propertyPath' => $violation->getPropertyPath(),
'message' => $violation->getMessage(),
'code' => $violation->getCode(),
'type' => sprintf('urn:uuid:%s', $violation->getCode()),

This comment has been minimized.

@ogizanagi

ogizanagi May 21, 2018

Member

Symfony\Component\Validator\ConstraintViolationInterface::getCode() is nullable, so this format might currently result into urn:uuid:. Does the RFC 4122 states anything about such cases? Just set null instead?

This comment has been minimized.

@dunglas

dunglas May 21, 2018

Member

This part isn't covered by the RFC, I just try to mimic it (for consistency) in this extension. I've updated the code to not include the type key if no UUID is provided.

@dunglas

This comment has been minimized.

Member

dunglas commented May 21, 2018

@fabpot looks good enough to me to be merged with the last changes.

@fabpot

This comment has been minimized.

Member

fabpot commented May 21, 2018

Thank you @dunglas.

@fabpot fabpot merged commit 3c789c6 into symfony:4.1 May 21, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 21, 2018

bug #27292 [Serializer] Fix and improve constraintViolationListNormal…
…izer's RFC7807 compliance (dunglas)

This PR was squashed before being merged into the 4.1 branch (closes #27292).

Discussion
----------

[Serializer] Fix and improve constraintViolationListNormalizer's RFC7807 compliance

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #22150 (comment)
| License       | MIT
| Doc PR        | todo

This PR fixes and improves [RFC 7807](https://tools.ietf.org/html/rfc7807#section-3.2) compliance of `ConstraintViolationListNormalizer` (introduced in 4.1):

* As recommended, use a specific namespace for Symfony validation error (`http://symfony.com/doc/current/validation.html`, because it already exists and gives information about the error.
* Allow to set all properties defined in the RFC using the serialization context
* Remove the `detail` key if no detail is provided (according to the spec)
* Change the Symfony specific extension to use the same terminology than the RFC itself (type and title)
* Use the proper `urn:uuid` scheme (RFC 4122) for the UUID code (more standard, and improve hypermedia capabilities).

ping @teohhanhui

Commits
-------

3c789c6 [Serializer] Fix and improve constraintViolationListNormalizer's RFC7807 compliance

@dunglas dunglas deleted the dunglas:rfc7807 branch May 21, 2018

@fabpot fabpot referenced this pull request May 26, 2018

Merged

Release v4.1.0-BETA3 #27390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment