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

Accept:* response not defaulting to valid application/json #356

Closed
jordanpadams opened this issue Jun 29, 2023 · 13 comments · Fixed by #360
Closed

Accept:* response not defaulting to valid application/json #356

jordanpadams opened this issue Jun 29, 2023 · 13 comments · Fixed by #360
Assignees
Labels
B14.0 bug Something isn't working i&t.done s.high High severity

Comments

@jordanpadams
Copy link
Member

Checked for duplicates

Yes - I've already checked

🐛 Describe the bug

When I did https://pds.nasa.gov/api/search/1/products/urn:nasa:pds:mars2020_sherloc::1.0 in my browser, and it does not pretty print with my JSON plugin.

🕵️ Expected behavior

I expected it would display pretty print JSON

📜 To Reproduce

  1. Go to here in browser: https://pds.nasa.gov/api/search/1/products/urn:nasa:pds:mars2020_sherloc::1.0
  2. See
Screenshot 2023-06-29 at 8 58 25 AM

🖥 Environment Info

No response

📚 Version of Software Used

No response

🩺 Test Data / Additional context

No response

🦄 Related requirements

🦄 #248

⚙️ Engineering Details

No response

@jordanpadams jordanpadams added B14.0 bug Something isn't working sprint-backlog labels Jun 29, 2023
@jordanpadams jordanpadams self-assigned this Jun 29, 2023
@jordanpadams jordanpadams added the s.medium Medium level severity label Jun 29, 2023
@jordanpadams jordanpadams changed the title Default Accept:* response not defaulting to valid application/json Accept:* response not defaulting to valid application/json Jun 29, 2023
@jordanpadams jordanpadams removed their assignment Jun 29, 2023
@jordanpadams jordanpadams added s.high High severity and removed s.medium Medium level severity labels Jun 29, 2023
@al-niessner
Copy link
Contributor

@jordanpadams

What browser and what plugin. May be fixed but my browser shows the same. Need to know the plugin. Thanks.

@jordanpadams
Copy link
Member Author

jordanpadams commented Jul 13, 2023

@al-niessner

  • Chrome 114.0.5735.198, JSON Formatter 0.7.1 and JSONView don't work
  • Firefox 102.13.0esr, Basic JSON Formatter did not work, JSON Lite does work

Also on Apple M2 Max OSx v13.4.1

@al-niessner
Copy link
Contributor

@jordanpadams

Just tried JSONView and it does not work for me either but they do include an example. They return:

< accept-ranges: bytes
< content-length: 260
< cache-control: max-age=0, no-transform, immutable
< expires: Thu, 13 Jul 2023 19:50:20 GMT
< vary: User-Agent
< x-frame-options: DENY
< x-content-type-options: nosniff
< content-type: application/json; charset=utf-8

while we return

* Mark bundle as not supporting multiuse
< HTTP/1.1 200 
< Content-Disposition: inline;filename=f.txt
< Content-Type: application/json
< Transfer-Encoding: chunked
< Date: Thu, 13 Jul 2023 19:48:36 GMT

The case is different for our Content-Type than theirs. I have not tracked down if that is a problem yet. If the browser really cares about case then we are in trouble. The 'Content-Type' is done in spring-boot or some layer there about. Certainly out of our direct control.

Unless you toss in a rescue line, going down the deep dark hole of why our application/json is not being detected starting in Firefox.

@jordanpadams
Copy link
Member Author

@al-niessner very odd. I know other users have indicated this as an issue as well. @msbentley I believe the JSON was not pretty printed when you were trying to access the API from a browser? If so, what is the browser/extension you are using?

@al-niessner
Copy link
Contributor

@jordanpadams @tloubrieu-jpl

UHG! I remember this now. It is a browser problem to start with then a coding problem after. The request from a browser like firefox use this for the accept:

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8

So we do text/html, application/xml, and /. The problem is how to prioritize that list. In the case of the example with JSONView etc is that they return application/json no matter what while see to be responding to text/html (a big hacking string). It is because we prioritize the most desired text/html over all the rest. So, when I do a curl it is just / and it thus returns JSON. When I do a browser, not so much.

Now it gets even more complicated because Jackson is no longer calling our module(s) when it converts Java objects to JSON. Not sure when that changed, but I can easily show it is not calling our code at all via log messages.

Hence, in order to return objects that JSONView detects we have to ignore Accept (cannot tell when it comes from a browser or not) or tell our browsers that we want application/json only and set the Accept accordingly. Even after that is fixed, we may have a problem with Jackson.

