-
Notifications
You must be signed in to change notification settings - Fork 309
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
Avoid extra BindParam
allocation to generate placeholder in queries
#2157
Conversation
Resolves rsim#2152 and follow rails/rails#41577. The tests that fail below have not yet been resolved. rails/rails@6ee96a8.
Compared to this statement with that of Oracle enhanced adapter release61 branch, it does not have
Then just calling super at
|
@koic Would you add this change to this pull request? then CI should be green. $ git diff
diff --git a/lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb b/lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb
index 54144633..631dcffa 100644
--- a/lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb
+++ b/lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb
@@ -79,7 +79,7 @@ module ActiveRecord
# New method in ActiveRecord 3.1
# Will add RETURNING clause in case of trigger generated primary keys
def sql_for_insert(sql, pk, binds)
- unless pk == false || pk.nil? || pk.is_a?(Array)
+ unless pk == false || pk.nil? || pk.is_a?(Array) || pk.is_a?(String)
sql = "#{sql} RETURNING #{quote_column_name(pk)} INTO :returning_id"
(binds = binds.dup) << ActiveRecord::Relation::QueryAttribute.new("returning_id", nil, Type::OracleEnhanced::Integer.new)
end
$ |
This PR commit suppresses and auto-corrects the following RuboCop's offense. ```console % bundle exec rubocop -a Inspecting 70 files ...................W.................................................. Offenses: lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:13:52: C: [Corrected] Style/RedundantBegin: Redundant begin block detected. self.class.quoted_column_names[name] ||= begin ^^^^^ lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:14:11: C: [Corrected] Layout/IndentationWidth: Use 2 (not 4) spaces for indentation. "\"#{name.upcase}\"" ^^^^ lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:15:13: C: [Corrected] Layout/ElseAlignment: Align else with self.class.quoted_column_names[name]. else ^^^^ lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:16:15: C: [Corrected] Layout/CommentIndentation: Incorrect indentation detected (column 14 instead of 12). # remove double quotes which cannot be used inside quoted identifier ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:17:11: C: [Corrected] Layout/IndentationWidth: Use 2 (not 4) spaces for indentation. "\"#{name.gsub('"', '')}\"" ^^^^ lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:18:13: W: [Corrected] Layout/EndAlignment: end at 18, 12 is not aligned with self.class.quoted_column_names[name] ||= if at 13, 10. end ^^^ lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:19:1: C: [Corrected] Layout/EmptyLinesAroundMethodBody: Extra empty line detected at method body end. lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:19:1: C: [Corrected] Layout/TrailingWhitespace: Trailing whitespace detected. 70 files inspected, 8 offenses detected, 8 offenses corrected ```
The RuboCop offense reported since RuboCop 1.12.0. 1.11.0 does not report this offense then it is likely due to rubocop/rubocop#9601 |
@yahonda Awesome! Thank you for your investigation and resolution. I have updated this PR. |
Failure against Oracle database 11g at https://travis-ci.com/github/rsim/oracle-enhanced/jobs/494935830 will be taken care of by another pull request. |
I meant to say I'm going to merge this pull request when CI runs successfully against Oracle Database 18c at GitHub Actions. |
Bug report templates failures are expected since it has not merged to master yet. |
Thanks for addressing the showstopper. |
Thank you too! |
Resolves #2152 and follow rails/rails#41577.
The tests that fail below have not yet been resolved.
rails/rails@6ee96a8.