Skip to content
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

Fix Rails application name and SQL Server check #1111

Merged

Conversation

aovertus
Copy link
Contributor

Hi,

I've been trying to use the main version of the adapter on our project and can't make it running yet.
Here are 2 issues I've found so far.

  1. Small bug

rails_application_name is called on the instance level instead of class

  1. Issue with sqlserver? declaration
3.2.0 :001 > User.where(id: 1).exists?
/Users/alexandreovertus/.rvm/gems/ruby-3.2.0/bundler/gems/activerecord-sqlserver-adapter-d1a3b1dd1977/lib/active_record/connection_adapters/sqlserver/core_ext/finder_methods.rb:14:in `construct_relation_for_exists’: undefined method `sqlserver?' for #<ActiveRecord::ConnectionAdapters::Mysql2Adapter:0x0000000117eaf020 @transaction_manager=#<ActiveRecord::ConnectionAdapters::TransactionManager:0x00000001162e0100 @stack=[], @connection=#

sqlserver? is unknown from the other adapter. As I understand the goal of defining a method to easily detect which adapter is currently used rather than relying on the name, its not known by the abstracted layer. Ideally the abstract layer would define a method dynamically based on the name of the adapter, in the meantime we can explicitly add a method on the abstract module to make it shared.

Let me know if it makes sense as if its easier to share those notes or make a useable PR to contribute.

Thanks for your support and help with this library 🎉

@aidanharan aidanharan changed the title Issues on main Fix Rails application name and SQL Server check Oct 10, 2023
Allow AbstractAdapter to detect sqlserver connection
@aovertus aovertus force-pushed the add_mysql2_on_abstract_adapter branch from adab3d3 to 324850a Compare October 11, 2023 00:56
@aovertus aovertus requested a review from aidanharan October 11, 2023 00:57
@aidanharan aidanharan marked this pull request as ready for review October 11, 2023 15:17
@aidanharan aidanharan self-assigned this Oct 11, 2023
@aidanharan aidanharan merged commit cf3a15f into rails-sqlserver:main Oct 11, 2023
0 of 4 checks passed
@aidanharan
Copy link
Contributor

@aovertus Thanks for the PR! Great to fix the issues you found as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants