Permalink
Browse files

we only need to support `asc` and `ASC`. No need for mixed cases. #14263



This is a result of the discussion at https://github.com/rails/rails/pull/14263/files#r10291489
  • Loading branch information...
1 parent 8d486c6 commit f6aeb8b1a3687c8523e4a56309fe3736011b2935 @senny senny committed Mar 5, 2014
Showing with 25 additions and 34 deletions.
  1. +14 −13 activerecord/lib/active_record/relation/query_methods.rb
  2. +11 −21 activerecord/test/cases/relations_test.rb
@@ -275,15 +275,6 @@ def group!(*args) # :nodoc:
# Allows to specify an order attribute:
#
- # User.order('name')
- # => SELECT "users".* FROM "users" ORDER BY name
- #
- # User.order('name DESC')
- # => SELECT "users".* FROM "users" ORDER BY name DESC
- #
- # User.order('name DESC, email')
- # => SELECT "users".* FROM "users" ORDER BY name DESC, email
- #
# User.order(:name)
# => SELECT "users".* FROM "users" ORDER BY "users"."name" ASC
#
@@ -292,6 +283,15 @@ def group!(*args) # :nodoc:
#
# User.order(:name, email: :desc)
# => SELECT "users".* FROM "users" ORDER BY "users"."name" ASC, "users"."email" DESC
+ #
+ # User.order('name')
+ # => SELECT "users".* FROM "users" ORDER BY name
+ #
+ # User.order('name DESC')
+ # => SELECT "users".* FROM "users" ORDER BY name DESC
+ #
+ # User.order('name DESC, email')
+ # => SELECT "users".* FROM "users" ORDER BY name DESC, email
def order(*args)
check_if_method_has_arguments!(:order, args)
spawn.order!(*args)
@@ -1030,13 +1030,14 @@ def build_order(arel)
arel.order(*orders) unless orders.empty?
end
- VALID_DIRECTIONS = [:asc, :desc, 'asc', 'desc'] # :nodoc:
+ VALID_DIRECTIONS = [:asc, :desc, :ASC, :DESC,
+ 'asc', 'desc', 'ASC', 'DESC'] # :nodoc:
def validate_order_args(args)
args.grep(Hash) do |h|
@bogdan

bogdan Mar 5, 2014

Contributor

One more intermediate Array here.
While Aaron is putting hard work to make it adequate ... we are keeping our inadequate staff in master.

args.each do |arg|
  next if arg.is_a?(Hash)
  ....
end
@senny

senny Mar 5, 2014

Member

probably next unless ... otherwise that change is pretty inadequate.

@bogdan

bogdan Mar 5, 2014

Contributor

y, sure.

- h.values.map(&:downcase).each do |value|
- raise ArgumentError, "Direction '#{value}' is invalid. Valid " \
- "directions are asc and desc." unless VALID_DIRECTIONS.include?(value)
+ h.values.each do |value|
@bogdan

bogdan Mar 5, 2014

Contributor

!!

h.each do |_, value|
  raise ... unless VALID_DIRECTIONS.incude?(value)
end
@senny

senny Mar 5, 2014

Member

😓 good catch thanks.

@carlosantoniodasilva

carlosantoniodasilva Mar 5, 2014

Owner

Could also use each_value.

+ raise ArgumentError, "Direction \"#{value}\" is invalid. Valid " \
+ "directions are: #{VALID_DIRECTIONS.inspect}" unless VALID_DIRECTIONS.include?(value)
end
end
end
@@ -205,31 +205,21 @@ def test_finding_with_asc_order_with_string
assert_equal [topics(:first), topics(:second), topics(:third), topics(:fourth), topics(:fifth)], topics.to_a
end
- def test_nothing_raises_on_upcase_desc_arg
- Topic.order(id: "DESC")
- end
-
- def test_nothing_raises_on_downcase_desc_arg
- Topic.order(id: "desc")
- end
-
- def test_nothing_raises_on_upcase_asc_arg
- Topic.order(id: "ASC")
- end
-
- def test_nothing_raises_on_downcase_asc_arg
- Topic.order(id: "asc")
- end
+ def test_support_upper_and_lower_case_directions
+ assert_includes Topic.order(id: "ASC").to_sql, "ASC"
+ assert_includes Topic.order(id: "asc").to_sql, "ASC"
+ assert_includes Topic.order(id: :ASC).to_sql, "ASC"
+ assert_includes Topic.order(id: :asc).to_sql, "ASC"
- def test_nothing_raises_on_case_insensitive_args
- Topic.order(id: "DeSc")
- Topic.order(id: :DeSc)
- Topic.order(id: "aSc")
- Topic.order(id: :aSc)
+ assert_includes Topic.order(id: "DESC").to_sql, "DESC"
+ assert_includes Topic.order(id: "desc").to_sql, "DESC"
+ assert_includes Topic.order(id: :DESC).to_sql, "DESC"
+ assert_includes Topic.order(id: :desc).to_sql,"DESC"
end
def test_raising_exception_on_invalid_hash_params
- assert_raise(ArgumentError) { Topic.order(:name, "id DESC", id: :asfsdf) }
+ e = assert_raise(ArgumentError) { Topic.order(:name, "id DESC", id: :asfsdf) }
+ assert_equal 'Direction "asfsdf" is invalid. Valid directions are: [:asc, :desc, :ASC, :DESC, "asc", "desc", "ASC", "DESC"]', e.message
end
def test_finding_last_with_arel_order

1 comment on commit f6aeb8b

Member

senny commented on f6aeb8b Mar 5, 2014

Please sign in to comment.