have_db_index: normalize_columns_to_array is always an array

[dniman]

In activerecord-5.0.0\lib\active_record\connection_adapter\postgresql\schema_statements.rb columns variable(in code below) can be string or [].

def indexes
.....
    if indkey.include?(0) || opclass > 0
      columns = expression
    else
      columns = Hash[query(<<-SQL.strip_heredoc, "SCHEMA")].values_at(*indkey).compact
        SELECT a.attnum, a.attname
        FROM pg_attribute a
        WHERE a.attrelid = #{oid}
          AND a.attnum IN (#{indkey.join(",")})
      SQL

      # add info on sort order for columns (only desc order is explicitly specified, asc is the default)
      orders = Hash[
        expressions.scan(/(\w+) DESC/).flatten.map { |order_column| [order_column, :desc] }
       ]
     end 
 ..........
 end

In shoulda-matchers-3.1.1\lib\shoulda\matchers\active_record\have_db_index_matchar.rb
@columns is always an [] after normalize_columns_to_array method.

.....
def initialize(columns)
  @columns = normalize_columns_to_array(columns)
  @options = {}
end
.....
def matched_index
  indexes.detect {|each| each.columns == @columns}
end
.....

def indexes
  ::ActiveRecord::Base.connection.indexes(table_name)
end
.....
def normalize_columns_to_array(columns)
  Array.wrap(columns).map(&:to_s)
end

Therefore we cannot use functional indexes like lower((code)::text) with have_db_index.
In my_model_spec.rb:

it { should have_db_index("lower((code)::text)").unique(:true)}

and after running spec:

Failure/Error: it { should have_db_index("lower((code)::text)").unique(:true)}
  Expected MyModel to have a unique index on columns lower((code)::text) ()

I changed the matched_index method locally in shoulda-matchers-3.1.1\lib\shoulda\matchers\active_record\have_db_index_matcher.rb
to fix this from

 def matched_index
   indexes.detect { |each| each.columns == @columns }      
 end

to

def matched_index
  indexes.detect { |each| each.columns == (each.columns.is_a?(String)? @columns.join('') : @columns) }
end

[tejasbubane]

Could you use markdown code highlighting with proper indentation?
Refer: https://guides.github.com/features/mastering-markdown/ At the moment, it is difficult to make out what your code is doing.

If you can create a gist and share the link here, that would be even better.

[mcmire]

@dniman I know you posted this a while back. I'm just now getting to it.

Do you still need this feature? I'm struggling to understand the exact use case for this as I've never had this need. Shouldn't you be able to refer to an index by name?

[dniman]

@mcmire
Hi!Sorry for my late reply.
Rails adds support for expression/functional indexes
https://blog.bigbinary.com/2016/07/20/rails-5-adds-support-for-expression-indexes-for-postgresql.html

Do you still need this feature? I'm struggling to understand the exact use case for this as I've never had this need.

It's need when you create this kind of index:

CREATE UNIQUE INDEX <index_name>
  ON <table_name>
  USING btree
  (lower(code::text))

and test it like this:

it { should have_db_index("lower((code)::text)").unique(:true)}

Some explanation about functional indexes here
https://www.postgresql.org/docs/9.1/static/indexes-expressional.html
and here
http://use-the-index-luke.com/sql/where-clause/functions/case-insensitive-search

[dniman]

@mcmire
My version of db_index_matcher to fix this issue:

module Shoulda
  module Matchers
    module ActiveRecord
      # The `have_db_index` matcher tests that the table that backs your model
      # has a index on a specific column.
      #
      #     class CreateBlogs < ActiveRecord::Migration
      #       def change
      #         create_table :blogs do |t|
      #           t.integer :user_id
      #         end
      #
      #         add_index :blogs, :user_id
      #       end
      #     end
      #
      #     # RSpec
      #     describe Blog do
      #       it { should have_db_index(:user_id) }
      #     end
      #
      #     # Minitest (Shoulda)
      #     class BlogTest < ActiveSupport::TestCase
      #       should have_db_index(:user_id)
      #     end
      #
      # #### Qualifiers
      #
      # ##### unique
      #
      # Use `unique` to assert that the index is unique.
      #
      #     class CreateBlogs < ActiveRecord::Migration
      #       def change
      #         create_table :blogs do |t|
      #           t.string :name
      #         end
      #
      #         add_index :blogs, :name, unique: true
      #       end
      #     end
      #
      #     # RSpec
      #     describe Blog do
      #       it { should have_db_index(:name).unique(true) }
      #     end
      #
      #     # Minitest (Shoulda)
      #     class BlogTest < ActiveSupport::TestCase
      #       should have_db_index(:name).unique(true)
      #     end
      #
      # Since it only ever makes since for `unique` to be `true`, you can also
      # leave off the argument to save some keystrokes:
      #
      #     # RSpec
      #     describe Blog do
      #       it { should have_db_index(:name).unique }
      #     end
      #
      #     # Minitest (Shoulda)
      #     class BlogTest < ActiveSupport::TestCase
      #       should have_db_index(:name).unique
      #     end
      #
      # @return [HaveDbIndexMatcher]
      #
      def have_db_index(columns)
        HaveDbIndexMatcher.new(columns)
      end

      # @private
      class HaveDbIndexMatcher
        def initialize(columns)
          @columns = normalize_columns_to_array(columns)
          @options = {}
        end

        def unique(unique = true)
          @options[:unique] = unique
          self
        end

        def matches?(subject)
          @subject = subject
          index_exists? && correct_unique?
        end

        def failure_message
          "Expected #{expectation} (#{@missing})"
        end

        def failure_message_when_negated
          "Did not expect #{expectation}"
        end

        def description
          if @options.key?(:unique)
            "have a #{index_type} index on columns #{@columns.join(' and ')}"
          else
            "have an index on columns #{@columns.join(' and ')}"
          end
        end

        protected

        def index_exists?
          ! matched_index.nil?
        end

        def correct_unique?
          return true unless @options.key?(:unique)

          is_unique = matched_index.unique

          is_unique = !is_unique unless @options[:unique]

          unless is_unique
            @missing = "#{table_name} has an index named #{matched_index.name} " <<
            "of unique #{matched_index.unique}, not #{@options[:unique]}."
          end

          is_unique
        end

        def matched_index
          indexes.detect { |each| each.columns == (each.columns.is_a?(String)? @columns.join('') : @columns) }
        end

        def model_class
          @subject.class
        end

        def table_name
          model_class.table_name
        end

        def indexes
          ::ActiveRecord::Base.connection.indexes(table_name)
        end

        def expectation
          "#{model_class.name} to #{description}"
        end

        def index_type
          if @options[:unique]
            'unique'
          else
            'non-unique'
          end
        end

        def normalize_columns_to_array(columns)
          Array.wrap(columns).map(&:to_s)
        end
      end
    end
  end
end

[mcmire]

@dniman Right, okay. I don't use the have_db_index matcher myself so I didn't realize that what it takes is not the name of an index, but the column(s) for which your index applies. I also didn't realize that CREATE [UNIQUE] INDEX doesn't really take a column, but it also takes an expression which it will use to build the index.

I'll include this in the next release, but let me take care of some things first and then I'll get to this. :)

[PeterMozesMerl]

The issue is the Array.wrap.

ActiveRecord::Base.connection.indexes returns an Array most of the time. Array.wrap always returns an Array.

Most of the time != Always.

2.5.1 :039 > ActiveRecord::Base.connection.indexes('posts').detect { |index| puts "#{index.columns.inspect} - #{index.columns.class}" }
["user_id"] - Array
 => nil

2.5.1 :040 > ActiveRecord::Base.connection.indexes('messages').detect { |index| puts "#{index.columns.inspect} - #{index.columns.class}" }
["sender_id", "created_at"] - Array
 => nil

2.5.1 :041 > ActiveRecord::Base.connection.indexes('table3').detect { |index| puts "#{index.columns.inspect} - #{index.columns.class}" }
"country_id, lower((code)::text)" - String

As for whether the issue is urgent or not. I’ve been having a dozens of

#it { pending; is_expected.to have_db_index([:country_id, 'lower(code)']) }

lines in my project for at least five years waiting for the fix. A little bit more waiting won’t hurt.

Although, I consider it an upstream issue. As long as .indexes returns ["sender_id", "created_at"], there is no explanation why it does not return ['country_id', 'lower((code)::text)'].

[PeterMozesMerl]

I believe this is a fix:

        def matched_index
          indexes.detect { |each| Array.wrap(each.columns).join(', ') == @columns.join(', ') }
        end

Now they all pass (multiple models):

	it { is_expected.to have_db_index([:sender_id, :created_at]) }
	it { is_expected.to have_db_index([:country_id, 'lower((code)::text)']).unique true }
	it { is_expected.not_to have_db_index(:country_id) }

[mcmire]

Unfortunately we have so much stuff planned for the next release that we won't be able to fit this. We'll include it in a future release, though.

[PeterMozesMerl]

Thank you. That’s fine.