@tloubrieu-jpl
Copy link
Member

We add a new query parameter 'format' when it is provided by the user it overrides the default content negociation behavior.

For example: https://pds.nasa.gov/api/search/1/products?format=json

@jordanpadams
Copy link
Member Author

jordanpadams commented Jul 16, 2023

Comment from @al-niessner on #349 that I believe was intended for here:

@alexdunnjpl @jordanpadams @tloubrieu-jpl

Update for all those that had opinion on ?format=json. It is not so simple and may not work out at all. Once our code (SwaggerJavaTransmutter) is done (says how long it took) and returns the response object to spring-boot, it all goes awry. Hardcoded the response object to be application/json no matter what is given by the user and after the last of our code runs:

g.n.p.a.r.c.SwaggerJavaTransmuter        : Transmuter processing of request took: 19 ms
o.s.w.s.m.m.a.HttpEntityMethodProcessor  : Using 'text/html', given [text/html, application/xhtml+xml, image/avif, image/webp, application/xml;q=0.9, */*;q=0.8] and supported [application/csv, application/json, application/kvp+json, application/vnd.nasa.pds.pds4+json, application/vnd.nasa.pds.pds4+xml, application/xml, text/csv, text/html, text/xml, */*]

Some other super helpful module says oh look we really want it to be text/html not application/json from the accept we were given. Will look to see if that can be overridden but my hopes are not high.

@jordanpadams
Copy link
Member Author

@al-niessner @tloubrieu-jpl @alexdunnjpl if the code generator is starting to become a headache, we may need to just use that as a staging area we can copy-paste boilerplate code when we update endpoints, but that have our own forked codebase we work from.

@al-niessner
Copy link
Contributor

@alexdunnjpl @jordanpadams @tloubrieu-jpl

Thanks @jordanpadams it was intended for here. It is not code generation as much as it is the whole underlying spring-boot or spring-framework that we use to handle the http(s) non-application stuff. It is pulled in by maven and we get constant update warnings for security fixes to these maven artifacts. It would be extremely difficult and costly to "hold it still" and do our own patches.

@al-niessner
Copy link
Contributor

@alexdunnjpl @jordanpadams @tloubrieu-jpl

Okay, things are making more sense now. It seems that #254 pushed some changes that broke the test/html for PdsProduct(s) but probably needed for more standard items. It is taking the response object and doing a toString() on it to get the output. Explains why I cannot get a hold of the object.

@al-niessner
Copy link
Contributor

@alexdunnjpl @jordanpadams @nutjob4life @tloubrieu-jpl

Progress is slow. If anyone knows how to run SpringBootMain as an eclipse application without l4j throwing an AbstractMethodError, it would be appreciated. I think it is a properties file thing but cannot find nor fix how nor where l4j reads properties so that it can instantiate the correct stuff that it does when run with maven. I have been pressing onward with text message manipulation but it is so slow.

Now, the update: It seems that in #254 the default test/http converter for all things practically because Jackson mapper outside of our code. It is now (seems new in my google search) that with Jackson in our path spring helpfully adds this converter. Unlike the documentation it seems to be doing after we build our custom list. In fact they seem immutable but need a debugger to figure it out - hence the wine and cheese appetizer.

Kind of stuck so going to walk for lunch and think about it.

@alexdunnjpl
Copy link
Contributor

@al-niessner I doubt this is very palatable, but if the Eclipse route doesn't prove tractable I can share my IntelliJ setup/runconfigs

@al-niessner
Copy link
Contributor

@alexdunnjpl @jordanpadams @tloubrieu-jpl

Finally, a solution but not the one we thought; it is the one @jordanpadams wanted though. Summary: moved the spring help aside through code order to do what we want.

Turns out that most of the problems are code ordering. Yes, have to be careful if you do getHeaders() before or after getBody(). I mean why would not know that! Seems that given some the spring beans that the get functions have side effects. Anyway, that was the last of the stumpers.

The first stumper was in WebMVCConfig. The list had been altered and after I found the loop used to determine which convert is used, I realized that order of the converters in the list matter.

Fixed all the ordering and forced text/html to become application/json when it is a PDS product only. So, no need for yet another URL argument. While this is probably what is desired, it just seems wrong to change the content-type. However, even without an app to view JSON in the browser, it still shows as boring plain text. I think I am going to go back and put the pretty printer back in place for those cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B14.0 bug Something isn't working i&t.done s.high High severity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants