Support Rails 7.0 #26

lorint · 2023-02-02T10:34:26Z

Based on feedback from @bensheldon and @matthewd in #20, here is a new PR to bring support for ActiveRecord 7.0 to ARTA. Key goals:

Focus only on Rails 7.0
Change the adapter's interface/implementation using ActiveRecord version checks, and not feature detection

dhruvCW

Thank you so much for doing this, and apologies for the intrusion, just figured I could leave some comments that could help speed up the process a bit 😁 👍

activerecord-trilogy-adapter.gemspec

lib/active_record/connection_adapters/trilogy_adapter.rb

lib/trilogy_adapter/errors.rb

lorint · 2023-02-09T09:47:32Z

... comments that could help speed up the process a bit 😁 👍

Thank you, Dhruv! I'll take time later this evening to implement your suggestions. I'm just one time zone away -- Nottingham, UK -- so should be updated by early tomorrow morning your time.

dhruvCW · 2023-02-10T08:59:00Z

@lorint that looks awesome 🍻 , I think following the same thought process of splitting controversial changes into their own commits could you split code related to @verified flag out into it's own commit ?

lorint · 2023-02-10T10:14:34Z

lib/active_record/connection_adapters/trilogy_adapter.rb

+          end
+        end
+
+        def execute(sql, name = nil, **kwargs)


Allows this example to pass:

execute_uses_AbstractAdapter#transform_query_when_available

lorint · 2023-02-10T10:15:05Z

lib/active_record/connection_adapters/trilogy_adapter.rb

+          self
+        end
+
+        def reconnect!


This in conjunction with the way reset! works allows these three examples to pass:

#reset_closes_connection_with_existing_connection
#reset_answers_new_connection_with_existing_connection
#reset_answers_new_connection_without_existing_connection

dhruvCW · 2023-02-10T09:09:34Z

lib/active_record/connection_adapters/trilogy_adapter.rb

+        def exec_rollback_db_transaction
+          # 16384 tests the bit flag for SERVER_SESSION_STATE_CHANGED, which gets set when the
+          # last statement executed has caused a change in the server's state.
+          if active? || (@raw_connection.server_status & 16_384).positive?


Q: what is the reason for checking the server status ? IMO rollback should be a no-op if the server doesn't have any modifications right ?

After careful analysis, I've found that this check for session state changed becomes useful if we were to support AR 6.1 and older, so it could be removed -- will drop in a commit for this later this evening.

(I do appreciate your interest in being thorough!)

lorint · 2023-02-10T10:04:18Z

could you split code related to @verified flag out into it's own commit ?

Some of the key reasons that Trilogy is interesting boils down to its reliable nature under load -- specifically how it does auto-reconnects. Much of this behaviour is possible due to AR 7.1's new @verified flag, along with other related enhancements in ConnectionAdapters. This is one reason that both Github and Shopify run their environments on Edge Rails, and have worked together to make Trilogy great. If we remove @verified then it would change quite a bit of the fundamental nature of this driver, and require that a lot of test cases be skipped -- essentially everything that has to do with auto-reconnect.

To create a more "vanilla" AR 7.0 option, I would recommend a fork of this driver where all of those enhancements are removed, paring back the majority of the auto-reconnect tests. Would end up being quite a bit more trim, and likely run just fine in smaller shops -- places that don't use Percona / Vitess / etc. I could create this kind of "ARTA Lite for AR 7.0" version if that sounds interesting.

lorint · 2023-02-10T10:14:34Z

lib/active_record/connection_adapters/trilogy_adapter.rb

+          end
+        end
+
+        def execute(sql, name = nil, **kwargs)


Allows this example to pass:

execute_uses_AbstractAdapter#transform_query_when_available

lorint · 2023-02-10T10:15:05Z

lib/active_record/connection_adapters/trilogy_adapter.rb

+          self
+        end
+
+        def reconnect!


This in conjunction with the way reset! works allows these three examples to pass:

#reset_closes_connection_with_existing_connection
#reset_answers_new_connection_with_existing_connection
#reset_answers_new_connection_without_existing_connection

dhruvCW · 2023-02-10T10:17:21Z

