ResourceTypeRepository::getFieldMapping is supposed to throw an exception if there is a naming conflict between a generated alias and a fieldname. That check does not work currently since it accesses the wrong variable ($field_name[$alias], but $field_name is the loop variable, a string).

What's more, I think the mapping must be unique in both directions since the ResourceType inverts the mapping (after filtering for string values).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cspitzlay created an issue. See original summary.

cspitzlay’s picture

Status: Active » Needs review
FileSize
1.33 KB

Here is a patch that checks against both the existing field names as well as the already generated mapping.

cspitzlay’s picture

Fixed a formatting error.

cspitzlay’s picture

cspitzlay’s picture

Issue summary: View changes
Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +API-First Initiative, +Needs tests

Woah! Nice find! 🤯

This will need a failing test to prove it is broken though. This can be added to \Drupal\Tests\jsonapi\Kernel\ResourceType\ResourceTypeRepositoryTest. Do you think you could do that? 😊

cspitzlay’s picture

Here is a test that demonstrates the issue.

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

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
FileSize
3.33 KB

This looks perfect! 🤩 — see point 3, sorry!

  1. +++ b/core/modules/jsonapi/tests/src/Kernel/ResourceType/ResourceTypeRepositoryTest.php
    @@ -111,4 +112,37 @@ public function testCaching() {
    +   * Ensures that a naming conflict in the mapping causes an exception to be thrown.
    ...
    +   * These field name lists are designed to trigger a naming conflict in the mapping:
    +   * the special-cased names "type" or "id", and the name "{$entity_type_id}_type" or "{$entity_type_id}_id",
    

    Nit: violate 80 cols.

  2. +++ b/core/modules/jsonapi/tests/src/Kernel/ResourceType/ResourceTypeRepositoryTest.php
    @@ -111,4 +112,37 @@ public function testCaching() {
    +  }
     }
    

    Übernit: missing blank line after last method closing brace.

  3. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
    @@ -233,8 +233,11 @@ protected static function getFieldMapping(array $field_names, EntityTypeInterfac
    +        if (in_array($alias, $mapping, TRUE)) {
    +          throw new \LogicException("The generated alias '{$alias}' for field name '{$field_name}' is already present in the mapping. Please report this in the JSON:API issue queue!");
    +        }
    +        if (in_array($alias, $field_names, TRUE)) {
    +          throw new \LogicException("The generated alias '{$alias}' for field name '{$field_name}' conflicts with an existing field. Please report this in the JSON:API issue queue!");
             }
    
    +++ b/core/modules/jsonapi/tests/src/Kernel/ResourceType/ResourceTypeRepositoryTest.php
    @@ -111,4 +112,37 @@ public function testCaching() {
    +    $this->expectException(\LogicException::class);
    

    I spoke too soon 😅 We need explicit test coverage for both of these if-tests.

    We can achieve that by not only expecting a LogicException but also a particular message, using $this->expectExceptionMessage().

cspitzlay’s picture

After rereading getFieldMapping I think the conflict in the mapping array cannot happen with the current code.
Thus I could not think of a test for that and I removed the second if.
I still added the check for the message.
I hope I also addressed the nitpicks.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, thanks! 👍

The last submitted patch, 10: 3077661-10-test-only.patch, failed testing. View results

  • catch committed d9936f2 on 8.8.x
    Issue #3077661 by cspitzlay, Wim Leers: Check for conflict in fieldname...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed d9936f2 and pushed to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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