basecamp / basecamp/marginalia

Marginalia comments prevents Rails 6.0 from getting index names with SQLite3

Open
#128 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.