JSON API's normalization of config entities' "dependencies" key-value pair omits essential information when there's only a single dependency [#2942979]

Problem/Motivation

Discovered while working on #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.

Quoting #2930028-55: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only:

While working on the Block test coverage, I found a significant bug in the normalization of config entities. #2733097: [FEATURE] Add GET support for configuration entities added config entity GET support, but it did not add any integration test coverage. The only test coverage is \Drupal\Tests\jsonapi\Kernel\Normalizer\JsonApiDocumentTopLevelNormalizerTest::testNormalizeConfig(), but that didn't verify that a config entity's dependencies key is exposed correctly.

This is what we'd expect:
…
          'dependencies' => [
            'theme' => [
              'classy',
            ],
          ],
…
but in reality we get:
…
          'dependencies' => [
            'classy',
          ],
…
Obviously that's losing very important information!

\Drupal\jsonapi\Normalizer\ConfigEntityNormalizer::serializeField() does:

    $output = new FieldNormalizerValue(
      [new FieldItemNormalizerValue($field)],
      1
    );

That 1 means "single cardinality" … which during rasterization results in \Drupal\jsonapi\Normalizer\Value\FieldNormalizerValue::rasterizeValue() being called and this code executing:

    if ($this->cardinality == 1) {
      assert(count($this->values) === 1);
      return $this->values[0] instanceof FieldItemNormalizerValue
        ? $this->values[0]->rasterizeValue() : NULL;
    }

… which then results in only the first value being retained, and this explains the data loss shown above.

Proposed resolution

Fix this in ConfigEntityNormalizer. Details TBD.

Remaining tasks

TBD

User interface changes

None.

API changes

None. (Well, responses for config entities change, but it was impossible until this very issue to use anything under the dependencies key due to this bug!)

Data model changes

None.

Comment	File	Size	Author
#15	2942979-15.patch	25.23 KB	wim leers
#15	interdiff-12-15.txt	1.93 KB	wim leers
#12	2942979-12.patch	24.62 KB	wim leers
#12	interdiff-11-12.txt	2.09 KB	wim leers
#11	2942979-11-tests_only_FAIL.patch	22.58 KB	wim leers

Comments

Comment #1

7 February 2018 at 18:03

Wim Leers created an issue. See original summary.

Comment #2

wim leers

Ghent 🇧🇪🇪🇺

commented 7 February 2018 at 18:04

Issue summary:

View changes

Comment #3

e0ipso

Catalan

Can Picafort

commented 9 February 2018 at 06:11

Wow! That was a good catch. We should add this issue to our current focus points.

Comment #4

wim leers

Ghent 🇧🇪🇪🇺

commented 15 February 2018 at 13:40

Ran into this again in #2932031-24: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases and #2932031-28: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases.

I wrote

The fix is to not just check whether there's >1 value in the array, but whether it's a numerically indexed array or an associatively indexed array. In the latter case, indexes/keys have meaning, and shouldn't be dropped!

But @gabesullice correctly pointed out that that doesn't work:

I think that's still a little too naive and will break some lauded JSON API DX improvements. We want to check (1) the cardinality of the field and (2) whether its data definition is a sequence or a map.
A sequence w/ cardinality = 1 -> strip wrapper
A sequence w/ cardinality > 1 -> keep wrapper
A map                         -> keep wrapper

To which I replied:

Except there are no data definitions for config entity types.

Which brings me to what I think we actually need to do: special case config entities and just make it output exactly what is stored, no attempts to simplify the data structure can be made.

Comment #5

gabesullice

English

commented 15 February 2018 at 14:19

Which brings me to what I think we actually need to do: special case config entities and just make it output exactly what is stored, no attempts to simplify the data structure can be made.

Edit: Actually, don't they have data definitions? I thought that they did have definitions when derived from a value, you just can't get them out of thin air.

I.e.

$definition = $config_entity->getTypedData()->getDefinition(); // works
$definition = DataDefinition::createFromDataType($config_entity->getTypedData()->getDataType()); // would not work (because it would lose its context)

^{I have not confirmed this fuzzy memory.}

Edit #2: I'm still fine just making config an exception as suggested, but am curious what @e0ipso thinks if my memory is accurate

Comment #6

e0ipso

Catalan

Can Picafort

