Field properties are not being denormalized

This hard-blocks #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands, which in turn hard blocks #2931785: The path for JSON API to core.

Before I really start digging into this, I'd like @e0ipso's thoughts and explicit +1. I don't want to sink hours into this to then get a veto. Attached is a very rough initial patch to show where changes would need to be made at minimum.

What this blocks and its relation to existing core issues

I started working on #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands again, and this is a blocker for that.

In parallel with this issue, a similar issue was filed against the serialization.module component in Drupal core: #2957385: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method. That is basically about the exact same problem. (It was discovered independently, in #2926507-36: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem.)

Of course, if this is an existing bug in Drupal core … then arguably this issue should not be in the the path of blocking JSON API getting into core. This issue blocks #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands (and was discovered there), which in turn blocks/is required by #2931785: The path for JSON API to core, which is the issue that lists all must-haves before JSON API can go into core.

Nuance

There is a nuanced difference between how Drupal core is impacted and how JSON API is impacted though.

Drupal core has a history/tradition of creating @FieldType-level normalizers. Those that do exist will not go away. A notable example is TimestampItemNormalizer, which #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API is deprecating in favor of the @DataType-level TimestampNormalizer.
It is thanks to the fact that TimestampItemNormalizer does still exist for Drupal core that this is less of a problem for Drupal core: we can't remove TimestampItemNormalizer due to BC reasons: some developers may have code extending this class or customizing it.

But in JSON API, we do not have a @FieldType-level equivalent. In fact, since #2933343: Define, document and mark scope of PHP and HTTP API, jsonapi.api.php explicitly documents that JSON API does NOT support @FieldType-level normalizers! Hence #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands is hard-blocked on this issue: because the work-around that core uses is not available in JSON API! (And core only has this work-around through sheer accident.)

Possible solutions

Based on the analysis above, I see only two ways forward:

1. Expand JSON API's API surface: allow @FieldType-level normalizers. This has enormous long-term maintainability consequences, including API-First ecosystem consequences. (To have a new field type/data type work in JSON API and REST as expected, you'd have to implement different normalizers for each! At least twice the work!
2. Fix this issue, without waiting for core to fix it in #2957385: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method first, even though that is in principle the right thing to do. Note that this will not be the first time that JSON API leads the way. Core REST has already had multiple things ported from JSON API where JSON API led the way. #2938035: When PATCHing a field is disallowed, no reason is given for *why* this happens is a good example.)

Conclusion & next steps

Solution #1 would be a big step backwards. Therefore, I'm working on solution #2. Stay tuned for a patch.

First, let's add that test coverage. (This patch is not relative to #3.)

The tests should FAIL.

Drupal\Tests\jsonapi\Functional\ExternalNormalizersTest failed as expected in #7, great!

The next step is to actually do what the IS describes:

1. Fix \Drupal\jsonapi\Normalizer\FieldItemNormalizer::denormalize()

