JSON:API should provide a helpful error response if there is a typo in a field property name

I saw this behavior when the fields and properties had mis-matched casing (eg, for an address field, Address_line1 would throw the error, while address_line1 works fine.)

It would be great to add an exception if a property isn't found, instead of the current behavior which tries to call getClass() on a NULL, which doesn't reveal the incorrect field property in the error logs.

An assert() could be used instead of an exception, but the downside there is it wouldn't catch these errors on production where API clients may be doing their implementation work.

I agree that improving the error message would be a good thing. A "call on null" error has wreaked havoc in another issue a while back.

I think we can leave this as a bug rather than a feature request. Adding the DX tag.

I have always encountered the same error when sync some entities to half:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /en/batch?id=1988&op=do_nojs&op=do
StatusText: parsererror
ResponseText:

and check the dblog, the error message is :
Error: Call to a member function getClass() on null in Drupal\jsonapi\Normalizer\FieldItemNormalizer->denormalize() (line 130 of /home/wwwroot/sancy-php7.4-d8.9.18-ok/core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php)

Hope someone can solve it.
Thanks very much!

Sorry, this patch just a workaround.

Having the same issue with field type: "Text (plain, long)" when passing in JSON.stringified / serialized string.

zzz

I encountered this error when I forgot to use JSON.stringify() before POSTing a json object to a "JSONB" field.

I'm attaching a basic patch that throws an exception for this error as suggested in #4.

I do not think the workaround in #9 is appropriate because it ignores this error, which is a genuine error and should halt processing.

Error: Call to a member function getClass() on null in Drupal\jsonapi\Normalizer\FieldItemNormalizer->denormalize() (line 130 of /var/www/html/core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php)
#0 /var/www/html/vendor/symfony/serializer/Serializer.php(182): Drupal\jsonapi\Normalizer\FieldItemNormalizer->denormalize()
#1 /var/www/html/core/modules/jsonapi/src/Serializer/Serializer.php(75): Symfony\Component\Serializer\Serializer->denormalize()
#2 /var/www/html/core/modules/jsonapi/src/Normalizer/FieldNormalizer.php(65): Drupal\jsonapi\Serializer\Serializer->denormalize()
#3 /var/www/html/vendor/symfony/serializer/Serializer.php(182): Drupal\jsonapi\Normalizer\FieldNormalizer->denormalize()
#4 /var/www/html/core/modules/jsonapi/src/Serializer/Serializer.php(75): Symfony\Component\Serializer\Serializer->denormalize()
#5 /var/www/html/core/modules/jsonapi/src/Normalizer/ContentEntityDenormalizer.php(85): Drupal\jsonapi\Serializer\Serializer->denormalize()
#6 /var/www/html/core/modules/jsonapi/src/Normalizer/EntityDenormalizerBase.php(99): Drupal\jsonapi\Normalizer\ContentEntityDenormalizer->prepareInput()
#7 /var/www/html/vendor/symfony/serializer/Serializer.php(182): Drupal\jsonapi\Normalizer\EntityDenormalizerBase->denormalize()
#8 /var/www/html/core/modules/jsonapi/src/Serializer/Serializer.php(75): Symfony\Component\Serializer\Serializer->denormalize()
#9 /var/www/html/core/modules/jsonapi/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php(167): Drupal\jsonapi\Serializer\Serializer->denormalize()
#10 /var/www/html/modules/contrib/entity_share/modules/entity_share_client/src/Service/JsonapiHelper.php(287): Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer->denormalize()
#11 /var/www/html/modules/contrib/entity_share/modules/entity_share_client/src/Service/JsonapiHelper.php(467): Drupal\entity_share_client\Service\JsonapiHelper->extractEntity()
#12 /var/www/html/modules/contrib/entity_share/modules/entity_share_async/src/Plugin/QueueWorker/EntityShareAsyncWorker.php(144): Drupal\entity_share_client\Service\JsonapiHelper->importEntityListData()
#13 /var/www/html/core/lib/Drupal/Core/Cron.php(180): Drupal\entity_share_async\Plugin\QueueWorker\EntityShareAsyncWorker->processItem()
#14 /var/www/html/modules/contrib/ultimate_cron/src/UltimateCron.php(69): Drupal\Core\Cron->processQueues()
#15 /var/www/html/modules/contrib/ultimate_cron/src/ProxyClass/UltimateCron.php(70): Drupal\ultimate_cron\UltimateCron->run()
#16 /var/www/html/core/modules/automated_cron/src/EventSubscriber/AutomatedCron.php(65): Drupal\ultimate_cron\ProxyClass\UltimateCron->run()
#17 [internal function]: Drupal\automated_cron\EventSubscriber\AutomatedCron->onTerminate()
#18 /var/www/html/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func()
#19 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(88): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch()
#20 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(32): Symfony\Component\HttpKernel\HttpKernel->terminate()
#21 /var/www/html/core/lib/Drupal/Core/DrupalKernel.php(686): Stack\StackedHttpKernel->terminate()
#22 /var/www/html/index.php(27): Drupal\Core\DrupalKernel->terminate()
#23 {main}

