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

Update API default response in browsers to force application/json #360

Merged
merged 4 commits into from
Aug 9, 2023

Conversation

al-niessner
Copy link
Contributor

🗒️ Summary

Part of the problem with the PDS product is that when in a browser it is better viewed using a plugin. To this end, force the return type for text/html of a PDS product or list of products to be application/json. Lovely, but if you are accessig a product directly, it is kind of what you deserve and probably desire.

Found how to set the content-type for the output. It can be overridden but required a rework of the list in WebMVCConfig. Turns out is is very order dependent. The current order seems to have everything working as desired while maybe not correctly.

It also turns out that setting the content-type is also order dependent with requesting the body. Yikes so much hidden order requirements in the bowels of spring. Anyway, marked all of the converters with comments to hopefully keep their order correct.

⚙️ Test Data and/or Report

See screenshot:
json_browser

♻️ Related Issues

Closes #356

Part of the problem with the PDS product is that when in a browser it is better viewed using a plugin. To this end, force the return type for text/html of a PDS product or list of products to be application/json. Lovely, but if you are accessig a product directly, it is kind of what you deserve and probably desire.

Found how to set the content-type for the output. It can be overridden but required a rework of the list in WebMVCConfig. Turns out is is very order dependent. The current order seems to have everything working as desired while maybe not correctly.

It also turns out that setting the content-type is also order dependent with requesting the body. Yikes so much hidden order requirements in the bowels of spring. Anyway, marked all of the converters with comments to hopefully keep their order correct.
@sonatype-lift
Copy link

sonatype-lift bot commented Jul 19, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@al-niessner
Copy link
Contributor Author

@jordanpadams

If you can build and run this branch give it a try. It should do what you want. It does with my firefox and the JSONView plugin as shown in the screenshot.

Copy link
Member

@tloubrieu-jpl tloubrieu-jpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I have 2 different behavior with my 2 chrome profiles:

The one which use to work to display json with the browser's plugin, does not work anymore:
image

The one which did not work, now works, although in a peculiar way I don't really understand:
image

Content-type of the response is still text/html in both cases.

The request Accept headers are the same in both cases:

  • text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7
  • text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7

@tloubrieu-jpl
Copy link
Member

tloubrieu-jpl commented Jul 19, 2023

@jordanpadams @al-niessner I don't have the same JSON plugin under my 2 profiles...
case 1: https://github.com/tulios/json-viewer, worked before, does not work with PR
case 2 : https://github.com/callumlocke/json-formatter, did not work before, works with PR

@al-niessner
Copy link
Contributor Author

@jordanpadams @tloubrieu-jpl

If you are seeing content-type of text/html then something is wrong with your build:

$ curl -vX GET -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7' 'http://localhost:8080/products/urn:nasa:pds:mars2020.spice::1.0' -o /dev/null 
Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying 127.0.0.1:8080...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /products/urn:nasa:pds:mars2020.spice::1.0 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.81.0
> Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 
< Content-Disposition: inline;filename=f.txt
< Content-Type: application/json
< Transfer-Encoding: chunked
< Date: Wed, 19 Jul 2023 19:28:22 GMT
< 
{ [6241 bytes data]
100  6228    0  6228    0     0   127k      0 --:--:-- --:--:-- --:--:--  129k
* Connection #0 to host localhost left intact

@jordanpadams jordanpadams changed the title force the content-type Update API default response in browsers to force application/json Jul 31, 2023
@alexdunnjpl
Copy link
Contributor

@tloubrieu-jpl have you tried in either Chrome or Firefox, without any plugins?

No plugins should be necessary, and third-party plugin compatibility shouldn't be a concern imho.

@alexdunnjpl
Copy link
Contributor

Will leave approval for @tloubrieu-jpl or @jordanpadams as I don't have context on exact requirements so it'd be a rubber-stamp.

Copy link
Member

@tloubrieu-jpl tloubrieu-jpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works

@tloubrieu-jpl tloubrieu-jpl merged commit b32f015 into main Aug 9, 2023
1 check passed
@tloubrieu-jpl tloubrieu-jpl deleted the issue_356 branch August 9, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept:* response not defaulting to valid application/json
3 participants