Skip to content
This repository has been archived by the owner on Mar 9, 2024. It is now read-only.

[BUG] minimum age gets melted into node API in query #316

Closed
1 task done
surchs opened this issue Dec 6, 2023 · 3 comments · Fixed by #317
Closed
1 task done

[BUG] minimum age gets melted into node API in query #316

surchs opened this issue Dec 6, 2023 · 3 comments · Fixed by #317
Assignees
Labels
bug:functional Functional defects resulting from feature changes. importance:urgent We will address this as soon as possible. type:bug Defects in shipped code and fixes for those defects

Comments

@surchs
Copy link
Contributor

surchs commented Dec 6, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Expected Behavior

I can run a query for a single node and define a minimum age

Current Behavior

if I set sex, all is well: https://federate.neurobagel.org/query/?&node_url=https://api.neurobagel.org/&sex=snomed:248153007

if I set minimum age, this happens: https://federate.neurobagel.org/query/?&node_url=https://api.neurobagel.org/min_age=1

Error message

federation API rightly complains that

"Unrecognized Neurobagel node URL(s): ['https://api.neurobagel.org/min_age=1/']

Environment

  • any single or multiple node selection except "All"

How to reproduce

  1. Select any node other than "All"
  2. define a minimum age
  3. watch it break with "Oops"

Anything else?

see also OpenNeuroOrg/openneuro#2807

most likely this is our culprit:
https://github.com/neurobagel/query-tool/blob/dadbee332c593f95f5bab9bc037837f2241a6f76/components/QueryForm.vue#L222-L227

i.e. we do not concatenate min_age correctly.

The reason this doesn't break for the "All" node is that we encode "All" as "no node parameter set":

https://federate.neurobagel.org/query/?min_age=1

so in this very specific case, min_age actually is the first query parameter and gets parsed correctly. This is most likely a leftover thing from when a query tool only ever talked directly to a node API and the federation API didn't exist yet.

@surchs surchs added the type:bug Defects in shipped code and fixes for those defects label Dec 6, 2023
@surchs surchs added this to Neurobagel Dec 6, 2023
@surchs surchs moved this to Specify - Active in Neurobagel Dec 6, 2023
@surchs surchs added importance:urgent We will address this as soon as possible. bug:functional Functional defects resulting from feature changes. labels Dec 6, 2023
@surchs
Copy link
Contributor Author

surchs commented Dec 6, 2023

Options we have here:

  • be more smart about when we prefix a query parameter with &, i.e. only if it follows a previous parameter
  • add & everytime. It's not "proper" but it is valid 🤷

@surchs surchs moved this from Specify - Active to Specify - Done in Neurobagel Dec 6, 2023
@rmanaem
Copy link
Collaborator

rmanaem commented Dec 6, 2023

I wonder if there is a library we can use that allows us to pass params as an object something similar to params of httpx.get we use in federation-api.

@rmanaem rmanaem moved this from Specify - Done to Implement - Active in Neurobagel Dec 6, 2023
@rmanaem rmanaem self-assigned this Dec 6, 2023
@surchs
Copy link
Contributor Author

surchs commented Dec 6, 2023

you mean as part of a post body as opposed to the get we use here? Is that used somewhere else already?
For this specific bug, we need to make sure we only pass min_age without a leading & if we're certain that it is the first query parameter. This used to be always the case, but now the node query parameter can be before min_age whenever any node is selected except "All"

@rmanaem rmanaem moved this from Implement - Active to Implement - Done in Neurobagel Dec 6, 2023
@surchs surchs moved this from Implement - Done to Review - Active in Neurobagel Dec 6, 2023
@github-project-automation github-project-automation bot moved this from Review - Active to Review - Done in Neurobagel Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug:functional Functional defects resulting from feature changes. importance:urgent We will address this as soon as possible. type:bug Defects in shipped code and fixes for those defects
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants