Denormalizing NULL for an optional @FieldType=address or @FieldType=geolocation field fails due to either no main property name or computed read-only main property

I would expect that if I changed an attribute value in such a result and send the content the server gave me directly back for an update (PATCH) it would succeed (say I just change the title of the node, the server should be able to deserialize what it serialized to me)

Absolutely! We even have explicit test coverage for this.

Error: Call to a member function getClass() on null in /drupal/web/modules/contrib/jsonapi/src/Normalizer/FieldItemNormalizer.php on line 117
…

That's this code:

    if (!is_array($data)) {
      $property_value = $data;
      $property_name = $item_definition->getMainPropertyName();
      $property_value_class = $property_definitions[$property_name]->getClass();
      return $denormalize_property($property_name, $property_value, $property_value_class, $format, $context);
    }

Which field type are we talking about? It looks like its main property name doesn't match an existing property on the field, which would be a bug in that field type. I'll only know for sure if I can look at the code for the field type. Ah you already dug into exactly that, great!

The problem is now obviously a mismatch between the expectations of JSON:API's normalizer (which assumes there will be a main property) and the implementation of \Drupal\address\Plugin\Field\FieldType\AddressItem::mainPropertyName():

  public static function mainPropertyName() {
    return NULL;
  }

(which clearly is not designating a particular property to be the main one — this was a change made in #2823536: AddressItem should override FieldItemBase::mainPropertyName()).

#1: ah, but there is a crucial difference: @FieldType=geolocation does have

  public static function mainPropertyName() {
    return 'value';
  }

but it also has:

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
…
    $properties['value'] = DataDefinition::create('string')
      ->setLabel(t('Computed lat,lng value'))
      ->setComputed(TRUE)
      ->setInternal(FALSE)
      ->setClass(GeolocationComputed::class);

    return $properties;
  }

(which was introduced in #2907560: Add support for search_api_location indexing).

So in this case, while the symptom is similar, it has a different cause: a main property exists, but it's computed and read-only.

First, let's add test coverage verifying that fields like @FieldType=address or @FieldType=geolocation indeed normalize to null when they're empty/not set. We can't test with either of those fields since they're in contrib rather than in core, but we can fortunately approximate this using the map field type. And in fact, we already have test coverage for the map field type that we can simply expand :)

Next, let's add explicit test coverage for the scenario you described: an entity with an optional/empty @FieldType=map where only a different field (such as the label field) is being modified should result in a successful PATCH request without any other request body modifications.

If this fails, this is successfully reproducing the problem reported by @adamspe!

Here's the fix.

Thanks! This looks good to me.

Great! Then porting this to a core patch that should be committed first.

Looks like a random fail. Retesting.

Needs a reroll.

Rerolled the #10 patch.

Diffed both patches, they're making the same changes, only context has changed. 👍

Thanks, @yogeshmpawar! 🙏

