Discovered while working on #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only. See #2930028-21: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only. Needs to be investigated.

The spec permits this: http://jsonapi.org/format/#crud-creating-client-ids

Let's discuss the pros/cons.

CommentFileSizeAuthor
#20 2934386-20.patch7.26 KBgabesullice
#20 interdiff-2934386-20.txt807 bytesgabesullice
#17 2934386-17.patch7.12 KBgabesullice
#17 interdiff-2934386-17.txt3.82 KBgabesullice
#15 2934386-15.patch6.49 KBgabesullice
#15 interdiff-2934386-15.txt5.9 KBgabesullice
#3 2934386-3.patch930 bytesgabesullice

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » gabesullice
gabesullice’s picture

gabesullice
Primary language English
commented
Title: Deserialization *and* POSTing entity both replace given UUID with random UUID » Deserialization does not preserve UUID
Status: Active » Needs review
StatusFileSize
new 2934386-3.patch930 bytes

Status: Needs review » Needs work

The last submitted patch, 3: 2934386-3.patch, failed testing. View results

gabesullice’s picture

gabesullice
Primary language English
commented
Title: Deserialization does not preserve UUID » Allow client-generated UUIDs
Category: Task » Feature request
Issue summary: View changes

Making this a feature request. It looks like this was an intentional omission.

Gabriel Sullice e0ipso: is there a reason we're _not_ keeping client side generated UUIDs?
2:02 PM I figured it was an accident, but failing test here makes me doubt that: https://www.drupal.org/project/jsonapi/issues/2934386#new

2:04 PM E gabesullice: UUIDs are the IDs for JSON API.
...
2:04 PM E just like we don't allow to create the serial entity ID by the client we do the same for uuid
2:04 PM moreover
2:05 PM using a generation tool reduces chances of collision to 1/trillions
2:05 PM but accepting something from clients increases that to 1/7
2:06 PM gabesullice: we could avoid that with more checks and more handholding, but I'm unsure about the value.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002)

Two things:

  1. There's still a valid use case: deserializing a JSON API representation of an entity should result in an Entity object whose values match those of A) the stored entity, B) the JSON API representation.
    Intentionally ignoring the provided ID/UUID when POSTing is independent of that! 
  2. 2:04 PM E just like we don't allow to create the serial entity ID by the client we do the same for uuid

    That's actually not true: see #2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002). I didn't know that either until somebody filed that issue and Entity API maintainer @tstoeckler spoke up!

gabesullice’s picture

gabesullice
Primary language English
commented

FWIW, not supporting client-generated UUIDs, we make offline use cases much more difficult.

I'm not sure I understand the 1/7 likelihood of collisions.

Checking for collisions seems like a reasonable price to pay to permit offline/asynchronous creation of entities or synchronization between sites which use UUIDs (like the Entity Share module).

gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: gabesullice » Unassigned
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Right, it'll be likely necessary for use cases with offline content creation and absolutely be a requirement for supporting Workflow Initiative-related use cases.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hello,

Thanks @gabesullice for pinging me.

As I am not sure if the "client-generated UUIDs" is in a GET or a POST request, I will recap the needs for the Entity Share module:

  • For Site A to be able to pull content from Site B: Site A get JSON API content by requesting SIte B. The unserialized version of an entity should keep the same UUID to be able to preserve it when saving it on Site A. So it can be updated later.
  • For Site A to be able to push content to Site B (not implemented yet, there are some blockers in JSONAPI): the UUID of an entity must be preserved when posting an entity data with an UUID.
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I'm not sure I understand the 1/7 likelihood of collisions.

LOL. Gabe, come on, you know me. That was a joke. It was a snarky way to say that I don't trust consumers to submit uuids without collisions.

My honest thoughts around this are:

There's still a valid use case: deserializing a JSON API representation of an entity should result in an Entity object whose values match those

Beach developer speaking: that's a formality that I don't think gives us anything in return, so … meh. Again, you can't provide an nid, but the deserialized entity will eventually get one. As usual, I don't think that the current state harms us (wrt the serialization formality), but the proposal either. So I'm not opposed to the change.

That's actually not true: see #2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002)

I am slightly horrified by that, but I admit that I won't discuss that there can be use cases for that.

Checking for collisions seems like a reasonable price to pay to permit

Well, that's hard to quantify. UUIDs will need to be checked against all entity types possible. Not the end of the world but I'm not dismissing the impact as easily.

we could avoid that with more checks and more handholding, but I'm unsure about the value.

Quoting myself here, mum would be proud. There are ample reasons here that make me go from unsure about the value to yeah, that's pretty useful.

TL;DR I agree with the majority, let's do this. I don't think this is necessarily high priority (it's a feature request), but when we get to it let's follow http://jsonapi.org/format/#crud-creating-client-ids to the letter.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Quoting myself here, mum would be proud.

:D

let's follow http://jsonapi.org/format/#crud-creating-client-ids to the letter.

+1

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Allow client-generated UUIDs » Accept client-generated IDs (UUIDs)

Retitling to match http://jsonapi.org/format/#crud-creating-client-ids's wording.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Priority: Normal » Major

Increasing priority on this one.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2934386-15.txt5.9 KB
new 2934386-15.patch6.49 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work
Issue tags: +API-First Initiative 
  1. +++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -313,6 +315,10 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
+      throw new AccessDeniedHttpException('IDs must by properly generated and formatted UUID as described in RFC 4122.');

    This doesn't seem like it should be a 403, but a 422?

    It also reads strangely. 

  2. +++ b/tests/src/Unit/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
@@ -120,7 +124,10 @@ class JsonApiDocumentTopLevelNormalizerTest extends UnitTestCase {
-        ['title' => 'dummy_title'],
+        [
+          'title' => 'dummy_title',
+          'uuid' => 'e1a613f6-f2b9-4e17-9d33-727eb6509d8b',
+        ],
       ],
       [
         [
@@ -131,6 +138,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends UnitTestCase {

@@ -131,6 +138,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends UnitTestCase {
           ],
         ],
         [
+          'uuid' => '0676d1bf-55b3-4bbc-9fbc-3df10f4599d5',
           'field_dummy' => [
           [
             'target_id' => 1,
@@ -160,6 +168,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends UnitTestCase {

@@ -160,6 +168,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends UnitTestCase {
           ],
         ],
         [
+          'uuid' => '535ba297-8d79-4fc1-b0d6-dc2f047765a1',
           'field_dummy' => [
           [
             'target_id' => 1,
@@ -193,6 +202,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends UnitTestCase {

@@ -193,6 +202,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends UnitTestCase {
           ],
         ],
         [
+          'uuid' => '535ba297-8d79-4fc1-b0d6-dc2f047765a1',
           'field_dummy' => [

    This is now sending a UUID field in every "denormalize" test case. Which means we're losing test coverage, because sending this is not required, it's optional.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2934386-17.txt3.82 KB
new 2934386-17.patch7.12 KB

This doesn't seem like it should be a 403, but a 422? It also reads strangely.

I agree, but to follow the spec "to the letter" (#10), that's what we have to respond with.

A server MUST return 403 Forbidden in response to an unsupported request to create a resource with a client-generated ID.

Fixed the wording :)

This is now sending a UUID field in every "denormalize" test case.

That's not quite accurate. The input data has always had UUIDs under $input['data']['id'] in every case. These additions are just asserting that, yes, indeed those input IDs are being properly used as the uuid value before denormalizing into an actual entity.

Regardless, you're right, we should be testing both cases and I've added coverage for it to the testDenormalizeUuid method.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

  • I agree, but to follow the spec "to the letter" (#10), that's what we have to respond with.

    Oh! That seems like a bug in the spec, but oh well. Can we then add a comment like:

    // This should be a 422 response, but the JSON API spec dictates that this should be a 403 response. We follow the JSON API spec.

  • +++ b/tests/src/Unit/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
@@ -193,18 +198,70 @@ class JsonApiDocumentTopLevelNormalizerTest extends UnitTestCase {
+    $data['data'] = (isset($id)) ?
+      ['type' => 'node--article', 'id' => $id] :
+      ['type' => 'node--article'];

    I personally prefer

    $foo = $condition
  ? $first
  : $second;

    I find that more legible. Learned it from a fellow core developer, don't know who :)

    Just leave this, it's an insane nit, and it's not defined in the coding standards. Just wondering if you simply prefer what you wrote, or were unaware of this alternative? :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Needs work

Oops, NW for that first nit, to help our future selves.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2934386-20.txt807 bytes
new 2934386-20.patch7.26 KB

Added the comment because it is an easy thing to get tripped up on.

As for the spec bug, I really think you can debate it either way. It could easily be that trusted clients are permitted to generate UUIDs while others are not, which would make the 403 correct. And it's not "semantically" incorrect to provide a UUID in those cases.

I really do prefer it my way :P yours feels like putting an opening curly brace on its own line (/me shudders) :P It's says, this statement is not over, it's not missing a semicolon. But this is definitely a bike shed situation.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

But this is definitely a bike shed situation.

🚲 🎨 💥

  • Wim Leers committed a1f1eeb on 8.x-1.x authored by gabesullice 
    Issue #2934386 by gabesullice, Wim Leers, e0ipso, Grimreaper: Accept...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

I felt safe committing this, because @e0ipso wrote:

TL;DR I agree with the majority, let's do this. I don't think this is necessarily high priority (it's a feature request), but when we get to it let's follow http://jsonapi.org/format/#crud-creating-client-ids to the letter.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

The regression in core in this same area was also fixed ~2 weeks ago btw: #2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002) :)

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

WRT to the ternary operator, I favor the question mark at the beginning. That's how linters want it in node.js land so I've grown to like it.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2942243: Follow-up for #2934386: server-generated IDs for newly POSTed entities should not be exposed in 403 responses

Follow-up created: #2942243: Follow-up for #2934386: server-generated IDs for newly POSTed entities should not be exposed in 403 responses.

  • Wim Leers committed b9909c9 on 8.x-1.x 
    Issue #2942243 by Wim Leers, gabesullice: Follow-up for #2934386: server...

  • Wim Leers committed 6ac575a on 8.x-1.x 
    Issue #2942243 by Wim Leers, gabesullice: Follow-up for #2934386: server...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.