Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zoeken op waarden met een komma erin, geeft een foutmelding #472

Closed
sytskevanhasselt opened this issue Nov 8, 2024 · 20 comments · Fixed by #488
Closed

zoeken op waarden met een komma erin, geeft een foutmelding #472

sytskevanhasselt opened this issue Nov 8, 2024 · 20 comments · Fixed by #488
Assignees

Comments

@sytskevanhasselt
Copy link

Product versie / Product version

Objects API (2.3.1 (v2))

Omschrijf het probleem / Describe the bug

Als je in de Objectenregistratie een object probeert op te halen op een tekststring waarin een komma zit, dan is de repsonse:
400 Bad request

response body:

[
    "not enough values to unpack (expected 3, got 1)"
]

Stappen om te reproduceren / Steps to reproduce

Zie ook screenshot uit Postman met een aantal calls, na elkaar gedaan. Allemaal geven ze dezelfde response. We hebben deze calls geprobeerd

https://objecten.dev.kiss-demo.nl/api/v2/objects?data_attrs=naam__icontains__Advies%2C+support+en+kennis
https://objecten.dev.kiss-demo.nl/api/v2/objects?data_attrs=naam__icontains__Advies, support en kennis
https://objecten.dev.kiss-demo.nl/api/v2/objects?data_attrs=naam__icontains__Advies%2C+support+en+kennis+%28ASK%29
https://objecten.dev.kiss-demo.nl/api/v2/objects?data_attrs=naam__icontains__Advies, support en kennis (ASK)

NB; deze gaan wel goed:
https://objecten.dev.kiss-demo.nl/api/v2/objects?data_attrs=naam__icontains__Sociaal+Wijkteam
https://objecten.dev.kiss-demo.nl/api/v2/objects?data_attrs=naam__icontains__Advies+support+en+kennis+%28ASK%29

image-20241106-203014

(Versienr van de API gekopieerd vanaf https://objecten.dev.kiss-demo.nl/api/v2/schema/)

Verwacht gedrag / Expected behavior

Wel kunnen zoeken op strings waar een komma in staat

@sytskevanhasselt sytskevanhasselt added bug Something isn't working triage labels Nov 8, 2024
@joeribekker
Copy link
Member

Even uit mn hoofd: dit lijkt me conflicteren met het feit dat filters sowieso al komma gescheiden zijn. Icontains zou dan ook geen kommas ondersteunen.

Volgens mij kan je doen: bla__icontains__x,bla__icontains__y

@joeribekker
Copy link
Member

Ik vermoed ook dat we uitkomen op de discussie: AND versus OR en uiteindelijk moeten we SQL implementeren in query params :)

@sytskevanhasselt
Copy link
Author

@mstokericatt
Copy link

@joeribekker op basis van je reactie twijfel ik of het probleem helder is: we willen kunnen zoeken op teksten waarin een komma voorkomt. De komma maakt gewoon deel uit van de zoekopdracht.
Het doel is dus niet om door gebruik van komma's meerdere AND/OR parameters mee te kunnen sturen oid.

@joeribekker
Copy link
Member

Dat was dan niet helder :)

@joeribekker
Copy link
Member

Als je het encode verwacht ik wel dat het zou werken, maar dat is dus niet zo?

@mstokericatt
Copy link

In het issue worden een aantal variaties beschreven met encodering die we hebben uitgeprobeerd. Die werken niet.

@sytskevanhasselt
Copy link
Author

Nee, deze query geeft ook die melding over not enough values terug
https://objecten.dev.kiss-demo.nl/api/v2/objects?data_attrs=naam__icontains__Advies%2C+support+en+kennis

En we zijn opzoek naar een object waarvan de naam is: "Advies, support en kennis"

@annashamray
Copy link
Collaborator

We've analyzed the issued and confirm that it's a bug, and percent encoding doesn't solve the issue

@mariusvandam
Copy link

This issue is considered a blocking issue for the release of PodiumD 3.0 for Deventer. We would like to align on planning of this issue and a next release of Objects.

@mariusvandam
Copy link

mariusvandam commented Dec 3, 2024

Good to see this is picked up!
Two follow-up question:

  • What is the release planning for Objecten? (heard this is already answered and will be added to release end of this month)
  • Is this a fix that only allows specifically for comma's or does this more broadly allow for punctuation marks? If so, which would be supported?

@joeribekker
Copy link
Member

Discussed: Anna is trying to see if we can parse it differently, so we know whether its a variable separation or a "comma in a value" case. If that doesnt pan out, we're looking into a env var to enable different query param behaviour that requires you 1 key/value pair per "data_attrs" param.

@annashamray
Copy link
Collaborator

Removed "bug" label since it's explicitly stated in the OAS that commas are not supported:
image

@annashamray annashamray moved this from In Progress to Implemented in Data en API fundament Dec 9, 2024
@annashamray annashamray moved this from Implemented to Todo in Data en API fundament Dec 10, 2024
@annashamray annashamray moved this from Todo to Implemented in Data en API fundament Dec 10, 2024
@joeribekker
Copy link
Member

Normal usage:

?data_attrs=naam__icontains__Anna,last_name__icontains__Shamray

What they want:

?data_attrs=departmentname__icontains__Support, advice and shizzle

@joeribekker
Copy link
Member

In the PR there are 4 options. We picked option 1 but with a sligh change: To prevent issueing a new major release, we add a new query param (data_attr so no "s") with the behaviour to only allow 1 key/value pair per query param. This new query param can be used multiple times and thus allows for ",".

@sytskevanhasselt
Copy link
Author

sytskevanhasselt commented Dec 14, 2024

So, do I understand correctly that, with the next release, we should be using:
?data_attr=departmentname__icontains__Support, advice and shizzle or
?data_attr=departmentname__icontains__Support%2C+support+and+shizzle?
(I am very much in favour of a department of shizzle)

And , in light of @mariusvandam 's question, does that also mean we can use any type of punctuation, except maybe double underscores?

@annashamray
Copy link
Collaborator

Yes, it should support space, comma and other punctuation characters (except double underscore and url reserved characters)

@sytskevanhasselt
Copy link
Author

At this point our code uses data_attrs=naam__icontains__[value]
But we have to change this to data_attr=naam__icontains__[value], right?
Then I will create an issue on our own backlog to address this.

@annashamray
Copy link
Collaborator

If value contains commas then yes, if not you can keep using the old query param (data_attrs)

@sytskevanhasselt
Copy link
Author

Thanks for confirming! And our values can contain comma's, unfortunatly ...

annashamray added a commit that referenced this issue Dec 18, 2024
@github-project-automation github-project-automation bot moved this from Implemented to Done in Data en API fundament Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants