Add Rack 3 compatibility #146
Conversation
|
@bethesque Sorry for the ping, but this is blocking a security upgrade for us, so I wanted to check if you'll be able to take a look at this soon? Thank you! |
| @@ -66,7 +66,11 @@ def responsive? | |||
| end | |||
|
|
|||
| def run_default_server(app, port) | |||
| require 'rack/handler/webrick' | |||
There was a problem hiding this comment.
Given that we have rackup declared as a dependency now, do we need to use this approach, or could we go straight to requiring rackup?
|
@YOU54F there's nothing in the pact-ruby-standalone that needs ruby 2.7 support any more is there? |
Negative, we were in-fact blocked by the pact mock service, pinning to rack 2.x in upgrading to the latest version of Rack, it was only pact mock service that relied on it in the pact-ruby-standalone bundle some related PR's in pact-ruby related to the rack 3 dep |
|
Thank you @agfor, appreciate this alot! Will get the Rack dep updated in Pact ruby standalone 👍🏾 |
| require 'rack/handler/webrick' | ||
| rescue LoadError | ||
| require 'rackup/handler/webrick' | ||
| end | ||
| Rack::Handler::WEBrick.run(app, **webrick_opts) do |server| |
There was a problem hiding this comment.
if rackup/handler/webrick is required in previous rescue, then this needs to be Rackup::Handler::WEBrick otherwise it throws a NameError. If it needs to be compatible with rack then we need to handle the error doing something like below (though there might be a neater way of doing it)
def run_default_server(app, port)
begin
require 'rack/handler/webrick'
rescue LoadError
require 'rackup/handler/webrick'
end
begin
Rack::Handler::WEBrick.run(app, **webrick_opts) do |server|
@port = server[:Port]
end
rescue NameError
Rackup::Handler::WEBrick.run(app, **webrick_opts) do |server|
@port = server[:Port]
end
end
end
There was a problem hiding this comment.
hey, I assume this is causing you issues as it stands?
happy to consider an additional pull request.
it may be worth tracking your particular problem as a new issue for visibility
There was a problem hiding this comment.
ive just seen the issue and pr now, thanks!
There was a problem hiding this comment.
Looking at this now, it looks wrong / incomplete to me too, but we're running it on Rack 3 successfully and it worked when I tested it last year.
Ah, I see on the new PR, it's only Rack 3.1 where this is an issue.
There was a problem hiding this comment.
yep sorry should have followed up with the issue we've created for reference #151
This commit adds compatibility with Rack 3 and rack-test 2 while maintaining backwards compatibility.
Rack no longer depends on WEBrick since it's not a default gem in recent versions of Ruby, so add the Rackup gem to restore that functionality.
Use
readinstead ofstringto get the request body in a compatible way.spec/integration/cli_spec.rbexpected the pact files to get written before the server is shut down, whereas all the other similar specs, e.g.spec/integration/control_server_cli_spec.rb, shut the server down before expecting the files to be present. I'm not sure why exactly this works with Rack 2 but not Rack 3, or why it was just this one file that was different, but given all the other files follow the shut-down-the-server pattern, I changed this file to do the same. All of the assertions are still tested, the setup is the only thing that was changed.Fixes #145
There is at least one known security vulnerability in Rack 2 rack/rack#1732 so releasing a new version with this change ASAP would be excellent.