d34dman / drupal-jsonapi-params

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

addFilter Generates Invalid Params When Filtering Multiple Ids on Reference Field #28

Closed wluisi closed 2 years ago

wluisi commented 2 years ago

I’m trying to generate query params for filtering multiple term ids on an entity reference field. The expected query params I want are these:

filter[field_erm_audience-216-and][group][conjunction]=AND
&filter[field_erm_audience.meta.drupal_internal__target_id-216][condition][path]=field_erm_audience.meta.drupal_internal__target_id
&filter[field_erm_audience.meta.drupal_internal__target_id-216][condition][value]=216
&filter[field_erm_audience.meta.drupal_internal__target_id-216][condition][memberOf]=field_erm_audience-216-and

&filter[field_erm_audience-217-and][group][conjunction]=AND
&filter[field_erm_audience.meta.drupal_internal__target_id-217][condition][path]=field_erm_audience.meta.drupal_internal__target_id
&filter[field_erm_audience.meta.drupal_internal__target_id-217][condition][value]=217
&filter[field_erm_audience.meta.drupal_internal__target_id-217][condition][memberOf]=field_erm_audience-217-and

However, using this package, I can’t generate the correct query params for this. My simplified code, is basically this:

const tids = ["216", "217"];

tids.forEach((tid) => {
  const fieldName = "field_erm_audience";
  const groupName = `${fieldName}-${tid}-and`;
  const filterPath = `${fieldName}.meta.drupal_internal__target_id`;
  apiParams.addGroup(groupName, "AND");
  apiParams.addFilter(filterPath, tid, "=", groupName);
});

The output of this results in [3] as the filter key, which is an invalid query.

filter[3][condition][path]=field_erm_audience.meta.drupal_internal__target_id
&filter[3][condition][value]=217
&filter[3][condition][memberOf]=field_erm_audience-217-and
&filter[field_erm_audience-216-and][group][conjunction]=AND
&filter[field_erm_audience.meta.drupal_internal__target_id][condition][path]=field_erm_audience.meta.drupal_internal__target_id
&filter[field_erm_audience.meta.drupal_internal__target_id][condition][value]=216
&filter[field_erm_audience.meta.drupal_internal__target_id][condition][memberOf]=field_erm_audience-216-and
&filter[field_erm_audience-217-and][group][conjunction]=AND

Code sandbox example is here to reproduce the problem:

https://codesandbox.io/s/drupal-jsonapi-params-bug-wtgjt?file=/src/index.js

The problem seems to be inside the addFilter() method, the way name is set, will not work since the key won’t be unique as it's the same field name, just with multiple values.

I’ve experimented with patching, would need to test more, but changing how the name const is set to ensure its unique, generates the correct query.

https://github.com/wluisi/drupal-jsonapi-params/blob/master/src/index.ts#L221

Curious if this is a bug, or if there is another way to use this package to generate the correct query params? If it's a bug, I'd be happy to work on a PR to fix.

For context, this type of query I got from this drupal.org comment:

https://www.drupal.org/project/drupal/issues/3066202#comment-13181270

d34dman commented 2 years ago

Hi @wluisi , thanks for the detailed report.

Background:

Unfortunately library doesn't support choosing arbitrary field names by design. It is a personal choice. I choose not to deal with validating user supplied values for uniqueness for intended purpose. Instead the library ensures its correctness.

Examination of bug report

Examining your case, I don't see this is a bug. The generated code which uses index "3" would perform identical to the one you would like to use. In this case "field_erm_audience.meta.drupal_internaltarget_id-217". Also I see you would also like to swap "field_erm_audience.meta.drupal_internaltarget_id" with "field_erm_audience.meta.drupal_internal__target_id-216" in the code generated by the library

Explanation for the behaviour of the library

As you have already found out, the index is a function of "field name". However in case it is already used/allocated, it would generate a numerical index based on the size of the current elements in the query. This grantees uniqueness. Thats why, in the first pass, when the term id is 216, it used the field path for index field_erm_audience.meta.drupal_internal__target_id and thereafter, it starts using a numerical id, when the "path" was same for the case of 217. The order in which the query get printed out might confuse you.

