Support Rails 6.1 and 7.0 (DNM -- for reference only) #20

lorint · 2022-12-23T02:35:50Z

Seeing the performance advantages of the Trilogy driver compared with mysql2 has motivated me to make it compatible with older Rails versions. I'm hoping it can become the MySQL driver of choice in the Rails community. After trying several approaches, have ended up with the solution you see in this PR.

ActiveRecord Trilogy Adapter was originally written with a forward-thinking mind set -- one exclusive to Rails 7.1 and later. The older database adapter framework found in prior versions of Rails differs a fair bit from that found in 7.1, so there are some core differences to bridge in order to have Trilogy become compatible with prior Rails versions. This PR adds backwards compatibility in such a way that it does not interfere with the lean nature of how AR Trilogy has been architected -- it adds that compatibility layer via a series of patches all tucked away in a separate file. Inside that compatibility file, backwards_ar_compatibility.rb, checks are done to identify which newer methods are missing. They are selectively added back in, with the ultimate goal of establishing the broadest compatibility possible. Only those patches which are necessary for any given Rails version get applied, making it minimally invasive.

This approach requires no changes to Trilogy itself, so it functions in exactly the same way as it always has done -- as if everything is just ActiveRecord 7.1+. As such newer Rails 7.1 apps are completely unaffected because for them none of that patching needs be applied.

Eager to hear what you guys think!

lorint · 2022-12-24T09:41:57Z

Thank you @matthewd for triggering that first test run -- have fixed those two failures by simplifying how the connection gets made.

Seeing the performance advantages of the Trilogy driver compared with mysql2 has motivated me to make it compatible with older Rails versions. I'm hoping it can become the MySQL driver of choice in the Rails community. After trying several approaches, have ended up with the solution you see in this PR. **ActiveRecord Trilogy Adapter** was originally written with a forward-thinking mind set -- one exclusive to Rails 7.1 and later. The older database adapter framework found in prior versions of Rails differs a fair bit from that found in 7.1, so there are some core differences to bridge in order to have Trilogy become compatible with prior Rails versions. This PR adds backwards compatibility in such a way that it does not interfere with the lean nature of how AR Trilogy has been architected -- it adds that compatibility layer via a series of patches all tucked away in a separate file. Inside that compatibility file, backwards_ar_compatibility.rb, checks are done to identify which newer methods are missing. They are selectively added back in, with the ultimate goal of establishing the broadest compatibility possible. Only those patches which are necessary for any given Rails version get applied, making it minimally invasive. This approach requires no changes to Trilogy itself, so it functions in exactly the same way as it always has done -- as if everything is just ActiveRecord 7.1+. As such newer Rails 7.1 apps are completely unaffected because for them none of that patching needs be applied.

You're likely familiar with how to set up and run the tests for this project -- after confirming that inside of `bin/setup` on the last line it shows the same path out to `mysql` as you see when running `which mysql` then to test against edge Rails here are the commands: ``` bin/setup bundle exec rake ``` And once you've done that, which configures the test database appropriately then thankfully it's pretty easy to re-bundle in order to then run the tests under an older version: ``` rm Gemfile.lock RAILS_VERSION="~> 6.1" bundle RAILS_VERSION="~> 6.1" bundle exec rake ```

lorint · 2022-12-28T20:01:19Z

As to be expected, no test failures when running under ActiveRecord 7.1 as this update is inactive in that environment. When running under previous versions then the compatibility layer does get utilised. Here are the counts of errors and failures for the most recent release of each major AR version:

AR Version	failures	errors	skips
edge	0	0	0
7.0.4	1	3	0
6.1.7	2	5	2
6.0.6	2	8	2
5.2.8.1	11	8	2
5.1.7	11	13	2

bensheldon · 2022-12-30T18:02:17Z

@lorint Thanks for opening this PR! Our team will be able to do a better review of this after the New Years. I just wanted to give some quick feedback that I think would help expedite this work:

I think 7.0 compatibility would take precedence, and it might be nice to split that out into a focused PR that we can review/approve before tackling older versions.
I think we also need to have a multi-version local/CI test strategy and tooling. I have experience with Appraisal (on GoodJob) to make it easy to generate multiple gemfiles. I can work with you on that unless you (or someone else on my team) has stronger opinions.

btw, I also saw your post on Rails Discuss; thank you again!

lorint · 2022-12-31T21:14:53Z

7.0 compatibility would take precedence

Have a multi-version local/CI test strategy and tooling

Sounds well, @bensheldon -- have now implemented Appraisal in the same way that you had done for GoodJob. Haven't touched the ci.yml file yet, and would be interested to have you or someone else familiar with GHA approach that one.

Have also pulled out support for 5.1 / 5.2 / 6.0. Being able to fix a couple more failures for 6.1 for the moment I've chosen to leave that in place, and let me know if you feel that should be a part of a different PR. Here's the current counts of errors / fails / skips:

AR Version	failures	errors	skips
unreleased	0	0	0
7.0.4	1	3	0
6.1.7	2	2	2

