update_columns doesn't work with strong parameters

[bendilley]

Steps to reproduce

record.update_columns ActionController::Parameters.new(approval_state: 'denied').permit(:approval_state)

Expected behavior

I would expect update_columns to accept attributes from an instance of ActionController::Parameters in the same way as update_attributes does.

Actual behavior

NoMethodError: undefined method `each_key' for #<ActionController::Parameters:0x00007f9eb9827a80>
Did you mean?  each_pair
from ~/.rbenv/versions/2.6.4/lib/ruby/gems/2.6.0/gems/activerecord-5.2.4.2/lib/active_record/persistence.rb:472:in `update_columns'

System configuration

Rails version: 5.2.4.2

Ruby version: 2.6.4

[santib]

I get your point, on the other hand if we add that, we'd be coupling ActiveRecord to ActionController which is something that currently doesn't happen.

So, I'd say to not make this change, and maybe clarify in the documentation that you should do .to_h to the strong parameters before calling update_columns. Thoughts?

[eugeneius]

This works since #33758, where each_key was added to ActionController::Parameters.

[santib]

@eugeneius I don't think it's working.. I just tried it in a fresh Rails 6 app:

irb(main):005:0> post.update_columns(ActionController::Parameters.new(title: 'b').permit(:title))
Traceback (most recent call last):
        1: from (irb):5
NoMethodError (undefined method `map' for <ActionController::Parameters {"title"=>"b"} permitted: true>:ActionController::Parameters)
Did you mean?  tap

Should we add map (or Enumerable?) to ActionController::Parameters too?

[eugeneius]

Whoops! I tested this in a real application, where it's only working because of a monkey patch 😅

Rather than adding more methods to ActionController::Parameters, I think the right solution here is to pass the argument through sanitize_for_mass_assignment before using it.

[rafaelfranca]

There is a reason for update_columns not call sanitize_for_mass_assignment. It is considered low level API and it has less features than update. Not sure if we should add mass assignment check to it. It should not receive unsafe inputs from end-users.

If developers want to pass ActionController::Parameters to it anyway, I don't see why they can't do record.update_columns ActionController::Parameters.new(approval_state: 'denied').permit(:approval_state).to_h.

[santib]

@rafaelfranca I understand that we can manually call .to_h and I'm fine with that. But just curious, would something like #39189 be ok? Or do you think it can be harmful in any situation?

[rafaelfranca]

I think @kaspth already answered that. In my opinion it is more intention revealing that you call to_h when mass assignment protection is not supported.

But I think there is something to be changed here. Do we need to skip sanitize_for_mass_assignment in update_columns? Would it passing a permitted ActionController::Parameters to update_columns be more dangerous than passing to update?

I can't think in any case where you would end up in trouble if you pass a permitted ActionController::Parameters to update_columns that you wouldn't have if you pass to update_attributes so I think it is fine to add mass assignment check there.

I'd not add support to this in exec_query and other lower level APIs, but I think in update_columns is fine. Can you open a PR doing that? If possible I'd like to see what is the impact on performance when you don't pass a ActionController::Parameters instance with this new patch.

[santib]

@rafaelfranca I just opened a PR. Let me know if you'd like to benchmark it in some other way (multiple columns, or something else).

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-0-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.