Skip to content

Fix Issue board when using Ruby 2.4

username-removed-128633 requested to merge 35769-fix-ruby-2-4-compatibility into master

What does this MR do?

As I found in the issue (https://gitlab.com/gitlab-org/gitlab-ce/issues/35769#note_36342006), and explained in the commit message

    # In Ruby 2.4, Hash#compact exists and returns a Hash, while in Ruby 2.3,
    # Hash#compact is implemented by Rails and returns a new
    # `ActionController::Parameters` instance in this case.
    # By using Hash#compact! we're safe since we're modifying the hash in place.

Using Hash#compact! on a ActionController::Parameters breaks things in a subtle manner. I've checked and it seems that was the only place where we used compact on a ActionController::Parameters hash.

Ruby 2.3 / Rails 4.2.8

[1] pry(main)> params = ActionController::Parameters.new(foo: 'bar')
=> {"foo"=>"bar"}
[2] pry(main)> params.class
=> ActionController::Parameters
[3] pry(main)> params.compact.class
=> ActionController::Parameters
[4] pry(main)> params.compact!
=> nil
[5] pry(main)> params.class
=> ActionController::Parameters

Ruby 2.4 / Rails 4.2.8

[1] pry(main)> params = ActionController::Parameters.new(foo: 'bar')
=> {"foo"=>"bar"}
[2] pry(main)> params.class
=> ActionController::Parameters
[3] pry(main)> params.compact.class
=> Hash
[4] pry(main)> params.compact!
=> nil
[5] pry(main)> params.class
=> ActionController::Parameters

Ruby 2.3 / Rails 5.0.5

>> params = ActionController::Parameters.new(foo: 'bar')
=> <ActionController::Parameters {"foo"=>"bar"} permitted: >
>> params.class
=> ActionController::Parameters
>> params.compact.class
DEPRECATION WARNING: Method compact is deprecated and will be removed in Rails 5.1, as `ActionController::Parameters` no longer inherits from hash. Using this deprecated behavior exposes potential security problems. If you continue to use this method you may be creating a security vulnerability in your app that can be exploited. Instead, consider using one of these documented methods which are not deprecated: http://api.rubyonrails.org/v5.0.5/classes/ActionController/Parameters.html (called from irb_binding at (irb):3)
=> ActiveSupport::HashWithIndifferentAccess

Ruby 2.4 / Rails 5.0.5

>> params = ActionController::Parameters.new(foo: 'bar')
=> <ActionController::Parameters {"foo"=>"bar"} permitted: >
>> params.class
=> ActionController::Parameters
>> params.compact.class
DEPRECATION WARNING: Method compact is deprecated and will be removed in Rails 5.1, as `ActionController::Parameters` no longer inherits from hash. Using this deprecated behavior exposes potential security problems. If you continue to use this method you may be creating a security vulnerability in your app that can be exploited. Instead, consider using one of these documented methods which are not deprecated: http://api.rubyonrails.org/v5.0.5/classes/ActionController/Parameters.html (called from irb_binding at (irb):3)
=> ActiveSupport::HashWithIndifferentAccess

Are there points in the code the reviewer needs to double check?

It seems that this has been solved in Rails 4.2.9: https://github.com/rails/rails/blob/v4.2.9/activesupport/lib/active_support/hash_with_indifferent_access.rb#L253-L255 (https://github.com/rails/rails/pull/27392).

So, upgrading to Rails 4.2.9 would also solve this issue without having to change our code.

In Rails 5.0.5, ActionController::Parameters#compact and ActionController::Parameters#compact! are called from method_missing directly on the underlying @parameters which is a ActiveSupport::HashWithIndifferentAccess while in Rails 4.2.9, ActionController::Parameters is inheriting from ActiveSupport::HashWithIndifferentAccess.

In Rails 5.0.5, ActionController::Parameters#compact and ActionController::Parameters#compact! are deprecated:

DEPRECATION WARNING: Method compact is deprecated and will be removed in Rails 5.1, as `ActionController::Parameters` no longer inherits from hash. Using this deprecated behavior exposes potential security problems. If you continue to use this method you may be creating a security vulnerability in your app that can be exploited. Instead, consider using one of these documented methods which are not deprecated: http://api.rubyonrails.org/v5.0.5/classes/ActionController/Parameters.html (called from irb_binding at (irb):3)

So we'll have to solve this with another solution in the future...

Why was this MR needed?

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #35769 (closed)

Edited by username-removed-128633

Merge request reports