/rails

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

Bump minimum SQLite version to 3.8 #32923

Merged
merged 1 commit into from May 21, 2018

Conversation

Reviewers

@matthewd matthewd

@kamipo kamipo

Assignees

@rafaelfranca rafaelfranca

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@yahonda @rails-bot @matthewd @kamipo @rafaelfranca
@yahonda
Contributor

yahonda commented May 17, 2018

Summary

These OS versions have SQLite 3.8 or higher by default.

  • macOS 10.10 (Yosemite) or higher
  • Ubuntu 14.04 LTS or higher

Raising the minimum version of SQLite 3.8 introduces these changes:

  • All of the bundled adapters support supports_multi_insert?
  • SQLite 3.8 always satisifies supports_foreign_keys_in_create? and supports_partial_index?
  • sqlite adapter can support alter_table method for foreign key referenced tables by #32865
@rails-bot

This comment has been minimized.

Sign in to view

rails-bot commented May 17, 2018

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@rails-bot rails-bot assigned rafaelfranca May 17, 2018

@yahonda yahonda referenced this pull request May 17, 2018

Merged

Disable foreign keys during `alter_table` for sqlite3 adapter #32865

activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb Outdated
@@ -48,9 +48,7 @@ def visit_TableDefinition(o)
statements.concat(o.indexes.map { |column_name, options| index_in_create(o.name, column_name, options) })
end
if supports_foreign_keys_in_create?

This comment has been minimized.

Sign in to view
Copy link
@matthewd

matthewd May 18, 2018

Member

This isn't true for the abstract adapter, so I think we still need the conditional for now. (Or, if we're okay to declare that a baseline requirement, it'd need to change there too.)

This comment has been minimized.

Sign in to view
Copy link
@yahonda

yahonda May 20, 2018

Contributor

Make sense. Reverted this change.
activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -1062,13 +1062,7 @@ def assume_migrated_upto_version(version, migrations_paths)
if (duplicate = inserting.detect { |v| inserting.count(v) > 1 })
raise "Duplicate migration #{duplicate}. Please renumber your migrations to resolve the conflict."
end
if supports_multi_insert?

This comment has been minimized.

Sign in to view
Copy link
@matthewd

matthewd May 18, 2018

Member

👍 happy to kill this one
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb Outdated
@@ -140,7 +144,7 @@ def supports_json?
end
def supports_multi_insert?

This comment has been minimized.

Sign in to view
Copy link
@matthewd

matthewd May 18, 2018

Member

I think we can just delete this method and leave the inherited one. And as we're removing the caller, I think we should deprecate the method in the abstract adapter.

This comment has been minimized.

Sign in to view
Copy link
@yahonda

yahonda May 20, 2018

Contributor

Agreed. deprecated supports_multi_insert? method.
activerecord/test/cases/schema_dumper_test.rb Outdated
@@ -192,7 +174,7 @@ def test_schema_dumps_index_columns_in_right_order
def test_schema_dumps_partial_indices
index_definition = dump_table_schema("companies").split(/\n/).grep(/t\.index.*company_partial_index/).first.strip
if current_adapter?(:PostgreSQLAdapter, :SQLite3Adapter) && ActiveRecord::Base.connection.supports_partial_index?
if current_adapter?(:PostgreSQLAdapter, :SQLite3Adapter)

This comment has been minimized.

Sign in to view
Copy link
@matthewd

matthewd May 18, 2018

Member

I'm not sure whether this should be using an adapter type check, or the supports_partial_index? condition (as we do for index sort order below). Agree it definitely shouldn't be both.

This comment has been minimized.

Sign in to view
Copy link
@yahonda

yahonda May 20, 2018

Contributor

Changed the condition to use ActiveRecord::Base.connection.supports_partial_index? only.
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb Outdated
@@ -104,6 +104,10 @@ def initialize(connection, logger, connection_options, config)
@active = true
@statements = StatementPool.new(self.class.type_cast_config_to_integer(config[:statement_limit]))
if sqlite_version < "3.8.0"
raise "Your version of SQLite (#{sqlite_version.inspect.match(/(\[\d+\,\ \d+\,\ \d+\])/)[0]}) is too old. Active Record supports SQLite >= 3.8."

This comment has been minimized.

Sign in to view
Copy link
@matthewd

matthewd May 18, 2018

Member

Let's add a to_s to AbstractAdapter::Version.

This comment has been minimized.

Sign in to view
Copy link
@yahonda

yahonda May 20, 2018

Contributor

I'm adding to_s method to AbstractAdapter::Version. Still, I need to call sqlite_version.to_s
and users will not see unnecessary exceptions here.
activerecord/lib/active_record/connection_adapters/abstract_adapter.rb Outdated
@@ -137,6 +137,10 @@ def initialize(version_string)
def <=>(version_string)
@version <=> version_string.split(".").map(&:to_i)
end
def to_s
@version.to_s

This comment has been minimized.

Sign in to view
Copy link
@kamipo

kamipo May 20, 2018

Member

@version.join(".")

This comment has been minimized.

Sign in to view
Copy link
@yahonda

yahonda May 20, 2018

Contributor

Updated.
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb Outdated
$stderr.puts <<-EOS.squish
"Your version of SQLite (#{sqlite_version.to_s}) is too old. Active Record supports SQLite >= 3.8."
EOS
exit 1

This comment has been minimized.

Sign in to view
Copy link
@kamipo

kamipo May 20, 2018

Member

Is there any reason not to use raise unlike other adapters?

rails/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

Lines 56 to 58 in ce4d467

if version < "5.1.10"
raise "Your version of MySQL (#{version_string}) is too old. Active Record supports MySQL >= 5.1.10."
end

rails/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

Lines 220 to 222 in ce4d467

if postgresql_version < 90100
raise "Your version of PostgreSQL (#{postgresql_version}) is too old. Active Record supports PostgreSQL >= 9.1."
end

This comment has been minimized.

Sign in to view
Copy link
@yahonda

yahonda May 20, 2018

Contributor

I'm thinking about implementing version check like #28939 just showing a necessary message for users by omitting stack traces like this.

$ sqlite3 --version
3.7.9 2011-11-01 00:52:41 c7c6050ef060877ebe77b41d959e9df13f8c9b5e
$ bundle exec ruby -w -Itest test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
Using sqlite3
"Your version of SQLite (3.7.9) is too old. Active Record supports SQLite >= 3.8."
$

I'd like to get some feedback on this change. I agree to make all bundled adapters behave similarly for the database version check. then I'll revert this change or open another pull request to change mysql2 and posgresql adapter version checks.

This comment has been minimized.

Sign in to view
Copy link
@kamipo

kamipo May 20, 2018

Member

Usually, a library does/should not terminate a process itself.
If a library may terminate a process itself, it hard to handle that case by users.

This comment has been minimized.

Sign in to view
Copy link
@yahonda

yahonda May 20, 2018

Contributor

Understood. Thanks for the comment.
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb Outdated
@@ -104,6 +104,13 @@ def initialize(connection, logger, connection_options, config)
@active = true
@statements = StatementPool.new(self.class.type_cast_config_to_integer(config[:statement_limit]))
if sqlite_version < "3.8.0"
$stderr.puts <<-EOS.squish
"Your version of SQLite (#{sqlite_version.to_s}) is too old. Active Record supports SQLite >= 3.8."

This comment has been minimized.

Sign in to view
Copy link
@kamipo

kamipo May 20, 2018

Member

Unneeded the redundant to_s in string interpolation.
@yahonda
Bump minimum SQLite version to 3.8 
These OS versions have SQLite 3.8 or higher by default.

- macOS 10.10 (Yosemite) or higher
- Ubuntu 14.04 LTS or higher

Raising the minimum version of SQLite 3.8 introduces these changes:

- All of bundled adapters support `supports_multi_insert?`
- SQLite 3.8 always satisifies `supports_foreign_keys_in_create?` and `supports_partial_index?`
- sqlite adapter can support `alter_table` method for foreign key referenced tables by #32865
- Deprecated `supports_multi_insert?` method
Loading status checks…
d1a74c1

kamipo added a commit that referenced this pull request May 21, 2018

@kamipo
Enable `Lint/StringConversionInInterpolation` rubocop rule 
To prevent redundant `to_s` like #32923 (comment)
automatically in the future.
a77447f

@kamipo kamipo merged commit 054893d into rails:master May 21, 2018
2 checks passed

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

yahonda added a commit to yahonda/rails that referenced this pull request May 22, 2018

@yahonda
Disable foreign keys during `alter_table` for sqlite3 adapter 
Unlike other databases, changing SQLite3 table definitions need to create a temporary table.
While changing table operations, the original table needs dropped which caused
`SQLite3::ConstraintException: FOREIGN KEY constraint failed` if the table is referenced by foreign keys.
This pull request disables foreign keys by `disable_referential_integrity`.

Also `disable_referential_integrity` method needs to execute `defer_foreign_keys = ON`
to defer re-enabling foreign keys until the transaction is committed.

https://www.sqlite.org/pragma.html#pragma_defer_foreign_keys

Fixes rails#31988

- This `defer_foreign_keys = ON` has been supported since SQLite 3.8.0
https://www.sqlite.org/releaselog/3_8_0.html and Rails 6 requires SQLite 3.8 rails#32923 now

- <Models>.reset_column_information added to address `ActiveModel::UnknownAttributeError`

```
Error:
ActiveRecord::Migration::ForeignKeyChangeColumnTest#test_change_column_of_parent_table:
ActiveModel::UnknownAttributeError: unknown attribute 'name' for ActiveRecord::Migration::ForeignKeyChangeColumnTest::Post.
```
45881b0

@yahonda yahonda deleted the yahonda:bump_sqlite3_version_to_38 branch May 31, 2018

yahonda added a commit to yahonda/rails that referenced this pull request Jun 1, 2018

@yahonda
[skip ci] Add changelog about bumping minimum SQLite version to 3.8 
Related to rails#32923
2252306

@yahonda yahonda referenced this pull request Jun 1, 2018

Merged

[skip ci] Add changelog about bumping minimum SQLite version to 3.8 #33041

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