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

Vocabulary search using "0" as the query argument throws an error #1260

Closed
Ohiekkar opened this issue Jan 3, 2022 · 4 comments · Fixed by #1261 or #1267
Closed

Vocabulary search using "0" as the query argument throws an error #1260

Ohiekkar opened this issue Jan 3, 2022 · 4 comments · Fixed by #1261 or #1267
Assignees
Milestone

Comments

@Ohiekkar
Copy link

Ohiekkar commented Jan 3, 2022

At which URL did you encounter the problem?

https://api.finto.fi/rest/v1/ykl/search

What steps will reproduce the problem?

  1. GET https://api.finto.fi/rest/v1/ykl/search?query=0

What is the expected output? What do you see instead?

As searching by notation is a thing and 0 is a valid notation I would expect the search to work and find this item: http://urn.fi/URN:NBN:fi:au:ykl:0, just like it works with https://api.finto.fi/rest/v1/ykl/search?query=1.

Currently it throws 400 Bad Request : query parameter missing. I'd assume this comes from php evaluating "0" to false

@kinow
Copy link
Collaborator

kinow commented Jan 4, 2022

I think your analysis of the problem is spot-on @Ohiekkar ! See linked PR for a tentative fix. Thanks!

@osma
Copy link
Member

osma commented Jan 20, 2022

Thanks for the report @Ohiekkar, and the quick confirmation and PR @kinow! I believe this affects not just the REST API but also the search in the web UI. Example:

@osma osma self-assigned this Jan 20, 2022
@osma
Copy link
Member

osma commented Jan 20, 2022

The REST side was fixed in PR #1261 but the web UI search still suffers from the same problem (see above comment). I'm reopening this issue to keep track of the problem on the web UI side.

@osma
Copy link
Member

osma commented Jan 27, 2022

Like @Ohiekkar mentioned above, the problem stems from PHP evaluating "0" to false. For the REST API, it was fixed in #1261 by changing the test in RestController from this:

if (!$term && (!$request->getQueryParam('group') && !$request->getQueryParam('parent'))) {

to this:

if ((!isset($term) || strlen(trim($term)) === 0) && (!$request->getQueryParam('group') && !$request->getQueryParam('parent'))) {

Now a similar problem still exists on the web UI side - we need to avoid evaluating $term (or similar variables/expressions containing the search term) directly and instead do a more careful check of the contents, for example with isset and strlen as above.

I suspect that at least this test in ConceptSearchParameters needs to be changed as !$term will evaluate to true if $term is "0":

if (!$term && $this->rest)

@osma osma assigned miguelvaara and unassigned osma Jan 27, 2022
@MikkoAleksanteri MikkoAleksanteri moved this to Proposed items for this sprint in Finton Skosmos-sprintti 1/2022 Feb 9, 2022
@joelit joelit moved this from Proposed items for this sprint to Selected items for this sprint in Finton Skosmos-sprintti 1/2022 Feb 14, 2022
@joelit joelit moved this from Selected items for this sprint to In Progress in Finton Skosmos-sprintti 1/2022 Feb 15, 2022
@miguelvaara miguelvaara moved this from In Progress to Selected items for this sprint in Finton Skosmos-sprintti 1/2022 Feb 16, 2022
@osma osma closed this as completed in #1267 Mar 3, 2022
Repository owner moved this from Selected items for this sprint to Issue/PR Closed in Finton Skosmos-sprintti 1/2022 Mar 3, 2022
@osma osma added this to the 2.14 milestone Mar 3, 2022
@osma osma moved this from Issue/PR Closed to Done in Finton Skosmos-sprintti 1/2022 Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment