The whole of the Query namespace was largely untested. This patch rectifies that problem.

The logic for building an entity query was also tightly coupled to the JSON API query parameter specs. I've separated the entity query building logic out from our JSON API implementation. That was done by removing the query builder and factoring its condition tree logic out into an more generic entity condition and condition group value objects (I would like to explore the idea of moving these into core). Then, I refactored the query string expansion logic into denormalizers.

Using denormalizers means that users can implement their own JSON API filtering syntax for their specific needs. It also means that if a different JSON API extension for filtering is written, we could easily make that an option.

CommentFileSizeAuthor
#18 2874601-18-querybuilder-refactor.patch132.45 KBgabesullice
#18 interdiff-17-18.txt8.29 KBgabesullice
#14 2874601-14-querybuilder-refactor.patch141.02 KBgabesullice
#14 interdiff-12-14.txt601 bytesgabesullice
#12 2874601-12-querybuilder-refactor.patch141.06 KBgabesullice
#5 interdiff-2874601-2-5.txt1.89 KBgabesullice
#5 2874601-5-querybuilder-refactor.patch108.8 KBgabesullice
#2 2874601-2-querybuilder-refactor.patch107.96 KBgabesullice
#17 interdiff-14-17.txt9.08 KBgabesullice
#17 2874601-17-querybuilder-refactor.patch130.81 KBgabesullice
#20 interdiff-18-20.txt7.95 KBgabesullice
#20 2874601-20-querybuilder-refactor.patch133.54 KBgabesullice
#21 interdiff--20-21.txt4.75 KBe0ipso
#22 interdiff--20-22.txt4.2 KBe0ipso
#22 2874601--querybuilder-refactor--22.patch141.69 KBe0ipso
#25 2874601--querybuilder-refactor--25.patch142.04 KBe0ipso
#27 2874601--querybuilder-refactor--26.patch141.43 KBe0ipso
#27 interdiff--25-26.txt1.47 KBe0ipso
#30 interdiff-26-30.txt4.71 KBgabesullice
#30 2874601-30-querybuilder-refactor.patch140.6 KBgabesullice
#34 2874601-34-querybuilder-new-test-only.patch1.22 KBgarphy
#34 2874601-34-querybuilder-refactor.patch141.58 KBgarphy
#34 interdiff.txt1.22 KBgarphy
#36 2874601-36-querybuilder-refactor.patch142.24 KBgarphy
#36 interdiff.txt949 bytesgarphy
#41 2874601-41-querybuilder-refactor.patch142.71 KBgarphy
#41 interdiff.txt789 bytesgarphy
#43 interdiff-30-42.txt2.88 KBgabesullice
#43 2874601-42-querybuilder-refactor.patch142.63 KBgabesullice
#47 interdiff-2874601-42-47.txt789 bytesgarphy
#47 2874601-47-querybuilder-refactor.patch143.11 KBgarphy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

gabesullice’s picture

Status: Active » Needs review

Kicking tests.

Status: Needs review » Needs work

The last submitted patch, 2: 2874601-2-querybuilder-refactor.patch, failed testing.

gabesullice’s picture

Status: Needs work » Needs review
FileSize
108.8 KB
1.89 KB

Status: Needs review » Needs work

The last submitted patch, 5: 2874601-5-querybuilder-refactor.patch, failed testing.

e0ipso’s picture

This is looking good! I didn't do a proper review because of the red tests, but I skimmed through the code and got excited.

e0ipso’s picture

@gabesullice any luck with this?

e0ipso’s picture

It would be awesome if we could have some @gabesullice time to land this during DrupalCon Vienna. I'm a bit of a foreigner to the current implementation.

matthew.perry’s picture

@gabesullice and @e0ipso, I was able to take some time to dive into this issue hoping the refactor of the QueryBuilder would fix the issues I'm having when filtering results.

