Skip to content

The SQLite3 adapter now implements the supports_deferrable_constraints? contract#49376

Merged
yahonda merged 1 commit into
rails:mainfrom
fractaledmind:ar-sqlite-deferred-fks
Oct 29, 2023
Merged

The SQLite3 adapter now implements the supports_deferrable_constraints? contract#49376
yahonda merged 1 commit into
rails:mainfrom
fractaledmind:ar-sqlite-deferred-fks

Conversation

@fractaledmind

Copy link
Copy Markdown
Contributor

Motivation / Background

SQLite is a feature-rich database engine, and production usage is only growing. We need Rails' support to offer developers the range and scope of its features.

The PostgreSQL adapter supports deferred foreign keys, and SQLite itself has support deferred foreign keys since at least 2011.

Detail

Implementing the full supports_deferrable_constraints? contract allows foreign keys to be deferred by adding the :deferrable key to the foreign_key options in the add_reference and add_foreign_key methods.

add_reference :person, :alias, foreign_key: { deferrable: :deferred }
add_reference :alias, :person, foreign_key: { deferrable: :deferred }

In this PR, I am adding full support for the SQLite3Adapter by implementing:

ActiveRecord::ConnectionAdapters::SQLite3::SchemaCreation#visit_AddForeignKey
ActiveRecord::ConnectionAdapters::SQLite3::SchemaCreation#visit_ForeignKeyDefinition
ActiveRecord::ConnectionAdapters::SQLite3Adapter#supports_deferrable_constraints?
ActiveRecord::ConnectionAdapters::SQLite3::SchemaStatements#assert_valid_deferrable

and altering:

ActiveRecord::ConnectionAdapters::SQLite3::SchemaStatements#add_foreign_key
ActiveRecord::ConnectionAdapters::SQLite3Adapter#foreign_keys

Additional Info

In order to get the ActiveRecord::ConnectionAdapters::SQLite3Adapter#foreign_keys method working properly, I had to add a query to get the CREATE TABLE sql and parse which foreign key constraints are deferrable and which of those are deferred. In order to support this use-case, I extracted a table_structure_sql method which now table_structure_with_collation relies on.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@fractaledmind

Copy link
Copy Markdown
Contributor Author

Could the test failure be a flaky test? I can't reproduce locally and the error (SQLite3::IOException: disk I/O error) is a reflection of an OS-level problem:

his is what SQLite returns when it gets a hardware error while trying to do something.
https://sqlite.org/forum/info/f0740edfea56d61b733d83e469a55436df66f34defb219ff806dccceb0e086eb

Could we retry the build?

@zzak

zzak commented Sep 25, 2023

Copy link
Copy Markdown
Member

Could the test failure be a flaky test? I can't reproduce locally and the error (SQLite3::IOException: disk I/O error) is a reflection of an OS-level problem:

Could we retry the build?

I've issued a rebuild. 🙏

Edit: The rebuild failed, but this test is also flaking on main.

@fractaledmind

Copy link
Copy Markdown
Contributor Author

@zzak the fix for this flaky test is in main, can we rebuild this PR?

@zzak

zzak commented Sep 27, 2023

Copy link
Copy Markdown
Member

@fractaledmind Could you rebase please? 🙇

@zzak zzak added this to the 7.1.0 milestone Sep 27, 2023
@rafaelfranca rafaelfranca removed this from the 7.1.0 milestone Sep 27, 2023
@fractaledmind fractaledmind force-pushed the ar-sqlite-deferred-fks branch 2 times, most recently from d54697d to c85acbc Compare September 27, 2023 19:09
@rafaelfranca rafaelfranca added the ready PRs ready to merge label Sep 29, 2023
Comment thread activerecord/test/cases/migration/references_foreign_key_test.rb Outdated
@fractaledmind fractaledmind force-pushed the ar-sqlite-deferred-fks branch 2 times, most recently from cb18ebd to a3e2c2b Compare October 9, 2023 19:32
Comment thread activerecord/CHANGELOG.md Outdated
@fractaledmind fractaledmind force-pushed the ar-sqlite-deferred-fks branch from a3e2c2b to 38dd5f4 Compare October 13, 2023 08:44
@yahonda

yahonda commented Oct 28, 2023

Copy link
Copy Markdown
Member

Would you address the activerecord/CHANGELOG.md conflict?

…ts?` contract

Implementing the full `supports_deferrable_constraints?` contract allows foreign keys to be deferred by adding the `:deferrable` key to the `foreign_key` options in the `add_reference` and `add_foreign_key` methods.

```ruby
add_reference :person, :alias, foreign_key: { deferrable: :deferred }
add_reference :alias, :person, foreign_key: { deferrable: :deferred }
```
@fractaledmind fractaledmind force-pushed the ar-sqlite-deferred-fks branch from 38dd5f4 to ea17941 Compare October 28, 2023 10:38
@fractaledmind

Copy link
Copy Markdown
Contributor Author

@yahonda: Done (for both of my PRs)

end

def add_foreign_key(from_table, to_table, **options)
if options[:deferrable] == true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails main branch version is already 7.2.0.alpha, I'd like to keep this deprecation message as the PostgreSQL adapter does.

ActiveRecord.deprecator.warn(<<~MSG)
`deferrable: true` is deprecated in favor of `deferrable: :immediate`, and will be removed in Rails 7.2.
MSG

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

activerecord ready PRs ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants