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

summary-only does not work as expected #167

Closed
al-niessner opened this issue Jul 20, 2022 · 5 comments · Fixed by #166
Closed

summary-only does not work as expected #167

al-niessner opened this issue Jul 20, 2022 · 5 comments · Fixed by #166
Assignees
Labels
B13.0 bug Something isn't working s.medium Medium level severity

Comments

@al-niessner
Copy link
Contributor

🐛 Describe the bug

Related to #152. On the main branch summary-only is signaled via limit=0, and it is being too efficient.

$ curl --location --request GET 'http://localhost:8080/uid/urn:nasa:pds:insight_rad:data_derived::7.0/referencing/product?limit=0&fields=lid,lidvid' --header 'Accept: application/kvp+json'
{
  "summary":{"q":"","hits":0,"took":18,"start":0,"limit":0,"sort":[],"properties":[]},
  "data":[
  ]
}

Missing hits, and properties. On release 1.0.1 when Accept: applicaation/json it also has a null point exception during processing (#152 ) which is a different problem but related.

📜 To Reproduce

See above example

🕵️ Expected behavior

$ curl --location --request GET 'http://localhost:8080/uid/urn:nasa:pds:insight_rad:data_derived::7.0/referencing/product?limit=0&fields=lid,lidvid' --header 'Accept: application/kvp+json'
{
  "summary":{"q":"","hits":7,"took":7,"start":0,"limit":0,"sort":[],"properties":["lid","lidvid"]}
}

📚 Version of Software Used

main branch and release 1.0.1

@jordanpadams
Copy link
Member

@al-niessner @tloubrieu-jpl how do we want to handle this as it relates to NASA-PDS/pds-api#173?

@al-niessner
Copy link
Contributor Author

@jordanpadams @tloubrieu-jpl

Continue on with NASA-PDS/pds-api#173 but correct the limit=0 behavior. Technically it is start=0 and limit=0 to make it summary-only. If it is start=10 limit=0 then nothing happens as it is expected to be a typo.

The code has been optimized to exist when the limit has been reached. Obviously this was a mistake on my part and needs to ignore this shortcut for summary purposes. I do not see it a problem for replacing summary-only.

@tloubrieu-jpl
Copy link
Member

Hi @al-niessner I don't think start need to be = 0 , I would say whenever limit is 0 we return the summary only and don't care about the start which can be 0 or 10 or anything.
Let me know what are your thoughts on that.

Thanks

@al-niessner
Copy link
Contributor Author

@tloubrieu-jpl @jordanpadams

Sorry, I confused it with something else. Yes, limit=0 is the only requirement and it does not matter what start is.

@tloubrieu-jpl
Copy link
Member

I will close this ticket, the remaining issue with missing properties is given on #171

@jordanpadams jordanpadams changed the title summary-only does not behave summary-only does not work as expected Aug 18, 2022
@jordanpadams jordanpadams added the s.medium Medium level severity label Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B13.0 bug Something isn't working s.medium Medium level severity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants