After following the instructions locally AND on simplytest.me with a fresh install, and the exact example here: https://www.drupal.org/node/2803141

with this:

filter[and-group][group][conjunction]:AND
filter[name-filter][condition][field]:uid.name
filter[name-filter][condition][value]:admin
filter[name-filter][condition][memberOf]:and-group
filter[status-filter][condition][field]:status
filter[status-filter][condition][field]:1
filter[status-filter][condition][memberOf]:and-group

this ("path" instead of "field"):

filter[and-group][group][conjunction]:AND
filter[name-filter][condition][path]:uid.name
filter[name-filter][condition][value]:admin
filter[name-filter][condition][memberOf]:and-group
filter[status-filter][condition][path]:status
filter[status-filter][condition][field]:1
filter[status-filter][condition][memberOf]:and-group

this:

filter[and-group][group][conjunction]:AND
filter[name-filter][condition][field]:uid.name
filter[name-filter][condition][value]:admin
filter[name-filter][condition][operator]:%3D
filter[name-filter][condition][memberOf]:and-group
filter[status-filter][condition][field]:status
filter[status-filter][condition][value]:1
ilter[status-filter][condition][operator]:%3D
filter[status-filter][condition][memberOf]:and-group

Did not work, leading to a:

{
  "errors": [
    {
      "title": "Internal Server Error",
      "status": 500,
      "detail": "SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')AND (node_field_data.type = 'article') \nGROUP BY base_table.vid, base_table.nid' at line 4: SELECT base_table.vid AS vid, base_table.nid AS nid\nFROM \n{node} base_table\nINNER JOIN {node_field_data} node_field_data ON node_field_data.nid = base_table.nid\nLEFT OUTER JOIN {users} users ON users.uid = node_field_data.uid\nINNER JOIN {users_field_data} users_field_data ON users_field_data.uid = users.uid\nWHERE  (node_field_data.status = :db_condition_placeholder_0) AND (users_field_data.name LIKE :db_condition_placeholder_1 ESCAPE '\\\\') AND()AND (node_field_data.type = :db_condition_placeholder_2) \nGROUP BY base_table.vid, base_table.nid\nLIMIT 51 OFFSET 0; Array\n(\n    [:db_condition_placeholder_0] => 1\n    [:db_condition_placeholder_1] => admin\n    [:db_condition_placeholder_2] => article\n)\n",
      "links": {
        "info": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.5.1"
      },
      "code": 0
    }
  ]
}
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dremy created an issue. See original summary.

Grimreaper’s picture

Version: 8.x-1.0-alpha3 » 8.x-1.x-dev

Hello,

I can reproduce the bug locally but what I don't understand why I can't reproduce the bug on simplytest.me and why the automated tests are green while I use the same URL : jsonapi/node/article?filter[uid.name][value]=admin&include=uid, as in jsonapi/tests/src/Functional/JsonApiFunctionalTest.php testRead case 13: "Test filtering when using short syntax."

Grimreaper’s picture

A precision. I have not exactly the same error.

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'node_field_data.uid_int' in 'on clause': SELECT base_table.vid AS vid, base_table.nid AS nid
FROM
{node} base_table
INNER JOIN {node_field_data} node_field_data ON node_field_data.nid = base_table.nid
LEFT OUTER JOIN {users} users ON users.uid = node_field_data.uid_int
INNER JOIN {users_field_data} users_field_data ON users_field_data.uid = users.uid
WHERE (users_field_data.name LIKE :db_condition_placeholder_0 ESCAPE '\\') AND (node_field_data.type = :db_condition_placeholder_1)
GROUP BY base_table.vid, base_table.nid
LIMIT 51 OFFSET 0; Array
(
[:db_condition_placeholder_0] => admin
[:db_condition_placeholder_1] => article
)

Grimreaper’s picture

I found what code adds "_int" modules/contrib/dynamic_entity_reference/src/Query/Tables.php

Arf, I wanted to test dynamic entity reference module for the entity share module, and it impacts other things than dynamic entity reference fields.

I will retest without this module to see if I can reproduce the bug reported by @dremy.

Grimreaper’s picture

About the filtering examples on https://www.drupal.org/node/2803141, I haved tested on jsonapi/node/article with a Drupal 8.3.0-beta1 with the last JSON API dev version.

Cases 1 to 4 are ok.
I got a memory limit for the case 5 :

jsonapi/node/article?filter[and-group][group][conjunction]=AND&filter[or-group][group][conjunction]=OR&filter[or-group][group][memberOf]=and-group&filter[admin-filter][condition][path]=uid.name&filter[admin-filter][condition][value]=admin&filter[admin-filter][condition][memberOf]=and-group&filter[sticky-filter][condition][path]=sticky&filter[sticky-filter][condition][value]=1&filter[sticky-filter][condition][memberOf]=or-group&filter[promote-filter][condition][path]=promote&filter[promote-filter][condition][value]=1&filter[promote-filter][condition][memberOf]=or-group

Is it ok to close the issue or should we investigate this last example?

Grimreaper’s picture

Issue created for the compatibility with dynamic entity reference #2867102: Incompatibility with jsonapi.

Should I create a follow up in the JSON API issue queue?

e0ipso’s picture

Status: Active » Postponed (maintainer needs more info)

@dremy just to make sure you should be using = instead of :.

@Grimreaper it seems that the issue is not with JSON API. We are only using core's Entity Query to compose database queries. Can you confirm this is true?

Grimreaper’s picture

Hello,

@eOipso, yeah the bug I encountered was linked to dynamic_entity_reference and not JSON API, so I opened an issue in its issue queue.

BUT in the documentation, without dynamic entity reference enabled, on the last example of nested group, I got a memory limit.

I just rechecked on simplytest.me and I also got 500 error (I think also memory limit).

So if you (or someone else) can test the last documentation example or I think I will (next week) write a test that should break.

Grimreaper’s picture

Assigned: Unassigned » Grimreaper

I still have a memory limit with the example 5.

I will try to provide a patch to highlight the bug.

Grimreaper’s picture

Assigned: Grimreaper » Unassigned
Status: Postponed (maintainer needs more info) » Needs review
FileSize
2.65 KB

Let's break the Test bot...

Status: Needs review » Needs work

The last submitted patch, 10: jsonapi-test_grouped_filter-2841850-10.patch, failed testing.

Grimreaper’s picture

Assigned: Unassigned » Grimreaper

So there is actually an infinite loop in src/Query/GroupOption.php hasChild().

I will first provide a patch to have tests for all the documentation examples and then create a patch to fix the bug.

Grimreaper’s picture

Status: Needs work » Needs review
FileSize
6.64 KB
4.65 KB

Tests should be green for the first 4 cases.

Grimreaper’s picture

Huge thanks to barig https://www.drupal.org/u/barig for helping me with this patch.

The problem was that when building the filters tree, there was a case not handled when puting a QueryOptionInterface into a group already in a group.

But now I have an SQL error:

Fatal error: Call to undefined method Drupal\Core\Entity\Query\Sql\Condition::orConditionGroup() in /project/www/modules/contrib/jsonapi/src/Query/GroupOption.php on line 112

And I also have a warning.

I will fix that.

Grimreaper’s picture

And here is an almost final patch that fixes the bugs.

The problem I see is that the interface for the method apply should be changed.

Thanks for the review.

e0ipso’s picture

Status: Needs review » Needs work

@Grimreaper, can we get a test that fails before the patch and then succeeds with the patch applied? This is the part of the code that gabesullice originally wrote, and I'm not super familiar with it, so I want to be extra careful with it.

Grimreaper’s picture

@e0ipso, The green tests in comment 4 and the failing test in comment 10 cover the documentation examples.

In comment 15: there are tests for the five examples and the fix.

Do you want some unit or kernel tests too? I am not very familiar with it.

And also, I forgot to mention in my previous comment that the code of this part is very hard to understand and to debug with all those array_reduce with calls to anonymous functions. I don't want to blame anyone, just saying that I found it very hard to understand this part of the module, as the code is condensed and there are almost no comment.

So yeah, I understand very well that you want to be very careful.

gabesullice’s picture

  1. +++ b/src/Query/GroupOption.php
    @@ -92,6 +92,10 @@ class GroupOption implements QueryOptionInterface, QueryOptionTreeItemInterface
    +    // Direct child.
    

    This is a great catch! This must have taken a tremendous effort to find and fix. Good work.

  2. +++ b/src/Query/GroupOption.php
    @@ -102,15 +106,18 @@ class GroupOption implements QueryOptionInterface, QueryOptionTreeItemInterface
    +  public function apply($query, $original_query = NULL) {
    
    @@ -121,8 +128,8 @@ class GroupOption implements QueryOptionInterface, QueryOptionTreeItemInterface
           }, $group);
    

    It's been a while since I've worked on this, so bear with me...

    I not 100% sure this is the issue you were addressing by adding $original_query, but hopefully if I can explain it, you can correct me if I'm wrong.

    Prior to introducing $original_query here, a query like:

    groupA
      groupB
      groupC
      groupD
    

    would actually result in:

    groupA
      groupB
        groupC
          groupD
    

    By using $original_query, we now add these child groups to groupA properly.

    If this is the case, I would like to suggest a smaller change.

    I don't think you need $original_query at all. Instead, it's a one line change:

  3. +++ b/src/Query/GroupOption.php
    @@ -121,8 +128,8 @@ class GroupOption implements QueryOptionInterface, QueryOptionTreeItemInterface
    -        return $child->apply($group);
    

    Just becomes:

    $child->apply($group);
    return $group;
    

    The significant difference is that in the second version, we do not return the result of $child->apply(..). This keeps the groups from being nested on subsequent applications of any sibling groups. It also means we can keep the simple interface of ->apply(query) with only a single argument uniform across all the different QueryOptions.

Thank you so much for all your work! I understand that this code can be a bit mind-warping :\. I've wanted to refactor it for some time to make it more explicit, but also because I think there are some concepts that could be applicable beyond just this module.

gabesullice’s picture

Here is a quick change, I haven't run all the tests on this though. So we'll see...

Status: Needs review » Needs work

The last submitted patch, 19: jsonapi-test_grouped_filter-2841850-19.patch, failed testing.

Grimreaper’s picture

Hello,

@gabesullice Thanks for your reply.

// Direct child.

Yep, if we were not two people on the issue to debug I would still be on it :D

About the $original_query, the problem is that without it, the first time apply() is executed for the first group of the tree, $query is an object of type Drupal\Core\Entity\Query\Sql\Query, but

$group = $query->or/andConditionGroup();

$group is an object of type Drupal\Core\Entity\Query\Sql\Condition, so when creating the groups inside the group the object is not of the same type and there is no or/andConditionGroup() method on this object.

If we want to make this request directly it would be:

  $entity_query = \Drupal::entityQuery("node");
  $or_group = $entity_query->orConditionGroup()
    ->condition('sticky', 1)
    ->condition('promote', 1);
  $and_group = $entity_query->andConditionGroup()
    ->condition('uid.entity.name', 'admin')
    ->condition($or_group);
  $entity_query->condition($and_group);
  $result = $entity_query->execute();

To create the groups, you can see that we use $entity_query for all the groups.

Is the problem more understandable?

What is the next step?

e0ipso’s picture

@Grimreaper I am concerned about mutable parameters, specially what you are describing. Thanks for uncovering all that.

Do you think we can have a patch that fixes that? So $query stays as Drupal\Core\Entity\Query\Sql\Query and we pass around a separate object for the Drupal\Core\Entity\Query\Sql\Condition? We can do this in a follow up.

I am good with merging the patch in #15, but the name $original_query confuses me a lot. Do you think you can find a better name for it?

Very close!

Grimreaper’s picture

Hello,

@e0ipso, here a patch that change the wording.

But the problem of variable naming is also in src/Query/ConditionOption.php apply($query) because in that object, query can be a query object or a group and it is ok in that case (I think it was made with that case in mind).

Maybe the solution would be 2 distinct interfaces??? or not.

Grimreaper’s picture

Hello,

@e0ipso, sorry but I don't know when I will have time to help on variable naming and interface.

You can merge the patch if you want to fix the bug and we can see for a better handling of the variables in another issue later.

Thanks.

gabesullice’s picture

@Grimreaper I just want to let you know that this hasn't fallen under the radar.

I've finally understood your issue with needing the query in order to generate condition groups.

I've actually spent the weekend refactoring the query stuff to make the code easier to reason about and including substantive tests for everything.

