-
Notifications
You must be signed in to change notification settings - Fork 323
Support Rails 8.0 #2425
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?
Support Rails 8.0 #2425
Conversation
Yes! Thank you.
…On Tue, Mar 11, 2025 at 12:15 PM DanielClarke750 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In activerecord-oracle_enhanced-adapter.gemspec
<#2425 (comment)>
:
> @@ -26,7 +26,7 @@ This adapter is superset of original ActiveRecord Oracle adapter.
"rubygems_mfa_required" => "true"
}
- s.add_runtime_dependency("activerecord", ["~> 7.1.0"])
+ s.add_runtime_dependency("activerecord", ["~> 7.2.0"])
Should this be bumped up to 8.0?
—
Reply to this email directly, view it on GitHub
<#2425 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAONGMKCRALSMVBQT6OQF32T4DZVAVCNFSM6AAAAABXW7IN5GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNZVGI3TCOJYGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
The VERSION should change too, but I'm not sure at what point in the PR process that is most appropriate. |
File dbms_output.rb, line 35
must be
|
That is how it used to be. From the commit: @instrumenter is not defined. It was moved to a method. |
This patch was only mentioned in the 8.0.2 changelog I've tested this branch in 8.0.2 and found no issues. The only issue was the absence of the |
🤷 I don't have keys to this kingdom either. I'd love for this project to continue, but I'm not sure how to accomplish that beyond the PRs. |
@rsim Is there anything that I can do to assist? I'm glad to help in any way that I can. |
@rsim One more comment. The latest Rails that this gem is compatible with is 7.1 which has reached end of active support and will reach the end of security support in four months. I'm here to help in any way that I can. |
@yahonda As the last person to release a new version of this gem, I assume you might be the right person to ask? Is there anything we can do to help get this released? |
fetch_options = { get_lob_value: (name != "Writable Large Object") } | ||
while row = cursor.fetch(fetch_options) | ||
rows << row | ||
if sql =~ /\A\s*SELECT/i # This seems a naive way to detect queries that will have row results. |
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.
This is causing an issue on my end.
I'm doing a recursive select with my_model.find_by_sql(...)
(using with cte (...) as (...) SEARCH DEPTH FIRST BY ... SET ... SELECT ...
)
I tried your fork on the rails-8-support
branch in order to hopefully bump our Rails version but with this change I can't seamlessly upgrade :/
Is this row truly necessary ?
Edit: I tried deleting this condition and my Rails application failed while creating a view with this error ORA-24374
.
There's obviously something that I'm missing about all of this and that is beyond my knowledge unfortunately 😅
Edit 2 : I tried replacing my call with a MyModel.with_recursive
(available as of Rails 7.2) and this obviously fails too. It generates the following SQL statements : WITH RECURSIVE ... AS ...
which doesn't seem to be compatible with Oracle SQL anyway.
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.
@tseuret Does this fail on the current version of the gem as well or only on the Rails 8 version?
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 agree that this query-regex is insufficient. Perhaps we can optimistically attempt to fetch from the cursor and handle any errors if there are no results.
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.
Better yet, let's try to rely on the good work being done in OCI8 to determine what the type of the cursor is.
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.
@yahonda Really appreciate the 7.1.1 release. Anything I can help with to move this issue forward? |
Would you rebase master branch? |
f5d1603
to
c0b12c4
Compare
Active record wants '?' as a bind placeholder. Without this it will not provide bind variables to exec. Oracle requires :N style bind parameters, so it a translation is required. Something went wrong in the translation between rails 7.1 and rails 7.2 so that the where("CONTAINS(table.col, ?, 0) >0", term) was no longer getting it's bind parameter translated to ':a1' like it should be. This change drops down to AREL nodes which ensures that the contains query is getting passed through the visitor pattern, specifically hitting the visit_Arel_Nodes_BindParam method to convert the param to ':a1'. I still do not understand what changed in rails, but this workaround is effective.
First test failure involves a missing method `transform_query` Seems to have been removed in this rails commit: fd24e5bfc9540fc00764a59ddf39a993bbd63ba2 rails/rails@fd24e5b This method was replaced with `preprocess_query`.
@instrumenter is not defined. It was moved to a method. rails/rails@dc522a3
- Add `write_query?` implementation (mimicking postgres) to support the ability to prevent writes to a database - rails/rails@f39d72d - Replace the local implementation of execute, exec_query and its alias internal_exec_query with the new interface defined here rails/rails@fd24e5b#diff-e6fd03cad5e3437c76b6c5db106359124dc9feab8341ade39b9ae54af001fac9 of `raw_execute`, `cast_result`, and `affected_rows`. To support the affected_rows count this also had to add a `row_count` method to the coi and jdbc cursors. - without_prepared_statement? was removed rails/rails@2306c10
drop 3.1 add 3.4
Use OCI8's cursor.type to tell us if it is a select statement. https://www.rubydoc.info/gems/ruby-oci8/OCI8/Cursor#type-instance_method
b8a5454
to
114a467
Compare
fork synced. |
Building on top of #2424 this adds support for existing oracle_enhanced functionality to Rails 8.0. It may not fully support new Rails 8.0 features; For example there seems to be retry logic in rails now, and the oracle enhanced adapter is still doing its own thing.
Add
write_query?
implementation (mimicking postgres) to support the ability to prevent writes to a database - rails/rails@f39d72dReplace the local implementation of execute, exec_query and its alias internal_exec_query with the new interface defined here rails/rails@fd24e5b of
raw_execute
,cast_result
, andaffected_rows
. To support the affected_rows count this also had to add arow_count
method to the coi and jdbc cursors.without_prepared_statement? was removed rails/rails@2306c10
Fixes #2419