From 21a6dbd313efa9fb7feac4d95acaf6271112801f Mon Sep 17 00:00:00 2001 From: fatkodima Date: Tue, 14 Jun 2022 02:40:03 +0300 Subject: [PATCH] Enable strict strings mode for `SQLite3Adapter` Co-authored-by: Jean Boussier --- Gemfile.lock | 2 +- activerecord/CHANGELOG.md | 21 ++++++++++++ .../sqlite3/schema_statements.rb | 4 +-- .../connection_adapters/sqlite3_adapter.rb | 12 ++++++- activerecord/lib/active_record/railtie.rb | 9 +++++ .../adapters/sqlite3/sqlite3_adapter_test.rb | 32 +++++++++++++++-- activerecord/test/config.example.yml | 2 ++ guides/source/configuring.md | 19 +++++++++++ guides/source/upgrading_ruby_on_rails.md | 17 ++++++++++ .../lib/rails/application/configuration.rb | 4 +++ .../new_framework_defaults_7_1.rb.tt | 9 +++++ .../test/application/configuration_test.rb | 34 +++++++++++++++++++ 12 files changed, 158 insertions(+), 7 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index f237f76eb7b..eedd1de8a31 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -499,7 +499,7 @@ GEM actionpack (>= 5.2) activesupport (>= 5.2) sprockets (>= 3.0.0) - sqlite3 (1.4.2) + sqlite3 (1.4.3) stackprof (0.2.17) stimulus-rails (1.0.2) railties (>= 6.0.0) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5a693b5bf67..6396b3b2986 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -29,6 +29,27 @@ *Cameron Bothner and Mitch Vollebregt* +* Enable strict strings mode for `SQLite3Adapter`. + + Configures SQLite with a strict strings mode, which disables double-quoted string literals. + + SQLite has some quirks around double-quoted string literals. + It first tries to consider double-quoted strings as identifier names, but if they don't exist + it then considers them as string literals. Because of this, typos can silently go unnoticed. + For example, it is possible to create an index for a non existing column. + See [SQLite documentation](https://www.sqlite.org/quirks.html#double_quoted_string_literals_are_accepted) for more details. + + If you don't want this behavior, you can disable it via: + + ```ruby + # config/application.rb + config.active_record.sqlite3_adapter_strict_strings_by_default = false + ``` + + Fixes #27782. + + *fatkodima*, *Jean Boussier* + * Resolve issue where a relation cache_version could be left stale. Previously, when `reset` was called on a relation object it did not reset the cache_versions diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb index fe33335e75f..39ce83bce8b 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb @@ -84,11 +84,11 @@ module ActiveRecord table_sql = query_value(<<-SQL, "SCHEMA") SELECT sql FROM sqlite_master - WHERE name = #{quote_table_name(table_name)} AND type = 'table' + WHERE name = #{quote(table_name)} AND type = 'table' UNION ALL SELECT sql FROM sqlite_temp_master - WHERE name = #{quote_table_name(table_name)} AND type = 'table' + WHERE name = #{quote(table_name)} AND type = 'table' SQL table_sql.to_s.scan(/CONSTRAINT\s+(?\w+)\s+CHECK\s+\((?(:?[^()]|\(\g\))+)\)/i).map do |name, expression| diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index dfd2429c3a4..de1fbbdf465 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -34,7 +34,7 @@ module ActiveRecord db = SQLite3::Database.new( config[:database].to_s, - config.merge(results_as_hash: true) + config.merge(results_as_hash: true, strict: ConnectionAdapters::SQLite3Adapter.strict_strings_by_default) ) ConnectionAdapters::SQLite3Adapter.new(db, logger, nil, config) @@ -61,6 +61,16 @@ module ActiveRecord include SQLite3::SchemaStatements include SQLite3::DatabaseStatements + ## + # :singleton-method: + # Configure the SQLite3Adapter to be used in a strict strings mode. + # This will disable double-quoted string literals, because otherwise typos can silently go unnoticed. + # For example, it is possible to create an index for a non existing column. + # If you wish to enable this mode you can add the following line to your application.rb file: + # + # config.active_record.sqlite3_adapter_strict_strings_by_default = true + class_attribute :strict_strings_by_default, default: false + NATIVE_DATABASE_TYPES = { primary_key: "integer PRIMARY KEY AUTOINCREMENT NOT NULL", string: { name: "varchar" }, diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index ab0d7ab8ce2..8d0b4edc633 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -220,6 +220,14 @@ To keep using the current cache store, you can turn off cache versioning entirel end end + initializer "active_record.sqlite3_adapter_strict_strings_by_default" do + if config.active_record.sqlite3_adapter_strict_strings_by_default + ActiveSupport.on_load(:active_record_sqlite3adapter) do + self.strict_strings_by_default = true + end + end + end + initializer "active_record.set_configs" do |app| configs = app.config.active_record @@ -246,6 +254,7 @@ To keep using the current cache store, you can turn off cache versioning entirel :query_log_tags, :cache_query_log_tags, :sqlite3_production_warning, + :sqlite3_adapter_strict_strings_by_default, :check_schema_cache_dump_version, :use_schema_cache_dump ) diff --git a/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb b/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb index 885e0b2c775..57f27f9fdef 100644 --- a/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb +++ b/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb @@ -141,7 +141,7 @@ module ActiveRecord assert_equal 2, result.columns.length assert_equal %w{ id data }, result.columns - @conn.exec_query('INSERT INTO ex (id, data) VALUES (1, "foo")') + @conn.exec_query("INSERT INTO ex (id, data) VALUES (1, 'foo')") result = @conn.exec_query("SELECT id, data FROM ex") assert_equal 1, result.rows.length assert_equal 2, result.columns.length @@ -152,7 +152,7 @@ module ActiveRecord def test_exec_query_with_binds with_example_table "id int, data string" do - @conn.exec_query('INSERT INTO ex (id, data) VALUES (1, "foo")') + @conn.exec_query("INSERT INTO ex (id, data) VALUES (1, 'foo')") result = @conn.exec_query( "SELECT id, data FROM ex WHERE id = ?", nil, [Relation::QueryAttribute.new(nil, 1, Type::Value.new)]) @@ -165,7 +165,7 @@ module ActiveRecord def test_exec_query_typecasts_bind_vals with_example_table "id int, data string" do - @conn.exec_query('INSERT INTO ex (id, data) VALUES (1, "foo")') + @conn.exec_query("INSERT INTO ex (id, data) VALUES (1, 'foo')") result = @conn.exec_query( "SELECT id, data FROM ex WHERE id = ?", nil, [Relation::QueryAttribute.new("id", "1-fuu", Type::Integer.new)]) @@ -616,6 +616,25 @@ module ActiveRecord end end + def test_strict_strings_by_default + conn = Base.sqlite3_connection(database: ":memory:", adapter: "sqlite3") + conn.create_table :testings + + assert_nothing_raised do + conn.add_index :testings, :non_existent + end + + with_strict_strings_by_default do + conn = Base.sqlite3_connection(database: ":memory:", adapter: "sqlite3") + conn.create_table :testings + + error = assert_raises(StandardError) do + conn.add_index :testings, :non_existent2 + end + assert_match(/no such column: non_existent2/, error.message) + end + end + private def assert_logged(logs) subscriber = SQLSubscriber.new @@ -633,6 +652,13 @@ module ActiveRecord SQL super(@conn, table_name, definition, &block) end + + def with_strict_strings_by_default + SQLite3Adapter.strict_strings_by_default = true + yield + ensure + SQLite3Adapter.strict_strings_by_default = false + end end end end diff --git a/activerecord/test/config.example.yml b/activerecord/test/config.example.yml index 9c356a37605..09aeb917f8b 100644 --- a/activerecord/test/config.example.yml +++ b/activerecord/test/config.example.yml @@ -94,9 +94,11 @@ connections: arunit: database: <%= FIXTURES_ROOT %>/fixture_database.sqlite3 timeout: 5000 + strict: true arunit2: database: <%= FIXTURES_ROOT %>/fixture_database_2.sqlite3 timeout: 5000 + strict: true sqlite3_mem: arunit: diff --git a/guides/source/configuring.md b/guides/source/configuring.md index ae74f0b8c81..dfb5c674c1d 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -66,6 +66,7 @@ Below are the default values associated with each target version. In cases of co - [`config.active_support.default_message_verifier_serializer`](#config-active-support-default-message-verifier-serializer): `:json` - [`config.action_controller.allow_deprecated_parameters_hash_equality`](#config-action-controller-allow-deprecated-parameters-hash-equality): `false` - [`config.log_file_size`](#config-log-file-size): `100.megabytes` +- [`config.active_record.sqlite3_adapter_strict_strings_by_default`](#config-active-record-sqlite3-adapter-strict-strings-by-default): `false` #### Default Values for Target Version 7.0 @@ -985,6 +986,24 @@ regular expressions. Specifies if source locations of methods that call database queries should be logged below relevant queries. By default, the flag is `true` in development and `false` in all other environments. +#### `config.active_record.sqlite3_adapter_strict_strings_by_default` + +Specifies whether the SQLite3Adapter should be used in a strict strings mode. +The use of a strict strings mode disables double-quoted string literals. + +SQLite has some quirks around double-quoted string literals. +It first tries to consider double-quoted strings as identifier names, but if they don't exist +it then considers them as string literals. Because of this, typos can silently go unnoticed. +For example, it is possible to create an index for a non existing column. +See [SQLite documentation](https://www.sqlite.org/quirks.html#double_quoted_string_literals_are_accepted) for more details. + +The default value depends on the `config.load_defaults` target version: + +| Starting with version | The default value is | +| --------------------- | -------------------- | +| (original) | `false` | +| 7.1 | `true` | + #### `config.active_record.async_query_executor` Specifies how asynchronous queries are pooled. diff --git a/guides/source/upgrading_ruby_on_rails.md b/guides/source/upgrading_ruby_on_rails.md index e26c08f4601..ab656646925 100644 --- a/guides/source/upgrading_ruby_on_rails.md +++ b/guides/source/upgrading_ruby_on_rails.md @@ -262,6 +262,23 @@ config.cache_store = :mem_cache_store, "cache.example.com", pool: false See the [caching with Rails](https://guides.rubyonrails.org/caching_with_rails.html#connection-pool-options) guide for more information. +### `SQLite3Adapter` now configured to be used in a strict strings mode + +The use of a strict strings mode disables double-quoted string literals. + +SQLite has some quirks around double-quoted string literals. +It first tries to consider double-quoted strings as identifier names, but if they don't exist +it then considers them as string literals. Because of this, typos can silently go unnoticed. +For example, it is possible to create an index for a non existing column. +See [SQLite documentation](https://www.sqlite.org/quirks.html#double_quoted_string_literals_are_accepted) for more details. + +If you don't want to use `SQLite3Adapter` in a strict mode, you can disable this behavior: + +```ruby +# config/application.rb +config.active_record.sqlite3_adapter_strict_strings_by_default = false +``` + Upgrading from Rails 6.1 to Rails 7.0 ------------------------------------- diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index 6df81c1b353..e3a0dbc7e76 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -284,6 +284,10 @@ module Rails if respond_to?(:action_controller) action_controller.allow_deprecated_parameters_hash_equality = false end + + if respond_to?(:active_record) + active_record.sqlite3_adapter_strict_strings_by_default = true + end else raise "Unknown version #{target_version.to_s.inspect}" end diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt index d033659b55f..86a4196aa80 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt +++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt @@ -35,3 +35,12 @@ # state which matches what was committed to the database, typically the last # instance to save. # Rails.application.config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = false + +# Configures SQLite with a strict strings mode, which disables double-quoted string literals. +# +# SQLite has some quirks around double-quoted string literals. +# It first tries to consider double-quoted strings as identifier names, but if they don't exist +# it then considers them as string literals. Because of this, typos can silently go unnoticed. +# For example, it is possible to create an index for a non existing column. +# See https://www.sqlite.org/quirks.html#double_quoted_string_literals_are_accepted for more details. +# Rails.application.config.active_record.sqlite3_adapter_strict_strings_by_default = true diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 5861ac19ee9..7fe12628e24 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -2408,6 +2408,40 @@ module ApplicationTests assert_equal false, ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction end + test "SQLite3Adapter.strict_strings_by_default is true by default for new apps" do + app_file "config/initializers/active_record.rb", <<~RUBY + ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:") + RUBY + + app "development" + + assert_equal true, ActiveRecord::ConnectionAdapters::SQLite3Adapter.strict_strings_by_default + end + + test "SQLite3Adapter.strict_strings_by_default is false by default for upgraded apps" do + remove_from_config '.*config\.load_defaults.*\n' + app_file "config/initializers/active_record.rb", <<~RUBY + ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:") + RUBY + + app "development" + + assert_equal false, ActiveRecord::ConnectionAdapters::SQLite3Adapter.strict_strings_by_default + end + + test "SQLite3Adapter.strict_strings_by_default can be configured via config.active_record.sqlite3_adapter_strict_strings_by_default" do + remove_from_config '.*config\.load_defaults.*\n' + add_to_config "config.active_record.sqlite3_adapter_strict_strings_by_default = true" + + app_file "config/initializers/active_record.rb", <<~RUBY + ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:") + RUBY + + app "development" + + assert_equal true, ActiveRecord::ConnectionAdapters::SQLite3Adapter.strict_strings_by_default + end + test "ActiveSupport::MessageEncryptor.use_authenticated_message_encryption is true by default for new apps" do app "development"