-
Notifications
You must be signed in to change notification settings - Fork 57
Trilogy adapter support in Rails 8.1 #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
235b0d3
3a4eb44
580bc8f
0a35ff9
b276a0e
90426d5
a525937
fcabff4
6b38704
7b1112f
e0989bd
e38df06
efbb11f
1947f30
2aaef93
fa01812
d9f47fa
801e492
1e6470e
3cbb35c
edce126
2ab05a5
b315873
4e5b9b9
5602188
aad936b
41a1fd5
d2a2880
9fbc420
db7679d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,4 +10,5 @@ end | |
|
|
||
| appraise 'rails-8-1' do | ||
| gem 'rails', '8.1.1' | ||
| gem 'trilogy' | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,16 @@ | ||
| # This file was generated by Appraisal | ||
|
|
||
| source "https://rubygems.org" | ||
| source 'https://rubygems.org' | ||
|
|
||
| gem "base64" | ||
| gem "codeclimate-test-reporter", "~> 1.0.3", group: :test, require: nil | ||
| gem "lhm" | ||
| gem "logger" | ||
| gem "mutex_m", require: false | ||
| gem "rubocop", "~> 1.74.0", require: false | ||
| gem "rubocop-performance", "~> 1.20.2", require: false | ||
| gem "zeitwerk", "< 2.7.0" | ||
| gem "bigdecimal" | ||
| gem "rails", "7.2.2.1" | ||
| gem 'base64' | ||
| gem 'bigdecimal' | ||
| gem 'codeclimate-test-reporter', '~> 1.0.3', group: :test, require: nil | ||
| gem 'lhm' | ||
| gem 'logger' | ||
| gem 'mutex_m', require: false | ||
| gem 'rails', '7.2.2.1' | ||
| gem 'rubocop', '~> 1.74.0', require: false | ||
| gem 'rubocop-performance', '~> 1.20.2', require: false | ||
| gem 'zeitwerk', '< 2.7.0' | ||
|
|
||
| gemspec path: "../" | ||
| gemspec path: '../' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,16 @@ | ||
| # This file was generated by Appraisal | ||
|
|
||
| source "https://rubygems.org" | ||
| source 'https://rubygems.org' | ||
|
|
||
| gem "base64" | ||
| gem "codeclimate-test-reporter", "~> 1.0.3", group: :test, require: nil | ||
| gem "lhm" | ||
| gem "logger" | ||
| gem "mutex_m", require: false | ||
| gem "rubocop", "~> 1.74.0", require: false | ||
| gem "rubocop-performance", "~> 1.20.2", require: false | ||
| gem "zeitwerk", "< 2.7.0" | ||
| gem "bigdecimal" | ||
| gem "rails", "8.0.2.1" | ||
| gem 'base64' | ||
| gem 'bigdecimal' | ||
| gem 'codeclimate-test-reporter', '~> 1.0.3', group: :test, require: nil | ||
| gem 'lhm' | ||
| gem 'logger' | ||
| gem 'mutex_m', require: false | ||
| gem 'rails', '8.0.2.1' | ||
| gem 'rubocop', '~> 1.74.0', require: false | ||
| gem 'rubocop-performance', '~> 1.20.2', require: false | ||
| gem 'zeitwerk', '< 2.7.0' | ||
|
|
||
| gemspec path: "../" | ||
| gemspec path: '../' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,16 @@ | ||
| # This file was generated by Appraisal | ||
|
|
||
| source "https://rubygems.org" | ||
| source 'https://rubygems.org' | ||
|
|
||
| gem "base64" | ||
| gem "codeclimate-test-reporter", "~> 1.0.3", group: :test, require: nil | ||
| gem "lhm" | ||
| gem "logger" | ||
| gem "mutex_m", require: false | ||
| gem "rubocop", "~> 1.74.0", require: false | ||
| gem "rubocop-performance", "~> 1.20.2", require: false | ||
| gem "zeitwerk", "< 2.7.0" | ||
| gem "rails", "8.1.1" | ||
| gem 'base64' | ||
| gem 'codeclimate-test-reporter', '~> 1.0.3', group: :test, require: nil | ||
| gem 'lhm' | ||
| gem 'logger' | ||
| gem 'mutex_m', require: false | ||
| gem 'rails', '8.1.1' | ||
| gem 'rubocop', '~> 1.74.0', require: false | ||
| gem 'rubocop-performance', '~> 1.20.2', require: false | ||
| gem 'trilogy' | ||
| gem 'zeitwerk', '< 2.7.0' | ||
|
|
||
| gemspec path: "../" | ||
| gemspec path: '../' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,12 +6,12 @@ | |
|
|
||
| module ActiveRecord | ||
| module ConnectionAdapters | ||
| class Rails81DepartureAdapter < ActiveRecord::ConnectionAdapters::Mysql2Adapter | ||
| class Rails81Mysql2Adapter < ActiveRecord::ConnectionAdapters::Mysql2Adapter | ||
| TYPE_MAP = Type::TypeMap.new.tap { |m| initialize_type_map(m) } if defined?(initialize_type_map) | ||
|
|
||
| class Column < ActiveRecord::ConnectionAdapters::MySQL::Column | ||
| def adapter | ||
| Rails81DepartureAdapter | ||
| Rails81Mysql2Adapter | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -33,8 +33,6 @@ def visit_DropForeignKey(name) # rubocop:disable Naming/MethodName | |
| end | ||
| end | ||
|
|
||
| extend Forwardable | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we forget this here in previous PRs?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙈 yes my bad |
||
|
|
||
| include ForAlterStatements unless method_defined?(:change_column_for_alter) | ||
|
|
||
| ADAPTER_NAME = 'Percona'.freeze | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| require 'active_record/connection_adapters/abstract_mysql_adapter' | ||
| require 'active_record/connection_adapters/trilogy_adapter' | ||
| require 'active_record/connection_adapters/patch_connection_handling' | ||
| require 'departure' | ||
| require 'forwardable' | ||
|
|
||
| module ActiveRecord | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Core change - add rails 8.1 trilogy adapter |
||
| module ConnectionAdapters | ||
| class Rails81TrilogyAdapter < ActiveRecord::ConnectionAdapters::TrilogyAdapter | ||
| class Column < ActiveRecord::ConnectionAdapters::MySQL::Column | ||
| def adapter | ||
| Rails81TrilogyAdapter | ||
| end | ||
| end | ||
|
|
||
| # https://github.com/departurerb/departure/commit/f178ca26cd3befa1c68301d3b57810f8cdcff9eb | ||
| # For `DROP FOREIGN KEY constraint_name` with pt-online-schema-change requires specifying `_constraint_name` | ||
| # rather than the real constraint_name due to to a limitation in MySQL | ||
| # pt-online-schema-change adds a leading underscore to foreign key constraint names when creating the new table. | ||
| # https://www.percona.com/blog/2017/03/21/dropping-foreign-key-constraint-using-pt-online-schema-change-2/ | ||
| class SchemaCreation < ActiveRecord::ConnectionAdapters::MySQL::SchemaCreation | ||
| def visit_DropForeignKey(name) # rubocop:disable Naming/MethodName | ||
| fk_name = | ||
| if name =~ /^__(.+)/ | ||
| Regexp.last_match(1) | ||
| else | ||
| "_#{name}" | ||
| end | ||
|
|
||
| "DROP FOREIGN KEY #{fk_name}" | ||
| end | ||
| end | ||
|
|
||
| include ForAlterStatements unless method_defined?(:change_column_for_alter) | ||
|
|
||
| ADAPTER_NAME = 'Percona'.freeze | ||
|
|
||
| def self.new_client(config) | ||
| original_client = super | ||
|
|
||
| Departure::DbClient.new(config, original_client) | ||
| end | ||
|
|
||
| # add_index is modified from the underlying mysql adapter implementation to ensure we add ALTER TABLE to it | ||
| def add_index(table_name, column_name, options = {}) | ||
| index_definition, = add_index_options(table_name, column_name, **options) | ||
| execute <<-SQL.squish | ||
| ALTER TABLE #{quote_table_name(index_definition.table)} | ||
| ADD #{schema_creation.accept(index_definition)} | ||
| SQL | ||
| end | ||
|
|
||
| # remove_index is modified from the underlying mysql adapter implementation to ensure we add ALTER TABLE to it | ||
| def remove_index(table_name, column_name = nil, **options) | ||
| return if options[:if_exists] && !index_exists?(table_name, column_name, **options) | ||
|
|
||
| index_name = index_name_for_remove(table_name, column_name, options) | ||
|
|
||
| execute "ALTER TABLE #{quote_table_name(table_name)} DROP INDEX #{quote_column_name(index_name)}" | ||
| end | ||
|
|
||
| def schema_creation | ||
| SchemaCreation.new(self) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # rubocop:disable Metrics/ParameterLists | ||
| def perform_query(raw_connection, sql, binds, type_casted_binds, prepare:, notification_payload:, batch: false) | ||
| return raw_connection.send_to_pt_online_schema_change(sql) if raw_connection.alter_statement?(sql) | ||
|
|
||
| super | ||
| end | ||
| # rubocop:enable Metrics/ParameterLists | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,8 +23,6 @@ | |
| # We need the OS not to buffer the IO to see pt-osc's output while migrating | ||
| $stdout.sync = true | ||
|
|
||
| Departure::RailsAdapter.for_current.register_integrations | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this not needed anymore? Did this get moved to the railtie?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still tinkering but hoping to pull all the registration into the railtie, if not this will have to work and go back in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 on this, a railtie is the most appropriate place for registration. |
||
|
|
||
| module Departure | ||
| class << self | ||
| attr_accessor :configuration | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a problem to support trilogy since rails 7.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also wondering if it wouldn't be possible to get that from the original connection. On my original attempt to implement trilogy support I remember trying that but ended up with the configuration like you did, but I was wondering whether that wasn't possible now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can derive it from the connection, i'm not 100% there yet with this pr but once i am we can answer the pre 8.1 for complexity if someone is actually needing in rails 8.0 or before.
My initial thought is to put it in 8.1 and beyond so that as people upgrade they will be able to use it.
7.1 is EOL October 1st
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7.1 -> 8.0 and 8.0 -> 8.1 are easy upgrades, so I won't bother about supporting previous versions.