In EntityCondition.php it's missing some of the documented allowed operators:

  • IS NULL
  • IS NOT NULL
  • LIKE
  • NOT LIKE
  • EXISTS
  • NOT EXISTS
  protected static $allowedOperators = [
    '=', '<>',
    '>', '>=', '<', '<=',
    'STARTS_WITH', 'CONTAINS', 'ENDS_WITH',
    'IN', 'NOT IN',
    'BETWEEN', 'NOT BETWEEN',
    'IS NULL', 'IS NOT NULL',
    'LIKE', 'NOT LIKE',
    'EXISTS', 'NOT EXISTS',
  ];

The reason the tests are failing is because without the QueryBuilder it's no longer running $this->fieldResolver->resolveInternal($field).
http://cgit.drupalcode.org/jsonapi/tree/src/Query/QueryBuilder.php#n227

Since the resolveInternal() is not being triggered, the field path is being passed into the SQL builder as uid.uuid, but it's expected to be uid.entity.uuid.

gabesullice’s picture

@matthew.perry that is very useful! Thank you! I plan on taking time this week at DrupalCon to finally get all of this merged. Thanks for your work on this.

gabesullice’s picture

Status: Needs work » Needs review
FileSize
141.06 KB

Alright, I've fixed original issue from #5. Thank you @matthew.perry, you were spot on.

I've also re-rolled this for the latest on 8.x.1.x.

I have not yet added the missing query operators because I haven't tested them and I'm not sure they were all there when I started this process.

It shouldn't be too heavy a lift to get those in tomorrow.

Notes:

  • No interdiff provided because of the re-roll
  • Moving to "Needs Review" to kick tests, but this still "needs work" because of the query operators
gabesullice’s picture

Status: Needs review » Needs work

Passed! \o/

Just a little more left...

gabesullice’s picture

Status: Needs work » Needs review
FileSize
141.02 KB
601 bytes

@matthew.perry, I've corrected the documentation instead of adding new allowed operators. That was a good catch.

The system is built on top of the entity query system, therefore we can only support the operators supported here: QueryInterface::condition.

Technically, we could also support the exists and notExists methods, but because that is not supported now, let's put that into a feature request. At this time, path <> NULL and path = NULL achieve the same thing as a workaround.

Wim Leers’s picture

Status: Needs review » Needs work

@matthew.perry Wow, awesome research! Thank you! ❤️

@gabesullice This patch looks epic :D Great work!