Module:entity_share
Drupal:8.9

how to fix this issue?
please give me the idea
.

any update on this issue?

#15 works for me!

Thanks for filing this bug report and for fixing the issue. Bug fixing is very valuable. In order to commit a bug fix, we need an automated test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal, see the following links:

1. https://www.drupal.org/docs/testing/phpunit-in-drupal/phpunit-javascript...
2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/9.3.x

In addition to adding a test as per #18, I think this should throw an exception as per #4. When a property is requested that doesn't actually have a value set, that is an error, and we should throw an exception to stop processing and make debugging easier.

As I noted in #11, at least in the case of forgetting to use JSON.stringify() before POSTing a json object to a JSON field, it seems clear that an exception should be thrown because that is an error on the part of the developer.

Thank you, @jhedstrom in #4, for making this issue actionable through your crystal-clear comment! 👍

\Drupal\jsonapi\Controller\EntityResource::deserialize() does this:

    try {
      $context = ['resource_type' => $resource_type];
      if ($relationship_field_name) {
        $context['related'] = $resource_type->getInternalName($relationship_field_name);
      }
      return $this->serializer->denormalize($decoded, $class, 'api_json', $context);
    }
    // These two serialization exception types mean there was a problem with
    // the structure of the decoded data and it's not valid.
    catch (UnexpectedValueException $e) {
      throw new UnprocessableEntityHttpException($e->getMessage());
    }
    catch (InvalidArgumentException $e) {
      throw new UnprocessableEntityHttpException($e->getMessage());
    }

so AFAICT the proposed fix in #15 is wrong here. Instead, we should be able to just throw UnexpectedValueException? Similar but different to what #11 was doing.

Test.

Fix.

That worked. Time to run the full test suite!

The patch in #24 is what I was trying to do in #11, but better. 😊 This throws a clear error that makes it immediately clear to the developer/API user what the issue is, which is a big win for DX and will save a lot of time debugging.

Patch #24 applied successfully in version 10.1.x.

@pradhumanjainOSL The test results say the same thing… and more. I'm not sure what your point is? 🤔

This looke like a great improvement for DX really really glad to see this change.

One of the things that is sometimes a little annoying as a developer is when validation only really validated parts of what I ask of the system. This means that when i solve validation i try again, get the next error and try again.

What if we do that here also.

So instead of the current:

    $data_internal = [];
    if (!empty($property_definitions)) {
      foreach ($data as $property_name => $property_value) {
        if (!isset($property_definitions[$property_name])) {
          throw new UnexpectedValueException(sprintf("The property '%s' does not exist on the '%s' field of type '%s'.", $property_name, $item_definition->getFieldDefinition()->getName(), $item_definition->getFieldDefinition()->getType()));
        }
        $property_value_class = $property_definitions[$property_name]->getClass();
        $data_internal[$property_name] = $denormalize_property($property_name, $property_value, $property_value_class, $format, $context);
      }
    }
    else {
      $data_internal = $data;
    }

We change this to first loop, then error.

    $data_internal = [];
    $invalid_properties = [];
    if (!empty($property_definitions)) {
      foreach ($data as $property_name => $property_value) {
        if (!isset($property_definitions[$property_name])) {
          $invalid_properties[] = sprintf("The property '%s' does not exist on the '%s' field of type '%s'.", $property_name, $item_definition->getFieldDefinition()->getName(), $item_definition->getFieldDefinition()->getType());
          continue;
        }
        $property_value_class = $property_definitions[$property_name]->getClass();
        $data_internal[$property_name] = $denormalize_property($property_name, $property_value, $property_value_class, $format, $context);
      }
    }
    else {
      $data_internal = $data;
    }

    if (count($invalid_properties) > 0) {
      throw new UnexpectedValueException(implode(PHP_EOL, $invalid_properties));
    }

Although this is a relatively small improvement of the possible error messages the developer gets, the extra code it requires is minimal.

Possible drawbacks might be;

1. There is an error while denormalizing, which then throws an exception and prevents the code to reach $invalid_properties.
2. There might be more processing going on while denormalizing, which makes the request take longer (and more cpu) before throwing the exception.

I'm not married to this improvement, but I feel like it would be a net gain if we allow multiple errors right there.

I like it!

The second drawback is not really a drawback IMHO. This extra loop is negligible compared to the time interacting with the database.

The first drawback is mitigated easily: extract this validation step completely, and run it first. IOW: don't even try to denormalize until we've made sure all properties are valid.

P.S.: this is also how we validate all the things in CKEditor 5. We even have extensive test coverage for it. See \Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::providerConstruct() for \Drupal\ckeditor5\HTMLRestrictions::validateAllowedRestrictionsPhase*, \Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::providerTestInvalidPluginDefinitions() for \Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition::validate(CKEditor5|Drupal)Aspects(), and more 🤓 I actually learned that lesson while working on REST and JSON:API: concrete, specific error messages are such an enormous productivity boost!

Hmm, good one to separate the logic. That sounds like a great plan.

Looks pretty structured in ckeditor. Lovely to see that iteration on validation.

Implemented that. And went further still 🤓 Automatically finding the property name that the user probably meant! (Copy/pasted\Drupal\Component\DependencyInjection\Container::getAlternatives() for that.)

But first … a failing test that will prove that @bbrala's dream in #28 will become a reality.

I think I'm in love with this change. I just removed the drupalci.yml additions. BUt RTBC for me.

+++ b/core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php
@@ -140,6 +165,31 @@ public function denormalize($data, $class, $format = NULL, array $context = []):
+   */
+  private static function getAlternatives($search_key, array $keys) : array {
+    $alternatives = [];
+    foreach ($keys as $key) {
+      $lev = levenshtein($search_key, $key);
+      if ($lev <= strlen($search_key) / 3 || strpos($key, $search_key) !== FALSE) {
+        $alternatives[] = $key;
+      }
+    }
+
+    return $alternatives;
+  }

levenstein() has a maximum string length of 255 characters. I can't remember what the maximum length of a configurable field is, but should we throw an exception when the string is two long (either field length, or 255) given this is user data?

I can't remember what the maximum length of a configurable field is, but should we throw an exception when the string is two long (either field length, or 255) given this is user data?

🤯 Wow :D

Let's not throw an exception, let's just not find alternatives 🤓

Change is minimal. But while looking. Shouldn't the static methods argument be typehinted? First should be a string right.

Drupalci is having a fit again. Unrelated test failure.

Was gonna rtbc anyways, but the method without a rypehint could actually result in weirdness and possible bugs at some point.

Oh, right, we can do that now! 👍 Done.

Change is good. Tests pass. Rtbc.

+++ b/core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php
@@ -128,6 +130,29 @@ public function denormalize($data, $class, $format = NULL, array $context = []):
+          $alt = static::getAlternatives($property_name, array_keys($property_definitions));
+          $invalid_property_names[$property_name] = reset($alt);
...
+      if (!empty($invalid_property_names)) {
+        $format = count($invalid_property_names) === 1
+          ? "The property '%s' does not exist on the '%s' field of type '%s'. Did you mean '%s'? (Writable properties are: '%s'.)"
+          : "The properties '%s' do not exist on the '%s' field of type '%s'. Did you mean '%s'? (Writable properties are: '%s'.)";
+        throw new UnexpectedValueException(sprintf(
+          $format,
+          implode("', '", array_keys($invalid_property_names)),
+          $item_definition->getFieldDefinition()->getName(),
+          $item_definition->getFieldDefinition()->getType(),
+          implode("', '", array_values(array_filter($invalid_property_names))),
+          implode("', '", array_keys(array_filter($property_definitions, function (DataDefinitionInterface $data_definition) : bool {
+            return !$data_definition->isReadOnly();
+          })))
+        ));

It's quite possible that $invalid_property_names will be empty. In this case the exception does not make much sense. I think having both the guess and full list of writable properties is unnecessary and odd. I'm not sure I think that getAlternatives is actually worth it. Also why are we listing all writable properties but the alternatives could include non-writable properties?

It's quite possible that $invalid_property_names will be empty. In this case the exception does not make much sense.

Not sure I follow, because if $invalid_property_names is empty, no exception is thrown? 🤔

I think having both the guess and full list of writable properties is unnecessary and odd. I'm not sure I think that getAlternatives is actually worth it.

I disagree. If you make a minor typo, getAlternatives() will find the right one. If it's wildly wrong, the full list of writable properties addresses the developer's confusion.

Oh wait — I think you didn't mean $invalid_property_names would be empty, but $invalid_property_names[$SOMETHING] — i.e. the list of alternatives could very well be empty. That's absolutely true, and is handled in a sort of brute-force way — it could be done more helpfully if we increase the LoC.

I believe this change addresses your concerns. 😊

OMG. d.o patch removal still results in my comment being posted prematurely 🤯

The patch that goes with #43.

Random fail: 1) Drupal\Tests\ckeditor5\FunctionalJavascript\AdminUiTest::testSettingsOnlyFireAjaxWithCkeditor5 — this was fixed recently in #3315319: Random fails in Drupal\Tests\ckeditor5\FunctionalJavascript\AdminUiTest and Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5Test. Re-testing.

Al seems good, but waiting for full testrun.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!