Conclusion

What I explained above is from theory. In case the following two query provides an entirely different results, I would keep this open for further investigation..

Query A (Manual query)

filter[field_erm_audience-216-and][group][conjunction]=AND
&filter[field_erm_audience.meta.drupal_internal__target_id-216][condition][path]=field_erm_audience.meta.drupal_internal__target_id
&filter[field_erm_audience.meta.drupal_internal__target_id-216][condition][value]=216
&filter[field_erm_audience.meta.drupal_internal__target_id-216][condition][memberOf]=field_erm_audience-216-and

&filter[field_erm_audience-217-and][group][conjunction]=AND
&filter[field_erm_audience.meta.drupal_internal__target_id-217][condition][path]=field_erm_audience.meta.drupal_internal__target_id
&filter[field_erm_audience.meta.drupal_internal__target_id-217][condition][value]=217
&filter[field_erm_audience.meta.drupal_internal__target_id-217][condition][memberOf]=field_erm_audience-217-and

Query B (lib Query)

filter[3][condition][path]=field_erm_audience.meta.drupal_internal__target_id
&filter[3][condition][value]=217
&filter[3][condition][memberOf]=field_erm_audience-217-and
&filter[field_erm_audience-216-and][group][conjunction]=AND
&filter[field_erm_audience.meta.drupal_internal__target_id][condition][path]=field_erm_audience.meta.drupal_internal__target_id
&filter[field_erm_audience.meta.drupal_internal__target_id][condition][value]=216
&filter[field_erm_audience.meta.drupal_internal__target_id][condition][memberOf]=field_erm_audience-216-and
&filter[field_erm_audience-217-and][group][conjunction]=AND

Could you please let me know the result of both the query after running it against a Drupal instance?

wluisi commented 2 years ago

@d34dman Thanks for the quick response. Query A works as expected and returns the correct results. When I use Query B, I get a 500 internal server error and '@root' is a reserved filter id.. In the Drupal logs I get

UnexpectedValueException: '@root' is a reserved filter id. in Drupal\jsonapi\Query\Filter::expand() (line 190 of /var/www/html/web/core/modules/jsonapi/src/Query/Filter.php).

I believe these errors comes from the filter[3].

d34dman commented 2 years ago

@wluisi ok in that case I will setup a test gig and run few test myself. Based on your report, I am afraid, the library would have more serious issue. Any update in the method getIndexId would result in updating a lot of unit tests. So this won't be a simple task.

d34dman commented 2 years ago

Also thanks for the offer for PR. Really appreciate* it a lot. But before we jump in for the fix, I would like to know "why" this is happening.

d34dman commented 2 years ago

@wluisi I couldn't reproduce the issue.

I created a site based on "umami" in simplytest.me. And used your script to generate query against article with multiple terms inside "field_tags".

It generated the following query,

https://master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa/jsonapi/node/article?filter[3][condition][path]=field_tags.meta.drupal_internal__target_id&filter[3][condition][value]=24&filter[3][condition][memberOf]=field_tags-24-and&filter[field_tags-26-and][group][conjunction]=AND&filter[field_tags.meta.drupal_internal__target_id][condition][path]=field_tags.meta.drupal_internal__target_id&filter[field_tags.meta.drupal_internal__target_id][condition][value]=26&filter[field_tags.meta.drupal_internal__target_id][condition][memberOf]=field_tags-26-and&filter[field_tags-24-and][group][conjunction]=AND

and gave me results without any error, so am not able to reproduce from my side for now.

