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

Fallback to StringIO to prevent nil.rewind errors #3

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

samsm
Copy link

@samsm samsm commented Jun 17, 2024

Problem

Before Rack 3.1, last_request.env["rack.input"] was an empty StringIO object when there wasn't a rack input.
As of Rack 3.1 last_request.env["rack.input"] is nil when there is no input.

This is a stumbling block for rspec_api_documentation's inner functionings.

Solution, short term edition

As a "for now" solution, we can fall back in to StringIO when generating docs. This PR makes StringIO.new the default when last_request.env["rack.input"] is nil.

A good thing about this solution is that it will address the rspec doc generation problem without forcing a pin on rack. We do not want to pin rack because it is involved in production functionality and we want to be sure to keep up to date in order to capitalize on all security and performance updates.

In contrast, our posture towards rspec_api_documentation is weaker because there has not been an official release since 2018 and the library is not in the path of production requests.

Solution, longer term edition (not in this PR)

One of these:

  • Invest in a fork of rspec_api_documentation and actively keep it up to date with the Ruby ecosystem
  • Move away from using rspec_api_documentation for documentation

@samsm samsm marked this pull request as ready for review June 17, 2024 19:43
@zorab47
Copy link

zorab47 commented Jun 17, 2024

I do think this change is better than the one proposed on the original project: zipmark#541. 👍

@samsm samsm merged commit f4e5508 into master Jun 18, 2024
@samsm samsm deleted the nil-dot-rewind-errors branch June 18, 2024 12:29
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