Allow response_body_formatter config to format "binary" responses #458

davidstosik · 2020-01-24T09:24:39Z

Following this issue on Rack's repository: Fix incorrect MockResponse#body String encoding, it looks like we can expect, from now on, that Rack's response_body encoding will be Encoding::ASCII_8BIT:

I think the response body should probably always be ASCII-8BIT. Rack can't really know anything about the encoding of the bytes that users want to send. At the end of the day, it's just bytes written to the socket, and I don't think Rack should have any opinion about the encoding the user sends.

Therefore, rspec_api_documentation cannot rely on response_body.encoding to determine whether the response is binary data or not. This line becomes incorrect:

rspec_api_documentation/lib/rspec_api_documentation/client_base.rb

Lines 90 to 91 in 81e5c56

    
           if response_body.encoding == Encoding::ASCII_8BIT 
        
             "[binary data]"

The real fix would be to figure out a better way to define whether a string is binary or not, but I believe this is a bit outside the scope of my knowledge.

In this PR, I'm focusing on giving any application using the rspec_api_documentation gem the choice on how to process the response_body, and particularly on how to detect binary data, while not altering rspec_api_documentation's default behaviour.

As an additional benefit, this change would allow an application to display friendlier responses in its documentation for some binary types.

For example, PNG data:

config.response_body_formatter =
  Proc.new do |content_type, response_body|
    # http://www.libpng.org/pub/png/spec/1.2/PNG-Rationale.html#R.PNG-file-signature
    if response_body[0,8] == "\x89PNG\r\n\u001A\n"
      "<img src=\"data:image/png;base64,#{Base64.strict_encode64(response_body)}\" />"
    elsif content_type =~ /application\/.*json/
      JSON.pretty_generate(JSON.parse(response_body))
    else
      response_body
    end
  end

sikachu · 2020-01-24T09:41:53Z

Just want to drop a note that this PR will fix #456 as well.

zipmark#458

Do not display binary data in generated documentation. Closes #199.

bf4 · 2020-01-31T03:51:44Z

lib/rspec_api_documentation/configuration.rb

