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

Support AC::Parameters for PG HStore #27058

Merged
merged 1 commit into from Nov 15, 2016

Conversation

Projects
None yet
3 participants
@maclover7
Member

maclover7 commented Nov 15, 2016

Summary

  • changelog
  • regression test
  • make commit description more descriptive
@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 15, 2016

Member

This is fine to merge once CI is green.

Member

sgrif commented Nov 15, 2016

This is fine to merge once CI is green.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 15, 2016

Member

Please add a test though. :)

Member

sgrif commented Nov 15, 2016

Please add a test though. :)

@sgrif

to_unsafe_hash is the correct method to call.

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Nov 15, 2016

Member

@sgrif are you sure we want the to_unsafe_hash / to_unsafe_h method being called? I'm just worried about unpermitted keys slipping through, and potential sec issues...

Member

maclover7 commented Nov 15, 2016

@sgrif are you sure we want the to_unsafe_hash / to_unsafe_h method being called? I'm just worried about unpermitted keys slipping through, and potential sec issues...

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 15, 2016

Member

Yes. It isn't a sec issue. to_unsafe_h mirrors the behavior of 4.2 and earlier.

Member

sgrif commented Nov 15, 2016

Yes. It isn't a sec issue. to_unsafe_h mirrors the behavior of 4.2 and earlier.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 15, 2016

Member

to_h will also have unintended consequences which is why we almost never use it as a solution to the "params no longer inherit from hash" problem

Member

sgrif commented Nov 15, 2016

to_h will also have unintended consequences which is why we almost never use it as a solution to the "params no longer inherit from hash" problem

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Nov 15, 2016

Member

updated @sgrif

Member

maclover7 commented Nov 15, 2016

updated @sgrif

Show outdated Hide outdated activerecord/test/cases/adapters/postgresql/hstore_test.rb
@@ -10,6 +10,12 @@ class Hstore < ActiveRecord::Base
store_accessor :settings, :language, :timezone
end
class FakeParameters
def to_h

This comment has been minimized.

@sgrif

sgrif Nov 15, 2016

Member

This is wrong

@sgrif

sgrif Nov 15, 2016

Member

This is wrong

This comment has been minimized.

@maclover7

maclover7 Nov 15, 2016

Member

derp, sorry, fixing now

@maclover7

maclover7 Nov 15, 2016

Member

derp, sorry, fixing now

This comment has been minimized.

@sgrif

sgrif Nov 15, 2016

Member

Once that's fixed this is fine to squash and merge. Don't forget to backport to 5-0-stable.

@sgrif

sgrif Nov 15, 2016

Member

Once that's fixed this is fine to squash and merge. Don't forget to backport to 5-0-stable.

Support AC::Parameters for PG HStore
As reported via #26904, there is a regression in how values for
Postgres' HStore column type are being processed, beginning in Rails 5.
Currently, the way that Active Record checks whether or not values need
to be serialized and put into the correct storage format is whether or
not it is a `Hash` object. Since `ActionController::Parameters` no
longer inherits from `Hash` in Rails 5, this conditional now returns
false. To remedy this, we are now checking to see whether the `value`
parameters being passed in responds to a certain method, and then
calling the `serialize` method, except this time with a real Hash
object. Keeping things DRY!

Fixes #26904.

@maclover7 maclover7 merged commit bce3d1f into rails:master Nov 15, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maclover7 maclover7 deleted the maclover7:jm-fix-26904 branch Nov 15, 2016

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Nov 15, 2016

Member

Backport PR: #27061

Member

maclover7 commented Nov 15, 2016

Backport PR: #27061

@@ -24,6 +24,8 @@ def deserialize(value)
def serialize(value)
if value.is_a?(::Hash)
value.map { |k, v| "#{escape_hstore(k)}=>#{escape_hstore(v)}" }.join(", ")
elsif value.respond_to?(:to_unsafe_h)

This comment has been minimized.

@kirs

kirs Nov 21, 2016

Member

Why do we allow unpermitted params to be persisted?
IMO, we should only persist params that have been permitted with strong parameters API.

@kirs

kirs Nov 21, 2016

Member

Why do we allow unpermitted params to be persisted?
IMO, we should only persist params that have been permitted with strong parameters API.

This comment has been minimized.

@sgrif

sgrif Nov 22, 2016

Member

Because this is fundamentally different than attribute assignment elsewhere. It's assigning a value to a single known attribute. Similarly to how we don't require users to whitelist possible values for a string when assigning to a string column. Most importantly, this is matching the behavior of 4.2

@sgrif

sgrif Nov 22, 2016

Member

Because this is fundamentally different than attribute assignment elsewhere. It's assigning a value to a single known attribute. Similarly to how we don't require users to whitelist possible values for a string when assigning to a string column. Most importantly, this is matching the behavior of 4.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment