Hello,

When fixing #2924618: Fix channels operators, and seeing https://www.drupal.org/docs/8/modules/json-api/collections-filtering-sor...

4. Filtering with arrays: Get nodes created by users [admin, john].
You can give multiple values to a filter for it to search in. Next to the field and value keys you can add an operator to your condition. Usually it's "=" but you can also use "IN", "NOT IN", ">", "<", "<>", BETWEEN".

For this example we're going to use the IN operator. Note that I added two square brackets behind the value to make it into an array.

I have tested with the operators, =, <, >, <>, and I obtained fatal error due to malformed requests.

Is it the documentation that is outdated? or did I found a bug?

(in the case of a bug, I know it should be better to have tests ;), I also need tests for the Entity share module, I will try to provide it when I will get some time)

CommentFileSizeAuthor
#12 2926089-12.patch1.8 KBwim leers

Comments

Grimreaper created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Component: Miscellaneous » Code
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs steps to reproduce

Thanks for reporting this!

I thought #2874601: refactor(QueryBuilder): Improve testability/maintainability added test coverage for exactly this. Perhaps the error happens before we reach the query logic?

What does the fatal error say?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2874601: refactor(QueryBuilder): Improve testability/maintainability, +#2841850: Internal Server Error with Grouped Filter Conditions
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

On a URL like jsonapi/node/article?filter[title][condition][path]=title&filter[title][condition][operator]=<&filter[title][condition][value][0]=test1&filter[title][condition][value][1]=test2

I got the following error:

            "title": "Internal Server Error",
            "status": 500,
            "detail": "SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens: 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\nINNER JOIN {node_field_data} node_field_data_2 ON node_field_data_2.nid = base_table.nid\nWHERE (node_field_data.title < :db_condition_placeholder_0:db_condition_placeholder_1) AND (node_field_data_2.type = :db_condition_placeholder_2)\nGROUP BY base_table.vid, base_table.nid\nLIMIT 51 OFFSET 0; Array\n(\n    [:db_condition_placeholder_0] => test1\n    [:db_condition_placeholder_1] => test2\n    [:db_condition_placeholder_2] => article\n)\n",
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Category: Support request » Bug report
Status: Postponed (maintainer needs more info) » Active
Issue tags: -Needs steps to reproduce +Needs tests

So the code building the query binds a different number of variables for tokens than there are tokens.

Looks like #4 also provided steps to reproduce. This now needs a test. @Grimreaper, are you interested in doing that? :)

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I think that this issue should be more like: Throw a nicer 4XX error when a user passes multiple values to a single value operator.

Is this the case @Grimreaper?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +DX (Developer Experience), +API-First Initiative

If the JSON API filter is invalid, then #6 would make sense, but I don't think that's the case here?

That is the case for the query in #4, so then, yes. :)

Although I think it's not really "multiple values to a single value operator", I think it's that the < operator does not make sense for non-numeric data.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hello,

Thanks both for your replies.

I now see 4 points:

  • Implementing an automated tests
  • Testing operators like < with numerical value
  • Throwing a nicer error message in case of malformed query
  • Updating the documentation, because it is not explicit that certain operators require some specific values type

To respond to comment 4, I am interested in helping but I will not have time until many weeks or even months... sorry, I am on a big project and on other subjects.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

@Wim Leers @Grimreaper this issue is maybe better suited for Entity Query API. If any prior checking needs to be done it should be there. It should throw a nice error instead of letting the storage driver return with unparseable errors. But that is going to need a lot of special cases.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

this issue is maybe better suited for Entity Query API. If any prior checking needs to be done it should be there. I

But the Entity Query API is a low-level API, not designed for exposure to the broader world. I'm afraid it's initially on JSON API's shoulders to do the necessary validation logic, and then later we could try to port that to Entity Query API.

We should double-check with Entity Query API experts and maintainers to see whether this is correct.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Active » Needs work