commented 15 February 2018 at 14:37

Although we can probably derive the desired information based on the config schema, I think that

[…] special case config entities and just make it output exactly what is stored

is a good compromise. Mainly because config entities are as close to normalized data as one can get (that's why we were using ->toArray() to normalize them). We need to make sure to not apply any further processing (like the IS proves).

With regards to #5 that is probably a good enhancement to ensure that ill-validated config entities stored in Drupal don't provide bad output. Maybe a follow up for this?

Thoughts?

PS: We should add this to the Core issue MVP if it's not already.

Comment #7

wim leers

Ghent 🇧🇪🇪🇺

commented 15 February 2018 at 14:57

@gabesullice: You may be right, but if those exist, they're definitely not very precise; they'd be super generic. Actually useful data definitions would only exist if they'd be generated based on the config schema.

@e0ipso

Mainly because config entities are as close to normalized data as one can get

Exactly.

With regards to #5 that is probably a good enhancement to ensure that ill-validated config entities stored in Drupal don't provide bad output. Maybe a follow up for this?

Stored config is NEVER allowed to be invalid. We don't need to do validation when normalizing. We do need to validate when denormalizing. And that doesn't exist yet…

Comment #8

e0ipso

Catalan

Can Picafort

commented 15 February 2018 at 15:05

We do need to validate when denormalizing. And that doesn't exist yet…

Well for Core inclusion we don't need to worry about that. We already agreed that we won't be supporting unsafe methods until validation is in place, right?

Comment #9

wim leers

Ghent 🇧🇪🇪🇺

commented 15 February 2018 at 15:31

Exactly :)

Comment #10

wim leers

Ghent 🇧🇪🇪🇺

commented 16 February 2018 at 14:44

Assigned:

Unassigned

» wim leers

#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases landed, this can now move forward!

Comment #11

wim leers

Ghent 🇧🇪🇪🇺

commented 16 February 2018 at 15:30

Status:

Active

» Needs review

Status	File	Size
new	2942979-11-tests_only_FAIL.patch	22.58 KB

Let's first get some test coverage :) This should fail.

Comment #12

wim leers

Ghent 🇧🇪🇪🇺

commented 16 February 2018 at 15:32

Status	File	Size
new	interdiff-11-12.txt	2.09 KB
new	2942979-12.patch	24.62 KB

1 file was hidden/shown/deleted

Status	File	Size
hidden	2942979-11-tests_only_FAIL.patch	22.58 KB

I think that's still a little too naive and will break some lauded JSON API DX improvements. We want to check (1) the cardinality of the field and (2) whether its data definition is a sequence or a map.
A sequence w/ cardinality = 1 -> strip wrapper
A sequence w/ cardinality > 1 -> keep wrapper
A map                         -> keep wrapper

Actually, turns out this is not happening for config entities today anyway. In fact, because JSON API's field normalization architecture is centered on "content entity fields" and JSON API considers the data in config entities to also be "fields" (they aren't!), JSOn API's field normalization architecture is actually what's getting in the way!

For config entities, we don't need ANY post-processing!

In fact, the entire FieldNormalizerValue + FieldItemNormalizerValue value objects to be normalized are exactly what gets in the way for config entities! Because content entities have fields, and fields have items. Those items are keyed by deltas, which are always numerical. In other words: items are always a list with a certain sequence. IOW: the second level always has a numerical key.
But for config entities, JSON API treats their data as "fields", but it's possible that that second level has non-numerical keys. The FieldNormalizerValue + FieldItemNormalizerValue value objects do not allow this to be communicated, and this hence gets lost 100% of the time.

