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

The road towards 2.0 #225

Closed
perlun opened this issue Mar 27, 2018 · 4 comments
Closed

The road towards 2.0 #225

perlun opened this issue Mar 27, 2018 · 4 comments
Labels
pinned Issues that will never be marked as stale by the robots

Comments

@perlun
Copy link
Contributor

perlun commented Mar 27, 2018

So there; 1.0.0 was just released: https://rubygems.org/gems/rack-test/versions/1.0.0, close to 9 years since the first public version (0.1.0) saw daylight.

This issue exists to track and discuss potential breaking changes & improvements that could be made on the way towards a potential 2.0.0 release (no release date given as of yet.).

Change the semantics re. request params and request body

It became clear from #223 and the preceding PRs that it might not always be entirely clear when making a request, if the parameters provided should go into the URL query parameters (/foo?bar=baz&zot=qux) or be encoded into the request body (normally as multi-part encoded, i.e. bar=baz&zot=qux.)

Some HTTP methods are more unequivocal than others:

  • GET - can have a body, but the body must not impact the response, so normally you would expect the parameters to be provided in the query string here.
  • POST - can have both query parameters and body.
  • PUT - likewise
  • DELETE - some clients only put the parameters in the body (jQuery, whereas others put them in the query parameters (if I'm not mistaken, RestSharp does it ths way by default.) So here it is harder to say which default makes more sense.

The suggested approach would be to change the semantics to something like this:

# Old, current approach - ambigous, what should it choose?
get '/foo', bar: 'baz', zot: 'qux'

# New proposed syntax for query parameters
get '/foo', query: { bar: 'baz', zot: 'qux' }

# New proposed syntax for request body
post '/foo', body: { bar: 'baz', zot: 'qux' }

# Or combine them both if you like
post '/foo', body: { bar: 'baz', zot: 'qux' }, query: { limit: 100 }
  • Advantages:
    • Less ambiguous, more flexible.
  • Disadvantages
    • Breaks all existing usage of the gem.

Change to avoid using class-global state

The current way the gem is written is pretty nice and simple in one way, but: what if you are making multiple requests and need to e.g. compare the results between them? No can do; the gem uses a lot of temporal coupling and state from one request until the next request is being made.

This is fine for many use cases, but... what if we would be able to find a better way? Something like:

req = Rack::Test::Request.new
response = req.get '/foo', query: { bar: 'baz', zot: 'qux' }

expect(response.status).to eq 200
expect(response.body).to eq '42'

Arguably a bit more kludgy and ugly, but it would make some use cases a lot nicer. Making a mixin keep a lot of state when being used feels a bit impure in my eyes, but: I would be open to other people's opinions here also. Maybe it's just me and 1 000 000 of the rest of you are completely fine with the current API?

  • Advantages:
    • Can have multiple requests at the same time. Simplifies the code.
  • Disadvantages
    • Breaks all existing usage of the gem.

Your favourite feature here?

This is a work in progress - feel free to suggest improvements here.

@stale
Copy link

stale bot commented May 26, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label May 26, 2018
@perlun perlun added pinned Issues that will never be marked as stale by the robots and removed wontfix labels May 28, 2018
@perlun
Copy link
Contributor Author

perlun commented May 28, 2018

Pinning to keep stale from closing it. It's not certain that these feature will ever be implemented, but I'd at least want the discussion about them to be open for now.

@perlun
Copy link
Contributor Author

perlun commented Jun 21, 2018

Some of this is actually already there, as illustrated by #235 - if you create an explicit session, you can use Rack::Test in a more "OOP-oriented" (instead of mixin-oriented) fashion:

    let(:digest_session) do
      session = Rack::Test::Session.new(Rack::MockSession.new(digest_app))
      session.digest_authorize('test-name', 'test-password')
      session
    end

    it 'retries digest requests' do
      session = digest_session

      session.request('/')

      expect(session.last_request.env['rack-test.digest_auth_retry']).to be_truthy
    end 

@jeremyevans
Copy link
Contributor

I'm going to close this as #287 implements the query/body separation and I think #225 (comment) shows how you can already implement separate session support.

I'm certainly open to to a major bump in the future if it is really required, but even in a major version bump I would try to keep backwards compatibility as much as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Issues that will never be marked as stale by the robots
Projects
None yet
Development

No branches or pull requests

2 participants