So, in this patch (relative to #3+#7):

1. the received entity is still denormalized by EntityNormalizer, and that class still ends up doing a Entity::create(…) call with an array of data
2. but EntityNormalizer now denormalizes the received field value into the field item list class for that particular field type (yet still expects an array so that the above still works)
3. and FieldNormalizer now denormalizes the received field value per item, into the field item class for that particular field type (yet still expects an array so that the above still works)
4. and FieldItemNormalizer now denormalizes the received field item value per property (yet still expects an array so that the above still works)

IOW: we still end up doing

Entity::create(['field_name' => [$delta => [$property_name => $property_value, $other_property_name =>
 …]]])

, but at least now each of those levels can choose to have its own denormalization logic. This unblocks #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands. It'd be nicer to actually denormalize into those various objects, but … core's serialization module doesn't do that either; that's a nice-to-have follow-up for the future (@todos in place).

Ahhh, lots of test failures now. Clearly, I missed something! But what?

Turns out that any entity type that uses bundles is currently failing, because those bundles are actually entity reference fields, yet \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer::denormalize() was designed only for denormalizing relationships (for updating relationships).

This denormalization logic was introduced in #2738117: [FEATURE] Add support for POST in relationships, and is specifically designed for the /jsonapi/…/relationships/{related} endpoint, which does:

    $relationship_route->addDefaults(['serialization_class' => EntityReferenceFieldItemList::class]);

Because JSON API has been denormalizing entities by passing all the received field values simply as-is to Entity::create([…]), it's easy to see how \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer::denormalize() was only ever called/exercised by that relationship route! And that in turn explains this weird new error: EntityReferenceFieldNormalizer::denormalize() is tightly coupled to \Drupal\jsonapi\Controller\EntityResource::createRelationship().

Making all this clean and elegant is out-of-scope. We should make the minimal amount of changes necessary to still allow the relationship route to denormalize received data, while also allowing "normal" entity denormalization to work correctly: entity types with bundles should be able to denormalize their bundle entity references.

Turns out that's quite simple:

1. Update the route definition to specify Relationship::class as the serialization class rather than EntityReferenceFieldItemList::class.
2. Move all denormalization logic from JSON API's EntityReferenceFieldNormalizer to RelationshipNormalizer.

(Which means that EntityReferenceFieldNormalizer reuses/falls back to FieldNormalizer's denormalization logic, which is fine!)

In other words: this fixes unforeseen consequences of #2738117: [FEATURE] Add support for POST in relationships's less-than-tidy-but-good-enough-at-the-time implementation.

+++ b/src/Normalizer/EntityNormalizer.php
@@ -218,7 +238,26 @@ class EntityNormalizer extends NormalizerBase implements DenormalizerInterface {
+      if (!isset($field_map[$internal_name]) || !in_array($resource_type->getBundle(), $field_map[$internal_name]['bundles'], TRUE)) {
+        throw new UnprocessableEntityHttpException(sprintf(
+          'The attribute %s does not exist on the %s resource type.',
+          $internal_name,
+          $resource_type->getTypeName()
+        ));
+      }

In doing this, the changes that #2946746: Unhandled exceptions/fatal errors when POST/PATCH documents contain unknown field names made to EntityResource::createIndividual() and EntityResource::patchIndividual() can be massively simplified. And in fact, they need to be, because HEAD's denormalization logic happily accepts values for non-existing fields, but as of this change that's no longer true (because we look up the various field item (list) classes, and denormalize into them).

1. JsonApiDocumentTopLevelNormalizerTest::testDenormalizeUuid() failed, because entity denormalization no longer accepts values for non-existing field blindly, only to discard them. Turns out that the test coverage had a small bug all this time! Strictness FTW, because it makes for fewer edge cases :)
2. Drupal\Tests\jsonapi\Unit\Routing\RoutesTest failed, because #11 hadn't updated the route definition expectations yet.
3. EntityReferenceFieldNormalizerTest failed, because I failed to notice in #11 that 100% of the test coverage was for denormalization. So, renamed this to RelationshipFieldNormalizerTest
4. ConfigEntityNormalizerTest failed because it talked to the container directly (using \Drupal::service()). I still had to update its constructor.

With all that done, this should now be green!

Typo. 😅

Fixed CS violations.

Over at #2929932-19: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands, I wrote:

This actually belongs in 8.x-2.x, because it'll change the normalization of timestamp fields (e.g. the created and changed fields on Node entities). When core fixed this in #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, there was an update path. But because JSON API never complied with that, and hence never respected that BC flag, it's not reasonable to suddenly start supporting it, a year after core shipped with it.

Unfortunately, the same basic truth applies here too. If JSON API 8.x-1.x gets this committed, then once Drupal 8.6 is released with #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API, JSON API 8.x-1.x on Drupal 8.6.x will have a different normalization. This will be perceived as a BC break by JSON API users.

Therefore, even though this is a critical bugfix, it should only happen in the 8.x-2.x branch, to minimize disruption. All users will soon be encouraged to update to 8.x-2.x, but those on 8.x-1.x should be able to continue to use it they choose to do so.

#2942561: Assert denormalizing the JSON API response results in the expected object just landed. Reuploading for retesting. Based on the unexpected fails in #2929932-24: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands, it seems that that the test coverage that #2942561 added only works for content entities as of this patch.

That failed, as predicted.

This should solve it.

Yay! That passed as expected 🎉

IMHO this is RTBC.

This is still blocking #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands, which is otherwise RTBC.

Rebased #26.

#27:

1. ✔️👍
2. Will do this next.
3. Correct!
4. ✔️ No; plenty of our integration tests have single-cardinality fields, so this is IMHO already being tested sufficiently.
5. Will do this next.
6. ✔️
7. ✔️👍
8. ✔️🦅👁
9. ✔️🦅👁
10. ✔️ Wasn't necessary; was mostly to help myself understand what methods I had available :) So yep, development aid! Removed.
11. IDK!

Addressed everything except points 2 and 5. All other points were super trivial.

#2934149: [>=8.5] JSON API routes not specifying _content_type_format route requirement, resulting in bad DX is what triggered that new failure.

Addressed points 2 and 5, by stepping through them with a debugger. Conclusions:

2. Actually, createFieldItemList() requires the target entity. Which we don't have in this case. So this needs to remain the same.

5. Same, but the target field item list.

(Also fixed CS violations.)