(I really tried to make it work and then ended up with that conclusion. I should've seen that right away.)

The only way to make it work is to modify \Drupal\jsonapi\Normalizer\Value\FieldItemNormalizerValue::rasterizeValue. But looking at \Drupal\Core\Entity\Entity, \Drupal\Core\Config\Entity\ConfigEntityInterface and \Drupal\Core\Config\Entity\ConfigEntityBase, there's zero mentions of "fields".

~~Therefore I think the best solution is to just massively simplify ConfigEntityNormalizer, and make it as dumb and simple as \Drupal\serialization\Normalizer\ConfigEntityNormalizer.~~ Doing this also requires a new ConfigEntityNormalizerValue class, because the existing EntityNormalizerValue assumes that it contains FieldNormalizerValue objects.

So perhaps a better solution is to continue to map config entity's data onto "fields" after all, but to introduce a new \Drupal\jsonapi\Normalizer\Value\FieldNormalizerValueInterface implementation (to override FieldItemNormalizerValue::rasterizeValue() as mentioned before, and to also modify the constructor).

This approach turned out to be the easiest solution, but not the simplest. (Although it's definitely the simplest patch.) There's a lot of complexity and indirection wrt how config entities are normalized by JSON API … and most of that is arguably unnecessary and definitely unused. However, potentially rearchitecting that is out of scope for this issue.

Comment #13

16 February 2018 at 15:34

The last submitted patch, 11: 2942979-11-tests_only_FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Comment #14

16 February 2018 at 15:35

Status:

Needs review

» Needs work

The last submitted patch, 12: 2942979-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Comment #15

wim leers

Ghent 🇧🇪🇪🇺

commented 16 February 2018 at 15:40

Assigned:	wim leers	» Unassigned
Status:	Needs work	» Needs review

Status	File	Size
new	interdiff-12-15.txt	1.93 KB
new	2942979-15.patch	25.23 KB

1 file was hidden/shown/deleted

Status	File	Size
hidden	2942979-12.patch	24.62 KB

The ConfigEntityNormalizerTest unit test failed, but it's wrong… in fact, it was documenting exactly what the bug was! Fixed.

Also fixed all coding standards violations in the patch.

Comment #16

wim leers

Ghent 🇧🇪🇪🇺

commented 16 February 2018 at 15:41

+++ b/tests/src/Functional/ActionTest.php
@@ -84,13 +84,9 @@ class ActionTest extends ResourceTestBase {
-          // @todo Remove the first 3 lines in favor of the 5 commented lines in https://www.drupal.org/project/jsonapi/issues/2942979
-          // @codingStandardsIgnoreStart
-          'configuration' => 'anonymous',
-//          'configuration' => [
-//            'rid' => 'anonymous',
-//          ],
-          // @codingStandardsIgnoreEnd
+          'configuration' => [
+            'rid' => 'anonymous',
+          ],

+++ b/tests/src/Functional/EntityViewDisplayTest.php
@@ -91,15 +91,10 @@ class EntityViewDisplayTest extends ResourceTestBase {
           'content' => [
-            // @todo Remove the first 2 lines in favor of the 4 commented lines in https://www.drupal.org/project/jsonapi/issues/2942979
-            // @codingStandardsIgnoreStart
-            'region' => 'content',
-            'weight' => 100,
-//            'links' => [
-//              'region' => 'content',
-//              'weight' => 100,
-//            ],
-            // @codingStandardsIgnoreEnd
+            'links' => [
+              'region' => 'content',
+              'weight' => 100,
+            ],

By the way, this broke more than just dependencies.

It broke every case of a single-item array with a non-numerical key.

Comment #17

wim leers

Ghent 🇧🇪🇪🇺

commented 16 February 2018 at 15:50

#15 is green on both 8.4 and 8.6!

Comment #18

gabesullice

English

commented 19 February 2018 at 10:01

Status:

Needs review

» Reviewed & tested by the community

This looks great!

Comment #19

e0ipso

Catalan

Can Picafort

commented 24 February 2018 at 08:28

Status:

Reviewed & tested by the community

» Fixed

Boy! Thanks again for catching this one. Merging.

Comment #20

24 February 2018 at 08:33

e0ipso committed 9a9ca33 on 8.x-1.x authored by Wim Leers

Issue #2942979 by Wim Leers, e0ipso, gabesullice: JSON API's...

Comment #21

e0ipso

Catalan

Can Picafort

commented 24 February 2018 at 09:58

Edit: I posted in the wrong issue.

Comment #22

wim leers

Ghent 🇧🇪🇪🇺

commented 26 February 2018 at 07:07

Yay, glad to have this fixed, with test coverage!

Comment #23

12 March 2018 at 07:09

Status:

Fixed

» Closed (fixed)

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

JSON API's normalization of config entities' "dependencies" key-value pair omits essential information when there's only a single dependency

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Comment #23

Related issues

Referenced by

News items

Our community

Documentation

Drupal code base

Governance of community