Crediting @adamspe for filing the issue.

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -990,4 +996,44 @@ public function testLeakedCacheMetadataViaRdfFromIssue3053827() {
+      RequestOptions::AUTH => [$user->getUsername(), $user->pass_raw],

This is using deprecated code but we don't know that because the test is erroneously marked as @group legacy. Should be getAccountName()

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -990,4 +996,44 @@ public function testLeakedCacheMetadataViaRdfFromIssue3053827() {
+  /**
+   * Ensure optional `@FieldType=map` fields are denormalized correctly.
+   *
+   * @see https://www.drupal.org/project/jsonapi/issues/3048348
+   */
+  public function testEmptyMapFieldTypeDenormalizationFromIssue3048348() {

Let's remove the issue ID from the test name and the @see. This is not normal practice for core and I don';t think it should be. git blame is our friend.

Furthermore we really need another issue to remove the @group legacy and tidy this test up at bit and probably rename it. This test and all the others in this file are not regression tests as far as I know and I'm not sure why we care whether a test is for a regression or not. Once you have the test and fix in its a bug and test for that bugfix.

I.e. let's fix #3042745: Remove group @legacy from jsonapi tests and fix deprecation messages first.

Now that #3042745: Remove group @legacy from jsonapi tests and fix deprecation messages is closed, get back to reviewing this one.

Thanks @garphy :)

#18:

1. Fixed.
2. While atypical, this is not the only place. \Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest is the other example.

   git blame doesn't work when code has been imported from contrib to core, because we don't retain those commit histories.

   These really are regression tests. We have a tendency to make even tests abstract/generalized. And yes, REST/JSON:API tests are extra super guilty of this, but in that case it's out of necessity. Having these tests tied to very specific bug reports helps keep the tests simple and understandable, resist that temptation. I am the first to agree this is atypical! But it's an experiment, that so far seems to be working out quite well.
   Generally speaking, if you need a very particular combination of configuration/setup, we'll add a focused test method to JsonApiRegressionTest. If it's a generic bug that can reasonably be tested in an abstract way across all entity types, we'll add test coverage to \Drupal\Tests\jsonapi\Functional\ResourceTestBase.

D'oh, recent core commits made #21 not apply! Anybody care to reroll? :)

Here's a quick re-roll.

Perfect, thanks!

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -1085,4 +1091,44 @@ public function testInvalidDataTriggersUnprocessableEntityErrorFromIssue3052954(
+   * @see https://www.drupal.org/project/jsonapi/issues/3048348
...
+  public function testEmptyMapFieldTypeDenormalizationFromIssue3048348() {

Can we remove the issue reference here (I realise there are existing ones, but let's not add new ones) - we can use git blame to work out when this was added.

Damn, I didn't save my comment properly - also had this observation:

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -1085,4 +1091,44 @@ public function testInvalidDataTriggersUnprocessableEntityErrorFromIssue3052954(
+    $this->assertSame(200, $response->getStatusCode());

should we assert that the returned values are unchanged for the map field, per the issue summary that is also a problem in HEAD for these fields?

Done and done!

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -1129,6 +1128,7 @@ public function testEmptyMapFieldTypeDenormalizationFromIssue3048348() {
+    $this->assertSame($doc['data']['attributes']['data'], Json::decode((string) $response->getBody()['data']['attributes']['data']);

This contained a typo because I forgot to stage a change 🙃 😊

Committed 6aec5a3 and pushed to 8.8.x. Thanks!

Ported :)

Can last commit be causing https://www.drupal.org/pift-ci-job/1390858 ?

1) Drupal\Tests\jsonapi\Functional\JsonApiRegressionTest::testMapFieldTypeNormalizationFromIssue3040590
Failed asserting that null is identical to Array &0 (
'foo' => 'bar'
).

/var/www/html/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php:913

Yes it looks like it does.

Do we have a followup for the postgres fail or should I just revert this?

I planned to do some investigation to answer exactly that question, but haven't had the time yet.

I don't see how the changes in here can be the direct cause for the PostrgreSQL issue, besides fixing the real problem can not be that quick.
So either we should revert this or not, worth fixing the failure is its own issue.

I'll create one and link in here.

#3078639: PostgreSQL automated test failing for JsonApiRegressionTest after latest changes on json_api FieldItemNormalizer

@gabesullice found and fixed the issue on #3078639: PostgreSQL automated test failing for JsonApiRegressionTest after latest changes on json_api FieldItemNormalizer, but I think the window for last 8.7.x patch releases is now gone?
If that's the case, then this issue can now close.

but I think the window for last 8.7.x patch releases is now gone?

No, it is not. 8.8.0's release is still months away (December 4, 2019), we still want bugfixes in the mean time :D

But that does mean we need this patch to be rerolled for 8.7, to include the Postgres fix that is RTBC at #3078639: PostgreSQL automated test failing for JsonApiRegressionTest after latest changes on json_api FieldItemNormalizer.

Included the 8.8 Postgres fix into #33

Test-only with D8.8 postgres fix included.

Not quite sure if combining to RTBC issues with green Testbot result and having it done myself qualifies me to put this back on RTBC, so Needs Review it will be.

All looks good. Indeed #42 is #33 + the PostgreSQL fix already in 8.8.x branch with #3078639: PostgreSQL automated test failing for JsonApiRegressionTest after latest changes on json_api FieldItemNormalizer. So this is RTBC.

Hey folks, which patch is RTBC here for 8.7.x - chance we could upload it as the last one - I'm kind of lost

@larowlan Patch in #42 is the D8.7 patch

thanks!

Committed 93ddb31 and pushed to 8.7.x. Thanks!