JSON:API module does not support POSTing dynamic entity references with only `type`, `id`/entity reference field does not validate type when setting with entity property

Another reason to change the value set from target_id to entity is that this would allow the entity reference field to validate the entity type (and, if applicable, bundle) of the entity matches the field's settings. At the moment, there's nothing stopping a client from setting an entity of a different type to a field expecting an entirely different entity type. This is because the entity is loaded in the top-level document normalizer, but so long as it's valid, its ID is set unquestionably to target_id. If the same target_id exists on the correct type, you would end up with a totally different entity being referenced. And because entity IDs are serial (as opposed to UUIDs) this is a non-trivial risk of inadvertant data corruption.

Looks like the test in #2998662: Saving a tagged article in staged workspace causes fatal exception ::testNewEntitiesAllowedInDefaultWorkspace()was incorrect but passing due to the incorrect type checking when setting entity reference values by passing only the entity property. Fixing, but it would be nice to get some other eyes on this to ensure the spirit of the test is preserved/correct.

@bbrala should also receive credit for sprinting on this during DrupalCon.

There are some transient errors in Drupal 9.4 HEAD but this is green, now, after poking DrupalCI.

Thank you for the RTBC.

It would be great to finish up #3057545: ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type" along with this; the one does not require the other, but particularly when generating (or trying to...) OpenAPI specs for DER fields, we need to continue to improve the core ER field handling to make fewer assumptions about its operation.

Back to NR. Added two CRs, one as requested for the ER field and another documenting the change in json:api's exception translation. This should provide additional context to your other MR review question.

Also forked off #3279103: Test cleanup: Remove dead code from JsonApiFunctionalTest for the dead test code cleanup.

I added an additional CR to notify site owners/client implementators that they can drop the unnecessary meta information if the field type supports this. I thought this might be a CR on DER but the change is in core and at least theoretically, this affects other modules with a custom ER field type. I'm only really aware of DER though.

https://www.drupal.org/node/3279367

At the risk of delaying or over-complicating this, I think we have an opportunity to make this even more awesome-er.

While testing the entity that brought me to this issue in the first place, I noticed that the response resource relationship object included a meta object with entity_type set. This makes sense, as it's essentially the echo-back of the data that you could then post to Drupal to indicate the required property for extending fields (in this case, DER.)

Except, we don't need to do that anymore! Of course, the goal here is to both get out of the way of contrib field types as well as reduce unnecessary Drupalisms in the API surface.

I took a look at the related code, and it turns out json:api was doing a little more work than I think it needs to in order to determine which properties to send in the meta object. Properties can be marked internal, and the normalizer was already taking that into account in filtering properties. So let's mark entity and target_id in the core field as internal, and then modules like DER can do the same with their properties that don't need to be sent to the client, either! A fluent API for the win.

We'll see if this screws up the tests, but I don't think it should?

I think this can be added to the existing CR I made about property values in meta.

I'll open a corresponding issue in DER to mark entity_type as internal. This would be a BC break, since there may be some clients that require it. I don't think this should be a huge issue though, since to POST new content, you'd already need to know the entity_type out of band, somehow. That would work fine if json:api were the only API shape out there, but e.g. REST would take the target_id and entity_type properties directly, b/c both are necessary to create the field value.

Adding a related issue regarding removing the current exposure of the internal entity_id for ER fields. I believe this should be easily opt-out.

Thoughts?

Well, I realized setting the target ID property as internal was a little over-aggressive; REST, etc. need to actually normalize that. But, I think this is an opportunity for us to better respect the properties' internal flag, and consult that to determine whether we set the drupal_internal__target_id value. That allows paranoid site owners like me to override the field item class and tweak the property definitions to set it as internal, and makes that part of the code more fluent. I also think cleaning up the inline comment to reflect this as more field-agnostic is good?

If all of this is too much of a sidetrack, we can revert to #17/f3af2d79.

We should also respect the isInternal() flag when resolving the field query; e.g., if the data reference property in question; I'm going to hack on this a bit more but I think the code in FieldResolver::resolveInternalEntityQueryPath() needs some refinement in line with the changes we're making here. This is all rather similar to the work in #3057545: ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type" but I think it should be appropriate here in so far as we are addressing what properties of the ER field json:api is aware of, and how we handle those properties during normalization/denormalization.

Rebased and added additional validation of known field properties on denormalization. To be extra cautious, if the field name isn't found, I'm just copying as-is for the moment, since we otherwise aren't doing field name validation at that point in the process.

I considered if being able to set read-only or internal properties through meta is a security issue, but I couldn't come up with any real situation that didn't involve myself tied in a knot where this would be feasible given the known universe of ER fields.

Back to NR.

Added another draft CR covering the value array property filtering.

One final note for the day if reviewers are curious why this additional validation should matter - There are two core field types which extend EntityReferenceItem: FileItem and ImageItem. Both contain additional properties which can be set in meta. We have test coverage for this.

Honestly, I'd be interested to hear thoughts about tightening this up even further and throwing an exception (unprocessable entity?) if there are additional, unknown properties encountered during filtering. (We could first exclude the drupal_internal__* values out, like currently.) I don't necessarily think that well-behaved clients should be sending any unexpected data here. I guess in theory, you could be really wacky and use non-property data in the value for a custom field type, but I don't think it's our responsibility to support that antipattern. This would continue to help the client responses be more meaningful to public clients that do not have understandings of Drupal internals.

Thanks for the review; all easy stuff to address. I'll put NW for those nits.

Regarding your two specific points - agreed and I do not believe there's anything actionable on this issue. We can open a separate issue for the field parameter validation if we want. Agreed it's not in scope here.

Two failing tests, need to at least re-base and re-test.