Add SQLite3 support for supports_insert_returning?#49290
Add SQLite3 support for supports_insert_returning?#49290tenderlove merged 1 commit intorails:mainfrom
supports_insert_returning?#49290Conversation
80f2a27 to
77c7f38
Compare
|
Looks like this includes the changes from #49287 so reviewers should focus on that one first. |
6b1e244 to
9ee7e9a
Compare
adrianna-chang-shopify
left a comment
There was a problem hiding this comment.
Nice work on this!
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
Outdated
Show resolved
Hide resolved
9ee7e9a to
fb9f107
Compare
@adrianna-chang-shopify: Thank you for the kind words, and thank you even more for the great review. All points addressed, commits squashed, and new code force pushed and ready for review. |
adrianna-chang-shopify
left a comment
There was a problem hiding this comment.
I think we need to define a test table for use in activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb to fix those failing tests, but otherwise this looks good to me 👍 Someone from the Core team will need to take a look to merge tho :)
activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
Outdated
Show resolved
Hide resolved
fb9f107 to
c21a5f6
Compare
activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb
Outdated
Show resolved
Hide resolved
3f68298 to
0f67dd4
Compare
activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
Outdated
Show resolved
Hide resolved
0f67dd4 to
f0452fa
Compare
activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
Outdated
Show resolved
Hide resolved
f0452fa to
151d427
Compare
|
Looks good, but buildkite is red. Can you resolve the conflicts and also check why the build is failing? |
|
#49287 is merged so this can be rebased to include it |
151d427 to
c06b802
Compare
Implementing the full `supports_insert_returning?` contract means the SQLite3 adapter supports auto-populated columns (rails#48241) as well as custom primary keys.
c06b802 to
9bbbf4b
Compare
|
@guilleiguaran: Done Everything is green now. Thanks for the excellent reviews everyone. It was a joy to work with you all. In the coming weeks, I'll be working on adding support for virtual columns for the SQLite adapter as well. So, I'm looking forward to working with you all again relatively soon. |
It was introduced in rails#49290 likely to do the same as PostgresqlAdapter, but it's not used anywhere and there not really any reason to have that option. Postgres has it because INSERT RETURNING can't be used in some corner cases, see: rails#5698
It was introduced in rails#49290 likely to do the same as PostgresqlAdapter, but it's not used anywhere and there not really any reason to have that option. Postgres has it because INSERT RETURNING can't be used in some corner cases, see: rails#5698
It was introduced in rails#49290 likely to do the same as PostgresqlAdapter, but it's not used anywhere and there not really any reason to have that option. Postgres has it because INSERT RETURNING can't be used in some corner cases, see: rails#5698
…hancedsqlite3-adapter Most of the enhancements provided by activerecord-enhancedsqlite3-adapter have been upstreamed into Rails, including: * Add support for generated columns in SQLite3 adapter rails/rails#49346 * Add SQLite3 support for supports_insert_returning? rails/rails#49290 * Allow overriding SQLite defaults from database.yml rails/rails#50460 * Performance tune the SQLite3 adapter connection configuration rails/rails#49349 * Allow SQLite3 busy_handler to be configured with simple max number of retries rails/rails#49352 One enhancement Joy still needs is a way to load SQLite3 extensions. There is a sqlpkg gem that would provide this, which includes generating an initializer that overrides the Rails SQLite3 adapter. Instead, I‘ve just adapted the enhancedsqlite3-adapter code and droppped into the lib/ directory. The code here also knows how to load an extension by gem name which is makes it useful for the sqlite_ulid gem currently in use.
…hancedsqlite3-adapter Most of the enhancements provided by activerecord-enhancedsqlite3-adapter have been upstreamed into Rails, including: * Add support for generated columns in SQLite3 adapter rails/rails#49346 * Add SQLite3 support for supports_insert_returning? rails/rails#49290 * Allow overriding SQLite defaults from database.yml rails/rails#50460 * Performance tune the SQLite3 adapter connection configuration rails/rails#49349 * Allow SQLite3 busy_handler to be configured with simple max number of retries rails/rails#49352 One enhancement Joy still needs is a way to load SQLite3 extensions. There is a sqlpkg gem that would provide this, which includes generating an initializer that overrides the Rails SQLite3 adapter. Instead, I‘ve just adapted the enhancedsqlite3-adapter code and droppped into the lib/ directory. The code here also knows how to load an extension by gem name which is makes it useful for the sqlite_ulid gem currently in use.
…hancedsqlite3-adapter Most of the enhancements provided by activerecord-enhancedsqlite3-adapter have been upstreamed into Rails, including: * Add support for generated columns in SQLite3 adapter rails/rails#49346 * Add SQLite3 support for supports_insert_returning? rails/rails#49290 * Allow overriding SQLite defaults from database.yml rails/rails#50460 * Performance tune the SQLite3 adapter connection configuration rails/rails#49349 * Allow SQLite3 busy_handler to be configured with simple max number of retries rails/rails#49352 One enhancement Joy still needs is a way to load SQLite3 extensions. There is a sqlpkg gem that would provide this, which includes generating an initializer that overrides the Rails SQLite3 adapter. Instead, I‘ve just adapted the enhancedsqlite3-adapter code and droppped into the lib/ directory. The code here also knows how to load an extension by gem name which is makes it useful for the sqlite_ulid gem currently in use.
It was introduced in rails#49290 likely to do the same as PostgresqlAdapter, but it's not used anywhere and there not really any reason to have that option. Postgres has it because INSERT RETURNING can't be used in some corner cases, see: rails#5698
Implementing the full
supports_insert_returning?contract means the SQLite3 adapter supports auto-populated columns (#48241) as well as custom primary keys.Motivation / Background
Building on the background of #49287, this is my first major PR to begin making the
SQLite3Adapterfeature-comparable with theMySQLAdapterandPostgreSQLAdapters.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.
Detail
PR #48241 introduced the wonderful feature to have ActiveRecord models auto-populate their attributes with database defaults. Initially, this feature was only implemented for the
PostgreSQLAdapter. The essential SQL feature foundation for this behavior is use of theRETURNINGkeyword, which SQLite has supported since version 3.35.0 (2021-03-12) (see https://www.sqlite.org/lang_returning.html).In this PR, I am adding full support for the
SQLite3Adapterby implementing:ActiveRecord::ConnectionAdapters::SQLite3::DatabaseStatements#sql_for_insertActiveRecord::ConnectionAdapters::SQLite3::DatabaseStatements#returning_column_valuesActiveRecord::ConnectionAdapters::SQLite3Adapter#supports_insert_returning?ActiveRecord::ConnectionAdapters::SQLite3Adapter#return_value_after_insert?ActiveRecord::ConnectionAdapters::SQLite3Adapter#use_insert_returning?along with a couple of tweaks to a few methods.
Additional information
This builds on the work started in #49287
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]