{"jsonapi":{"version":"1.0","meta":{"links":{"self":{"href":"http:\/\/jsonapi.org\/format\/1.0\/"}}}},"data":[{"type":"node--article","id":"7f6e83c6-9483-4bce-93ce-17e3837c3857","links":{"self":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857?resourceVersion=id%3A24"}},"attributes":{"drupal_internal__nid":12,"drupal_internal__vid":24,"langcode":"en","revision_timestamp":"2022-01-31T18:19:17+00:00","revision_log":null,"status":true,"title":"The real deal for supermarket savvy shopping","created":"2022-01-31T18:19:17+00:00","changed":"2022-01-31T18:19:17+00:00","promote":true,"sticky":false,"default_langcode":true,"revision_translation_affected":null,"moderation_state":"published","path":{"alias":"\/articles\/the-real-deal-for-supermarket-savvy-shopping","pid":89,"langcode":"en"},"content_translation_source":"und","content_translation_outdated":false,"body":{"value":"\u003Cp\u003EThis may not surprise you - but your supermarket is a hot bed of marketing mayhem, designed to improve their profit and to encourage the consumer to spend more than they intended. The tricks that all supermarkets employ are sometimes sensible ploys that any retailer should do to improve sales - but some may be more subtle and less obvious than you might think.\u003C\/p\u003E\n\u003Cp\u003EWith consumer awareness articles and documentaries frequently picking up on this topic, it\u0027s likely the case that retailers find it harder to get away with the more obvious ploys. We are becoming ever more savvy consumers and there\u0027s probably not a great deal that gets past us. But here\u0027s a few retail tricks to keep in mind when you are rushing around the weekly supermarket stock-up.\u003C\/p\u003E\n\u003Ch2\u003ELost essentials\u003C\/h2\u003E\n\u003Cp\u003EThe layout of your supermarket may make sense to you when you have shopped there for a while. But for newcomers trying to find essentials, it may make very little sense at all. Some supermarkets have noted that people come to their store to buy milk, bread, or eggs and that by hiding these essentials in the far reaches of the store, they encourage the newcomer to wander the aisles - picking up other items as they go.\u003C\/p\u003E\n\u003Cp\u003ESure, this can be great for nudging the memory on essentials you might otherwise forget, but for saving the pennies it\u0027s tough to stick to grabbing only the things you came for and the supermarkets know it!\u003C\/p\u003E\n\u003Cblockquote\u003E\nOur tip: Make your shopping list before leaving the house, checking what you need and sticking to that list. You could be amazed by what you\u0027ll save over time.\n\u003C\/blockquote\u003E\n\u003Ch2\u003ENonsensical multibuys\u003C\/h2\u003E\n\u003Cp\u003EBuy one, get one free; two for \u0026pound;2 and meal deals. They all seem like a great deal. But in some cases these are loss leaders that are positioned to encourage you to take up the deal and buy other stuff while you are there. In other cases, deals for multi-buy or discounts on specific pack sizes might seem like a bargain, until you compare the pricing like-for-like on similar brands or with pack sizes for the same brand. These deals can mean you end up paying less but is it less for something you don\u0027t really need and in some cases you can end up paying more for the item. Remember, the supermarkets know you are often in a hurry and might not have the time to take in the full picture.\u003C\/p\u003E\n\u003Cblockquote\u003E\n\u003Cp\u003EOur tip: Don\u0027t be rushed, take the time to read the small print. The large print will draw you in but if you read the label small print, you should find the price per 100 grams or per ounce\/litre and you\u0027ll be surprised how often the headline deals are actually more expensive than just buying a different package type or size of the product.\u003C\/p\u003E\n\u003C\/blockquote\u003E\n\u003Ch2\u003EUnderstanding our shopping habits\u003C\/h2\u003E\n\u003Cp\u003EThe cheapest products in a supermarket are almost always positioned on the bottom of the shelving where you\u0027ll need to bend over to pick it up. You also may not be able to easily read the price ticket. Most people will shop on the middle rows because it is easier and often quicker. These are where the highest profit items are kept and they are the ones the supermarkets want you to buy.\u003C\/p\u003E\n\u003Cp\u003EThe layout, the music, the colors, and the product types are all decided based on principles laid down by industry experts on people - psychologists and behavioral experts who know how we think. And so the savvy shopper will certainly be able to take advantage of great deals in their weekly shop, but it takes a little time and effort just to be more aware of what we are being encouraged to reach for in the aisles.\u003C\/p\u003E\n","format":"basic_html","processed":"\u003Cp\u003EThis may not surprise you - but your supermarket is a hot bed of marketing mayhem, designed to improve their profit and to encourage the consumer to spend more than they intended. The tricks that all supermarkets employ are sometimes sensible ploys that any retailer should do to improve sales - but some may be more subtle and less obvious than you might think.\u003C\/p\u003E\n\u003Cp\u003EWith consumer awareness articles and documentaries frequently picking up on this topic, it\u0027s likely the case that retailers find it harder to get away with the more obvious ploys. We are becoming ever more savvy consumers and there\u0027s probably not a great deal that gets past us. But here\u0027s a few retail tricks to keep in mind when you are rushing around the weekly supermarket stock-up.\u003C\/p\u003E\n\u003Ch2\u003ELost essentials\u003C\/h2\u003E\n\u003Cp\u003EThe layout of your supermarket may make sense to you when you have shopped there for a while. But for newcomers trying to find essentials, it may make very little sense at all. Some supermarkets have noted that people come to their store to buy milk, bread, or eggs and that by hiding these essentials in the far reaches of the store, they encourage the newcomer to wander the aisles - picking up other items as they go.\u003C\/p\u003E\n\u003Cp\u003ESure, this can be great for nudging the memory on essentials you might otherwise forget, but for saving the pennies it\u0027s tough to stick to grabbing only the things you came for and the supermarkets know it!\u003C\/p\u003E\n\u003Cblockquote\u003E\u003Cp\u003E\nOur tip: Make your shopping list before leaving the house, checking what you need and sticking to that list. You could be amazed by what you\u0027ll save over time.\n\u003C\/p\u003E\u003C\/blockquote\u003E\n\u003Ch2\u003ENonsensical multibuys\u003C\/h2\u003E\n\u003Cp\u003EBuy one, get one free; two for \u00a32 and meal deals. They all seem like a great deal. But in some cases these are loss leaders that are positioned to encourage you to take up the deal and buy other stuff while you are there. In other cases, deals for multi-buy or discounts on specific pack sizes might seem like a bargain, until you compare the pricing like-for-like on similar brands or with pack sizes for the same brand. These deals can mean you end up paying less but is it less for something you don\u0027t really need and in some cases you can end up paying more for the item. Remember, the supermarkets know you are often in a hurry and might not have the time to take in the full picture.\u003C\/p\u003E\n\u003Cblockquote\u003E\u003Cp\u003EOur tip: Don\u0027t be rushed, take the time to read the small print. The large print will draw you in but if you read the label small print, you should find the price per 100 grams or per ounce\/litre and you\u0027ll be surprised how often the headline deals are actually more expensive than just buying a different package type or size of the product.\u003C\/p\u003E\n\u003C\/blockquote\u003E\n\u003Ch2\u003EUnderstanding our shopping habits\u003C\/h2\u003E\n\u003Cp\u003EThe cheapest products in a supermarket are almost always positioned on the bottom of the shelving where you\u0027ll need to bend over to pick it up. You also may not be able to easily read the price ticket. Most people will shop on the middle rows because it is easier and often quicker. These are where the highest profit items are kept and they are the ones the supermarkets want you to buy.\u003C\/p\u003E\n\u003Cp\u003EThe layout, the music, the colors, and the product types are all decided based on principles laid down by industry experts on people - psychologists and behavioral experts who know how we think. And so the savvy shopper will certainly be able to take advantage of great deals in their weekly shop, but it takes a little time and effort just to be more aware of what we are being encouraged to reach for in the aisles.\u003C\/p\u003E\n","summary":null}},"relationships":{"node_type":{"data":{"type":"node_type--node_type","id":"b0ec46e0-b563-485f-b0f6-f1990adbbd29","meta":{"drupal_internal__target_id":"article"}},"links":{"related":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/node_type?resourceVersion=id%3A24"},"self":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/relationships\/node_type?resourceVersion=id%3A24"}}},"revision_uid":{"data":{"type":"user--user","id":"90901dd7-2d16-457d-a9ee-7225b506e47a","meta":{"drupal_internal__target_id":6}},"links":{"related":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/revision_uid?resourceVersion=id%3A24"},"self":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/relationships\/revision_uid?resourceVersion=id%3A24"}}},"uid":{"data":{"type":"user--user","id":"90901dd7-2d16-457d-a9ee-7225b506e47a","meta":{"drupal_internal__target_id":6}},"links":{"related":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/uid?resourceVersion=id%3A24"},"self":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/relationships\/uid?resourceVersion=id%3A24"}}},"field_media_image":{"data":{"type":"media--image","id":"9c79bc14-9671-4854-9983-6df543c6e5c8","meta":{"drupal_internal__target_id":12}},"links":{"related":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/field_media_image?resourceVersion=id%3A24"},"self":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/relationships\/field_media_image?resourceVersion=id%3A24"}}},"field_multiple_tax":{"data":[],"links":{"related":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/field_multiple_tax?resourceVersion=id%3A24"},"self":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/relationships\/field_multiple_tax?resourceVersion=id%3A24"}}},"field_tags":{"data":[{"type":"taxonomy_term--tags","id":"eb678a55-66ed-4df0-aaa6-bf820f146738","meta":{"drupal_internal__target_id":26}},{"type":"taxonomy_term--tags","id":"93ba39f7-40eb-4c8d-bb4a-983a1ecb540e","meta":{"drupal_internal__target_id":24}}],"links":{"related":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/field_tags?resourceVersion=id%3A24"},"self":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/en\/jsonapi\/node\/article\/7f6e83c6-9483-4bce-93ce-17e3837c3857\/relationships\/field_tags?resourceVersion=id%3A24"}}}}}],"links":{"self":{"href":"https:\/\/master-7kvwzxc59zln2unf4lbnpr5d2uki59yo.tugboat.qa\/jsonapi\/node\/article?filter%5B3%5D%5Bcondition%5D%5Bpath%5D=field_tags.meta.drupal_internal__target_id\u0026filter%5B3%5D%5Bcondition%5D%5Bvalue%5D=24\u0026filter%5B3%5D%5Bcondition%5D%5BmemberOf%5D=field_tags-24-and\u0026filter%5Bfield_tags-26-and%5D%5Bgroup%5D%5Bconjunction%5D=AND\u0026filter%5Bfield_tags.meta.drupal_internal__target_id%5D%5Bcondition%5D%5Bpath%5D=field_tags.meta.drupal_internal__target_id\u0026filter%5Bfield_tags.meta.drupal_internal__target_id%5D%5Bcondition%5D%5Bvalue%5D=26\u0026filter%5Bfield_tags.meta.drupal_internal__target_id%5D%5Bcondition%5D%5BmemberOf%5D=field_tags-26-and\u0026filter%5Bfield_tags-24-and%5D%5Bgroup%5D%5Bconjunction%5D=AND"}}}

