Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord::ConnectionAdapters::SchemaStatements.remove_index does not work in Rails 6.1.2 migrations if both column and index name are given #2148

Open
rammpeter opened this issue Feb 14, 2021 · 2 comments

Comments

@rammpeter
Copy link
Contributor

rammpeter commented Feb 14, 2021

Steps to reproduce

This example ends in error like migrations do during rollback of add_index if a index name ist given

require 'bundler/inline'

gemfile(true) do
  gem 'activerecord', '6.1.2.1'
  gem 'activerecord-oracle_enhanced-adapter', '6.1.2'
end

require 'active_record'
require 'active_record/connection_adapters/oracle_enhanced_adapter'



ActiveRecord::Base.establish_connection(
  adapter:  :oracle_enhanced,
  host:     :localhost,
  port:     1521,
  database: '/orclpdb1',
  username: 'user',
  password: 'pw'
)

ActiveRecord::Schema.define do
  create_table :dummy, force: true do |t|
    t.string  :name
  end

  # Works with 6.1.2 if no columns are given
  add_index :dummy, :name, name: 'IX_DUMMY_NAME'
  remove_index :dummy, name: 'IX_DUMMY_NAME'

  # Works with 6.1.2 if no index_name is given
  add_index :dummy, :name, name: 'IX_DUMMY_NAME'
  remove_index :dummy, :name

  # Does not work if 6.1.2, but is what db:migrate now does while reversing "add_index :dummy, [:name], name: 'IX_DUMMY_NAME'"
  add_index :dummy, :name, name: 'IX_DUMMY_NAME'
  remove_index :dummy, :name, name: 'IX_DUMMY_NAME'
end

Expected behavior

Migration should be able to rollback like such one

class ExtendSchemaRights3 < ActiveRecord::Migration[6.0]
  def change
    add_index :schema_rights, [:user_id, :schema_id], name: 'IX_SCHEMA_RIGHTS_LOGICAL_PKEY'
  end
end

Actual behavior

ArgumentError: No indexes found on

with the options provided.
is raised in db:rollback
db:rollback calls remove_index in such cases with both column list and index name.

System configuration

Rails version:
6.1.2.1

Oracle enhanced adapter version:
6.1.2

Ruby version:
jRuby 9.2.0.13

Oracle Database version:
9.3.0.0

@rammpeter rammpeter changed the title remove_index does not work in Rails 6.1.2 if both column and index name are given ActiveRecord::ConnectionAdapters::SchemaStatements.remove_index does not work in Rails 6.1.2 migrations if both column and index name are given Feb 14, 2021
@yahonda
Copy link
Collaborator

yahonda commented Feb 16, 2021

Reproduced locally.

$ ruby rep2148.rb
Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Using concurrent-ruby 1.1.8
Using minitest 5.14.3
Using zeitwerk 2.4.2
Using ruby-plsql 0.7.1
Using bundler 2.2.5
Using ruby-oci8 2.2.9
Using i18n 1.8.9
Using tzinfo 2.0.4
Using activesupport 6.1.2.1
Using activemodel 6.1.2.1
Using activerecord 6.1.2.1
Using activerecord-oracle_enhanced-adapter 6.1.2
-- create_table(:dummy, {:force=>true})
   -> 0.3040s
-- add_index(:dummy, :name, {:name=>"IX_DUMMY_NAME"})
   -> 0.2003s
-- remove_index(:dummy, {:name=>"IX_DUMMY_NAME"})
   -> 0.0109s
   -> 0 rows
-- add_index(:dummy, :name, {:name=>"IX_DUMMY_NAME"})
   -> 0.1827s
-- remove_index(:dummy, :name)
   -> 0.2926s
   -> 0 rows
-- add_index(:dummy, :name, {:name=>"IX_DUMMY_NAME"})
   -> 0.1882s
-- remove_index(:dummy, :name, {:name=>"IX_DUMMY_NAME"})
/home/yahonda/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-6.1.2.1/lib/active_record/connection_adapters/abstract/schema_statements.rb:1411:in `index_name_for_remove': No indexes found on dummy with the options provided. (ArgumentError)
	from /home/yahonda/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-oracle_enhanced-adapter-6.1.2/lib/active_record/connection_adapters/oracle_enhanced/schema_statements.rb:333:in `remove_index'
	from /home/yahonda/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-6.1.2.1/lib/active_record/migration.rb:929:in `block in method_missing'
	from /home/yahonda/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-6.1.2.1/lib/active_record/migration.rb:897:in `block in say_with_time'
	from /home/yahonda/.rbenv/versions/3.0.0/lib/ruby/3.0.0/benchmark.rb:293:in `measure'
	from /home/yahonda/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-6.1.2.1/lib/active_record/migration.rb:897:in `say_with_time'
	from /home/yahonda/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-6.1.2.1/lib/active_record/migration.rb:918:in `method_missing'
	from rep2148.rb:39:in `block in <main>'
	from /home/yahonda/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-6.1.2.1/lib/active_record/schema.rb:50:in `instance_eval'
	from /home/yahonda/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-6.1.2.1/lib/active_record/schema.rb:50:in `define'
	from /home/yahonda/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activerecord-6.1.2.1/lib/active_record/schema.rb:46:in `define'
	from rep2148.rb:24:in `<main>'
$
$ more rep2148.rb
require 'bundler/inline'

gemfile(true) do
  source "https://rubygems.org"
  git_source(:github) { |repo| "https://github.com/#{repo}.git" }
  gem 'activerecord', '6.1.2.1'
  gem 'activerecord-oracle_enhanced-adapter', '6.1.2'
  gem 'ruby-oci8'
end

require 'active_record'
require 'active_record/connection_adapters/oracle_enhanced_adapter'


ActiveRecord::Base.establish_connection(
  adapter:  :oracle_enhanced,
  host:     :localhost,
  port:     1521,
  database: '/orclpdb1',
  username: 'oracle_enhanced',
  password: 'oracle_enhanced'
)

ActiveRecord::Schema.define do
  create_table :dummy, force: true do |t|
    t.string  :name
  end

  # Works with 6.1.2 if no columns are given
  add_index :dummy, :name, name: 'IX_DUMMY_NAME'
  remove_index :dummy, name: 'IX_DUMMY_NAME'

  # Works with 6.1.2 if no index_name is given
  add_index :dummy, :name, name: 'IX_DUMMY_NAME'
  remove_index :dummy, :name

  # Does not work if 6.1.2, but is what db:migrate now does while reversing "add_index :dummy, [:name], name: 'IX_DUMMY_NAME'"
  add_index :dummy, :name, name: 'IX_DUMMY_NAME'
  remove_index :dummy, :name, name: 'IX_DUMMY_NAME'
end
$

@yahonda
Copy link
Collaborator

yahonda commented Feb 25, 2021

It looks like this line does not return anything in this case.
https://github.com/rails/rails/blob/130c128eae233bf71231c73b9c3c3b3f3ede918b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb#L1405

          matching_indexes = indexes(table_name).select { |i| checks.all? { |check| check[i] } }

@rammpeter Would you consider opening a pull request? I think we can have our own index_name_for_remove method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants