Adding unscope to WhereChain #9200

Merged
merged 1 commit into from Mar 4, 2013

Conversation

Projects
None yet
4 participants
Contributor

wangjohn commented Feb 6, 2013

This pull request adds functionality so that you can unscope something in a where chain. For instance, if you are passed in a WhereChain as follows: Posts.select(:name => "John").order('id DESC') but you want to get rid of order, then this feature allows you to do:

Posts.select(:name => "John").order("id DESC").unscope(:order) == Posts.select(:name => "John")

This is implemented for all methods in the ActiveRecord query interface such as:

where
select
group
order
reorder
reverse_order
limit
offset
joins
includes
lock
readonly
from
having

Currently, joins, includes, lock, readonly, from, and having have not yet been tested.

activerecord/lib/active_record/relation/query_methods.rb
+ when Hash
+ scope.each do |key, target_value|
+ target_value = [target_value] if !target_value.kind_of?(Array)
+ target_value.each do |val|
@jeremy

jeremy Feb 7, 2013

Owner

Array(target_value).each ...

activerecord/lib/active_record/relation/query_methods.rb
+ when Symbol, String
+ symbol_unscoping(scope.to_sym)
+ when Hash
+ scope.each do |key, target_value|
@jeremy

jeremy Feb 7, 2013

Owner

Should raise ArgumentError if key is not :where

activerecord/lib/active_record/relation/query_methods.rb
+ args.each do |scope|
+ case scope
+ when Symbol, String
+ symbol_unscoping(scope.to_sym)
@jeremy

jeremy Feb 7, 2013

Owner

Think symbol args are sufficient. No need for strings.

activerecord/lib/active_record/relation/query_methods.rb
+ end
+ end
+ else
+ raise ArgumentError, "Unknown argument type for unscoping."
@jeremy

jeremy Feb 7, 2013

Owner

Maybe something more verbose, like
raise ArgumentError, "Unrecognized scoping: #{args.inspect}. Use .unscope(where: :attribute_name) or .unscope(:order), for example."

activerecord/lib/active_record/relation/query_methods.rb
+ def symbol_unscoping(scope)
+ single_val_method = Relation::SINGLE_VALUE_METHODS.include?(scope)
+ end_method = (single_val_method ? "value" : "values")
+ unscope_code = "#{scope}_#{end_method}="
@jeremy

jeremy Feb 7, 2013

Owner
unscope_code = :"#{scope}_value#{'s' unless single_val_method}="
activerecord/lib/active_record/relation/query_methods.rb
+ result = !reverse_order_value
+ when :reorder, :reordering
+ unscope_code = "order_values="
+ result = []
@jeremy

jeremy Feb 7, 2013

Owner

These are kinda funky. If I unscope(:order), I'd expect a previous reorder(:foo) to be unscoped as well.

activerecord/lib/active_record/relation/query_methods.rb
+ unscope_code = "order_values="
+ result = []
+ else
+ result = (single_val_method ? nil : [])
@jeremy

jeremy Feb 7, 2013

Owner
result = [] unless single_val_method
activerecord/lib/active_record/relation/query_methods.rb
+ when :reverse_order
+ result = !reverse_order_value
+ when :reorder, :reordering
+ unscope_code = "order_values="
@jeremy

jeremy Feb 7, 2013

Owner

:order_values=

activerecord/lib/active_record/relation/query_methods.rb
+ result = (single_val_method ? nil : [])
+ end
+
+ self.send(unscope_code.to_sym, result)
@jeremy

jeremy Feb 7, 2013

Owner

No need to to_sym -

send unscope_code, result
activerecord/lib/active_record/relation/query_methods.rb
+ self.send(unscope_code.to_sym, result)
+ end
+
+ UNSCOPING_OPERATORS = "(IS)(=)(!=)(IS NOT)(<)(>)(<=)(>=)"
@jeremy

jeremy Feb 7, 2013

Owner

Can build the regexp out here. Be sure to use /i modifier for case insensitivity:

UNSCOPING_OPERATORS = ['IS', '=', '!=', 'IS NOT', '<', '>', '<=', '>=']
MATCH_UNSCOPING_OPERATOR = /(#{Regexp.union(UNSCOPING_OPERATORS.map { |op| Regexp.escape op })})/i
activerecord/lib/active_record/relation/query_methods.rb
+ when Arel::Nodes::In, Arel::Nodes::Equality
+ (rel.left.name.to_sym == target_value_sym || rel.right.name.to_sym == target_value_sym)
+ when String
+ rel =~ /#{target_value_sym}\w*[#{UNSCOPING_OPERATORS}]/
@jeremy

jeremy Feb 7, 2013

Owner

Looks like \w* should be matching whitespace (?)

rel =~ /#{target_value_sym}\s*#{MATCH_UNSCOPING_OPERATOR}/i
Owner

jeremy commented Feb 7, 2013

Looking good John. Nice work!

Think order/reorder/reverse_order should be unscoped all at once with unscope(:order).

Also, not sure whether we should try hard to unscope already-generated SQL. Seems brittle.

Contributor

wangjohn commented Feb 7, 2013

Thanks for all the comments, I've implemented what you've said. I also added more tests for other methods from the query interface.

+ result = [] unless single_val_method
+ end
+
+ self.send(unscope_code, result)
@jeremy

jeremy Feb 7, 2013

Owner

Consider what happens when you call foo.unscope(:junkargument). We'll call self.junkargument_values = [] which will raise a NoMethodError. Perhaps we should whitelist the allowed single/multi value methods and raise ArgumentError if the scoping is not recognized.

activerecord/lib/active_record/relation/query_methods.rb
+ result = false
+ when :reorder, :reordering
+ unscope_code = :order_values=
+ result = []
@jeremy

jeremy Feb 7, 2013

Owner

I think we shouldn't support unscope(:reverse_order) or unscope(:reordering). They're modifiers of the order scoping. Makes sense to unscope(:order) instead, and have that set self.order_values = [] and self.reverse_order = false.

activerecord/lib/active_record/relation/query_methods.rb
+ subrelation = (rel.left.kind_of?(Arel::Attributes::Attribute) ? rel.left : rel.right)
+ subrelation.name.to_sym == target_value_sym
+ else
+ raise "Where clause unscoping for type #{rel.class} is unimplemented."
@jeremy

jeremy Feb 7, 2013

Owner

What other Arel nodes do you think we should support?

Would help to see the where value that was unscoped in the exception message as well:

raise "unscope(where: #{target_value.inspect}) failed: unscoping #{rel.class} is unimplemented"
+ received = Developer.includes(:projects).select(:id).unscope(:includes, :select).collect { |dev| dev.name }
+ assert_equal expected, received
+ end
+
@jeremy

jeremy Feb 7, 2013

Owner

Nice test coverage! Looks like we need coverage for the exceptional cases as well: non-:where hash key, non-symbol/hash argument, unrecognized scoping, and unsupported Arel unscoping.

Contributor

al2o3cr commented Feb 7, 2013

Would be useful to have some documentation about when this is appropriate vs. ActiveRecord::SpawnMethods#except, which otherwise seems very similar.

Owner

jeremy commented Feb 7, 2013

Good point @al2o3cr. Problem with #except is that it affects that relation's values only. It won't wipe out the order, grouping, etc when the relation is merged. So post.comments.except(:order) will still have an order if it comes from a default_scope on Comment. Example:

>> t.comments.except(:order).to_sql
=> "SELECT `comments`.* FROM `comments`  WHERE `comments`.`trashed` = 0 AND `comments`.`commentable_id` = 223304243 AND `comments`.`commentable_type` = 'Todo' ORDER BY created_at"

Perhaps #except should take on this larger role, but it'd need deeper consideration.

Contributor

wangjohn commented Feb 8, 2013

I've included the comments that were made. I've also added documentation and more extensive test coverage.

activerecord/lib/active_record/relation/query_methods.rb
+ #
+ # will still have an order if it comes from the default_scope on Comment.
+ def unscope(*args)
+ args.blank? ? self : spawn.unscope!(*args)
@jeremy

jeremy Feb 11, 2013

Owner

Is post.comments.unscope with no args meaningful?

Contributor

wangjohn commented Feb 11, 2013

Now unscope() called without any arguments will raise an ArgumentError. I've added a test to make sure that it does.

Owner

jeremy commented Feb 11, 2013

Cool. Last thing: update CHANGELOG.md to introduce the new feature. And, if you're feeling ambitious, update the active_record_querying guide 😁

Contributor

wangjohn commented Feb 11, 2013

Cool, added a changelog and active_record_querying guide entry.

Contributor

wangjohn commented Feb 20, 2013

I've updated this to use the has_arguments? method to check to make sure that there are actually arguments to .unscope().

Created an unscope method for removing relations from a chain of
relations. Specific where values can be unscoped, and the unscope method
still works when relations are merged or combined.

jeremy added a commit that referenced this pull request Mar 4, 2013

@jeremy jeremy merged commit 1909808 into rails:master Mar 4, 2013

Contributor

henrik commented on 2938754 Mar 4, 2013

The guide isn't clear about why you'd ever want except when you have unscoped. Other than the seemingly unlikely case where you only want to remove a condition if it's not from a merge. Could except be deprecated/removed? Or unscope replace except, to keep the except/only pairing?

Contributor

wangjohn replied Mar 4, 2013

@henrik I was talking to @jeremy about this, and I think we might want to deprecate except at some point. I believe that the unscope method does a superset of the functions that except performed.

Contributor

henrik replied Mar 5, 2013

@wangjohn Sounds good. Thanks!

Owner

jeremy replied Mar 5, 2013

Other than the seemingly unlikely case where you only want to remove a condition if it's not from a merge

@henrik check out how #except is used internally. Agreed that it'll probably end up deprecated as public API.

@wangjohn wangjohn deleted the wangjohn:unscoping_activerecord_merging branch Mar 5, 2013

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