Skip to content

Commit cd575a1

Browse files
tompngkou
andauthored
Deprecate accepting array as an element in XPath.match, first and each (#252)
`XPath.match`, `XPath.first`, `XPath.each`, `XPathParser#parse` and `XPathParser#match` accepted nodeset as element. This pull request changes the first parameter of these method to be an element instead of nodeset. Passing nodeset will be deprecated. ```ruby # Documented usage. OK REXML::XPath.match(element, xpath) # Undocumented usage. Deprecate in this pull request nodeset = [element] REXML::XPath.match(nodeset, xpath) ``` ### Background #249 will introduce a temporary cache. ```ruby def parse path, nodeset path_stack = @parser.parse( path ) nodeset.first.document.send(:enable_cache) do match( path_stack, nodeset ) end end ``` But the signature `XPathParser#match(path, nodeset)` does not guarantee that all nodes in the nodeset has the same root document. So cache does not work in the code below. It's still slow. ```ruby REXML::XPath.match(2.times.map { REXML::Document.new('<a>'*400+'</a>'*400) }, 'a//a') ``` The interface is holding our back, so I propose to drop accepting array as element. This change is a backward incompatibility, but it just drops undocumented feature. I think only the test code was unintentionally using this feature. ### XPath.match with array XPath.match only traverse the first element of the array for some selectors. ```ruby nodeset = [REXML::Document.new("<a><b/></a>"), REXML::Document.new("<a><c/></a>")] REXML::XPath.match(nodeset, "a/*") #=> [<b/>, <c/>] REXML::XPath.match(nodeset, "//a/*") #=> [<b/>] # I expect [<b/>, <c/>] but the second document is ignored ``` It indicates that `XPath.match` is not designed to search inside multiple nodes/documents. --------- Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
1 parent 249d770 commit cd575a1

4 files changed

Lines changed: 36 additions & 22 deletions

File tree

lib/rexml/xpath.rb

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ def XPath::first(element, path=nil, namespaces=nil, variables={}, options={})
3535
parser.namespaces = namespaces
3636
parser.variables = variables
3737
path = "*" unless path
38-
element = [element] unless element.kind_of? Array
3938
parser.parse(path, element).flatten[0]
4039
end
4140

@@ -64,7 +63,6 @@ def XPath::each(element, path=nil, namespaces=nil, variables={}, options={}, &bl
6463
parser.namespaces = namespaces
6564
parser.variables = variables
6665
path = "*" unless path
67-
element = [element] unless element.kind_of? Array
6866
parser.parse(path, element).each( &block )
6967
end
7068

@@ -74,7 +72,6 @@ def XPath::match(element, path=nil, namespaces=nil, variables={}, options={})
7472
parser.namespaces = namespaces
7573
parser.variables = variables
7674
path = "*" unless path
77-
element = [element] unless element.kind_of? Array
7875
parser.parse(path,element)
7976
end
8077
end

lib/rexml/xpath_parser.rb

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -76,19 +76,19 @@ def variables=( vars={} )
7676
@variables = vars
7777
end
7878

79-
def parse path, nodeset
79+
def parse path, node
8080
path_stack = @parser.parse( path )
81-
match( path_stack, nodeset )
81+
match( path_stack, node )
8282
end
8383

84-
def get_first path, nodeset
84+
def get_first path, node
8585
path_stack = @parser.parse( path )
86-
first( path_stack, nodeset )
86+
first( path_stack, node )
8787
end
8888

89-
def predicate path, nodeset
89+
def predicate path, node
9090
path_stack = @parser.parse( path )
91-
match( path_stack, nodeset )
91+
match( path_stack, node )
9292
end
9393

9494
def []=( variable_name, value )
@@ -136,11 +136,13 @@ def first( path_stack, node )
136136
end
137137

138138

139-
def match(path_stack, nodeset)
140-
nodeset = nodeset.collect.with_index do |node, i|
141-
position = i + 1
142-
XPathNode.new(node, position: position)
139+
def match(path_stack, node)
140+
if node.is_a?(Array)
141+
Kernel.warn("REXML::XPath.each, REXML::XPath.first, REXML::XPath.match dropped support for nodeset...", uplevel: 1)
142+
return [] if node.empty?
143+
node = node.first
143144
end
145+
nodeset = [XPathNode.new(node, position: 1)]
144146
result = expr(path_stack, nodeset)
145147
case result
146148
when Array # nodeset

test/test_jaxen.rb

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,9 @@ def process_test_case(name)
5656

5757
# processes a tests/document/context node
5858
def process_context(doc, context)
59-
test_context = XPath.match(doc, context.attributes["select"])
59+
matched = XPath.match(doc, context.attributes["select"])
60+
assert_equal(1, matched.size)
61+
test_context = matched.first
6062
namespaces = context.namespaces
6163
namespaces.delete("var")
6264
namespaces = nil if namespaces.empty?
@@ -101,10 +103,14 @@ def process_nominal_test(context, variables, namespaces, test)
101103
assert_equal(Integer(expected, 10),
102104
matched.size,
103105
user_message(context, xpath, matched))
106+
else
107+
assert_operator(matched.size, :>, 0, user_message(context, xpath, matched))
104108
end
105109

106110
XPath.each(test, "valueOf") do |value_of|
107-
process_value_of(matched, variables, namespaces, value_of)
111+
matched.each do |subcontext|
112+
process_value_of(subcontext, variables, namespaces, value_of)
113+
end
108114
end
109115
end
110116

@@ -118,10 +124,8 @@ def process_exceptional_test(context, variables, namespaces, test)
118124

119125
def user_message(context, xpath, matched)
120126
message = ""
121-
context.each_with_index do |node, i|
122-
message << "Node#{i}:\n"
123-
message << "#{node}\n"
124-
end
127+
message << "Node:\n"
128+
message << "#{context}\n"
125129
message << "XPath: <#{xpath}>\n"
126130
message << "Matched <#{matched}>"
127131
message

test/xpath/test_base.rb

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -411,9 +411,10 @@ def test_preceding
411411

412412
s = "<a><b><c id='1'/></b><b><b><c id='2'/><c id='3'/></b><c id='4'/></b><c id='NOMATCH'><c id='5'/></c></a>"
413413
d = REXML::Document.new(s)
414-
c = REXML::XPath.match( d, "//c[@id = '5']")
415-
cs = REXML::XPath.match( c, "preceding::c" )
416-
assert_equal( 4, cs.length )
414+
c = REXML::XPath.match(d, "//c[@id = '5']")
415+
assert_equal(1, c.length)
416+
cs = REXML::XPath.match(c.first, "preceding::c")
417+
assert_equal(4, cs.length)
417418
end
418419

419420
def test_preceding_multiple
@@ -1255,5 +1256,15 @@ def test_or_and
12551256
end
12561257
assert_equal(["/"], hrefs, "Bug #3842 [ruby-core:32447]")
12571258
end
1259+
1260+
def test_match_with_deprecated_usage
1261+
verbose, $VERBOSE = $VERBOSE, nil
1262+
doc = Document.new("<a><b/></a>")
1263+
assert_equal(['b'], XPath.match([doc, doc], '//b').map(&:name))
1264+
assert_equal(['b'], XPath.match([doc], '//b').map(&:name))
1265+
assert_equal([], XPath.match([], '//b').map(&:name))
1266+
ensure
1267+
$VERBOSE = verbose
1268+
end
12581269
end
12591270
end

0 commit comments

Comments
 (0)