Refactor `length`, `order`, and `opclass` index options dumping

9 files changed, 67 insertions, 33 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
index 88ef8e0819..86406d95ad 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -6,7 +6,7 @@ module ActiveRecord
     # this type are typically created and returned by methods in database
     # adapters. e.g. ActiveRecord::ConnectionAdapters::MySQL::SchemaStatements#indexes
     class IndexDefinition # :nodoc:
-      attr_reader :table, :name, :unique, :columns, :lengths, :orders, :where, :type, :using, :opclass, :comment
+      attr_reader :table, :name, :unique, :columns, :lengths, :orders, :opclasses, :where, :type, :using, :comment
 
       def initialize(
         table, name,
@@ -14,24 +14,33 @@ module ActiveRecord
         columns = [],
         lengths: {},
         orders: {},
+        opclasses: {},
         where: nil,
         type: nil,
         using: nil,
-        opclass: {},
         comment: nil
       )
         @table = table
         @name = name
         @unique = unique
         @columns = columns
-        @lengths = lengths
-        @orders = orders
+        @lengths = concise_options(lengths)
+        @orders = concise_options(orders)
+        @opclasses = concise_options(opclasses)
         @where = where
         @type = type
         @using = using
-        @opclass = opclass
         @comment = comment
       end
+
+      private
+        def concise_options(options)
+          if columns.size == options.size && options.values.uniq.size == 1
+            options.values.first
+          else
+            options
+          end
+        end
     end
 
     # Abstract representation of a column definition. Instances of this type
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
index fac9e1cca2..efc934c34c 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -1202,7 +1202,7 @@ module ActiveRecord
             when Hash
               order = order.symbolize_keys
               quoted_columns.each { |name, column| column << " #{order[name].upcase}" if order[name].present? }
-            when String
+            else
               quoted_columns.each { |name, column| column << " #{order.upcase}" if order.present? }
             end
           end
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index 143dc6d08d..41c628302d 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -611,7 +611,7 @@ module ActiveRecord
             when Hash
               length = length.symbolize_keys
               quoted_columns.each { |name, column| column << "(#{length[name]})" if length[name].present? }
-            when Integer
+            else
               quoted_columns.each { |name, column| column << "(#{length})" }
             end
           end
diff --git a/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb
index a15c7d1787..d5b2c18ee6 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb
@@ -22,23 +22,26 @@ module ActiveRecord
                   index_using = mysql_index_type
                 end
 
-                indexes << IndexDefinition.new(
+                indexes << [
                   row[:Table],
                   row[:Key_name],
                   row[:Non_unique].to_i == 0,
+                  [],
+                  lengths: {},
+                  orders: {},
                   type: index_type,
                   using: index_using,
                   comment: row[:Index_comment].presence
-                )
+                ]
               end
 
-              indexes.last.columns << row[:Column_name]
-              indexes.last.lengths.merge!(row[:Column_name] => row[:Sub_part].to_i) if row[:Sub_part]
-              indexes.last.orders.merge!(row[:Column_name] => :desc) if row[:Collation] == "D"
+              indexes.last[-2] << row[:Column_name]
+              indexes.last[-1][:lengths].merge!(row[:Column_name] => row[:Sub_part].to_i) if row[:Sub_part]
+              indexes.last[-1][:orders].merge!(row[:Column_name] => :desc) if row[:Collation] == "D"
             end
           end
 
-          indexes
+          indexes.map { |index| IndexDefinition.new(*index) }
         end
 
         def remove_column(table_name, column_name, type = nil, options = {})
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
index 64f08e0ee1..7b4d5711cb 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -109,6 +109,9 @@ module ActiveRecord
 
             using, expressions, where = inddef.scan(/ USING (\w+?) \((.+?)\)(?: WHERE (.+))?\z/).flatten
 
+            orders = {}
+            opclasses = {}
+
             if indkey.include?(0)
               columns = expressions
             else
@@ -119,21 +122,12 @@ module ActiveRecord
                 AND a.attnum IN (#{indkey.join(",")})
               SQL
 
-              orders = {}
-              opclasses = {}
-
               # add info on sort order (only desc order is explicitly specified, asc is the default)
               # and non-default opclasses
               expressions.scan(/(\w+)(?: (?!DESC)(\w+))?(?: (DESC))?/).each do |column, opclass, desc|
-                opclasses[column] = opclass if opclass
+                opclasses[column] = opclass.to_sym if opclass
                 orders[column] = :desc if desc
               end
-
-              # Use a string for the opclass description (instead of a hash) when all columns
-              # have the same opclass specified.
-              if columns.count == opclasses.count && opclasses.values.uniq.count == 1
-                opclasses = opclasses.values.first
-              end
             end
 
             IndexDefinition.new(
@@ -142,9 +136,9 @@ module ActiveRecord
               unique,
               columns,
               orders: orders,
+              opclasses: opclasses,
               where: where,
               using: using.to_sym,
-              opclass: opclasses,
               comment: comment.presence
             )
           end
diff --git a/activerecord/lib/active_record/schema_dumper.rb b/activerecord/lib/active_record/schema_dumper.rb
index 8cb2851557..16ccba6b6c 100644
--- a/activerecord/lib/active_record/schema_dumper.rb
+++ b/activerecord/lib/active_record/schema_dumper.rb
@@ -184,12 +184,12 @@ HEADER
           "name: #{index.name.inspect}",
         ]
         index_parts << "unique: true" if index.unique
