Filtering variations over JSON:API is always access false

Entity API received a fix that solves JSON:API's custom entity access for all entity types extending the Entity API access control.

Variations do not use this access control, because we solve their access as an embedded entity. See https://git.drupalcode.org/project/commerce/-/blob/8.x-2.x/modules/produ...

The hook is hook_jsonapi_entity_filter_access.

The Entity API issue was [##3021930]

Working on this

Needs tests. But with manual testing this fixes it. Maybe we can cherry-pick into Commerce API as well.

Actually, this has a bigger impact. It also fixes the results returned on the variations relationship for a product. Currently inactive variations are returned, and this fixes it to exclude inactive variations.

Wrong issue for this comment

Here is a test only patch.

Linking related core issues uncovered during this issue.

Adds number comparator.

This will pass because we don't test the filtered collection. It is not out of the box so we need to copy \Drupal\Tests\jsonapi\Functional\NodeTest::testCollectionFilterAccess

Turns out Drupal\commerce_price\Comparator\NumberComparator will not help us here. the ResourceTestbase uses assertSame which doesn't load any of the registered Comparator and just uses IsIdentical

Changed $4.00 to $4.15 to avoid 0 precision differences.

That didn't work. Overriding assertSameDocument to use assertEquals

This is also, apparently, automatically fixed by Entity 1.1 which adds a generic query access handler to all entities.

Okay, the changes in entity:1.1 are what prevented this from failing and showing we need #5.

Okay, this fixes usages of $this->resourceType from ResourceTestBase (hacked core locally, forgot to override in test.)

This also tries to add a normalizer for price.number when testing with MySQL.

Another attempt at a test normalization due to #3163853: ResourceTestBase should use assertEquals over assertSame when comparing data.

1. +++ b/modules/price/tests/modules/commerce_price_test/commerce_price_test.services.yml
   @@ -3,3 +3,8 @@ services:
   +      # The priority must be higher than serialization.normalizer.field_item.
   

   This comment is incorrect, it runs before the primitive normalizer.

2. +++ b/modules/product/commerce_product.module
   @@ -358,3 +361,13 @@ function commerce_product_commerce_condition_info_alter(array &$definitions) {
   +/**
   + * Implements hook_jsonapi_ENTITY_TYPE_filter_access().
   + */
   

   Should we comment that this is needed since we do not have a query_access handler?

3. +++ b/modules/product/src/Entity/Product.php
   @@ -279,7 +279,7 @@ class Product extends CommerceContentEntityBase implements ProductInterface {
   -      if ($variation->product_id->isEmpty()) {
   +      if ($variation && $variation->product_id->isEmpty()) {
   

   Somehow the JSON:API testing had deleted the variation and the product was stale and no longer had a valid reference.

4. +++ b/modules/product/src/ProductVariationAccessControlHandler.php
   @@ -36,12 +36,14 @@ class ProductVariationAccessControlHandler extends CoreEntityAccessControlHandle
   +      assert($result instanceof AccessResult);
   +      $result->addCacheableDependency($entity);
   ...
   +      $result = AccessResult::allowedIfHasPermission($account, "manage $bundle commerce_product_variation")->cachePerPermissions();
   

   Fixes cache expectations from JSON:API. Which means we maybe had a bug and it could go into its own issue.

5. +++ b/modules/product/tests/modules/commerce_product_test/commerce_product_test.module
   @@ -21,3 +21,14 @@ function commerce_product_test_entity_access(EntityInterface $entity, $operation
   +  // Remove the EventOnlyQueryAccessHandler added to all entities in entity:1.1
   +  // for testing.
   +  if ($hook === 'entity_type_alter') {
   +    unset($implementations['entity']);
   +  }
   

   I wonder if the generic query_access handler is actually causing more problems than just in this test.

6. +++ b/modules/product/tests/src/Functional/Jsonapi/ProductVariationResourceTest.php
   @@ -0,0 +1,445 @@
   +  }
   +  /**
   

   missing empty line

7. +++ b/modules/product/tests/src/Functional/Jsonapi/ProductVariationResourceTest.php
   @@ -0,0 +1,445 @@
   +   * Overridden to use `assertEquals` due to how decimal numbers are handled
   +   * between \json_encode and SQL storages.
   

   Comment is wrong. We might be able to remove this, now, that we have the normalizer.

Addressed my self review. We were able to drop that test method override due to the price normalizer.

Crediting @jsacksick for rubber ducking over Slack

Committed!

Wrong status.

Attaching a patch that applies to 2.20