screencapture-master-7kvwzxc59zln2unf4lbnpr5d2uki59yo-tugboat-qa-jsonapi-node-article-2022-01-31-20_01_54

wluisi commented 2 years ago

@d34dman Oh wow, interesting. I can look into my d9 setup a bit more and see if there's a conflict from another contrib module or core patch. But curious, what version of D9 are you running? Any d9 core patches related to json api? And are you using any json api add-on modules such as jsonapi_extras ?

d34dman commented 2 years ago

@wluisi

I didn't use jsonapi_extras or any patches. The test was done on "umami demo" on https://simplytest.me/. After I got my instance, I enabled json_api module and ran the above query.

The preview site is gone already, but you may retry the procedure to get more details on simplytest.me site

wluisi commented 2 years ago

@d34dman Hey, just want to say thanks again for helping me debug this! I was able to narrow down the issue to jsonapi_extras contrib module. Added a drupal.org issue here: https://www.drupal.org/project/jsonapi_extras/issues/3262994

Seems like any integer as a filter label will trigger the '@root' is a reserved filter id. issue if jsonapi_extras is installed.

d34dman commented 2 years ago

@wluisi thanks for figuring this out. I would close this for now.

And in case you would you like to have a feature in this library to use "alphanumeric" keys always, please create a feature request.