Skip to content

Rails 7.2 support! #2424

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Rails 7.2 support! #2424

wants to merge 8 commits into from

Conversation

andynu
Copy link
Contributor

@andynu andynu commented Jan 31, 2025

This makes significant progress towards supporting Rails 7.2

This was referenced Jan 31, 2025
@andynu
Copy link
Contributor Author

andynu commented Jan 31, 2025

  • Emulate OracleAdapter isn't working because the loading process changed significantly. oracle_enhanced_connection is never called.
  • The fix here for Model.contains context index searching is a work around for that specific case, but I'm worried that there is a more general conversion from activerecord bind variables '?' to compatible oracle bind variables :N that is not well handled.
  • The timezone tests do not reliably run for me, perhaps oracle client version related.

@dlagerro
Copy link

I've been looking at contains errors for a fork we have at work. The ? binds issue is bigger than contains, it affects anytime a user tries to write a query with a where with a ?, like Book.where("title = ?", params[:title]). The cause seems to be that the oracle visitor does not define a bind block, so it falls back to the arel use of ?.

Adding

      BIND_BLOCK = proc { |i| ":a#{i}" }
      private_constant :BIND_BLOCK

      def bind_block; BIND_BLOCK; end

to lib/arel/visitors/oracle.rb fixes the issue for me.

The bind_block is essentially what visit_ActiveModel_Attribute and visit_Arel_Nodes_BindParam are doing, so with the bind block set, those methods are no longer needed.

@andynu
Copy link
Contributor Author

andynu commented Feb 19, 2025

@dlagerro Thank you!!!! I knew I was papering over a broader issue in the hopes of being able to resolve other issues here first. I had spent more time on the broader issue but not made the breakthrough you appear to have made. I will look at your proposed change and revert the AREL conversion of the contains method. Thank you so much.

andynu added a commit to andynu/oracle-enhanced that referenced this pull request Feb 19, 2025
Provides the expected :aN formatted bind parameter values needed by
oci8.

Thanks to dlaggero for this.
rsim#2424 (comment)
andynu added a commit to andynu/oracle-enhanced that referenced this pull request Feb 19, 2025
Introduce a new `bind_block` method. Mimicking this change in the
postgres adapter: rails/rails@2b35775

Provides the expected :aN formatted bind parameter values needed by
oci8.

Thanks to dlaggero for this.
rsim#2424 (comment)
The initial connection creation method invocations were re-written to
reduce convention by adding an explicit registration mapping between
adapter name and class.

This implements two.
- adapter: oracle_enhanced
  This is the primary one that people will use.
- adapter: oracle
  This replaces the previous emulate_oracle_adapter database config
setting which we can no longer use, since the config is not available
when we are registering this adapter name to class mapping, but
accomplishes the same end. So if you have 'emulate_oracle_adapter: true`
in your configs, remove that, and set your `adapter: oracle`.

rails/rails@009c7e7
This makes the quote_column_name and quote_table_names methods organized
more similarly to how they are the new mysql and postgres adapters.
Introduce a new `bind_block` method. Mimicking this change in the
postgres adapter: rails/rails@2b35775

Provides the expected :aN formatted bind parameter values needed by
oci8.

Thanks to dlaggero for this.
rsim#2424 (comment)
@andynu andynu changed the title WIP Rails 7 2 support Rails 7.2 support! Feb 19, 2025
@andynu
Copy link
Contributor Author

andynu commented Feb 19, 2025

@dlagerro's suggestion worked great for the bind parameters issue, and was similar to changes found in the postgres adapter.

I've also changed how you invoke the emulate_oracle_adapter feature. You used to have a database config named emulate_oracle_adapter and there was a handler that would dynamically require the OracleAdapter subclass of the OracleEnhancedAdapter class. That dynamic inclusion has moved into the framework itself where it is now easy to register different classes to match with the different adapter keywords. So I've replaced the feature here to use that functionality.
Usually, people will want and use adapter: oracle_enhanced which maps to the OracleEnhancedAdapter class. And if you want to get an emulated OracleAdapter class, all you need to do is set your adapter: oracle and you'll get it, no special database config option needed anymore.

I believe this PR is ready to go. I welcome feedback.

These are not supported by Rails 7.2
@andynu
Copy link
Contributor Author

andynu commented Feb 19, 2025

I have removed ruby 2.7 and 3.0 from the test matrix. They are not compatible with Rails 7.2

@andynu andynu mentioned this pull request Feb 24, 2025
@matthewtusker
Copy link

I have removed ruby 2.7 and 3.0 from the test matrix. They are not compatible with Rails 7.2

Would it be worth adding Ruby 3.4 to the test matrix?

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.

3 participants