@@ -118,7 +118,9 @@ def self.add_setting(name, opts = {})
    # See RspecApiDocumentation::DSL::Endpoint#do_request
    add_setting :response_body_formatter, default: Proc.new { |_, _|
      Proc.new do |content_type, response_body|
-        if content_type =~ /application\/.*json/
+        if response_body.encoding == Encoding::ASCII_8BIT
+          "[binary data]"


I see, so this is backwards compatible. 👍

README.md

bf4 · 2020-02-04T18:11:42Z

README.md

@@ -250,7 +250,8 @@ RspecApiDocumentation.configure do |config|

  # Change how the response body is formatted by default
  # Is proc that will be called with the response_content_type & response_body
-  # by default response_content_type of `application/json` are pretty formated.
+  # by default, a response body that is likely to be binary is replaced with the string
+  # "[binary data]" regardless of the media type.  Otherwise, a response_content_type of `application/json` is pretty formatted.


Suggested change

# "[binary data]" regardless of the media type. Otherwise, a response_content_type of `application/json` is pretty formatted.

# "[binary data]" regardless of the media type. Otherwise, a response_content_type identified as JSON is pretty formatted.

bf4 · 2020-02-04T18:13:08Z

@davidstosik you have passing tests locally? I tried and got mixed results with the feature specs, and issues with Webmock... see my closed attempt #460

davidstosik · 2020-02-06T02:28:33Z

To be frank, I had not even tried to run tests locally! 😅
As I saw Travis tests passed on two (oldest) out of 3 versions of Ruby, I assumed the failure was not related to my changes...

Locally on Ruby 2.6.5, I managed to run the tests, but ended up with 14 failures (mainly "undefined method `<<' for Hash" and one "undefined method `close' for StubSocket"). 🤔

Webmock 1.22.6 is very old and it is likely it does not work on more recent versions of Ruby. 🤔 (It looks like support for Ruby 2.4 was added in 2.3.1...)

I think removing Gemfile.lock is correct.

Without Gemfile.lock however, I get more recent versions of the gem dependencies. It's fine for runtime dependencies (as a gem should be able to work with most), however, version requirements on development dependencies might be more restrictive than what rspec_api_documentation.gemspec shows... 🤔

Update: check this: #461 🙏

bf4 · 2020-02-06T14:00:15Z

Awesome job! My own policy for gemspec is development deps should only be what is required to run tests, then use the Gemfile(s) to add other things or version specific gems or constraints

mz026 · 2020-02-21T09:07:43Z

Any update on this?

the-spectator · 2020-02-26T06:50:51Z

@davidstosik Thank you for your work!
@oestrich Any update on this getting merged?

Following this issue on Rack's repository: [Fix incorrect MockResponse#body String encoding](rack/rack#1486), it looks like we can expect, from now on, that Rack's `response_body` encoding will be `Encoding::ASCII_8BIT`: > I think the response body should probably always be ASCII-8BIT. Rack can't really know anything about the encoding of the bytes that users want to send. At the end of the day, it's just bytes written to the socket, and I don't think Rack should have any opinion about the encoding the user sends. Therefore, `rspec_api_documentation` cannot rely on `response_body.encoding` to determine whether the response is binary data or not. This line becomes incorrect: https://github.com/zipmark/rspec_api_documentation/blob/81e5c563ce6787f143cf775c64e2bd08c35d3585/lib/rspec_api_documentation/client_base.rb#L90-L91 The real fix would be to figure out a better way to define whether a string is binary or not, but I believe this is a bit outside the scope of my knowledge. In this PR, I'm focusing on giving any application using the `rspec_api_documentation` the choice on how to process the response_body, and particularly on how to detect binary data, while not altering `rspec_api_documentation`'s default behaviour. As an additional benefit, this change would allow an application to display friendlier responses in the documentation for some binary types. For example, PNG data: ```rb Proc.new do |content_type, response_body| # http://www.libpng.org/pub/png/spec/1.2/PNG-Rationale.html#R.PNG-file-signature if response_body[0,8] == "\x89PNG\r\n\u001A\n" "<img src=\"data:image/png;base64,#{Base64.strict_encode64(response_body)}\" />" elsif content_type =~ /application\/.*json/ JSON.pretty_generate(JSON.parse(response_body)) else response_body end end ```

Co-Authored-By: Benjamin Fleischer <[email protected]>

davidstosik · 2020-10-02T02:39:28Z

@jakehow Thanks for merging my pull request that restores CI! (#461)

I rebased this PR on top of master, and now CI is green!

…pmark#458) * Allow response_body_formatter config to format "binary" responses Following this issue on Rack's repository: [Fix incorrect MockResponse#body String encoding](rack/rack#1486), it looks like we can expect, from now on, that Rack's `response_body` encoding will be `Encoding::ASCII_8BIT`: > I think the response body should probably always be ASCII-8BIT. Rack can't really know anything about the encoding of the bytes that users want to send. At the end of the day, it's just bytes written to the socket, and I don't think Rack should have any opinion about the encoding the user sends. Therefore, `rspec_api_documentation` cannot rely on `response_body.encoding` to determine whether the response is binary data or not. This line becomes incorrect: https://github.com/zipmark/rspec_api_documentation/blob/81e5c563ce6787f143cf775c64e2bd08c35d3585/lib/rspec_api_documentation/client_base.rb#L90-L91 The real fix would be to figure out a better way to define whether a string is binary or not, but I believe this is a bit outside the scope of my knowledge. In this PR, I'm focusing on giving any application using the `rspec_api_documentation` the choice on how to process the response_body, and particularly on how to detect binary data, while not altering `rspec_api_documentation`'s default behaviour. As an additional benefit, this change would allow an application to display friendlier responses in the documentation for some binary types. For example, PNG data: ```rb Proc.new do |content_type, response_body| # http://www.libpng.org/pub/png/spec/1.2/PNG-Rationale.html#R.PNG-file-signature if response_body[0,8] == "\x89PNG\r\n\u001A\n" "<img src=\"data:image/png;base64,#{Base64.strict_encode64(response_body)}\" />" elsif content_type =~ /application\/.*json/ JSON.pretty_generate(JSON.parse(response_body)) else response_body end end ``` * Update README.md Co-Authored-By: Benjamin Fleischer <[email protected]>

ChrisBr approved these changes Jan 24, 2020

View reviewed changes

samnang added a commit to samnang/rspec_api_documentation that referenced this pull request Jan 29, 2020

Fix binary data response

7b4cef7

zipmark#458

bf4 referenced this pull request Jan 31, 2020

If test client encounters ASCII_8BIT data, mark it as binary

8e670ea

Do not display binary data in generated documentation. Closes #199.

bf4 reviewed Jan 31, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

bf4 mentioned this pull request Jan 31, 2020

Fixes jejacks0n/apitome#115

Merged

bf4 reviewed Feb 4, 2020

View reviewed changes

davidstosik mentioned this pull request Feb 6, 2020

Try to restore CI #461

Merged

davidstosik and others added 2 commits October 2, 2020 11:34

Update README.md

6675985

Co-Authored-By: Benjamin Fleischer <[email protected]>

davidstosik force-pushed the response_body_formatter_binary branch from f7889eb to 6675985 Compare October 2, 2020 02:35

davidstosik mentioned this pull request Oct 2, 2020

Documentation no longer include JSON response body when using with Rack >= 2.1.0 #456

Open

jakehow merged commit b0e5889 into zipmark:master Oct 2, 2020

davidstosik deleted the response_body_formatter_binary branch October 2, 2020 02:51

davidstosik restored the response_body_formatter_binary branch October 7, 2020 04:17

senid231 mentioned this pull request Feb 21, 2022

fix api docs response body yeti-switch/yeti-web#1112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow response_body_formatter config to format "binary" responses #458

Allow response_body_formatter config to format "binary" responses #458

davidstosik commented Jan 24, 2020 •

edited

Loading

sikachu commented Jan 24, 2020

bf4 Jan 31, 2020

bf4 Feb 4, 2020

bf4 commented Feb 4, 2020

davidstosik commented Feb 6, 2020 •

edited

Loading

bf4 commented Feb 6, 2020

mz026 commented Feb 21, 2020

the-spectator commented Feb 26, 2020

davidstosik commented Oct 2, 2020

	if response_body.encoding == Encoding::ASCII_8BIT
	"[binary data]"

	# "[binary data]" regardless of the media type. Otherwise, a response_content_type of `application/json` is pretty formatted.
	# "[binary data]" regardless of the media type. Otherwise, a response_content_type identified as JSON is pretty formatted.

Allow response_body_formatter config to format "binary" responses #458

Allow response_body_formatter config to format "binary" responses #458

Conversation

davidstosik commented Jan 24, 2020 • edited Loading

sikachu commented Jan 24, 2020

bf4 Jan 31, 2020

Choose a reason for hiding this comment

bf4 Feb 4, 2020

Choose a reason for hiding this comment

bf4 commented Feb 4, 2020

davidstosik commented Feb 6, 2020 • edited Loading

bf4 commented Feb 6, 2020

mz026 commented Feb 21, 2020

the-spectator commented Feb 26, 2020

davidstosik commented Oct 2, 2020

davidstosik commented Jan 24, 2020 •

edited

Loading

davidstosik commented Feb 6, 2020 •

edited

Loading