search

d34dman / drupal-jsonapi-params

A package to manage json-api params
ISC License
59 stars 8 forks source link

handle operators IS NULL and IS NOT NULL appropriately. #33

Closed rowrowrowrow closed 1 year ago

rowrowrowrow commented 1 year ago

I've seen and experienced some errors with the shorthand logic and null operators. An error is thrown when trying to use the 'IS NULL' or 'IS NOT NULL' operators with the shorthand notation. This may be the case with other operators but I know of these two for now. I think this addresses the issue.

See https://www.drupal.org/docs/core-modules-and-themes/core-modules/jsonapi-module/filtering in reference to the nullish examples. They specifically require the longform and not the shorthand.

rowrowrowrow commented 1 year ago

@d34dman bump

d34dman commented 1 year ago

Thanks for the bug report and MR @rowrowrowrow , you are awesome!

As per official docs https://www.drupal.org/docs/core-modules-and-themes/core-modules/jsonapi-module/filtering#shortcuts it seems, the shortcuts are only applicable (officially) for = operator. I had created an issue surrounding the behaviour of shortcuts here https://www.drupal.org/project/drupal/issues/3264769. Looks like shortcuts for other operators was an un-intended consequence which is un-reliable.

d34dman commented 1 year ago

I need to run some test against various Drupal versions to see if anything changed

rowrowrowrow commented 1 year ago

I need to run some test against various Drupal versions to see if anything changed

Sounds good

d34dman commented 1 year ago

@rowrowrowrow , the usage for IS NULL and IS NOT NULL operators requires null to be passed in as second argument. If you pass, an empty string then it will fail.

Am able to reproduce the expected behaviour by doing api.addFilter('field_test_date', null, 'IS NULL'). This has been mentioned in the Readme section of the library as well.

Code sandbox: https://codesandbox.io/s/drupal-jsonapi-params-bug-pr-33-41gio5?file=/src/index.js

Could you please check if am missing something?

EDIT NOTE: Sorry for too many edits for improving clarity.

rowrowrowrow commented 1 year ago

@rowrowrowrow , the usage for IS NULL and IS NOT NULL operators requires null to be passed in as second argument. If you pass, an empty string then it will fail.

Am able to reproduce the expected behaviour by doing api.addFilter('field_test_date', null, 'IS NULL'). This has been mentioned in the Readme section of the library as well.

Code sandbox: https://codesandbox.io/s/drupal-jsonapi-params-bug-pr-33-41gio5?file=/src/index.js

Could you please check if am missing something?

EDIT NOTE: Sorry for too many edits for improving clarity.

I can't check myself at the moment, to see if you can enforce the long form query by supplying a value of null for IS NULL and IS NOT NULL, but in general I think since the logic for processing the query params is operator based on the backend we should also begin with the operator. To me, requiring a specific value for null to get the correct query only leaves room for error rather than any flexibility for a feature.

d34dman commented 1 year ago

To me, requiring a specific value for null to get the correct query only leaves room for error rather than any flexibility for a feature

Yes, it is indeed. The order of parameter is based on how the condition queries are written in Drupal. Check https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!Query!QueryInterface.php/function/QueryInterface%3A%3Acondition/8.2.x for more info. I specifically choose that order to provide a better DX for Drupal Developers using this library.

'IS NULL', 'IS NOT NULL': These operators ignore $value, for that reason it is recommended to use a $value of NULL for clarity. Adding the above is an excerpt from Drupal.Org for reference.

Having said that, I can see we can still improve the library behaviour by allowing use of "ANY" value (empty string, 0, null, or whatever), which we can just ignore when constructing the parameter.

Unfortunately changing the order of parameters would be a breaking change, which I won't be entertaining in 2.x

TLDR; I am think we can convert this bug report to a feature request where we,

rowrowrowrow commented 1 year ago

I think I opted for solution A after looking into how the Filters are actually applied. In the end the value is ignored on the backend for these operators.

I think the guidance in https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!Query!QueryInterface.php/function/QueryInterface%3A%3Acondition/8.2.x is just a recommendation. See the screenshot of "drupal/web/core/modules/jsonapi/src/Query/Filter.php" below. If the operators are "IS NULL" or "IS NOT NULL" the value is ignored, so I think we should do the same.

Screenshot 2023-01-05 at 12 10 51 PM

Additionally, or more importantly... without ignoring the value (Solution B) the query generated can't be used to recreate itself using this package. So there are consistency issues. I would consider this result a bug that we don't want to introduce.

d34dman commented 1 year ago

@rowrowrowrow agreed :) Let me work on this. Btw, do you have a Drupal.Org profile? I would like to create an issue over there and give you due credits.

d34dman commented 1 year ago

Fixed in version 2.2.0.

rowrowrowrow commented 1 year ago

@rowrowrowrow agreed :) Let me work on this. Btw, do you have a Drupal.Org profile? I would like to create an issue over there and give you due credits.

Yep, thanks!

https://www.drupal.org/u/rowrowrowrow