Skip to content

Conversation

@mattbrictson
Copy link
Contributor

Recent versions of Rails support multiple database connections within the same app. It is possible for these connections to use different adapters. For example, one adapter may use SQL Server, and another uses PostgreSQL.

This gem applies some monkey patches to ActiveRecord for SQL Server compatibility. These patches could break other adapters, though, in a multiple-database scenario.

This PR modifies the patches so that they are applied only if the connection is SQL Server. If not, the original ActiveRecord implementation (super) is used instead.

Fixes #929

@aidanharan @wpolicarpo what do you think of this approach? Any suggestions on how to test?

@mattbrictson
Copy link
Contributor Author

Hmm. Appveyor CI is failing with one failing test, but that same test fails for me locally on main...

On main, running locally:

Failure:
LogSubscriberTest#test_where_in_binds_logging_include_attribute_names [/Users/mattb/.rbenv/versions/3.0.2/lib/ruby/gems/3.0.0/bundler/gems/rails-f94f41dd59b5/activerecord/test/cases/log_subscriber_test.rb:252]:
Expected /\["id",\ 1\],\ \["id",\ 2\],\ \["id",\ 3\],\ \["id",\ 4\],\ \["id",\ 5\]/ to match "Developer Load (2.3ms)  EXEC sp_executesql N'SELECT [developers].[id], [developers].[name], [developers].[salary], [developers].[firm_id], [developers].[mentor_id], [developers].[legacy_created_at], [developers].[legacy_updated_at], [developers].[legacy_created_on], [developers].[legacy_updated_on] FROM [developers] WHERE [developers].[id] IN (@0, @1, @2, @3, @4)', N'@0 bigint, @1 bigint, @2 bigint, @3 bigint, @4 bigint', @0 = 1, @1 = 2, @2 = 3, @3 = 4, @4 = 5  [[\"id\", nil], [\"id\", nil], [\"id\", nil], [\"id\", nil], [\"id\", nil]]".

@aidanharan
Copy link
Contributor

Yeah this fix looks fine to me. The LogSubscriberTest#test_where_in_binds_logging_include_attribute_names test should be fixed already by #931

@wpolicarpo
Copy link
Member

Also, could you rebase your branch and add a changelog entry?

@mattbrictson mattbrictson force-pushed the improve-monkey-patch-safety branch from 53d91c6 to 3e9a38c Compare July 28, 2021 17:26
Recent versions of Rails support multiple database connections within
the same app. It is possible for these connections to use different
adapters. For example, one adapter may use SQL Server, and another uses
PostgreSQL.

This gem applies some monkey patches to ActiveRecord for SQL Server
compatibility. These patches could break other adapters, though, in a
multiple-database scenario.

This commit modifies the patches so that they are applied only if the
connection is SQL Server. If not, the original ActiveRecord
implementation (`super`) is used instead.

Fixes rails-sqlserver#929
@mattbrictson mattbrictson force-pushed the improve-monkey-patch-safety branch from 37d9a67 to 22fb119 Compare July 28, 2021 17:38
@mattbrictson
Copy link
Contributor Author

@wpolicarpo thanks for the feedback! I think I've addressed all of your concerns:

  • Rebased against main
  • Changed is_a?(SQLServerAdapter) to adapter_name == "SQLServer"
  • Added an Unreleased section to the CHANGELOG with an entry for this PR

I also squashed this PR down to one commit.

Let me know if there is anything else you need.

@wpolicarpo
Copy link
Member

All good. Thanks for your contribution.

@wpolicarpo wpolicarpo merged commit e294a94 into rails-sqlserver:main Jul 29, 2021
@mattbrictson mattbrictson deleted the improve-monkey-patch-safety branch July 29, 2021 17:36
lavika pushed a commit to lavika/activerecord-sqlserver-adapter that referenced this pull request Sep 26, 2023
…#933)

Recent versions of Rails support multiple database connections within
the same app. It is possible for these connections to use different
adapters. For example, one adapter may use SQL Server, and another uses
PostgreSQL.

This gem applies some monkey patches to ActiveRecord for SQL Server
compatibility. These patches could break other adapters, though, in a
multiple-database scenario.

This commit modifies the patches so that they are applied only if the
connection is SQL Server. If not, the original ActiveRecord
implementation (`super`) is used instead.

Fixes rails-sqlserver#929
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Do the monkey patches in this gem interfere with the Rails built-in PostgreSQL adapter?

3 participants