-        index_parts << "length: { #{format_options(index.lengths)} }" if index.lengths.present?
-        index_parts << "order: { #{format_options(index.orders)} }" if index.orders.present?
+        index_parts << "length: #{format_index_parts(index.lengths)}" if index.lengths.present?
+        index_parts << "order: #{format_index_parts(index.orders)}" if index.orders.present?
+        index_parts << "opclass: #{format_index_parts(index.opclasses)}" if index.opclasses.present?
         index_parts << "where: #{index.where.inspect}" if index.where
         index_parts << "using: #{index.using.inspect}" if !@connection.default_index_type?(index)
         index_parts << "type: #{index.type.inspect}" if index.type
-        index_parts << "opclass: #{index.opclass.inspect}" if index.opclass.present?
         index_parts << "comment: #{index.comment.inspect}" if index.comment
         index_parts
       end
@@ -232,6 +232,14 @@ HEADER
         options.map { |key, value| "#{key}: #{value.inspect}" }.join(", ")
       end
 
+      def format_index_parts(options)
+        if options.is_a?(Hash)
+          "{ #{format_options(options)} }"
+        else
+          options.inspect
+        end
+      end
+
       def remove_prefix_and_suffix(table)
         prefix = Regexp.escape(@options[:table_name_prefix].to_s)
         suffix = Regexp.escape(@options[:table_name_suffix].to_s)
diff --git a/activerecord/test/cases/adapters/postgresql/schema_test.rb b/activerecord/test/cases/adapters/postgresql/schema_test.rb
index a6ae18a826..1126908761 100644
--- a/activerecord/test/cases/adapters/postgresql/schema_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/schema_test.rb
@@ -459,7 +459,7 @@ class SchemaTest < ActiveRecord::PostgreSQLTestCase
         assert_equal :btree, index_d.using
         assert_equal :gin,   index_e.using
 
-        assert_equal :desc,  index_d.orders[INDEX_D_COLUMN]
+        assert_equal :desc,  index_d.orders
       end
     end
 
@@ -520,7 +520,7 @@ class SchemaIndexOpclassTest < ActiveRecord::PostgreSQLTestCase
 
     output = dump_table_schema "trains"
 
-    assert_match(/opclass: "text_pattern_ops"/, output)
+    assert_match(/opclass: :text_pattern_ops/, output)
   end
 
   def test_non_default_opclass_is_dumped
@@ -528,7 +528,7 @@ class SchemaIndexOpclassTest < ActiveRecord::PostgreSQLTestCase
 
     output = dump_table_schema "trains"
 
-    assert_match(/opclass: \{"description"=>"text_pattern_ops"\}/, output)
+    assert_match(/opclass: \{ description: :text_pattern_ops \}/, output)
   end
 end
 
diff --git a/activerecord/test/cases/schema_dumper_test.rb b/activerecord/test/cases/schema_dumper_test.rb
index ac5092f1c1..a612ce9bb2 100644
--- a/activerecord/test/cases/schema_dumper_test.rb
+++ b/activerecord/test/cases/schema_dumper_test.rb
@@ -177,14 +177,14 @@ class SchemaDumperTest < ActiveRecord::TestCase
 
   def test_schema_dumps_index_columns_in_right_order
     index_definition = dump_table_schema("companies").split(/\n/).grep(/t\.index.*company_index/).first.strip
-    if current_adapter?(:PostgreSQLAdapter, :SQLite3Adapter)
-      assert_equal 't.index ["firm_id", "type", "rating"], name: "company_index", order: { rating: :desc }', index_definition
-    elsif current_adapter?(:Mysql2Adapter)
+    if current_adapter?(:Mysql2Adapter)
       if ActiveRecord::Base.connection.supports_index_sort_order?
         assert_equal 't.index ["firm_id", "type", "rating"], name: "company_index", length: { type: 10 }, order: { rating: :desc }', index_definition
       else
         assert_equal 't.index ["firm_id", "type", "rating"], name: "company_index", length: { type: 10 }', index_definition
       end
+    elsif ActiveRecord::Base.connection.supports_index_sort_order?
+      assert_equal 't.index ["firm_id", "type", "rating"], name: "company_index", order: { rating: :desc }', index_definition
     else
       assert_equal 't.index ["firm_id", "type", "rating"], name: "company_index"', index_definition
     end
@@ -199,6 +199,24 @@ class SchemaDumperTest < ActiveRecord::TestCase
     end
   end
 
+  def test_schema_dumps_index_sort_order
+    index_definition = dump_table_schema("companies").split(/\n/).grep(/t\.index.*_name_and_rating/).first.strip
+    if ActiveRecord::Base.connection.supports_index_sort_order?
+      assert_equal 't.index ["name", "rating"], name: "index_companies_on_name_and_rating", order: :desc', index_definition
+    else
+      assert_equal 't.index ["name", "rating"], name: "index_companies_on_name_and_rating"', index_definition
+    end
+  end
+
+  def test_schema_dumps_index_length
+    index_definition = dump_table_schema("companies").split(/\n/).grep(/t\.index.*_name_and_description/).first.strip
+    if current_adapter?(:Mysql2Adapter)
+      assert_equal 't.index ["name", "description"], name: "index_companies_on_name_and_description", length: 10', index_definition
+    else
+      assert_equal 't.index ["name", "description"], name: "index_companies_on_name_and_description"', index_definition
+    end
+  end
+
   def test_schema_dump_should_honor_nonstandard_primary_keys
     output = standard_dump
     match = output.match(%r{create_table "movies"(.*)do})
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index a4505a4892..bf66846840 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -205,6 +205,8 @@ ActiveRecord::Schema.define do
     t.bigint :rating, default: 1
     t.integer :account_id
     t.string :description, default: ""
+    t.index [:name, :rating], order: :desc
+    t.index [:name, :description], length: 10
     t.index [:firm_id, :type, :rating], name: "company_index", length: { type: 10 }, order: { rating: :desc }
     t.index [:firm_id, :type], name: "company_partial_index", where: "(rating > 10)"
     t.index :name, name: "company_name_index", using: :btree