I'd love for you to take a look and confirm that your issue is resolved once that's done.

gabesullice’s picture

@Grimreaper I hate to keep enlisting you for more work... would you mind applying the patch from #2874601: refactor(QueryBuilder): Improve testability/maintainability to ensure that it works as expected?

Grimreaper’s picture

Hello,

@gabesullice, I have just tested your patch from #2874601-5: refactor(QueryBuilder): Improve testability/maintainability with the query:

jsonapi/node/article?filter[and-group][group][conjunction]=AND&filter[or-group][group][conjunction]=OR&filter[or-group][group][memberOf]=and-group&filter[admin-filter][condition][path]=uid.entity.name&filter[admin-filter][condition][value]=admin&filter[admin-filter][condition][memberOf]=and-group&filter[sticky-filter][condition][path]=sticky&filter[sticky-filter][condition][value]=1&filter[sticky-filter][condition][memberOf]=or-group&filter[promote-filter][condition][path]=promote&filter[promote-filter][condition][value]=1&filter[promote-filter][condition][memberOf]=or-group

I had to change uid.name into uid.entity.name (just like the entity query :)).

The result is ok.

Warning: I didn't take a look at the patch.

gabesullice’s picture

@Grimreaper awesome!

Yes, expanding uid.name -> uid.entity.name internally was an oversight in the existing patch, I've fixed that locally but haven't put up a new patch.

Thanks for your help!

e0ipso’s picture

Status: Needs review » Needs work

I've fixed that locally but haven't put up a new patch.

Do you still have that patch? Maybe someone can finish it if it's not quite ready.

gabesullice’s picture

@e0ipso

I would love for your help pushing #2874601: refactor(QueryBuilder): Improve testability/maintainability over the top. All my latest is in that issue queue.

The only remaining issue is one Functional test that won't pass. It's returning a 502 internal error, but I couldn't figure the root cause out and couldn't find more time to work on it.

Grimreaper’s picture

Hello,

@e0ipso: Maybe you can merge the patch from comment 23 to fix the bug and then when it will be ready merge the refactor of @gabesullice?

e0ipso’s picture

@Grimreaper sorry, I'm unsure about the stat of this. I don't know if this patch still needs to be applied if #2874601: refactor(QueryBuilder): Improve testability/maintainability is moved forward. @gabesullice is a maintainer and he can manage this.

I leave you this in your hands @gabesullice.

Grimreaper’s picture

Hello,

In the Entity Share module, I have just added the possibility to add filters and groups on channels (in short: JSON API Collections).

But it does not work for complicated cases due to this issue.

Any news about merging the patch or commiting the refactoring of the other issue?

Thanks for any response.

e0ipso’s picture

@gabesullice any chance you can help move this forward?

gabesullice’s picture

@Grimreaper would you mind giving the patch here: https://www.drupal.org/node/2874601#comment-12274806 a try?

If it fixes the issue for you, feel free to mark it RTCB.

Grimreaper’s picture

Status: Needs work » Reviewed & tested by the community

Hello,

@gabesullice: just tested patch from #2874601-14: refactor(QueryBuilder): Improve testability/maintainability, it works ok with the query from comment 5 in this issue.

I am going to make a bof at Drupalcon Vienna about Entity share right now.

Grimreaper’s picture

Status: Reviewed & tested by the community » Fixed

Hello,

As #2874601: refactor(QueryBuilder): Improve testability/maintainability has been fixed, I have re-tested with the last version of JSON API.

It is ok.

Changing status to fixed.

gabesullice’s picture

@Grimreaper thank you!

Grimreaper’s picture

Status: Fixed » Needs review

Hello,

Sorry for re-opening but is it possible to at least commit the tests added by the patch of this issue please?

Because in 2874601, it doesn't seem that those tests have been added.

Thanks.

e0ipso’s picture

I love extra tests! I will take care of this soon. Thanks @Grimreaper.

e0ipso’s picture

Rolled back the tests in #23.

e0ipso’s picture

Status: Needs review » Fixed

Back to fixed. Thanks all!

e0ipso’s picture

  • e0ipso committed e39a904 on 8.x-1.x authored by Grimreaper
    test(Filters): Increase test coverage (#2841850 by Grimreaper,...

Status: Fixed » Closed (fixed)

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