Skip to content

Comments

Add SQLite3 support for supports_insert_returning?#49290

Merged
tenderlove merged 1 commit intorails:mainfrom
fractaledmind:ar-sqlite3-returning-columns
Sep 20, 2023
Merged

Add SQLite3 support for supports_insert_returning?#49290
tenderlove merged 1 commit intorails:mainfrom
fractaledmind:ar-sqlite3-returning-columns

Conversation

@fractaledmind
Copy link
Contributor

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 SQLite3Adapter feature-comparable with the MySQLAdapter and PostgreSQLAdapters.

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 the RETURNING keyword, 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 SQLite3Adapter by implementing:

  • ActiveRecord::ConnectionAdapters::SQLite3::DatabaseStatements#sql_for_insert
  • ActiveRecord::ConnectionAdapters::SQLite3::DatabaseStatements#returning_column_values
  • ActiveRecord::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:

  • 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 fractaledmind force-pushed the ar-sqlite3-returning-columns branch from 80f2a27 to 77c7f38 Compare September 15, 2023 17:58
@flavorjones
Copy link
Member

Looks like this includes the changes from #49287 so reviewers should focus on that one first.

@fractaledmind fractaledmind force-pushed the ar-sqlite3-returning-columns branch 2 times, most recently from 6b1e244 to 9ee7e9a Compare September 18, 2023 09:00
Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

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

Nice work on this!

@fractaledmind
Copy link
Contributor Author

Nice work on this!

@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.

Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

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

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 :)

@fractaledmind fractaledmind force-pushed the ar-sqlite3-returning-columns branch from fb9f107 to c21a5f6 Compare September 19, 2023 19:19
@fractaledmind fractaledmind force-pushed the ar-sqlite3-returning-columns branch 3 times, most recently from 3f68298 to 0f67dd4 Compare September 20, 2023 07:34
@fractaledmind fractaledmind force-pushed the ar-sqlite3-returning-columns branch from 0f67dd4 to f0452fa Compare September 20, 2023 13:07
@fractaledmind fractaledmind force-pushed the ar-sqlite3-returning-columns branch from f0452fa to 151d427 Compare September 20, 2023 15:43
@tenderlove
Copy link
Member

Looks good, but buildkite is red. Can you resolve the conflicts and also check why the build is failing?

@rafaelfranca rafaelfranca added the ready PRs ready to merge label Sep 20, 2023
@guilleiguaran
Copy link
Member

#49287 is merged so this can be rebased to include it

@fractaledmind fractaledmind force-pushed the ar-sqlite3-returning-columns branch from 151d427 to c06b802 Compare September 20, 2023 17:23
Implementing the full `supports_insert_returning?` contract means the SQLite3 adapter supports auto-populated columns (rails#48241) as well as custom primary keys.
@fractaledmind fractaledmind force-pushed the ar-sqlite3-returning-columns branch from c06b802 to 9bbbf4b Compare September 20, 2023 17:35
@fractaledmind
Copy link
Contributor Author

@guilleiguaran: Done
@tenderlove: Resolved

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.

@tenderlove tenderlove merged commit 0f92f8b into rails:main Sep 20, 2023
@fractaledmind fractaledmind deleted the ar-sqlite3-returning-columns branch September 21, 2023 07:58
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jul 31, 2024
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
joelhawksley pushed a commit to joelhawksley/rails that referenced this pull request Aug 8, 2024
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
joelhawksley pushed a commit to joelhawksley/rails that referenced this pull request Aug 8, 2024
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
rossta added a commit to joyofrails/joyofrails.com that referenced this pull request Oct 3, 2024
…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.
rossta added a commit to joyofrails/joyofrails.com that referenced this pull request Oct 3, 2024
…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.
rossta added a commit to joyofrails/joyofrails.com that referenced this pull request Oct 3, 2024
…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.
DanielaVelasquez pushed a commit to DanielaVelasquez/rails that referenced this pull request Oct 3, 2024
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
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.

9 participants