I don't think we want to tackle this in this module. I don't think JSON API should / can babysit intricacies of the database storages to provide good error messages. Having JSON API know about the database implementation details, instead of relying on an abstraction (Entity Query API) is not where I'd like us to go.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 2926089-12.patch1.8 KB

I honestly don't understand what you want to happen here.

\Drupal\Core\Entity\Query\QueryInterface is clearly a low-level API. It doesn't come with validation built-in. This is why e.g. Views includes tons of validation to prevent invalid/nonsensical queries.

I'm afraid that the only solution here is for JSON API to similarly validate what is allowed. It leaves the heavy lifting to the Entity Query API (\Drupal\Core\Entity\Query\QueryInterface). But that doesn't mean it doesn't need to do anything else.

Just like the REST module in Drupal core has to work around lots of things and has been working for years at this point to address gaps in underlying subsystems, the same is true for the JSON API module. The JSON API module has been mostly shielded from this because the REST module has fixed the most egregious problems already. But the REST module doesn't support collections, and therefore doesn't support filtering/sorting/… Which is why the JSON API module is now the first one to run into this problem.

I think that for now, we can just not yet deal with generating very helpful error messages, and just catch the generic exception and throw a generic error message. In most cases, I think users will be able to figure out that their query doesn't make sense. Having great error messages for every possible thing is great for DX, but obviously not a blocker for it to land in core.

Attached is a patch I'd propose for now to make the DX less scary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

In most cases, I think users will be able to figure out that their query doesn't make sense.

Maybe, hopefully.

Having great error messages for every possible thing is great for DX, but obviously not a blocker for it to land in core.

I agree. However it always depends on the cost to achieve that.

This is why e.g. Views includes tons of validation to prevent invalid/nonsensical queries.

Well, views is a query builder. Here we use the query builder.

I'm afraid that the only solution here is for JSON API to similarly validate what is allowed.

\Drupal\Core\Entity\Query\QueryInterface is clearly a low-level API.

There could be a wrapper around Entity Query API that validates the query before executing it. JSON API could use that without having to implement it on this module. That has the benefit of:

1. Other projects won't need to re-do this.
2. This is clearly not JSON API's responsibility, hence we avoid scope creep.
3. We don't couple release cycles of Entity Query API validation and JSON API.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

With regards to the patch, I'm ambivalent. If you think muffling the error in the response is good, let's go with that. However, that makes the dev's work more difficult since the database error may have contained a clue about what's going on. What if we suppress the response but log the database error to the watchdog?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

There could be a wrapper around Entity Query API that validates the query before executing it.

There could be, but there isn't, and there won't be for a long time to come. It's a huge undertaking.

What do you think about the patch in #12? Do you find that acceptable? If you don't, then the error responses will remain as explicit as they are today.

I don't have a strong opinion about which way to go — it's choosing between the lesser of two evils… 😔

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

There could be, but there isn't, and there won't be for a long time to come.

I was suggesting to start said wrapper outside of this project.

It's a huge undertaking.

Well, that's true regardless, right? I mean, it's a huge undertaking to do it in JSON API as well.

it's choosing between the lesser of two evils… 😔

Indeed! Evil me is happy about that 😈!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Well, that's true regardless, right? I mean, it's a huge undertaking to do it in JSON API as well.

The thing is that JSON API could do it gradually: it could add detection for this one case, then for another, etc. That'd then at least help JSON API users from running into the same few problems over and over again. Pragmatism over perfection. That'd be a far smaller effort.

But you're right that a complete solution would be just as huge an undertaking.

Indeed! Evil me is happy about that 😈!

Hah! :) But you still didn't say what you thought about the patch in #12…

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

You may have missed #14.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Postponed (maintainer needs more info)

@e0ipso @Wim Leers or @Grimreaper, are any of you clear on what actually needs to happen next here? I can't tell anymore.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)

Neither can I.