First: I think you can reduce the patch size by doing git diff -M5%. :)

  1. +++ b/jsonapi.services.yml
    @@ -1,5 +1,26 @@
    -  serializer.normalizer.htt_exception.jsonapi:
    ...
    +  serializer.normalizer.http_exception.jsonapi:
    

    Let's fix this in a separate issue, that'd be committed immediately!

  2. +++ b/jsonapi.services.yml
    @@ -1,5 +1,26 @@
    +  serializer.normalizer.sort.jsonapi:
    ...
    +  serializer.normalizer.offset_page.jsonapi:
    ...
    +  serializer.normalizer.entity_condition.jsonapi:
    ...
    +  serializer.normalizer.entity_condition_group.jsonapi:
    ...
    +  serializer.normalizer.filter.jsonapi:
    

    Nit: Should we prefix these with param, because \Drupal\jsonapi\Routing\Param\OffsetPage?

  3. +++ b/jsonapi.services.yml
    @@ -84,10 +102,6 @@ services:
    -  access_check.jsonapi.custom_query_parameter_names:
    
    +++ /dev/null
    @@ -1,59 +0,0 @@
    -class CustomQueryParameterNamesAccessCheck implements AccessInterface {
    

    I don't understand yet why we're removing this, but I think I'll understand it later in the patch :)

  4. +++ b/src/Context/FieldResolver.php
    @@ -79,8 +79,8 @@ class FieldResolver {
        * @return string
        *   The mapped field name.
        */
    -  public function resolveInternal($external_field_name) {
    -    $resource_type = $this->currentContext->getResourceType();
    +  public function resolveInternal($entity_type_id, $bundle, $external_field_name) {
    +    $resource_type = $this->resourceTypeRepository->get($entity_type_id, $bundle);
    

    Nit: Docblock not updated.

  5. +++ b/src/Normalizer/EntityConditionGroupNormalizer.php
    @@ -0,0 +1,40 @@
    +class EntityConditionGroupNormalizer implements DenormalizerInterface {
    
    +++ b/src/Normalizer/EntityConditionNormalizer.php
    @@ -0,0 +1,83 @@
    +class EntityConditionNormalizer implements DenormalizerInterface {
    
    +++ b/src/Normalizer/FilterNormalizer.php
    @@ -0,0 +1,246 @@
    +class FilterNormalizer implements DenormalizerInterface {
    

    Let's mark all of these @internal! (Didn't cite them all.)

  6. +++ b/src/Normalizer/EntityConditionGroupNormalizer.php
    @@ -0,0 +1,40 @@
    +  /**
    +   * The interface or class that this Normalizer supports.
    +   *
    +   * @var string
    +   */
    
    +++ b/src/Normalizer/EntityConditionNormalizer.php
    @@ -0,0 +1,83 @@
    +
    +  /**
    +   * The interface or class that this Normalizer supports.
    +   *
    +   * @var string
    +   */
    +  protected $supportedInterfaceOrClass = EntityCondition::class;
    

    Nit: Should be just @inheritdoc.

  7. +++ b/src/Normalizer/EntityConditionGroupNormalizer.php
    @@ -0,0 +1,40 @@
    +    return $type == $this->supportedInterfaceOrClass;
    
    +++ b/src/Normalizer/EntityConditionNormalizer.php
    @@ -0,0 +1,83 @@
    +    return $type == $this->supportedInterfaceOrClass;
    

    Nit: ===

  8. +++ b/src/Normalizer/EntityConditionNormalizer.php
    @@ -0,0 +1,83 @@
    +  protected function validate($data) {
    

    👏 This is not only providing a helpful error message, it's also using the right HTTP status code!

  9. +++ b/src/Normalizer/FilterNormalizer.php
    @@ -0,0 +1,246 @@
    +      if ($key == static::ROOT_ID) {
    

    ===

  10. +++ b/src/Normalizer/FilterNormalizer.php
    @@ -0,0 +1,246 @@
    +        ->resolveInternal(
    

    Supernit: I'd expect this to still be on the previous line.

  11. +++ b/src/Query/EntityConditionGroup.php
    @@ -0,0 +1,60 @@
    +   * The condition group conjunction. ¶
    

    Nit: trailing space.

  12. +++ b/tests/src/Kernel/Normalizer/EntityConditionGroupNormalizerTest.php
    @@ -0,0 +1,59 @@
    +class EntityConditionGroupNormalizerTest extends KernelTestBase {
    
    +++ b/tests/src/Kernel/Normalizer/EntityConditionNormalizerTest.php
    @@ -0,0 +1,69 @@
    +class EntityConditionNormalizerTest extends KernelTestBase {
    
    +++ b/tests/src/Kernel/Normalizer/FilterNormalizerTest.php
    @@ -0,0 +1,117 @@
    +class FilterNormalizerTest extends KernelTestBase {
    
    +++ b/tests/src/Kernel/Normalizer/OffsetPageNormalizerTest.php
    @@ -0,0 +1,71 @@
    +class OffsetPageNormalizerTest extends KernelTestBase {
    
    +++ b/tests/src/Kernel/Normalizer/SortNormalizerTest.php
    @@ -0,0 +1,103 @@
    +class SortNormalizerTest extends KernelTestBase {
    
    +++ b/tests/src/Kernel/Query/FilterTest.php
    @@ -0,0 +1,170 @@
    +class FilterTest extends JsonapiKernelTestBase {
    

    I'll need to review this in-depth later, to ensure all edge cases are covered. But obviously this is a huge step forward! 👏

matthew.perry’s picture

Hey @gabesullice, great work thanks for working on this issue. A few minor nitpicks nothing that impacts the current functionality just more so keeping inline with the rest of the module.

  1. +++ b/src/Routing/JsonApiParamEnhancer.php
    @@ -2,14 +2,14 @@
    +use \Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
    

    Should remove the leading \ to keep the syntax the same.

  2. +++ b/src/Routing/JsonApiParamEnhancer.php
    @@ -17,20 +17,33 @@ use Symfony\Component\Routing\Route;
    +   * @var Symfony\Component\Serializer\Normalizer\DenormalizerInterface
    ...
    +   * @var Symfony\Component\Serializer\Normalizer\DenormalizerInterface
    

    Add leading \ to keep inline with the reset of the module.

-----
As far as functionality goes I've dropped the patch into the environment and right away I'm seeing issues with code that was running fine prior to the patch.

+++ b/jsonapi.services.yml
@@ -84,10 +102,6 @@ services:
-  access_check.jsonapi.custom_query_parameter_names:

This route has been removed but the references to it as a route requirement have not been removed in jsonapi/src/Routing/Routes.php which is causing a server 500 error:

InvalidArgumentException: No check has been registered for access_check.jsonapi.custom_query_parameter_names in Drupal\Core\Access\CheckProvider->loadCheck() (line 97 of /var/www/html/core/lib/Drupal/Core/Access/CheckProvider.php).

Aside from that I can't seem to get my filters working again without adding back in 'IS NULL' and 'IS NOT NULL' the query that gets generated if you use path = NULL uses an INNER JOIN and will not filter down the data correctly.
If I update the code to allow path IS NULL then my filter functions as expected. For some added background information I'm trying to filter on an field which is a content reference when there is no reference value saved (IS NULL) which I'm expecting to generate the following condition: node__field_my_content.field_my_content_target_id IS NULL

gabesullice’s picture

Re:

+++ b/jsonapi.services.yml
@@ -84,10 +102,6 @@ services:
-  access_check.jsonapi.custom_query_parameter_names:
I don't understand yet why we're removing this, but I think I'll understand it later in the patch :)

This route has been removed but the references to it as a route requirement have not been removed in jsonapi/src/Routing/Routes.php

Nice catch everyone. This wasn't intentional, I just missed it when I re-rolled the patch. That's added back in now.

The attached fixes all the nits and docs issues, but does not yet have support for the IS NULL/IS NOT NULL operators.

@matthew.perry I did some digging into the support for them and it looks like it snuck in through this issue: #2878654: [FEATURE] Implement validation of filter params. Because of that, I'll work on putting support back in.

Those issues are unfortunately the kind of thing we have to deal with when we're refactoring largely untested code :( BUT, I'm so happy you're here to test with real use cases.

Honestly, I'm a bit shocked that those operators ever worked. It means that the entity query system also supports undocumented behavior. @Wim Leers, do you have any thoughts on that? Should we file an issue to validate/assert query operators or update docs? My assumption is that this only works when the query is executed against SQL storage.

gabesullice’s picture

The attached adds back tests that weren't running because of namespace issues. It also fixes a few issues that arose because of that.

@matthew.perry, I tried to re-implement the IS NULL and IS NOT NULL operators in this patch using the supported exists/notExists methods on the query interface. My hope is that this will work for you, though I'm not sure that it will since it doesn't really replicate what was happening unintentionally before.

Will you try out your queries and let me know?

matthew.perry’s picture

@gabesullice, we're getting pretty close.

  1. +++ b/src/Normalizer/EntityConditionNormalizer.php
    @@ -0,0 +1,93 @@
    +    if (count(array_diff([static::PATH_KEY, static::VALUE_KEY], array_keys($data)))) {
    ...
    +    if (in_array($data[static::OPERATOR_KEY], ['IS NULL', 'IS NOT NULL']) && isset($data[static::VALUE_KEY])) {
    

    My filters using IS NULL or IS NOT NULL never make it past the validate call the way it's currently setup. Since every time it goes through validate it hits if (count(array_diff([static::PATH_KEY, static::VALUE_KEY], array_keys($data)))) { which it fails since currently it only has these keys set:

    • filter[test-filter][condition][path]:field_my_test_field
    • filter[test-filter][condition][operator]:IS NULL
    • filter[test-filter][condition][memberOf]:and-group

    Something like this works:

      /**
       * Validates the filter has the required fields.
       */
      protected function validate($data) {
        if (!in_array($data[static::OPERATOR_KEY], EntityCondition::$allowedOperators)) {
          $reason = "The '" . $data[static::OPERATOR_KEY] . "' operator is not allowed in a filter parameter.";
          throw new BadRequestHttpException($reason);
        }
    
        if (in_array($data[static::OPERATOR_KEY], ['IS NULL', 'IS NOT NULL']) && isset($data[static::VALUE_KEY])) {
          $reason = "Filters using the '" . $data[static::OPERATOR_KEY] . "' operator should not provide a value.";
          throw new BadRequestHttpException($reason);
        }
        elseif (in_array($data[static::OPERATOR_KEY], ['IS NULL', 'IS NOT NULL']) && !isset($data[static::VALUE_KEY])) {
          // Return early when using IS NULL / IS NOT NULL to avoid VALUE_KEY check.
          return;
        }
    
        if (count(array_diff([static::PATH_KEY, static::VALUE_KEY], array_keys($data)))) {
          $reason = "All filters must provide at least a valid path and value.";
    
          if (!isset($data[static::PATH_KEY])) {
            $reason .= " Missing '" . static::PATH_KEY . "' key.";
          }
          elseif (!isset($data[static::VALUE_KEY])) {
            $reason .= " Missing '" . static::VALUE_KEY . "' key.";
          }
    
          throw new BadRequestHttpException($reason);
        }
      }
    
  2. +++ b/src/Normalizer/EntityConditionNormalizer.php
    @@ -0,0 +1,93 @@
    +        $reason .= " Missing '" . static::VALUE . "' key.";
    

    This needs to be static::VALUE_KEY

I put an early return inside the EntityConditionNormalizer:validate($data) method so I could get past the validation issue to check the results. The query and end results are all as I expect them to be.

gabesullice’s picture

@matthew.perry

Thanks for the quick response. I've added some tests to the validate work and tried to give some better structure to the method. Hopefully that should resolve everything.

e0ipso’s picture

Status: Needs review » Needs work
FileSize
4.75 KB

This is outstanding work everyone! This code was always the part of the code that we felt most insecure about, but was left untouched because it worked (except in some edge cases). Now we can get in some better test coverage.


1. FieldResolver::resolveInternal(). Why the change $resource_type = $this->currentContext->getResourceType(); by $resource_type = $this->resourceTypeRepository->get($entity_type_id, $bundle);? This change changes the signature of a public method in a class marked as @internal. This does not break BC, but I'm curious on why we need this change. Note that even though this is internal, JSON API Extras makes use of that method, so it will be broken for it. Let's make sure there's a good reason for this.
2. I'd like to have a AndConditionGroup and OrConditionGroup instead of EntityConditionGroup where we need to check for the ->conjunction(). This doesn't need to be fixed here, but I'd like to see an issue linked here before merging so we don't forget.

Note: I didn't finish the review. I will pick it up again later today. I also attached some nit fixes in an interdiff.

e0ipso’s picture

FileSize
4.2 KB
141.69 KB

3. Unfortunately serializer.normalizer.htt_exception.jsonapi cannot be dropped. We have not to support the typo to ensure BC. We can duplicate the service and mark serializer.normalizer.htt_exception.jsonapi deprecetated so we can prune it by jsonapi@8.x-2.x
4. I see changes like $page_param->getOffset() ➡️ $page_param->offset(), $page_param->size() ➡️ $page_param->size(), … If possible I'd like to keep the old names to make the changes in internal API surface minimal.


Added an interdiff with #20 and the updated patch. The updated patch only contains some minor fixes. The enumerated change requests are not implemented but left for feedback.

e0ipso’s picture

Status: Needs work » Needs review
e0ipso’s picture

Status: Needs review » Needs work
e0ipso’s picture

Status: Needs work » Needs review
FileSize
142.04 KB

Status: Needs review » Needs work

The last submitted patch, 25: 2874601--querybuilder-refactor--25.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

e0ipso’s picture

hmm. I'm not having those errors in my local, maybe they're due to a PHPUnit mismatch.

e0ipso’s picture

Status: Needs work » Needs review
e0ipso’s picture

Assigned: Unassigned » gabesullice

There you go. Sorry for the noise. Back to you @gabesullice. Thanks for the amazing work here, everything is much clearer now!

gabesullice’s picture

1. FieldResolver::resolveInternal(). Why the change $resource_type = $this->currentContext->getResourceType(); by $resource_type = $this->resourceTypeRepository->get($entity_type_id, $bundle);? This change changes the signature of a public method in a class marked as @internal. This does not break BC, but I'm curious on why we need this change. Note that even though this is internal, JSON API Extras makes use of that method, so it will be broken for it. Let's make sure there's a good reason for this.

This isn't a strictly necessary. However, I'd like to keep this change. I'd like us to progressively factor out CurrentContext whenever we get the chance. It's a very leaky abstraction and it has worked its way into everything. It makes many of our tests unnecessarily complicated and hides a bunch of state behind an innocuous looking interface.

2. I'd like to have a AndConditionGroup and OrConditionGroup instead of EntityConditionGroup where we need to check for the ->conjunction(). This doesn't need to be fixed here, but I'd like to see an issue linked here before merging so we don't forget

Done. See #2913540: (followup) Consider refactoring EntityConditionGroup into And/OrConditionGroup

3. Unfortunately serializer.normalizer.htt_exception.jsonapi cannot be dropped. We have not to support the typo to ensure BC. We can duplicate the service and mark serializer.normalizer.htt_exception.jsonapi deprecetated so we can prune it by jsonapi@8.x-2.x

Done.

4. I see changes like $page_param->getOffset() ➡️ $page_param->offset(), $page_param->size() ➡️ $page_param->size(), … If possible I'd like to keep the old names to make the changes in internal API surface minimal.

Done.


@e0ipso, it's a PHPUnit version thing. I ran into that too. You need to use setExpectedException not expectedException

gabesullice’s picture

@matthew.perry care to give this yet another shot?

garphy’s picture

Status: Needs review » Needs work

I tested the latest patch (#30) on my environment. It seems that it break "nested includes"

I've an article node type which contains a field_media entity reference field to media entities.

media entities of type image have a field_media_image field of type image.

I used to successfully query nodes like this :
GET /fr/jsonapi/node/article?include=field_media,field_media.field_media_image

With this patch, if field_media.field_media_image is present in the include parameter, I get a 400 response :

{"errors":[{"title":"Bad Request","status":400,"detail":"Invalid nested filtering. Invalid entity reference \u0022field_media\u0022.","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html#sec10.4.1"},"code":0}]}

e0ipso’s picture

@garphy it would be incredibly helpful if you could provide a test for that failure, so we can make sure that the next patch fixes it.

garphy’s picture

Here's a basic additional test which tries to include the term entity from an article node and the vocabulary entity from the included term.

The "new-test-only" only adds the new test to check that it pass on current HEAD.

Status: Needs review » Needs work

The last submitted patch, 34: 2874601-34-querybuilder-refactor.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

garphy’s picture

I think the issue originate from expandContext() which work with the original data from the request and fails when $resource_type is not the base entity type of the request.

Status: Needs review » Needs work

The last submitted patch, 36: 2874601-36-querybuilder-refactor.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

matthew.perry’s picture

Sorry I was so long getting back, the base filtering, sorting, ordering seem to work fine for me based on the patch: 2874601-30-querybuilder-refactor.patch

@garphy, @e0ipso
I was testing around I seem to be able to nest included relations to work:

Content Type -> Event -> has an entity reference to a content type of invitees (field_participants)
Content Type -> Invitee ->has an entity reference to a user (field_invited_user)

I seem to have no problems using patch #30 to include multiple relations:
{{api_url}}/jsonapi/node/event?include=field_participants,field_participants.field_invited_user
The results are a list of events with all the included Invitee nodes and all of the included users from the invitee nodes.

If I want the field to return only a specific field/fields I just need to specify fields[user--user]=uid
which results in the users returned in the include to only include the uid.

It most definitely doesn't like the trying to load field_tags.vid relation from articles, but it handles loading the author and the author's image:
{{api_url}}/jsonapi/node/article/?include=uid,uid.user_picture

Wim Leers’s picture

Issue tags: +API-First Initiative

rel 2878654
#17:

It means that the entity query system also supports undocumented behavior. @Wim Leers, do you have any thoughts on that?

None, other than that I share your shock, but at the same time I'm not surprised.
I did some actual digging, looks like this is just explicitly supported, see \Drupal\Core\Entity\Query\Null\Condition (and also \Drupal\Core\Entity\Query\Sql\ConditionAggregate::compile + \Drupal\Core\Entity\Query\Sql\Condition::compile()).

Should we file an issue to validate/assert query operators or update docs?

Yes!

My assumption is that this only works when the query is executed against SQL storage.

That'd be my assumption too.


#22 I like how you're actively thinking about reducing BC breaks :)


#30:

This isn't a strictly necessary. However, I'd like to keep this change. I'd like us to progressively factor out CurrentContext whenever we get the chance. It's a very leaky abstraction and it has worked its way into everything. It makes many of our tests unnecessarily complicated and hides a bunch of state behind an innocuous looking interface.

OMG YES.


@garphy (in #32 + #34): nice catch! Glad to have people testing big changes like this patch before it's committed :) And thanks to @matthew.perry for confirming that bug, but pointing out that similar but different cases do work.


+++ b/jsonapi.services.yml
@@ -1,5 +1,26 @@
+  serializer.normalizer.http_exception.jsonapi:
     class: Drupal\jsonapi\Normalizer\HttpExceptionNormalizer
     arguments: ['@current_user']
     tags:

@@ -120,3 +138,10 @@ services:
+  serializer.normalizer.htt_exception.jsonapi: # Use 'serializer.normalizer.http_exception.jsonapi' instead.
+    class: Drupal\jsonapi\Normalizer\HttpExceptionNormalizer
+    arguments: ['@current_user']
+    tags:
+      - { name: normalizer, priority: 1 }

This can be achieved in a much simpler way, without breaking BC:

serializer.normalizer.htt_exception.jsonapi:
  alias: serializer.normalizer.http_exception.jsonapi

And even better:

serializer.normalizer.htt_exception.jsonapi:
  alias: serializer.normalizer.http_exception.jsonapi
  deprecated: deprecated: The "%service_id%" service is deprecated. You should use the 'serializer.normalizer.http_exception.jsonapi' service instead.

(You can find examples of this in core.services.yml.)

garphy’s picture

I also experienced a similar issue with nested includes with the current version. See #2916074: [PP-1] Test coverage: FieldResolver: path validation for computed fields.
I'll try to test the same workaround on this branch to see what testbot think.

garphy’s picture

Status: Needs work » Needs review
FileSize
142.71 KB
789 bytes

Ok, let's see if it is beaking everything or not.

Status: Needs review » Needs work

The last submitted patch, 41: 2874601-41-querybuilder-refactor.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gabesullice’s picture

Thank you everyone! This was all really good feedback.

@Wim Leers thanks for the awesome review. I love that deprecated feature, that's so clean..

@garphy Thanks for the failing test, that was super useful.

@matthew.perry thank you for your ongoing help.

I think I've found a working solution, although I'm not that happy with it. Since this issue is about testing and maintainability at its heart, I'm leaving the failing test and workaround even though its not explicitly related to the Query namespace. I'm going to file a follow-up issue to address the root cause...

Finally, because it's a more accurate picture of the changes, the included interdiff is between #30 and this one.


If you're interested, more info on the bug:

@garphy, You were right about expandContext being the source of the error, but I think the deeper issue is that when we normalize relationships, we're reusing a method in the top-level normalizer where we probably shouldn't be. Worse, that normalizer modifies its own context (using expandContext) (expanding field names and includes, etc). We need to do this outside of the normalizer, not within it.

gabesullice’s picture

Status: Needs work » Needs review
gabesullice’s picture

Created a related issue for the problems discussed at the end of #43 here: #2917147: chore(Normalizer): Simplification and refactoring

gabesullice’s picture

garphy’s picture

It seems that I still need to add $this->fieldManager->getBaseFieldDefinitions($entity_type_id) to $definitions in FieldResolver::resolveInternal() to be able to include relationships coming from "computed" field.

Let's see if testbot is still happy.

The last submitted patch, 47: 2874601-47-querybuilder-refactor.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

+++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -195,7 +195,10 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
+    if (!isset($context['expanded']) || !$context['expanded']) {

@@ -255,6 +258,7 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
+      'expanded' => TRUE,

This will need @e0ipso's input for sure.

e0ipso’s picture

WRT #49: After a long look, I think that the one time expansion is legit.

WRT #43: I think reusing the top level normalizer is the correct thing to do. Relationships in many ways behave like the top level normalizer. Remember that many aspects of the JSON API spec (nested filters, includes of includes, etc.) are a big recursion. However I will agree that there are things that truly apply only to the top level, like the `expandContext`.

----

This is a fantastic work. I'm making the call and merging it. I'm also making something special, since not all issues are the same:

Gabe gets 4 authorship commits.
Matthew gets 2 authorship commits.
Simon gets 2 authorship commits.
Wim gets 1 authorship commits.
Mateu gets to work with you all.

  • e0ipso committed 07927d7 on 8.x-1.x authored by gabesullice
    Issue #2874601 by gabesullice, garphy, e0ipso, matthew.perry, Wim Leers...
  • e0ipso committed 13b6a5d on 8.x-1.x authored by garphy
    Issue #2874601 by gabesullice, garphy, e0ipso, matthew.perry, Wim Leers...
  • e0ipso committed 74237b0 on 8.x-1.x authored by garphy
    Issue #2874601 by gabesullice, garphy, e0ipso, matthew.perry, Wim Leers...
  • e0ipso committed 93e371c on 8.x-1.x authored by gabesullice
    Issue #2874601 by gabesullice, garphy, e0ipso, matthew.perry, Wim Leers...
  • e0ipso committed a88fae5 on 8.x-1.x authored by gabesullice
    Issue #2874601 by gabesullice, garphy, e0ipso, matthew.perry, Wim Leers...
  • e0ipso committed ba8a9b3 on 8.x-1.x authored by matthew.perry
    Issue #2874601 by gabesullice, garphy, e0ipso, matthew.perry, Wim Leers...
  • e0ipso committed c536915 on 8.x-1.x authored by gabesullice
    Issue #2874601 by gabesullice, garphy, e0ipso, matthew.perry, Wim Leers...
  • e0ipso committed da8b95b on 8.x-1.x authored by matthew.perry
    Issue #2874601 by gabesullice, garphy, e0ipso, matthew.perry, Wim Leers...
  • e0ipso committed f70e834 on 8.x-1.x authored by Wim Leers
    Issue #2874601 by gabesullice, garphy, e0ipso, matthew.perry, Wim Leers...
e0ipso’s picture

Title: [WIP] Refactor QueryBuilder for better testability/maintainability » refactor(QueryBuilder): Improve testability/maintainability
Status: Needs review » Fixed
Wim Leers’s picture

🎉👏

This is a huge leap forward!

OTOH, I think that @gabesullice was perhaps still working on this, so it may have been committed a bit too fast :P But I'm pretty sure he doesn't mind continuing in a follow-up issue, that'll make reviews much easier for sure!

e0ipso’s picture

Right. That's the main reason why I said I'm making the call and merging it. This was a point where everything was working and it is way nicer than it used to be.

Reviewing interdiffs only is dangerous. Reviewing the whole giant patch every time is tedious. I think that we can now build on top of this much quicker.

Wim Leers’s picture

👌

Totally agreed!

e0ipso’s picture

I think @skyred has found a bug with this patch regarding groups. He may be posting a failing test here soon.

e0ipso’s picture

My previous comment was incorrect. This has been cleared in the API-First Weekly Meeting.

YAY!

Wim Leers’s picture

Wim Leers’s picture

Republishing. Apparently this issue was unpublished somehow.

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Wim Leers’s picture