Rewrite CookieAdapter to work with Rack::Request and Rack::Response directly - #490
Conversation
|
Looks good to me, thanks so much for your contribution 🎉 |
|
Hi @andrew, any chance we can get a new gem release that includes this PR? Thanks! |
|
@itspriddle sorry for the delay, I'll get a release out within the next couple days |
|
@itspriddle shipped in 3.1: https://rubygems.org/gems/split/versions/3.1.0 |
| :expires => @expires | ||
| } | ||
| def set_cookie(value = {}) | ||
| @response.set_cookie :split.to_s, default_options.merge(value: JSON.generate(value)) |
There was a problem hiding this comment.
I think that this change may be causing split to send back multiple Set-Cookie headers in the case where a participant is involved in more than one split test. Version 3.0.0 did not have this problem. But after upgrading to 3.1.0 (which included this PR), I started seeing one Set-Cookie response headers for every experiment mentioned in the cookie submitted by the client.
In a few cases, the cookies had enough experiments in them that my load balancer rejected the response as invalid.
Downgrading to 3.0.0 fixed the problem.
I think that @response.set_cookie is writing a new cookie each time it is called. However, @cookies[:split] was overwriting the previous value of the cookie in place.
4 participants
CookieAdapter receives the 'context' passed down to the User instance. Which is different on a Sinatra(Dashboard) and a Rails App.
As they are both Rack instances, the easier way to keep it working for both Rails and Sinatra I used the request/response to read/write the cookies from Rack directly.
Also updated the Specs to test against mock Rack requests. @andrew Please let me know what you think!
Fixes #480