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

Use Rack to generate query information under test #33093

Merged
merged 1 commit into from Jun 11, 2018
Merged

Conversation

@tenderlove
Copy link
Member

@tenderlove tenderlove commented Jun 8, 2018

to_query sorts parameters before encoding them. This causes a round
tripping issue as noted here:

#23997 (comment)
#10529 (comment)
#30558

Unfortunately, that method is being used to generate cache keys, so its
results need to be stable:

10dec0e

However, the test harness is only using to_query to encode parameters
before sending them to the controller so the "cache key" usecase doesn't
apply here.

This commit adds a test that demonstrates the round trip problems and
changes the serialization strategy to use Rack for encoding the
parameters rather than to_query.

`to_query` sorts parameters before encoding them.  This causes a round
tripping issue as noted here:

  #23997 (comment)
  #10529 (comment)
  #30558

Unfortunately, that method is being used to generate cache keys, so its
results need to be stable:

  10dec0e

However, the test harness is only using `to_query` to encode parameters
before sending them to the controller so the "cache key" usecase doesn't
apply here.

This commit adds a test that demonstrates the round trip problems and
changes the serialization strategy to use Rack for encoding the
parameters rather than `to_query`.
@tenderlove
Copy link
Member Author

@tenderlove tenderlove commented Jun 8, 2018

I think the GET case has the same problem, but I don't have a failing test for it yet so I didn't touch it.

@tenderlove tenderlove merged commit 4eb1481 into master Jun 11, 2018
0 of 3 checks passed
0 of 3 checks passed
codeclimate 14 issues to fix
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@tenderlove tenderlove deleted the use-rack-to-dump-query branch Jun 11, 2018
tenderlove added a commit that referenced this pull request Jun 12, 2018
Use Rack to generate query information under test
tenderlove added a commit that referenced this pull request Jun 12, 2018
Use Rack to generate query information under test
tenderlove added a commit that referenced this pull request Jun 12, 2018
Use Rack to generate query information under test
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jun 26, 2018

This cause a regression when you pass Active Record objects to the parameters. Before when you do:

post :foo, params: { user: User.first }

You would get in the controller { "user" => "1" }.

Now you will get { "user": "#<User: ...>" }.

This change in behavior should be fine for the master branch if we deprecate it, but for stable branches this will break some people tests. (I found this because some tests broke when upgrading shopify to the stable branch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.