Getting down to the point that what's left are the more gnarly failures -- and overall they seem pretty inconsequential since Rails < 7.1 never had the connection verify logic in place, so even as it stands in some ways it's already stronger than just plain Rails would otherwise be. It only adds the 7.1 things for Trilogy and not for any other connection adapters.

Hope you folks are having a great end-of-year break, and eager to be back in touch when the team reconvenes!

bensheldon · 2023-01-04T00:31:35Z

@lorint thank you! And sorry, I realized now I sent you in one misdirection: there's already a ENV-based mechanism for switching the ActiveRecord version; I overlooked that so we don't need Appraisal here. I'll push up a commit to do that and update the CI to add the additional versions to the matrix.

Thank you for focusing on the more recent versions of Rails too. I'm still trying to draw up my engineers to take a look at this.

lorint · 2023-01-04T12:17:13Z

there's already a ENV-based mechanism for switching the ActiveRecord version

Cool! That does make it easy.

I've got some time tomorrow to try fixing the remaining test failures under 7.0 / 6.1 -- perhaps we won't need to put in any test skips. I don't think any of the current failures should be showstoppers anyway, just in case we do need to put in skips.

Set QUERY_FLAGS_LOCAL_TIMEZONE properly which should allow these two to pass: #test_query_flags_for_timezone_can_be_set_to_local #test_query_flags_for_timezone_can_be_set_to_local_and_reset_to_utc

lorint · 2023-01-14T12:50:20Z

With this latest commit all examples should pass for AR 7.1 / 7.0 / 6.1.

Also have a set of patches ready for AR 6.0 that pass all examples, but there's more missing bits that need to be brought in for that kind of compatibility, so it adds another 80 lines. It is a currently supported Rails version, so in that light perhaps it's tempting to include. I'm totally onboard with providing older compatibility for the v2.2.0 release of this ARTA gem, and then removing all patches older than AR 6.1 for v2.3.0. This would make it easy for folks who want that older 6.0 compatibility to just bundle in v2.2.0 of ARTA.

Speaking of further backwards compatibility, after adding in the AR 6.0 patches then from there it is possible to further add support for 5.1 and 5.2, which requires about another 100 lines. Ends up being very functional, but does not pass a few examples. (Pretty tough to fully back-port all the synchronize / auto-reconnect stuff from AR 7.1 into 5.x.)

With all the patches enabled -- so that full additional 180 lines of stuff -- here's the current failure matrix:

AR Version	failures	errors	skips
edge	0	0	0
7.0.4	0	0	0
6.1.7	0	0	2
6.0.6	0	0	2
5.2.8.1	3	1	2
5.1.7	5	2	2

Alternatively could release all these 5.x / 6.0 patches as a separate "Trilogy compatibility" gem for anyone running those unsupported / nearly-unsupported versions of Rails. (sure, 6.0 is still supported, but for only another 4 months!) Would just ask that in the .gemspec for ARTA 2.2.0 it allows AR 5.0 and up. Otherwise such a compatibility gem would have to be a full fork of ARTA and not an add-on kind of thing.

Just to be totally clear here -- the failure matrix for the current code in this branch is for the first 3 lines of the table above -- zero failures for 6.1 / 7.0 / edge. The last 3 lines listed there are only opened up after adding another ~180 lines of various patches.

There is a new generic Trilogy::ClientError class established with this PR which is useful to provide full AR 7.0 support: trilogy-libraries/trilogy#15

bensheldon · 2023-01-25T18:59:05Z

@lorint I did a review of this PR with @matthewd yesterday and we have some feedback:

Thank you for looking into this and really reducing the problem-space down to these compatibility issues. Super helpful! 🙇🏻
For the following, we want to put the work into a separate PR and keep this PR solely for reference.
We don't want to do Active Record feature-patching. If other gems are using feature-detection (or their own feature patching) we're worried that it would be bad/unmaintainable.
The strategy we'd like to go with is: Changing this adapter's interface/implementation using version checks (not feature detection) and not patching Active Record.
We'd like to do that solely with a focus on Rails 7.0 in a separate PR and work through any feedback issues before considering any versions of Rails prior to that.

lorint · 2023-01-26T10:17:25Z

We'd like to (change this adapter's interface/implementation using version checks), solely with a focus on Rails 7.0 in a separate PR ...

Cool -- thanks @bensheldon and @matthewd for taking a look. I think it's great to have compatibility be more intrinsic to ARTA, so certainly this works. I recognise that supporting at least 7.0 will be vital for community adoption.

You might have sensed after looking through this PR that it is useful to implement the new AR 7.1 @verified flag, which is found as a part of ActiveRecord::ConnectionAdapters::AbstractAdapter. Of everything I had done this change was the most impactful -- it allows reconnection attempts work in the same way as AR 7.1. Ended up being the very most useful thing to start with in the journey. I expect if you can implement @verified early then everything else is more straightforward; really from there it becomes a bunch of minor details to sort out.

If you'd like, I have some time this weekend where I could start a new PR and take a stab at implementing @verified to see what you think.

…PR#24

lorint requested a review from a team as a code owner December 23, 2022 02:35

lorint force-pushed the support_rails_6_and_7 branch from 80e1597 to 5107ecc Compare December 23, 2022 02:37

lorint mentioned this pull request Dec 23, 2022

Prepared statements issue with Rails main and MySQL 5.7.39 #5

Closed

lorint force-pushed the support_rails_6_and_7 branch 8 times, most recently from c365860 to d678672 Compare December 24, 2022 09:32

lorint force-pushed the support_rails_6_and_7 branch 11 times, most recently from 15c962b to 84a8908 Compare December 27, 2022 17:21

lorint added 2 commits December 28, 2022 19:55

lorint force-pushed the support_rails_6_and_7 branch from 84a8908 to 59bec56 Compare December 28, 2022 20:00

lorint force-pushed the support_rails_6_and_7 branch from 197fc14 to 7b38987 Compare December 31, 2022 20:52

lorint changed the title ~~Support Rails 5.2, 6.x, and 7.0~~ Support Rails 6.1 and 7.0 Dec 31, 2022

lorint force-pushed the support_rails_6_and_7 branch from 7b38987 to 787e6d9 Compare December 31, 2022 21:14

Remove Appraisals, add additional versions to CI matrix

8672123

A couple small Appraisal gem leftovers

411301d

lorint force-pushed the support_rails_6_and_7 branch from 9d4abd2 to e9035c3 Compare January 6, 2023 17:15

Fix query_flags timezone test failures under AR 6.1

9cc4a25

Set QUERY_FLAGS_LOCAL_TIMEZONE properly which should allow these two to pass: #test_query_flags_for_timezone_can_be_set_to_local #test_query_flags_for_timezone_can_be_set_to_local_and_reset_to_utc

lorint force-pushed the support_rails_6_and_7 branch 4 times, most recently from b027bc8 to c54f95b Compare January 12, 2023 18:21

lorint mentioned this pull request Jan 13, 2023

Refactor error classes trilogy-libraries/trilogy#15

Merged

Fix test failures for AR 6.1 and AR 7.0

be7812e

lorint force-pushed the support_rails_6_and_7 branch from 946b401 to be7812e Compare January 13, 2023 17:54

lorint added 2 commits January 13, 2023 17:54

Merge branch 'github:main' into support_rails_6_and_7

bc22c89

Cause #execute_fails_if_the_connection_is_closed to pass under MySQL 5.7

533e425

lorint force-pushed the support_rails_6_and_7 branch from ba2d4dc to 533e425 Compare January 14, 2023 09:54

Compatibility for updated error classes in Trilogy driver from PR#15

711a689

There is a new generic Trilogy::ClientError class established with this PR which is useful to provide full AR 7.0 support: trilogy-libraries/trilogy#15

lorint force-pushed the support_rails_6_and_7 branch from 8b4b78d to 86fb82e Compare January 16, 2023 18:40

Some good Rubocop love

9f60801

lorint force-pushed the support_rails_6_and_7 branch from 86fb82e to 9f60801 Compare January 24, 2023 18:30

Merge branch 'main' - Thank you, Adrianna for updates to errors from …

8674e31

…PR#24

lorint force-pushed the support_rails_6_and_7 branch from d0b39c3 to 8674e31 Compare February 2, 2023 09:38

lorint mentioned this pull request Feb 2, 2023

Support Rails 7.0 #26

Merged

lorint changed the title ~~Support Rails 6.1 and 7.0~~ Support Rails 6.1 and 7.0 (DNM -- for reference only) Feb 2, 2023

bensheldon closed this Jun 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Rails 6.1 and 7.0 (DNM -- for reference only) #20

Support Rails 6.1 and 7.0 (DNM -- for reference only) #20

lorint commented Dec 23, 2022 •

edited

lorint commented Dec 24, 2022

lorint commented Dec 28, 2022

bensheldon commented Dec 30, 2022 •

edited

lorint commented Dec 31, 2022

bensheldon commented Jan 4, 2023

lorint commented Jan 4, 2023 •

edited

lorint commented Jan 14, 2023 •

edited

bensheldon commented Jan 25, 2023

lorint commented Jan 26, 2023 •

edited

Support Rails 6.1 and 7.0 (DNM -- for reference only) #20

Support Rails 6.1 and 7.0 (DNM -- for reference only) #20

Conversation

lorint commented Dec 23, 2022 • edited

lorint commented Dec 24, 2022

lorint commented Dec 28, 2022

bensheldon commented Dec 30, 2022 • edited

lorint commented Dec 31, 2022

bensheldon commented Jan 4, 2023

lorint commented Jan 4, 2023 • edited

lorint commented Jan 14, 2023 • edited

bensheldon commented Jan 25, 2023

lorint commented Jan 26, 2023 • edited

lorint commented Dec 23, 2022 •

edited

bensheldon commented Dec 30, 2022 •

edited

lorint commented Jan 4, 2023 •

edited

lorint commented Jan 14, 2023 •

edited

lorint commented Jan 26, 2023 •

edited