basecamp / basecamp/marginalia
Marginalia comments prevents Rails 6.0 from getting index names with SQLite3
- Dominant language
- Ruby
- Stars
- 1.8k
- Forks
- 148
- PR merge metrics
- No merged PRs in 30d
Description
Migrating from Rails 5.2 to Rails 6.0.4.1, and I got this error when loading schema in sqlite:
```
#
```
I understood where the issue comes from and I need help about how it can be fixed. Here are the reproducing steps and the explanation for this error.
## Reproducing steps
Create a rails 6.0 app with marginalia and two migrations: one adding an expression index, the second adding another index on the same table.
```
rails new index_names_bug
cd index_names_bug
bundle add marginalia
rails generate model Product name:string description:text
rails generate migration AddExpressionIndexToProducts
```
set the content of this migration to
```
class AddExpressionIndexToProducts < ActiveRecord::Migration[6.0]
def change
add_index :products, 'lower(name)'
end
end
```
then add another migration
```
rails generate migration AddClassicIndexToProducts
```
set the content to
```
class AddClassicIndexToProducts < ActiveRecord::Migration[6.0]
def change
add_index :products, :description
end
end
```
run
```
rails db:migrate
```
to see the following error
```
== 20210824125823 AddExpressionIndexToProducts: migrating =====================
-- add_index(:products, "lower(name)")
-> 16.6489s
== 20210824125823 AddExpressionIndexToProducts: migrated (16.6491s) ===========
== 20210824125906 AddClassicIndexToProducts: migrating ========================
-- add_index(:products, :description)
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:
undefined method `size' for nil:NilClass
/app/db/migrate/20210824125906_add_classic_index_to_products.rb:3:in `change'
Caused by:
NoMethodError: undefined method `size' for nil:NilClass
/app/db/migrate/20210824125906_add_classic_index_to_products.rb:3:in `change'
Tasks: TOP => db:migrate
(See full trace by running task with --trace)
```
It fails at the last migration
## Why does it happen?
When adding an index, activerecord checks if the index name exists and get the list of indexes defined on the table. Fetching the list of indexes is done in [lib/active_record/connection_adapters/sqlite3/schema_statements.rb](https://github.com/rails/rails/blob/0d304eae601f085274b2e2c04316e025b443da62/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb#L8).
The query `PRAGMA index_list(table_name)` return one row per index, and each row is used to get the column name with a `PRAGMA index_info(index_name)`. As the index is an expression index, the column name is nil.
This is handled here:
https://github.com/rails/rails/blob/0d304eae601f085274b2e2c04316e025b443da62/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb#L24
https://github.com/rails/rails/blob/0d304eae601f085274b2e2c04316e025b443da62/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb#L32-L34
A regular expression is used to get the expression used to create the index from the SQL. The regexp is `/\bON\b\s*"?(\w+?)"?\s*\((?.+?)\)(?:\s*WHERE\b\s*(?.+))?\z/i`. This regexp does not match the trailing comment appended by marginalia, so `expressions` is set to `nil`, so `columns` is set to nil too, and [some code](https://github.com/rails/rails/blob/0d304eae601f085274b2e2c04316e025b443da62/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb#L38) asks for `columns.size` and produces the error `NoMethodError: undefined method 'size' for nil:NilClass`.
## How to fix?
That's where I need help. Options I've seen so far:
* use a regexp which ignores trailing comments.
using `# /\bON\b\s*"?(\w+?)"?\s*\((?.+?)\)(?:\s*WHERE\b\s*(?.+))?(?:\s\/\*.*\*\/)?\z/i =~ index_sql` will work. I can issue a pull request against rails but it would not be fixed in the 6.0 branch.
* monkey patch `ActiveRecord::ConnectionAdapters::SQLite3::SchemaStatements#indexes` with the good regexp.
I would prefer avoiding that.
* disable marginalia for some SQL commands.
Is it possible?
What do you recommend here?
Contributor guide
No contributing guide indexed for this repository
Research direction
Reproduce the two-migration case with Marginalia and SQLite, then inspect the Rails files cited in the report: activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb and activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb. Compare the index SQL parsing behavior with trailing comments and determine a compatible fix, including a regression test and a clear outcome for the migration.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rails, ruby, sqlite
- Domain
- backend, database
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 30/100