Cannot PATCH an entity with dangling references in an ER field

Thanks for the extremely clear issue summary! 🙏

I'll try to provide a failing test case which exhibit the behavior soon.

If you could do that, I can promise you that this will get a higher priority in getting fixed!

Note to self: we should check whether the same behavior exists in core's rest.module.

I made an attempt at writing the failing test case for the behaviour mentioned by @garphy. See attached file. Hope this helps.

Thanks @axle_foley00! I'll let @Wim Leers review it since I contributed a bit to the test patch.

WRT, the proposed solution:

A way to solve this would be to ensure we PATCH the "cleaned" entity, but I'm not sure if it's acceptable to silently update the ER field as it was no intended to by the original request. From another perspective, when you edit a node through Drupal UI, any ER field with dangling reference will be silently "corrected" (not sure of this if the user has no write access on the given field, though).

I'd like to shy away from "correcting" the data. Instead, I think we can simply filter the constraint violations by the submitted fields, so only PATCHed fields return a 422 and associated errors.

This can be fixed in 1.x.

Failure was unrelated to the patch, requeued tests

WOOOT! Thank you so much, @axle_foley00!

#3 seems to trigger the failure we want: it reproduces the reported bug!

I'd like to shy away from "correcting" the data.

I agree. That'd be magic. Magic gets in the way of clarity.

Instead, I think we can simply filter the constraint violations by the submitted fields

Agreed! Regarding how to implement that: \Drupal\jsonapi\Controller\EntityResource::validate() already does:

$violations->filterByFieldAccess();

That calls \Drupal\Core\Entity\EntityConstraintViolationListInterface::filterByFieldAccess(). I think we should be able to use \Drupal\Core\Entity\EntityConstraintViolationListInterface::filterByFields() for what we need here. But the challenge is: how to get the list of received fields? It looks like \Drupal\jsonapi\Controller\EntityResource::patchIndividual() may already contain this information :)

@wim-leers/@gabesullice:

Here is the first attempt at the patch. It's still not passing though, but figured I could get some feedback to see if I'm on the right track.

Looking good! 👍

+++ b/src/Controller/EntityResource.php
@@ -143,13 +143,18 @@ class EntityResource {
+  protected function validate(EntityInterface $entity, array $field_names = NULL) {
...
+    if ($field_names !== NULL) {

Why make this new parameter optional? Let's make it required, so that we always only validate the received fields.

I'm guessing that you went with this because in the case of EntityResource::createIndividual(), all fields should be validated?

I'm guessing that you went with this because in the case of EntityResource::createIndividual(), all fields should be validated?

So yes, In discussing it with @GabeSullice I wasn't sure if the change would cause any issues with other calls to the validate() method, so we figured it would be the safest way to make the change. Would it cause an issue in EntityResource::createIndividual() or anywhere else?

Right. But the patch is now only updating the ::validate() call in EntityResource::patchIndividual(). I'm wondering if it should also update those in:

1. ::createRelationship()
2. ::patchRelationship()
3. ::deleteRelationship()

Oh wait, those are always just modifying a single field: the entity reference field representing the relationship you're modifying. Of course!

Then yes, +1 to #10! Now we just need those tests brought back, and once this is green, it's ready! :) 🎉

@gabesullice just pointed out how similar this is to #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors in core REST; it's essentially the same solution!

Actually, on second thought, #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors goes quite a bit further. It tracks which fields were actually changed. It's a much more complete solution.

The patch here considers every received field a changed field. But as #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors shows, that's inaccurate. It's perfectly possible to send the current value for a field in a PATCH request; that field is then not being changed.

That being said, the patch here, while simpler and not as complete, still is a step forward. We should still do it.

@Wim-leers/@gabesullice, thanks for pointing out that #2821077 PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors was similar. I was looking through and noticed that you had done the following in the patch for the validate() method:

$violations->filterByFields(array_diff(array_keys($entity->getFieldDefinitions()), $field_names));

So I tried that, but the test is still not passing. I'm getting the following failure:

1) Drupal\Tests\jsonapi\Functional\JsonApiRegressionTest::testDanglingReferencesInAnEntityReferenceFieldFromIssue2968972
Exception: Notice: Undefined offset: 0
Drupal\jsonapi\Normalizer\RelationshipItem->__construct()() (Line: 75)

If I instead use what I had before:

$violations->filterByFields($field_names);

the test still fails as before with the 422 Unprocessable Entity code:

{"errors":[{"title":"Unprocessable Entity","status":422,"detail":"field_issue.0.target_id: The referenced entity (node: 1) does not exist.","code":0,"source":{"pointer":"\/data\/attributes\/field_issue\/0\/target_id"}}]}
Failed asserting that 422 is identical to 200.

I've been pushing #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors forward; now taking a look at this. I'd like to get this fixed today.

Apparently \Drupal\jsonapi\ResourceType\ResourceType::getRelatableResourceTypes() contains non-numerically keyed values for the field_issue field, and that's what's causing the failure:

Easy fix :)

@Wim-leers: Thanks! Your patch worked for me. And the test now passes.

I think it's actually probably better to port the test coverage from #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors.

And just now, @tstoeckler posted a comment at #2821077-95: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors showing how this could cause a security vulnerability… So postponing this issue on that one for now.

Since #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors is now practically RTBC, we can unpostpone this!

Well, actually, since \Drupal\Tests\jsonapi\Functional\ResourceTestBase reuses core's rest_test module, this is sort of painful to do in the JSON API module… ideally the core patch would land, and then JSON API would follow suit.

In the mean time, it's okay to use the patch above. It does not suffer from that (very very obscure) information disclosure vulnerability pointed out in #2821077-95: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors, because this patch does not limit validation to fields that are actually being modified; it only limits validation to fields that are actually being sent. That's safe.

Let's get this committed; it's a step forward. We should still port over #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors though. Creating an issue for that…

Done: #2979945: Port #2821077 to JSON API: limit validation of fields in PATCH requests to fields *changed*.

I'd like another maintainer to review & commit this, to double-check my analysis :)

LGTM! Thanks everyone!

@axle_foley00, congratulations on your first JSON API credit!

Great! Thanks @gabsullice and @wim-leers. Thanks for your help and thoughtful feedback. It made for a good first experience as well.

@axle_foley00 Thank you! And it's great to hear that it was a good first experience! ❤️ :)

Moving to Drupal core's issue queue.

I'm working on https://www.drupal.org/project/drupal/issues/3122113