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.
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff-18-20.txt | 7.95 KB | gabesullice |
#20 | 2874601-20-querybuilder-refactor.patch | 133.54 KB | gabesullice |
| |||
#43 | interdiff-30-42.txt | 2.88 KB | gabesullice |
#43 | 2874601-42-querybuilder-refactor.patch | 142.63 KB | gabesullice |
| |||
#47 | interdiff-2874601-42-47.txt | 789 bytes | garphy |
Comments
Comment #2
gabesulliceComment #3
gabesulliceKicking tests.
Comment #5
gabesulliceComment #7
e0ipsoThis is looking good! I didn't do a proper review because of the red tests, but I skimmed through the code and got excited.
Comment #8
e0ipso@gabesullice any luck with this?
Comment #9
e0ipsoIt 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.
Comment #10
matthew.perry CreditAttribution: matthew.perry commented@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:
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.
Comment #11
gabesullice@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.
Comment #12
gabesulliceAlright, 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:
Comment #13
gabesullicePassed! \o/
Just a little more left...
Comment #14
gabesullice@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
andnotExists
methods, but because that is not supported now, let's put that into a feature request. At this time,path <> NULL
andpath = NULL
achieve the same thing as a workaround.Comment #15
Wim Leers@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%
. :)Let's fix this in a separate issue, that'd be committed immediately!
Nit: Should we prefix these with
param
, because\Drupal\jsonapi\Routing\Param\OffsetPage
?I don't understand yet why we're removing this, but I think I'll understand it later in the patch :)
Nit: Docblock not updated.
Let's mark all of these
@internal
! (Didn't cite them all.)Nit: Should be just
@inheritdoc
.Nit: ===
👏 This is not only providing a helpful error message, it's also using the right HTTP status code!
===
Supernit: I'd expect this to still be on the previous line.
Nit: trailing space.
I'll need to review this in-depth later, to ensure all edge cases are covered. But obviously this is a huge step forward! 👏
Comment #16
matthew.perry CreditAttribution: matthew.perry commentedHey @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.
Should remove the leading \ to keep the syntax the same.
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.
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
Comment #17
gabesulliceRe:
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.
Comment #18
gabesulliceThe 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
andIS 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?
Comment #19
matthew.perry CreditAttribution: matthew.perry commented@gabesullice, we're getting pretty close.
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:Something like this works:
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.
Comment #20
gabesullice@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.
Comment #21
e0ipsoThis 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
andOrConditionGroup
instead ofEntityConditionGroup
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.
Comment #22
e0ipso3. 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 markserializer.normalizer.htt_exception.jsonapi
deprecetated so we can prune it byjsonapi@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.
Comment #23
e0ipsoComment #24
e0ipsoComment #25
e0ipsoComment #27
e0ipsohmm. I'm not having those errors in my local, maybe they're due to a PHPUnit mismatch.
Comment #28
e0ipsoComment #29
e0ipsoThere you go. Sorry for the noise. Back to you @gabesullice. Thanks for the amazing work here, everything is much clearer now!
Comment #30
gabesulliceThis 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.
Done. See #2913540: (followup) Consider refactoring EntityConditionGroup into And/OrConditionGroup
Done.
Done.
@e0ipso, it's a PHPUnit version thing. I ran into that too. You need to use
setExpectedException
notexpectedException
Comment #31
gabesullice@matthew.perry care to give this yet another shot?
Comment #32
garphy CreditAttribution: garphy at ICI LA LUNE commentedI tested the latest patch (#30) on my environment. It seems that it break "nested includes"
I've an
article
node type which contains afield_media
entity reference field tomedia
entities.media
entities of typeimage
have afield_media_image
field of typeimage
.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}]}
Comment #33
e0ipso@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.
Comment #34
garphy CreditAttribution: garphy at ICI LA LUNE commentedHere'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.
Comment #36
garphy CreditAttribution: garphy at ICI LA LUNE commentedI 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.Comment #38
matthew.perry CreditAttribution: matthew.perry commentedSorry 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
Comment #39
Wim Leersrel 2878654
#17:
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()
).Yes!That'd be my assumption too.#22 I like how you're actively thinking about reducing BC breaks :)
#30:
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.
This can be achieved in a much simpler way, without breaking BC:
And even better:
(You can find examples of this in
core.services.yml
.)Comment #40
garphy CreditAttribution: garphy at ICI LA LUNE commentedI 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.
Comment #41
garphy CreditAttribution: garphy at ICI LA LUNE commentedOk, let's see if it is beaking everything or not.
Comment #43
gabesulliceThank 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.Comment #44
gabesulliceComment #45
gabesulliceCreated a related issue for the problems discussed at the end of #43 here: #2917147: chore(Normalizer): Simplification and refactoring
Comment #46
gabesulliceComment #47
garphy CreditAttribution: garphy at ICI LA LUNE commentedIt 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.
Comment #49
Wim LeersThis will need @e0ipso's input for sure.
Comment #50
e0ipsoWRT #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.
Comment #52
e0ipsoComment #53
Wim Leers🎉👏
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!
Comment #54
e0ipsoRight. 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.
Comment #55
Wim Leers👌
Totally agreed!
Comment #56
e0ipsoI think @skyred has found a bug with this patch regarding groups. He may be posting a failing test here soon.
Comment #57
e0ipsoMy previous comment was incorrect. This has been cleared in the API-First Weekly Meeting.
YAY!
Comment #58
Wim Leers@skyred's colleague traced it to #2879860: JSON API Extras overrides not implemented for include properties instead of this issue, and is now working on a fix in #2920194: Inclusion of nested relationships fails for bundle-specific fields.
Comment #59
Wim LeersRepublishing. Apparently this issue was unpublished somehow.
Comment #61
Wim LeersClosed #2874601: refactor(QueryBuilder): Improve testability/maintainability as a duplicate.
Comment #62
Wim LeersGah, copy/paste fail. I of course meant #2900652: Test coverage: non-supported operator should generate a helpful 4xx response.
Comment #63
Wim Leers#2900652: Test coverage: non-supported operator should generate a helpful 4xx response was reopened, to add explicit test coverage.
Another thing that we overlooked: #2926089: Filter operators not working with multiple values.