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

set content-type to json for beamline get routes #1392

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

elmjag
Copy link
Contributor

@elmjag elmjag commented Sep 10, 2024

For beamline 'get attributes' API routes, make sure Content-Type header is application/json. The front-end is now checking the content-type header for replies, and assumes an error unless it is set to application/json.

This fixes the issue where changing data collection parameters in the Task Dialog breaks the validation, making it impossible to run the job with non-default parameters.

For beamline 'get attributes' routes, make sure Content-Type
header is 'application/json'.

The front-end is now checking the content-type header for replies,
and assumes an error unless it is set to 'application/json'.
@fabcor-maxiv
Copy link
Contributor

Naive question:

Should it be done systematically on all endpoints? Do we have endpoints that should not return JSON?

@@ -26,7 +26,7 @@ def get_func(name):
**{"return": getattr(app.mxcubecore.get_adapter(name), attr)(**args)}
)

return make_response(result.json(), 200)
return make_response(result.dict(), 200)
Copy link
Collaborator

@axelboc axelboc Sep 10, 2024

Choose a reason for hiding this comment

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

How this change leads to the Content-Type header being set to application/json is just hilarious 😹

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, one would have expected .json() to also set Content-Type to application/json

Copy link
Member

Choose a reason for hiding this comment

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

Well, actually, on second thought it somehow makes sense. I guess json() returns a string so the content is interpreted as text, while a dictionary always can be interpreted as json when returned as a response

Copy link
Contributor Author

@elmjag elmjag Sep 10, 2024

Choose a reason for hiding this comment

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

This is Flask logic. By default, if you give it a string, it creates a text response. If you give it a dictionary, it makes it a application/json response.

And yes, result is a pydantic model (I think), so it's json() method returns a string.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sure it does make sense.

@axelboc
Copy link
Collaborator

axelboc commented Sep 10, 2024

Should it be done systematically on all endpoints? Do we have endpoints that should not return JSON?

Ideally yes, similarly to how exceptions should always lead to 500 errors (#1355). Needs to be done with care, though. If I recall, some error responses (sometimes with the wrong 200 response code) include a message as raw text instead of JSON, for instance.

Copy link
Contributor

@fabcor-maxiv fabcor-maxiv left a comment

Choose a reason for hiding this comment

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

I am fine with this.

I think we should look into checking that all endpoints that send output should send it as JSON, always, even for errors (5xx, 4xx). And that the JavaScript UI frontend is ready to handle JSON in all cases.

#1393

@marcus-oscarsson marcus-oscarsson merged commit fc962b5 into mxcube:develop Sep 10, 2024
7 checks passed
@elmjag elmjag deleted the beamline-api-cont-type branch September 11, 2024 11:09
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.

4 participants