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

Raising an error when nil is passed to update_attributes. #9860

Merged

Conversation

@wangjohn
Copy link
Contributor

wangjohn commented Mar 21, 2013

In 9e4b715, @josevalim allowed the ability to send a nil argument to update_attributes (in response to #478). However, the reasoning behind that was because a pretty cryptic error was thrown. The current behavior in Rails is that thing.update_attributes(nil) will return and not update any attributes without throwing any error.

However, Instead of allowing you to send a nil argument, it might be better to raise an ArgumentError which has a clear error message. I've made that change here, but this PR is more intended as a discussion on which behavior is better.

Closes #9858.

@Intrepidd
Intrepidd reviewed Mar 21, 2013
View changes
activerecord/lib/active_record/attribute_assignment.rb Outdated
return if new_attributes.blank?
if !new_attributes
raise ArgumentError, "Must pass a non-nil hash as an argument when assigning attributes."
elsif new_attributes.empty?

This comment has been minimized.

Copy link
@Intrepidd

Intrepidd Mar 21, 2013

Contributor

Is this really necessary ? The behaviour would be the same without the elsif clause.

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 21, 2013

Member

It wouldn't: see the early return

This comment has been minimized.

Copy link
@mhuggins

mhuggins Mar 21, 2013

He's saying the behavior would be the same because returning vs. not returning has the same result when new_attributes is empty.

This comment has been minimized.

Copy link
@Intrepidd

Intrepidd Mar 21, 2013

Contributor

Exactly @mhuggins, the attributes won't be changed wether or not you return early.

This comment has been minimized.

Copy link
@Intrepidd

Intrepidd Mar 21, 2013

Contributor

Just a personnal point of view, but something like

raise ArgumentError, "Must pass a non-nil hash as an argument when assigning attributes." if new_attributes.nil?

is better looking to me than the if / elsif branch

This comment has been minimized.

Copy link
@wangjohn

wangjohn Mar 22, 2013

Author Contributor

@Intrepidd @mhuggins It actually does change the behavior. This is because if an empty string is passed asnew_attributes then the call to stringify_keys will raise a NoMethodError.

See the following test: https://github.com/rails/rails/blob/master/activerecord/test/cases/attribute_methods_test.rb#L85-88

This comment has been minimized.

Copy link
@mhuggins

mhuggins Mar 22, 2013

Why would someone pass an empty string to this method? The documentation makes it clear that a hash is expected, and I can't think of a scenario where someone would even consider passing a string. The nil check I can understand if a controller param isn't passed by the user.

This comment has been minimized.

Copy link
@wangjohn

wangjohn Mar 22, 2013

Author Contributor

That's true. I think this test is outdated. It was originally added here: 786342e. Which brings me to a second discussion, should we be raising an ArgumentError when the input is not a hash?

It seems like this second check will ensure that all arguments are correct and valid, and that stringify_keys will never throw a NoMethodError if we are sure that a new_attributes is a hash.

This comment has been minimized.

Copy link
@Intrepidd

Intrepidd Mar 22, 2013

Contributor

Since the documentation clearly states that the parameter must be a hash, I
don't think a check should be done on the type of the parameter.

Thousands of methods in rails expect hashes without raising an exception if
the parameter is not a hash.

I even think that throwing a NoMethodError when nil is passed might be a
good thing.

Adrien Siami
On Mar 22, 2013 12:33 AM, "John J. Wang" notifications@github.com wrote:

In activerecord/lib/active_record/attribute_assignment.rb:

@@ -12,7 +12,11 @@ module AttributeAssignment
# of this method is +false+ an ActiveModel::ForbiddenAttributesError
# exception is raised.
def assign_attributes(new_attributes)

  •  return if new_attributes.blank?
    
  •  if !new_attributes
    
  •    raise ArgumentError, "Must pass a non-nil hash as an argument when assigning attributes."
    
  •  elsif new_attributes.empty?
    

That's true. I think this test is outdated. It was originally added here:
786342ehttps://github.com/rails/rails/commit/786342e17f42799ef889cf6127fe97e9598272e0.
Which brings me to a second discussion, should we be raising an
ArgumentError when the input is not a hash?

It seems like this second check will ensure that all arguments are correct
and valid, and that stringify_keys will never throw a NoMethodError if we
are sure that a new_attributes is a hash.


Reply to this email directly or view it on GitHubhttps://github.com//pull/9860/files#r3482868
.

@Intrepidd
Copy link
Contributor

Intrepidd commented Mar 22, 2013

What if the users wants to pass a custom object that responds to stringify_keys ?

I'm totally for the error on nil because it is common and this allows developers to know what the problem is real quick.

However, I'm not fond of the racism concerning the type of the parameter ;)

As I said in the outdated diff, everywhere in rails, hashes are expected without being type checked, that's the beauty of ruby, if you want strong type check, ruby is not the good solution I think :)

What do you think of :

  • Only check for nil
  • Check for something that resond_to?(:stringify_keys)

Please let me know what you think of this, I'm not an expert so feel free to point out things that I may have missed.

@wangjohn
Copy link
Contributor Author

wangjohn commented Mar 23, 2013

@Intrepidd That's a pretty reasonable idea. I think checking for respond_to?(:stringify_keys) is a happy medium between checking explicitly for hashes and not checking the input arguments at all.

The PR has been updated.

@Intrepidd
Copy link
Contributor

Intrepidd commented Mar 23, 2013

Thanks! Let's see what the core team thinks about it!

Adrien Siami
On Mar 23, 2013 3:52 AM, "John J. Wang" notifications@github.com wrote:

@Intrepidd https://github.com/Intrepidd That's a pretty reasonable
idea. I think checking for respond_to?(:stringify_keys) is a happy medium
between checking explicitly for hashes and not checking the input arguments
at all.

The PR has been updated.


Reply to this email directly or view it on GitHubhttps://github.com//pull/9860#issuecomment-15331257
.

@senny
senny reviewed Apr 12, 2013
View changes
activerecord/lib/active_record/attribute_assignment.rb Outdated
@@ -12,7 +12,9 @@ module AttributeAssignment
# of this method is +false+ an <tt>ActiveModel::ForbiddenAttributesError</tt>
# exception is raised.
def assign_attributes(new_attributes)
return if new_attributes.blank?
unless new_attributes && new_attributes.respond_to?(:stringify_keys)

This comment has been minimized.

Copy link
@senny

senny Apr 12, 2013

Member

I always get confused when I have to read an unless with more than one condition. I'd like to write them using if.

That might only be my taste though.

/cc @carlosantoniodasilva

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Apr 12, 2013

Member

I think that only checking for respond_to would be enough? We don't expect nils to respond to stringify_keys I guess?

This comment has been minimized.

Copy link
@wangjohn

wangjohn Apr 12, 2013

Author Contributor

Ya, I just noticed that after I implemented @senny's change to an if statement.

@senny
Copy link
Member

senny commented Apr 12, 2013

this looks good to me.

/cc @rafaelfranca

@senny
Copy link
Member

senny commented Apr 12, 2013

As this changes the functionality I think we should add a CHANGELOG entry.

@carlosantoniodasilva
carlosantoniodasilva reviewed Apr 12, 2013
View changes
activerecord/lib/active_record/attribute_assignment.rb Outdated
@@ -12,7 +12,9 @@ module AttributeAssignment
# of this method is +false+ an <tt>ActiveModel::ForbiddenAttributesError</tt>
# exception is raised.
def assign_attributes(new_attributes)
return if new_attributes.blank?
unless new_attributes && new_attributes.respond_to?(:stringify_keys)
raise ArgumentError, "Must pass a non-nil hash as an argument when assigning attributes."

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Apr 12, 2013

Member

What's a non-nil hash? :)

@wangjohn
Copy link
Contributor Author

wangjohn commented Apr 12, 2013

@senny Added a changelog entry and rebased. @carlosantoniodasilva thanks for the comments!

@senny
Copy link
Member

senny commented May 28, 2013

@wangjohn this does no longer apply cleanly, can you push a rebased version?

@wangjohn
Copy link
Contributor Author

wangjohn commented May 28, 2013

@senny I've just rebased and pushed.

@senny
Copy link
Member

senny commented May 29, 2013

@rafaelfranca @carlosantoniodasilva can you take another look?

@wangjohn
Copy link
Contributor Author

wangjohn commented Jun 15, 2013

Any updates on this PR? If not, I'll go ahead and close it.

@senny
Copy link
Member

senny commented Jun 17, 2013

let's wait for more feedback. I like this change but I'm not sure if there are implications because it changes the public API.

@neerajdotname
Copy link
Member

neerajdotname commented Jun 17, 2013

I feel conflicted about this one. If user is passing nil then obviously user does not want anything to happen. After this code now user has to do sent_arguments || {} so that code does not raise exception. I think it is better if the code does not raise exception on nil.

@senny
Copy link
Member

senny commented Jun 17, 2013

@neerajdotname but for the default case with strong parameters you already get {}, don't you?

@neerajdotname
Copy link
Member

neerajdotname commented Jun 17, 2013

@senny you might but ActiveRecord can be used without rest of rails goodies.

Updated: replaced with by without.

@senny
Copy link
Member

senny commented Jun 17, 2013

@neerajdotname sure, I guess it's a matter of taste. It's important that the default case does not get overcomplicated. That's why I mentioned strong params.

Maybe we can get a word from @rafaelfranca and @jonleighton to wrap this PR up.

@carlosantoniodasilva
carlosantoniodasilva reviewed Jun 26, 2013
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,13 @@
* Calling ``update_attributes`` will now throw an ArgumentError whenever it
gets a nil argument. More specifically, it will throw an error if the
argument that it gets passed does not respond to to ``stringify_keys``.

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Jun 26, 2013

Member

I think you can use single backticks in the description here.

@carlosantoniodasilva
carlosantoniodasilva reviewed Jun 26, 2013
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,13 @@
* Calling ``update_attributes`` will now throw an ArgumentError whenever it

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Jun 26, 2013

Member

ArgumentError (with backticks for better syntax)

@carlosantoniodasilva
carlosantoniodasilva reviewed Jun 26, 2013
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,13 @@
* Calling ``update_attributes`` will now throw an ArgumentError whenever it
gets a nil argument. More specifically, it will throw an error if the

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Jun 26, 2013

Member

in nil as well.

@carlosantoniodasilva
carlosantoniodasilva reviewed Jun 26, 2013
View changes
activerecord/CHANGELOG.md Outdated
gets a nil argument. More specifically, it will throw an error if the
argument that it gets passed does not respond to to ``stringify_keys``.

For example:

This comment has been minimized.

Copy link
@carlosantoniodasilva
@carlosantoniodasilva
carlosantoniodasilva reviewed Jun 26, 2013
View changes
activerecord/CHANGELOG.md Outdated

For example:

@my_comment.update_attributes() # => raises ArgumentError

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Jun 26, 2013

Member

Two more spaces to properly syntax highlight

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jun 26, 2013

Please rebase from master. Thanks.

@wangjohn
Copy link
Contributor Author

wangjohn commented Jun 26, 2013

@carlosantoniodasilva Rebased and changed the changelog.

@Bertg
Copy link
Contributor

Bertg commented Sep 3, 2013

+1 It will make debugging a lot easier for the developer as typos will be caught easily. In previous tickets the core team has stated that this is the typo of thing Rails should cater to.

The old behavior can be easily returnned by doing arguments || {} which makes it clear that the developer expects nil as an input.

@rafaelfranca
Copy link
Member

rafaelfranca commented Sep 23, 2013

I'm fine with this change on master but I'd not put in a stable release.

I'm merging.

rafaelfranca added a commit that referenced this pull request Sep 24, 2013
…_with_nil

Raising an error when nil is passed to update_attributes.

Conflicts:
	activerecord/CHANGELOG.md
@rafaelfranca rafaelfranca merged commit 926c4b9 into rails:master Sep 24, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@rubys
Copy link
Contributor

rubys commented Sep 24, 2013

Causes failures in Agile Web Development with Rails test. Minimal test case: http://intertwingly.net/tmp/update_attributes.html

@wangjohn wangjohn deleted the wangjohn:update_attributes_throws_error_with_nil branch Sep 24, 2013
@wangjohn
Copy link
Contributor Author

wangjohn commented Sep 24, 2013

@rubys The problem with the failures is that I removed the line return if new_attributes.blank?. It seems like this is actually required (in the case of the tests, we're getting an empty hash, so the assign_attributes method should just return without doing anything).

The simplest fix would be to just add this line back in. I'll make a PR to do this.

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.

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