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

build_subselect does not have ordering #2070

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

koic
Copy link
Collaborator

@koic koic commented Nov 19, 2020

Fixes #2023.

This PR fixes ActiveRecord::StatementInvalid: OCIError: ORA-00907: missing right parenthesis error when using assoc has dependent: :delete_all and order.

has_many :assoc, -> { order(:foo) }, dependent: :delete_all

Oracle does not accept ORDER BY in DELETE and UPDATE's subquery. This PR adds the following reproduction test and passes it.

vagrant@rails-dev-box:~/src/oracle-enhanced$ bundle exec rspec
spec/active_record/connection_adapters/oracle_enhanced_adapter_spec.rb:187
==> Loading config from ENV or use default
==> Running specs with ruby version 2.7.2
==> Effective ActiveRecord version 6.1.0.rc1
Run options: include
{:locations=>{"./spec/active_record/connection_adapters/oracle_enhanced_adapter_spec.rb"=>[187]}}
F

Failures:

  1) OracleEnhancedAdapter `has_many` assoc has `dependent: :delete_all`
  with `order` should load included association with more than 1000
  records
     Failure/Error: @raw_cursor.exec

     ActiveRecord::StatementInvalid: OCIError: ORA-00907: missing right parenthesis
     # stmt.c:267:in oci8lib_270.so
     # /home/vagrant/.rvm/gems/ruby-2.7.2/bundler/gems/ruby-oci8-446ad166808c/lib/oci8/cursor.rb:137:in `exec'
     # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:150:in `exec_update'
     # ./lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:158:in `block (2 levels) in exec_update'

@koic koic force-pushed the build_subselect_doesnt_have_ordering branch from 4f6eba8 to 5ff49f8 Compare November 19, 2020 04:23
@yahonda
Copy link
Collaborator

yahonda commented Nov 19, 2020

Thanks for the pull request. What do you think about this change? It clearly shows the differences from other database drivers that Oracle DML does not support order by in subqueries.

$ git diff
diff --git a/lib/arel/visitors/oracle.rb b/lib/arel/visitors/oracle.rb
index 55830bd6..5bb59326 100644
--- a/lib/arel/visitors/oracle.rb
+++ b/lib/arel/visitors/oracle.rb
@@ -255,13 +255,7 @@ module Arel # :nodoc: all
         # This method has been overridden based on the following code.
         # https://github.com/rails/rails/blob/v6.1.0.rc1/activerecord/lib/arel/visitors/to_sql.rb#L815-L825
         def build_subselect(key, o)
-          stmt             = Nodes::SelectStatement.new
-          core             = stmt.cores.first
-          core.froms       = o.relation
-          core.wheres      = o.wheres
-          core.projections = [key]
-          stmt.limit       = o.limit
-          stmt.offset      = o.offset
+          stmt = super
           stmt.orders      = [] # `orders` will never be set to prevent `ORA-00907`.
           stmt
         end
diff --git a/lib/arel/visitors/oracle12.rb b/lib/arel/visitors/oracle12.rb
index 76a1b371..b08ee2bb 100644
--- a/lib/arel/visitors/oracle12.rb
+++ b/lib/arel/visitors/oracle12.rb
@@ -162,13 +162,7 @@ module Arel # :nodoc: all
         # This method has been overridden based on the following code.
         # https://github.com/rails/rails/blob/v6.1.0.rc1/activerecord/lib/arel/visitors/to_sql.rb#L815-L825
         def build_subselect(key, o)
-          stmt             = Nodes::SelectStatement.new
-          core             = stmt.cores.first
-          core.froms       = o.relation
-          core.wheres      = o.wheres
-          core.projections = [key]
-          stmt.limit       = o.limit
-          stmt.offset      = o.offset
+          stmt = super
           stmt.orders      = [] # `orders` will never be set to prevent `ORA-00907`.
           stmt
         end
$

Fixes rsim#2023.

This PR fixes `ActiveRecord::StatementInvalid: OCIError: ORA-00907: missing right parenthesis` error
when using assoc has `dependent: :delete_all` and `order`.

```ruby
has_many :assoc, -> { order(:foo) }, dependent: :delete_all
```

Oracle does not accept `ORDER BY` in `DELETE` and `UPDATE`'s subquery.
This PR adds the following reproduction test and passes it.

```console
vagrant@rails-dev-box:~/src/oracle-enhanced$ bundle exec rspec
spec/active_record/connection_adapters/oracle_enhanced_adapter_spec.rb:187
==> Loading config from ENV or use default
==> Running specs with ruby version 2.7.2
==> Effective ActiveRecord version 6.1.0.rc1
Run options: include
{:locations=>{"./spec/active_record/connection_adapters/oracle_enhanced_adapter_spec.rb"=>[187]}}
F

Failures:

  1) OracleEnhancedAdapter `has_many` assoc has `dependent: :delete_all`
  with `order` should load included association with more than 1000
  records
     Failure/Error: @raw_cursor.exec

     ActiveRecord::StatementInvalid: OCIError: ORA-00907: missing right parenthesis
     # stmt.c:267:in oci8lib_270.so
     # /home/vagrant/.rvm/gems/ruby-2.7.2/bundler/gems/ruby-oci8-446ad166808c/lib/oci8/cursor.rb:137:in `exec'
     # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:150:in `exec_update'
     # ./lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:158:in `block (2 levels) in exec_update'
```
@koic
Copy link
Collaborator Author

koic commented Nov 19, 2020

Wow, super cool suggestion! I updated this PR.

@koic koic force-pushed the build_subselect_doesnt_have_ordering branch from 5ff49f8 to 31c244f Compare November 19, 2020 09:34
@yahonda yahonda merged commit 662f543 into rsim:master Nov 19, 2020
@koic koic deleted the build_subselect_doesnt_have_ordering branch November 19, 2020 10:27
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 19, 2020
…dering

`build_subselect` does not have ordering
yahonda added a commit that referenced this pull request Nov 20, 2020
Merge pull request #2070 from koic/build_subselect_doesnt_have_ordering
@causztic
Copy link

is it possible to cut this for a patch version update pre 6.1.0?
Currently on this merged commit it targets 6.1 but I'm currently facing this issue on 6.0.3.3.

I am in the midst of upgrading my application from rails 5 to 6 so I don't want to jump to 6.1 immediately

@yahonda
Copy link
Collaborator

yahonda commented Nov 27, 2020

I understand your concern. Technically, this pull request is not feasible to backport because the Arel file has been migrated from Rails code to Oracle enhanced adapter via #2002 which is only available for Oracle enhanced adapter 6.1.x and above.

Let me consider if I can take some options.

@causztic
Copy link

Thank you - let me know if there is anything I can do to help as well

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 30, 2020
…dering

`build_subselect` does not have ordering
yahonda added a commit that referenced this pull request Nov 30, 2020
Merge pull request #2070 from koic/build_subselect_doesnt_have_ordering
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.

Version 6.0 bug: ORA-00907 missing right parenthesis
3 participants