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

fix: Ensure plain text response is only used for strings #29

Merged
merged 1 commit into from
May 21, 2024

Conversation

mattbearman
Copy link
Contributor

@mattbearman mattbearman commented May 17, 2024

It turns out when expected errors occur (eg: those defined via potential_error in an endpoint) they are still handled via the response rack_triplet method. This means the error hash was being treated as plain text instead of JSON if the endpoint specified a plain text response

This PR fixes that by only rendering a plain text response if the type is plain text and the response body is a string

@mattbearman mattbearman marked this pull request as ready for review May 17, 2024 16:00
Copy link

@jimehk jimehk left a comment

Choose a reason for hiding this comment

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

I don't think it'll cause any issues, but technically a body object that is an array of strings, or really anything that responds to #each is a valid body value according to Rack.

This this is specifically for the sake of errors, it might be cleaner if we could used some other indicator than the class of the body object to determine what to do.

@mattbearman
Copy link
Contributor Author

@jimehk true, although in terms of how Apia uses Rack responses it won't be an issue, as we only ever send hashes or strings.

It would definitely be good to make it a bit more robust in this sense, but as Apia was originally built for JSON responses only, it'll take quite a bit of work to make it more response-type agnostic. Definitely something to keep in mind for a future Apia update though

@mattbearman mattbearman merged commit dbf8a5e into main May 21, 2024
9 checks passed
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.

3 participants