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

The SQLite3 adapter now implements the supports_deferrable_constraints? contract #49376

Merged
merged 1 commit into from Oct 29, 2023

Conversation

fractaledmind
Copy link
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
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
Copy link
Member

zzak commented Sep 25, 2023

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
Contributor Author

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

@zzak
Copy link
Member

zzak commented Sep 27, 2023

@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
@fractaledmind fractaledmind force-pushed the ar-sqlite-deferred-fks branch 2 times, most recently from cb18ebd to a3e2c2b Compare October 9, 2023 19:32
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
@yahonda
Copy link
Member

yahonda commented Oct 28, 2023

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
Copy link
Contributor Author

@yahonda: Done (for both of my PRs)

@@ -53,6 +53,16 @@ def indexes(table_name)
end

def add_foreign_key(from_table, to_table, **options)
if options[:deferrable] == true
Copy link
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

@yahonda yahonda merged commit a561dd4 into rails:main Oct 29, 2023
4 checks passed
@fractaledmind fractaledmind deleted the ar-sqlite-deferred-fks branch October 29, 2023 07:26
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.

None yet

4 participants