-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Add plain text responses #25
Conversation
4897c54
to
2cb79e6
Compare
# Return a plain text triplet for the given body. | ||
# | ||
# @param body [String] | ||
# @param status [Integer] | ||
# @param headers [Hash] | ||
# @return [Array] | ||
def plain_triplet(body, status: 200, headers: {}) | ||
response_triplet(body, content_type: 'text/plain', status: status, headers: headers) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good @mattbearman !
Would you be able to have a go at adding a plain text endpoint to the example api used in the tests for our apia-openapi gem.
https://github.com/apiaframework/apia-openapi/blob/main/examples/core_api/base.rb
It would be good to confirm we can successfully add this change into the openapi spec that is generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I have left a few comments and questions, but nothing that I consider essential for this to be merged :)
lib/apia/rack.rb
Outdated
headers.merge('content-type' => 'application/json', 'content-length' => body_as_json.bytesize.to_s), | ||
[body_as_json] | ||
headers.merge('content-type' => content_type, 'content-length' => body.bytesize.to_s), | ||
[body] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: We could make this method a lot more powerful if we only wrap body
in an array when it does not respond to each
.
That would allow providing custom body objects which respond to each
, enabling all manner of fancy things like streaming responses and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, certainly wouldn't hurt 😄
case type | ||
when JSON | ||
Rack.json_triplet(body, headers: headers, status: status) | ||
when PLAIN | ||
Rack.plain_triplet(body, headers: headers, status: status) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Since we're in the area, and we have the Rack.response_triplet
helper method now, do you think it might be worth expanding the response object slightly to also allow fully custom response body objects and content types?
This might also have even further implications with the OpenAPI schema though. Basically, if this wouldn't be a quick win, let's ignore it until we need it :)
lib/apia/response.rb
Outdated
def plain! | ||
@type = PLAIN | ||
@body = '' | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the usage pattern within an endpoint is be something like this?
response.plain!
response.body = "my awesome response"
Do we care about requiring #plain!
to be called before the body value is set? Might a nicer usage pattern be a helper method? Though I'm not sure what such a method might be called, maybe respond_with(body, content_type = nil)
? What do you think?
…ody an array if it is not already enumerable
# @param headers [Hash] | ||
# @return [Array] | ||
def response_triplet(body, content_type:, status: 200, headers: {}) | ||
content_length = body.bytesize.to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that bytesize
is only available on String
objects. We should probably do a responds to check for it, and simply not set Content-Length
if it does not respond to the method.
However, we don't directly expose a means of sending a custom response. While the Response#plain_text_body
method does pass the body
value through as is, I would considering giving it anything other than a String
object as incorrect usage.
Hence, I'd say this is not a blocker for merging this PR, but something that needs to be dealt with in the future should we want to expose fully custom responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the caveat about the bytesize
method in my earlier comment, I think this is alright to merge.
Adds the option to return plain text from an Apia API, by calling
response.plain_text_body
, and passing in the body content