Skip to content

Commit 20808f2

Browse files
yahondaclaude
andcommitted
Move visit_Arel_Nodes_UpdateStatement into Arel::Visitors::OracleCommon
The implementation in oracle.rb and oracle12.rb was byte-identical: both strip ORDER BY from an UPDATE that has orders but no limit (Oracle does not allow ORDER BY in UPDATE), and dup the statement before mutating so the caller's AST stays intact. Moving the method into OracleCommon removes the duplication and keeps the UPDATE/DELETE ORDER BY handling alongside the IN-list variants already there. Adds a direct visitor spec at spec/active_record/connection_adapters/oracle_enhanced/arel/update_statement_spec.rb covering each branch: - UPDATE with orders and no limit strips ORDER BY before super emits SQL. - UPDATE with orders and a limit keeps ORDER BY (Oracle returns the error at execute time; the visitor does not try to smooth that over). - The original UpdateStatement's `orders` attribute is unmodified after compilation (dup-before-mutate is preserved). - Arel::Visitors::Oracle (legacy ROWNUM) and Oracle12 produce the same stripped SQL. Refs #2663 (task B). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent b6ab786 commit 20808f2

4 files changed

Lines changed: 63 additions & 24 deletions

File tree

lib/arel/visitors/oracle.rb

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -107,18 +107,6 @@ def value_before_type_cast(expr)
107107
end
108108
end
109109

110-
def visit_Arel_Nodes_UpdateStatement(o, collector)
111-
# Oracle does not allow ORDER BY/LIMIT in UPDATEs.
112-
if o.orders.any? && o.limit.nil?
113-
# However, there is no harm in silently eating the ORDER BY clause if no LIMIT has been provided,
114-
# otherwise let the user deal with the error
115-
o = o.dup
116-
o.orders = []
117-
end
118-
119-
super
120-
end
121-
122110
def is_distinct_from(o, collector)
123111
collector << "DECODE("
124112
collector = visit [o.left, o.right, 0, 1], collector

lib/arel/visitors/oracle12.rb

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -55,18 +55,6 @@ def visit_Arel_Nodes_Except(o, collector)
5555
collector << " )"
5656
end
5757

58-
def visit_Arel_Nodes_UpdateStatement(o, collector)
59-
# Oracle does not allow ORDER BY/LIMIT in UPDATEs.
60-
if o.orders.any? && o.limit.nil?
61-
# However, there is no harm in silently eating the ORDER BY clause if no LIMIT has been provided,
62-
# otherwise let the user deal with the error
63-
o = o.dup
64-
o.orders = []
65-
end
66-
67-
super
68-
end
69-
7058
def is_distinct_from(o, collector)
7159
collector << "DECODE("
7260
collector = visit [o.left, o.right, 0, 1], collector

lib/arel/visitors/oracle_common.rb

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,18 @@ def visit_Arel_Nodes_HomogeneousIn(o, collector)
145145
collector
146146
end
147147

148+
def visit_Arel_Nodes_UpdateStatement(o, collector)
149+
# Oracle does not allow ORDER BY/LIMIT in UPDATEs.
150+
if o.orders.any? && o.limit.nil?
151+
# However, there is no harm in silently eating the ORDER BY clause if no LIMIT has been provided,
152+
# otherwise let the user deal with the error
153+
o = o.dup
154+
o.orders = []
155+
end
156+
157+
super
158+
end
159+
148160
def visit_Arel_Nodes_In(o, collector)
149161
attr, values = o.left, o.right
150162
return super unless values.is_a?(Array)
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe "Arel::Visitors::OracleCommon#visit_Arel_Nodes_UpdateStatement" do
4+
before(:all) do
5+
ActiveRecord::Base.establish_connection(CONNECTION_PARAMS)
6+
end
7+
8+
before(:each) do
9+
@visitor = Arel::Visitors::Oracle12.new(ActiveRecord::Base.connection)
10+
@table = Arel::Table.new(:users)
11+
end
12+
13+
def compile(node, visitor: @visitor)
14+
visitor.accept(node, Arel::Collectors::SQLString.new).value
15+
end
16+
17+
def build_update(orders: [], limit: nil)
18+
stmt = Arel::Nodes::UpdateStatement.new
19+
stmt.relation = @table
20+
stmt.values = [Arel::Nodes::Assignment.new(@table[:name], Arel.sql("'foo'"))]
21+
stmt.orders = orders
22+
stmt.limit = limit
23+
stmt
24+
end
25+
26+
it "drops ORDER BY when no LIMIT is set" do
27+
stmt = build_update(orders: [Arel.sql("id ASC")])
28+
sql = compile(stmt)
29+
expect(sql).not_to match(/ORDER BY/i)
30+
end
31+
32+
it "keeps ORDER BY when a LIMIT is set (letting Oracle return the execute-time error)" do
33+
stmt = build_update(orders: [Arel.sql("id ASC")], limit: Arel::Nodes::Limit.new(10))
34+
sql = compile(stmt)
35+
expect(sql).to match(/ORDER BY\s+id ASC/i)
36+
end
37+
38+
it "leaves the original UpdateStatement's orders unmodified" do
39+
orders = [Arel.sql("id ASC")]
40+
stmt = build_update(orders: orders)
41+
compile(stmt)
42+
expect(stmt.orders).to eq(orders)
43+
end
44+
45+
it "strips ORDER BY the same way via Arel::Visitors::Oracle" do
46+
oracle = Arel::Visitors::Oracle.new(ActiveRecord::Base.connection)
47+
stmt = build_update(orders: [Arel.sql("id ASC")])
48+
sql = compile(stmt, visitor: oracle)
49+
expect(sql).not_to match(/ORDER BY/i)
50+
end
51+
end

0 commit comments

Comments
 (0)