Skip to content

Commit

Permalink
Treewalker: Yield for each array element itself, not just children
Browse files Browse the repository at this point in the history
Due to an oversight in how the treewalker was implemented, we would
never pass the direct array elements to the walk! block, only any
children the elements have.

In practice this is usually not an issue, since the array elements
are typically the generic PgQuery::Node object, and we would call the
walk! function successfully on its children. However, working with those
node objects directly is necessary when wanting to replace the node.

This may be a backwards incompatible change if one relies on the
parent_field to always be a symbol (it can now be an integer as well).
  • Loading branch information
lfittl committed Dec 15, 2023
1 parent b8b286b commit f8973b6
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 2 deletions.
9 changes: 7 additions & 2 deletions lib/pg_query/treewalker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,18 @@ def treewalker!(tree) # rubocop:disable Metrics/CyclomaticComplexity
node = parent_node[parent_field.to_s]
next if node.nil?
location = parent_location + [parent_field]

yield(parent_node, parent_field, node, location) if node.is_a?(Google::Protobuf::MessageExts) || node.is_a?(Google::Protobuf::RepeatedField)

nodes << [node, location] unless node.nil?
end
when Google::Protobuf::RepeatedField
nodes += parent_node.map.with_index { |e, idx| [e, parent_location + [idx]] }
parent_node.each_with_index do |node, parent_field|
next if node.nil?
location = parent_location + [parent_field]
yield(parent_node, parent_field, node, location) if node.is_a?(Google::Protobuf::MessageExts) || node.is_a?(Google::Protobuf::RepeatedField)

nodes << [node, location] unless node.nil?
end
end

break if nodes.empty?
Expand Down
36 changes: 36 additions & 0 deletions spec/lib/treewalker_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
require 'spec_helper'

describe PgQuery, '.treewalker' do
it 'walks nodes contained in repeated fields' do
locations = []
described_class.parse("SELECT to_timestamp($1)").walk! do |_, _, _, location|
locations << location
end
expect(locations).to match_array [
[:stmts],
[:stmts, 0],
[:stmts, 0, :stmt],
[:stmts, 0, :stmt, :select_stmt],
[:stmts, 0, :stmt, :select_stmt, :distinct_clause],
[:stmts, 0, :stmt, :select_stmt, :target_list],
[:stmts, 0, :stmt, :select_stmt, :from_clause],
[:stmts, 0, :stmt, :select_stmt, :group_clause],
[:stmts, 0, :stmt, :select_stmt, :window_clause],
[:stmts, 0, :stmt, :select_stmt, :values_lists],
[:stmts, 0, :stmt, :select_stmt, :sort_clause],
[:stmts, 0, :stmt, :select_stmt, :locking_clause],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :indirection],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :val],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :val, :func_call],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :val, :func_call, :funcname],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :val, :func_call, :args],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :val, :func_call, :agg_order],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :val, :func_call, :funcname, 0],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :val, :func_call, :args, 0],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :val, :func_call, :funcname, 0, :string],
[:stmts, 0, :stmt, :select_stmt, :target_list, 0, :res_target, :val, :func_call, :args, 0, :param_ref]
]
end
end

0 comments on commit f8973b6

Please sign in to comment.