Some of the key reasons that Trilogy is interesting boils down to its reliable nature under load -- specifically how it does auto-reconnects. Much of this behaviour is possible due to AR 7.1's new @verified flag, along with other related enhancements in ConnectionAdapters. This is one reason that both Github and Shopify run their environments on Edge Rails, and have worked together to make Trilogy great. If we remove @verified then it would change quite a bit of the fundamental nature of this driver, and require that a lot of test cases be skipped -- essentially everything that has to do with auto-reconnect.

Don't get me wrong I would love to keep the PR as is, I just don't want this delayed 😅 , personally the value for me also comes from the lack of dependencies and async API but the reconnection logic is indeed a massive plus.

lorint · 2023-02-10T10:38:26Z

I just don't want this delayed

Love your zeal! Interested in having feedback from anyone willing to try this out in a QA or production environment. Anyone out there ready to put this in their Gemfile to give it a real-world spin? :)

gem 'activerecord-trilogy-adapter', git: 'https://github.com/lorint/activerecord-trilogy-adapter.git',
                                    ref: '17fba11ede4d5efdda3609af00fed85fdf5580dc'

dhruvCW · 2023-02-10T14:01:20Z

Love your zeal! Interested in having feedback from anyone willing to try this out in a QA or production environment. Anyone out there ready to put this in their Gemfile to give it a real-world spin? :)

gem 'activerecord-trilogy-adapter', git: 'https://github.com/lorint/activerecord-trilogy-adapter.git',
                                    branch: 'support_rails_7',
                                    ref: 'd132990800ca9196b758e54f37c522a888b396fc'

so funny thing, I tried bundle exec rails db:create on a mysql 5.7 server locally with root username and an empty password and it just failed with unknown db auth error 😅

I think I am missing some steps here

lorint · 2023-02-10T14:36:25Z

unknown db auth error

From terminal run mysql -uroot and then this, changing out cool_username and my_database:

CREATE USER cool_username@localhost IDENTIFIED BY '';
CREATE DATABASE my_database;
GRANT ALL PRIVILEGES ON my_database.* TO cool_username@localhost;
QUIT

Change the username and database name to be what you have in your database.yml, and no need to do a db:create, only db:migrate / etc. From there it's likely that all will be well 😃

dhruvCW · 2023-02-10T14:43:32Z

From terminal run mysql -uroot and then this, changing out cool_username and my_database:

thanks but does this mean trilogy cannot be used to create a database and load a schema ?

PS: seems to only fail in db:create otherwise everything else seems to work just fine, would be interesting to know what database creation is not supported though not really a pressing issue 😁

lorint · 2023-02-10T14:56:32Z

trilogy cannot be used to create a database

Trilogy can be used, sure -- this is more of a MySQL configuration thing we're talking about, so would recommend moving this convo to Rails discussion -- create a topic out here and we can find a solution. I'll make a quick screencast about how to configure your root user so that you can do a db:create:
https://discuss.rubyonrails.org/

lib/active_record/connection_adapters/trilogy_adapter.rb

lorint · 2023-04-26T10:22:25Z

@matthewd and @bensheldon -- what do you think about this commit which now converts this gem into being solely a backwards-compaible offering for Rails 7.0?

bensheldon

@lorint I checked with my team and we like your suggestion of focusing this gem on versions of Rails < 7.1. I made a few suggestions for being clear on which versions those specifically are at this point in time. And let's do it!

Thank you, again, for your continued work on this 🙇🏻

bensheldon · 2023-05-05T00:22:07Z

CHANGELOG.md

@@ -5,6 +5,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).

 ## [Unreleased]

+### Added
+
+- Support Rails 7.0


We should probably change this to be clearer of "Removed support for Rails 7.1"

bensheldon · 2023-05-05T00:25:35Z

activerecord-trilogy-adapter.gemspec

@@ -19,7 +19,7 @@ Gem::Specification.new do |spec|
  }

  spec.add_dependency "trilogy", ">= 2.3.0"
-  spec.add_dependency "activerecord", "~> 7.1.a"
+  spec.add_dependency "activerecord", "< 7.1a"


Suggested change

spec.add_dependency "activerecord", "< 7.1a"

spec.add_dependency "activerecord", ">= 7.0", "< 7.1a"

bensheldon · 2023-05-05T00:26:29Z

README.md

@@ -2,11 +2,13 @@

 Active Record database adapter for [Trilogy](https://github.com/github/trilogy)

+(This gem offers Trilogy support for versions of ActiveRecord prior to 7.1.)


Suggested change

(This gem offers Trilogy support for versions of ActiveRecord prior to 7.1.)

This gem offers Trilogy support for versions of ActiveRecord prior to 7.1. Currently supports:

- Rails v7.0.x

lorint · 2023-05-05T07:14:47Z

let's do it!

Yay!

Big thanks out to you and the team for all the support along the way. Thrilled that backwards-compatibility for this adapter will be available at the same time that it is formally introduced in 7.1!

… Trilogy

… class

bensheldon

I have reviewed this with @composerinteralia.

I am seeing 2 test failures locally, @composerinteralia is seeing one. We believe they are related to our local mysql configuration and not inherent in the library (CI is passing). To document them here:

❯ bundle exec rake
Run options: --seed 35915

# Running:

.......................E..........F........................................................

Finished in 2.922064s, 31.1424 runs/s, 57.1514 assertions/s.

  1) Error:
ActiveRecord::ConnectionAdapters::TrilogyAdapterTest#test_strict_mode_can_be_disabled:
ActiveRecord::NotNullViolation: Trilogy::ProtocolError: 1364: Field 'body' doesn't have a default value
    /Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/lib/active_record/connection_adapters/trilogy_adapter.rb:179:in `query'
    /Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/lib/active_record/connection_adapters/trilogy_adapter.rb:179:in `block (2 levels) in raw_execute'
    /Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/lib/active_record/connection_adapters/trilogy_adapter.rb:169:in `block in with_trilogy_connection'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activesupport-7.0.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activesupport-7.0.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activesupport-7.0.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activesupport-7.0.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
    /Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/lib/active_record/connection_adapters/trilogy_adapter.rb:166:in `with_trilogy_connection'
    /Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/lib/active_record/connection_adapters/trilogy_adapter.rb:177:in `block in raw_execute'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activesupport-7.0.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activesupport-7.0.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activesupport-7.0.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activesupport-7.0.5/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activerecord-7.0.5/lib/active_record/connection_adapters/abstract_adapter.rb:769:in `block in log'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activesupport-7.0.5/lib/active_support/notifications/instrumenter.rb:24:in `instrument'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activerecord-7.0.5/lib/active_record/connection_adapters/abstract_adapter.rb:760:in `log'
    /Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/lib/active_record/connection_adapters/trilogy_adapter.rb:176:in `raw_execute'
    /Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/activerecord-7.0.5/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:200:in `execute'
    /Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/lib/active_record/connection_adapters/trilogy_adapter.rb:187:in `execute'
    /Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/test/lib/active_record/connection_adapters/trilogy_adapter_test.rb:728:in `block in <class:TrilogyAdapterTest>'

  2) Failure:
ActiveRecord::ConnectionAdapters::TrilogyAdapterTest#test_.new_client_on_access_denied_error [/Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/test/lib/active_record/connection_adapters/trilogy_adapter_test.rb:44]:
[ActiveRecord::DatabaseConnectionError] exception expected, not
Class: <Trilogy::QueryError>
Message: <"trilogy_auth_recv: TRILOGY_PROTOCOL_VIOLATION">
---Backtrace---
/Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/trilogy-2.4.1/lib/trilogy.rb:174:in `_initialize'
/Users/bensheldon/.rbenv/versions/3.0.6/lib/ruby/gems/3.0.0/gems/trilogy-2.4.1/lib/trilogy.rb:174:in `initialize'
/Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/lib/active_record/connection_adapters/trilogy_adapter.rb:82:in `new'
/Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/lib/active_record/connection_adapters/trilogy_adapter.rb:82:in `new_client'
/Users/bensheldon/Repositories/github/activerecord-trilogy-adapter/test/lib/active_record/connection_adapters/trilogy_adapter_test.rb:45:in `block (2 levels) in <class:TrilogyAdapterTest>'
---------------

91 runs, 167 assertions, 1 failures, 1 errors, 0 skips
rake aborted!
Command failed with status (1)
/Users/bensheldon/.rbenv/versions/3.0.6/bin/bundle:23:in `load'
/Users/bensheldon/.rbenv/versions/3.0.6/bin/bundle:23:in `<main>'
Tasks: TOP => default => test
(See full trace by running task with --trace)

We believe this could be trilogy-libraries/trilogy#43

bensheldon · 2023-06-16T18:06:46Z

This PR has been released: https://github.com/trilogy-libraries/activerecord-trilogy-adapter/releases/tag/v3.0.0

@lorint thank you 🎉

lorint · 2023-06-19T21:15:03Z

This is fantastic!

If there is interest, I've put together a PR with the minor tidbits necessary to offer AR 6.1 and 6.0 support.

Looking forward to seeing many folks use Trilogy in their projects.

lorint requested a review from a team as a code owner February 2, 2023 10:34

lorint force-pushed the support_rails_7 branch from 843cb56 to 9a512d8 Compare February 2, 2023 10:56

lorint added 2 commits February 2, 2023 18:10

Allow query_transformers to be called with 1 or 2 arguments

e93f61b

Support Rails 7.0

b289e39

lorint force-pushed the support_rails_7 branch from 9a512d8 to b289e39 Compare February 2, 2023 18:10

dhruvCW reviewed Feb 9, 2023

View reviewed changes

lorint added 2 commits February 10, 2023 05:09

General updates from dhruvCW's code review

883bb96

active? update from dhruvCW's code review

f27aabf

lorint force-pushed the support_rails_7 branch from 44d7c1d to d132990 Compare February 10, 2023 05:09

dhruvCW reviewed Feb 10, 2023

View reviewed changes

lorint commented Feb 10, 2023

View reviewed changes

lorint force-pushed the support_rails_7 branch from 8290f02 to a11db6b Compare February 21, 2023 15:36

Error updates from dhruvCW's code review

e020d1e

lorint force-pushed the support_rails_7 branch from a11db6b to 0b51bdc Compare February 21, 2023 15:46

Merge branch 'main' into support_rails_7

ef73403

lorint force-pushed the support_rails_7 branch from 0b51bdc to ef73403 Compare February 21, 2023 16:32

matthewd reviewed Feb 22, 2023

View reviewed changes

lib/active_record/connection_adapters/trilogy_adapter.rb Show resolved Hide resolved

lib/active_record/connection_adapters/trilogy_adapter.rb Outdated Show resolved Hide resolved

lib/active_record/connection_adapters/trilogy_adapter.rb Outdated Show resolved Hide resolved

olefriis reviewed Feb 22, 2023

View reviewed changes

lib/active_record/connection_adapters/trilogy_adapter.rb Outdated Show resolved Hide resolved

More appropriate update for default_timezone

a8cdec6

lorint force-pushed the support_rails_7 branch from e497391 to e18bdf4 Compare February 23, 2023 00:02

Move into AR 7.0 compatibility

09c971b

lorint force-pushed the support_rails_7 branch from 4cb0096 to 38d97da Compare March 7, 2023 08:14

Merge branch 'main' into support_rails_7

17fba11

lorint force-pushed the support_rails_7 branch from 38d97da to 17fba11 Compare March 16, 2023 16:50

lorint force-pushed the support_rails_7 branch 4 times, most recently from 56a0226 to 07d70e8 Compare April 26, 2023 10:17

bensheldon reviewed May 5, 2023

View reviewed changes

lorint force-pushed the support_rails_7 branch from ff981ba to 2c055db Compare May 5, 2023 07:07

lorint force-pushed the support_rails_7 branch 7 times, most recently from 812e8a2 to 87e4d02 Compare May 9, 2023 08:48

Transition this gem into offering backwards-compatible AR support for…

370c5e2

… Trilogy

lorint force-pushed the support_rails_7 branch from 87e4d02 to 4528508 Compare May 31, 2023 21:36

lorint added 4 commits June 1, 2023 10:05

Remove tests and features specific to ActiveRecord 7.1

4340078

Integrate all backwards compatibility methods into the TrilogyAdapter…

7ffcece

… class

Utilise only ActiveRecord#default_timezone

518ceab

Updates from @bensheldon's code review

2867e28

lorint force-pushed the support_rails_7 branch from 4528508 to 2867e28 Compare June 1, 2023 09:06

bensheldon approved these changes Jun 16, 2023

View reviewed changes

bensheldon merged commit 7976c68 into trilogy-libraries:main Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Rails 7.0 #26

Support Rails 7.0 #26

lorint commented Feb 2, 2023

dhruvCW left a comment

lorint commented Feb 9, 2023

dhruvCW commented Feb 10, 2023

lorint Feb 10, 2023 •

edited

Loading

lorint Feb 10, 2023

dhruvCW Feb 10, 2023

lorint Feb 10, 2023

lorint commented Feb 10, 2023 •

edited

Loading

lorint Feb 10, 2023 •

edited

Loading

lorint Feb 10, 2023

dhruvCW commented Feb 10, 2023

lorint commented Feb 10, 2023 •

edited

Loading

dhruvCW commented Feb 10, 2023

lorint commented Feb 10, 2023 •

edited

Loading

dhruvCW commented Feb 10, 2023 •

edited

Loading

lorint commented Feb 10, 2023

lorint commented Apr 26, 2023

bensheldon left a comment

bensheldon May 5, 2023

bensheldon May 5, 2023

bensheldon May 5, 2023

lorint commented May 5, 2023

bensheldon left a comment

bensheldon commented Jun 16, 2023

lorint commented Jun 19, 2023 •

edited

Loading

	spec.add_dependency "activerecord", "< 7.1a"
	spec.add_dependency "activerecord", ">= 7.0", "< 7.1a"

		@@ -2,11 +2,13 @@

		Active Record database adapter for [Trilogy](https://github.com/github/trilogy)

		(This gem offers Trilogy support for versions of ActiveRecord prior to 7.1.)

-(This gem offers Trilogy support for versions of ActiveRecord prior to 7.1.)
+This gem offers Trilogy support for versions of ActiveRecord prior to 7.1. Currently supports:
+- Rails v7.0.x

Support Rails 7.0 #26

Support Rails 7.0 #26

Conversation

lorint commented Feb 2, 2023

dhruvCW left a comment

Choose a reason for hiding this comment

lorint commented Feb 9, 2023

dhruvCW commented Feb 10, 2023

lorint Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

lorint Feb 10, 2023

Choose a reason for hiding this comment

dhruvCW Feb 10, 2023

Choose a reason for hiding this comment

lorint Feb 10, 2023

Choose a reason for hiding this comment

lorint commented Feb 10, 2023 • edited Loading

lorint Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

lorint Feb 10, 2023

Choose a reason for hiding this comment

dhruvCW commented Feb 10, 2023

lorint commented Feb 10, 2023 • edited Loading

dhruvCW commented Feb 10, 2023

lorint commented Feb 10, 2023 • edited Loading

dhruvCW commented Feb 10, 2023 • edited Loading

lorint commented Feb 10, 2023

lorint commented Apr 26, 2023

bensheldon left a comment

Choose a reason for hiding this comment

bensheldon May 5, 2023

Choose a reason for hiding this comment

bensheldon May 5, 2023

Choose a reason for hiding this comment

bensheldon May 5, 2023

Choose a reason for hiding this comment

lorint commented May 5, 2023

bensheldon left a comment

Choose a reason for hiding this comment

bensheldon commented Jun 16, 2023

lorint commented Jun 19, 2023 • edited Loading

lorint Feb 10, 2023 •

edited

Loading

lorint commented Feb 10, 2023 •

edited

Loading

lorint Feb 10, 2023 •

edited

Loading

lorint commented Feb 10, 2023 •

edited

Loading

lorint commented Feb 10, 2023 •

edited

Loading

dhruvCW commented Feb 10, 2023 •

edited

Loading

lorint commented Jun 19, 2023 •

edited

Loading