Skip to content

Commit

Permalink
Fix mass-assignment sanitization breaking in ActiveRecord 3.0.0 beta5+ [
Browse files Browse the repository at this point in the history 
#55 state:resolved]
  • Loading branch information
@amatsuda @obrie
amatsuda authored and obrie committed Jul 21, 2010
1 parent 2404b88 commit 1e5e04b
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 3 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.rdoc
View file
@@ -1,5 +1,7 @@
== master

* Fix mass-assignment sanitization breaking in ActiveRecord 3.0.0 beta5+ [Akira Matsuda]

== 0.9.3 / 2010-06-26

* Allow access to human state / event names in transitions and for the current state
Expand Down
10 changes: 7 additions & 3 deletions lib/state_machine/integrations/active_record.rb
View file
Expand Up @@ -415,12 +415,16 @@ def attributes=(new_attributes, *args)
if new_record? && !@initialized_state_machines
@initialized_state_machines = true
if new_attributes
ignore = if new_attributes
attributes = new_attributes.dup
attributes.stringify_keys!
ignore = remove_attributes_protected_from_mass_assignment(attributes).keys
if ::ActiveRecord::VERSION::MAJOR >= 3
sanitize_for_mass_assignment(attributes).keys
else
remove_attributes_protected_from_mass_assignment(attributes).keys
end
else
ignore = []
[]
end
initialize_state_machines(:dynamic => false, :ignore => ignore)
Expand Down

12 comments on commit 1e5e04b

@rolftimmermans
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that Rails 3 RC is out, it would be awesome to have this fix released!

@nicolasblanco
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 :)

@rymai
Copy link

@rymai rymai commented on 1e5e04b Jul 27, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! :)

@nobu
Copy link

@nobu nobu commented on 1e5e04b Jul 27, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not
if defined?(sanitize_for_mass_assignment)
?

@amatsuda
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nobu

I originally did so.
http://github.com/amatsuda/state_machine/commit/649de25471a2f28f3567b7e6154185237d8f8a27

@obrie
Copy link
Member

@obrie obrie commented on 1e5e04b Jul 28, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I'll have a new release out soon enough :)

With regard to using feature-checking vs. version-checking, I think there are fair arguments for both ways. I've decided to use version-checking in this instance for a few reasons:

  1. It's consistent with how things are handled in the rest of the code (including other integrations)
  2. It works even when you need to deal with changes to the method's signature (number / types of parameters)
  3. It avoids possible issues where other libraries may end up re-defining the method with slightly different semantics

I understand arguments against this, but for now, I think this is the solution worth moving forward with.

@lexrupy
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you let a rails 2.x compatible branch and drop compatibility in master branch (like devise do)

@obrie
Copy link
Member

@obrie obrie commented on 1e5e04b Jul 28, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work well here given that there's support for several versions of several libraries. The only alternative I would consider is making every integration a separate library (e.g. state_machine-activerecord, state_machine-datamapper, etc.). This would allow the ability to manage different branches / versions for that particular ORM.

However, there's a particular amount of complexity involved in this that I'm not sure is worth it. Instead, I think we could simply see state_machine drop support for certain versions of libraries, but that's won't be happening anytime soon.

If this is something other folks would like to debate further, I'd encourage you to discuss it on the mailing list rather than here.

@obrie
Copy link
Member

@obrie obrie commented on 1e5e04b Jul 28, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case anyone doesn't know, the mailing lists are here:

http://groups.google.com/group/pluginaweek-talk
http://groups.google.com/group/pluginaweek-core

@keeran
Copy link

@keeran keeran commented on 1e5e04b Jul 28, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix, is release imminent or shall I bundle this ref?

@amatsuda
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Orbie
I realized you're deeply considering the "feature-checking vs. version-checking problem". Thanks for sharing your thought about that.
Unless Nobu objects your opinion, I'll respect your decision for that since it seems rational to me.
(For any one of you not noticing who Nobu is, he is always our guide, he knows everything about Ruby, and he (and another famous guy) is "THE RUBY" http://dame.dyndns.org/misc/ruby-commit-ranking/ranking.png )

@obrie
Copy link
Member

@obrie obrie commented on 1e5e04b Jul 29, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keeran I plan to release within the next week. I have other issues with other integrations that need to get resolved first... and I'm a bit busy with other things right now :)

For what it's worth, I've also been considering a cleaner way of handling various library versions within state_machine. However, this refactor will not happen for another week or two. Stay tuned!

Please sign in to comment.