Fix crash with nil XPath variables #13
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to d91e124, an XPath variable could be set to nil, in which case it would be silently converted to an empty string. This is clearly still the intended behaviour of this method, as can be seen from the `when nil` further down. But, with the introduction of context support, this case was broken, because trying to do @@context[:node] would raise a NoMethodError if there was no context.
This is already covered by #to_s, for which nil returns "".
|
Thanks. |
|
Actually, is there any reason That wouldn't really be able to be tested, though, because there's no behaviour. Just a change in default value, which a single unit test in the middle of a run couldn't check. What do you think? If not, I've added a test for the current fix. |
kou
requested changes
Jan 31, 2019
There was a problem hiding this comment.
Thanks for adding a test.
I could understand the cause of this problem.
We need to distinguish "no argument" case and "passed nil as the argument" case. In this case, we should process for "passed nil as the argument" case. It means that we never use context. We need to use the argument as is.
Here is a right fix:
diff --git a/lib/rexml/functions.rb b/lib/rexml/functions.rb
index 219f9c8..40a3449 100644
--- a/lib/rexml/functions.rb
+++ b/lib/rexml/functions.rb
@@ -13,6 +13,8 @@ module REXML
@@namespace_context = {}
@@variables = {}
+ NOT_SPECIFIED = Object.new
+
INTERNAL_METHODS = [
:namespace_context,
:namespace_context=,
@@ -135,8 +137,8 @@ module REXML
#
# An object of a type other than the four basic types is converted to a
# string in a way that is dependent on that type.
- def Functions::string( object=nil )
- object = @@context[:node] if object.nil?
+ def Functions::string( object=NOT_SPECIFIED )
+ object = @@context[:node] if NOT_SPECIFIED == object
if object.respond_to?(:node_type)
case object.node_type
when :attributeActually, is there any reason
@@contextshouldn't be initialized to{}? That would probably be a more robust fix for this issue.
I think that it's not a robust. It just hides the real problem.
test/rexml/test_functions.rb
Outdated
| @@ -222,6 +222,22 @@ def test_normalize_space | |||
| assert_equal( [REXML::Comment.new("COMMENT A")], m ) | |||
| end | |||
|
|
|||
| def test_string_nil_without_context | |||
| # Reset to default value | |||
| REXML::Functions.class_variable_set(:@@context, nil) | |||
There was a problem hiding this comment.
We can use REXML::Functions.context = nil.
Could you create a setup method and call this? Because this is needed for all tests.
There was a problem hiding this comment.
All tests in this file? Or the entire test suite?
|
It will work too. |
|
@kou any plans to merge this fix into the next ruby release? thanks! |
|
This will be included in Ruby 2.7.0. |
naitoh
added a commit
to naitoh/rexml
that referenced
this pull request
Jul 12, 2024
## Why? :characters is not normalized in sax2. ## Change - text_unnormalized.rb ``` require 'rexml/document' require 'rexml/parsers/sax2parser' require 'rexml/parsers/pullparser' require 'rexml/parsers/streamparser' xml = <<EOS <root> <A><P>&ruby#13; <I> <B> Text </B> </I></A> </root> EOS class Listener def method_missing(name, *args) p [name, *args] end end puts "REXML(DOM)" REXML::Document.new(xml).elements.each("/root/A") {|element| puts element.text} puts "" puts "REXML(Pull)" parser = REXML::Parsers::PullParser.new(xml) while parser.has_next? res = parser.pull p res end puts "" puts "REXML(Stream)" parser = REXML::Parsers::StreamParser.new(xml, Listener.new).parse puts "" puts "REXML(SAX)" parser = REXML::Parsers::SAX2Parser.new(xml) parser.listen(Listener.new) parser.parse ``` ## Before (master) ``` $ ruby text_unnormalized.rb REXML(DOM) <I> <B> Text </B> </I> REXML(Pull) start_element: ["root", {}] text: ["\n ", "\n "] start_element: ["A", {}] text: ["<P>&ruby#13; <I> <B> Text </B> </I>", "<P>\r <I> <B> Text </B> </I>"] end_element: ["A"] text: ["\n", "\n"] end_element: ["root"] end_document: [] REXML(Stream) [:tag_start, "root", {}] [:text, "\n "] [:tag_start, "A", {}] [:text, "<P>\r <I> <B> Text </B> </I>"] [:tag_end, "A"] [:text, "\n"] [:tag_end, "root"] REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "<P>\r <I> <B> Text </B> </I>"] #<= This [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ``` ## After(This PR) ``` $ ruby text_unnormalized.rb REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "<P>\r <I> <B> Text </B> </I>"] [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ```
naitoh
added a commit
to naitoh/rexml
that referenced
this pull request
Jul 13, 2024
## Why? :characters is not normalized in sax2. ## Change - text_unnormalized.rb ``` require 'rexml/document' require 'rexml/parsers/sax2parser' require 'rexml/parsers/pullparser' require 'rexml/parsers/streamparser' xml = <<EOS <root> <A><P>&ruby#13; <I> <B> Text </B> </I></A> </root> EOS class Listener def method_missing(name, *args) p [name, *args] end end puts "REXML(DOM)" REXML::Document.new(xml).elements.each("/root/A") {|element| puts element.text} puts "" puts "REXML(Pull)" parser = REXML::Parsers::PullParser.new(xml) while parser.has_next? res = parser.pull p res end puts "" puts "REXML(Stream)" parser = REXML::Parsers::StreamParser.new(xml, Listener.new).parse puts "" puts "REXML(SAX)" parser = REXML::Parsers::SAX2Parser.new(xml) parser.listen(Listener.new) parser.parse ``` ## Before (master) ``` $ ruby text_unnormalized.rb REXML(DOM) <I> <B> Text </B> </I> REXML(Pull) start_element: ["root", {}] text: ["\n ", "\n "] start_element: ["A", {}] text: ["<P>&ruby#13; <I> <B> Text </B> </I>", "<P>\r <I> <B> Text </B> </I>"] end_element: ["A"] text: ["\n", "\n"] end_element: ["root"] end_document: [] REXML(Stream) [:tag_start, "root", {}] [:text, "\n "] [:tag_start, "A", {}] [:text, "<P>\r <I> <B> Text </B> </I>"] [:tag_end, "A"] [:text, "\n"] [:tag_end, "root"] REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "<P>\r <I> <B> Text </B> </I>"] #<= This [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ``` ## After(This PR) ``` $ ruby text_unnormalized.rb REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "<P>\r <I> <B> Text </B> </I>"] [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ```
kou
pushed a commit
that referenced
this pull request
Jul 14, 2024
… "characters" (#168) ## Why? SAX2 parser expand user-defined entity references and character references but doesn't expand predefined entity references. ## Change - text_unnormalized.rb ``` require 'rexml/document' require 'rexml/parsers/sax2parser' require 'rexml/parsers/pullparser' require 'rexml/parsers/streamparser' xml = <<EOS <root> <A><P> <I> <B> Text </B> </I></A> </root> EOS class Listener def method_missing(name, *args) p [name, *args] end end puts "REXML(DOM)" REXML::Document.new(xml).elements.each("/root/A") {|element| puts element.text} puts "" puts "REXML(Pull)" parser = REXML::Parsers::PullParser.new(xml) while parser.has_next? res = parser.pull p res end puts "" puts "REXML(Stream)" parser = REXML::Parsers::StreamParser.new(xml, Listener.new).parse puts "" puts "REXML(SAX)" parser = REXML::Parsers::SAX2Parser.new(xml) parser.listen(Listener.new) parser.parse ``` ## Before (master) ``` $ ruby text_unnormalized.rb REXML(DOM) <I> <B> Text </B> </I> REXML(Pull) start_element: ["root", {}] text: ["\n ", "\n "] start_element: ["A", {}] text: ["<P> <I> <B> Text </B> </I>", "<P>\r <I> <B> Text </B> </I>"] end_element: ["A"] text: ["\n", "\n"] end_element: ["root"] end_document: [] REXML(Stream) [:tag_start, "root", {}] [:text, "\n "] [:tag_start, "A", {}] [:text, "<P>\r <I> <B> Text </B> </I>"] [:tag_end, "A"] [:text, "\n"] [:tag_end, "root"] REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "<P>\r <I> <B> Text </B> </I>"] #<= This [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ``` ## After(This PR) ``` $ ruby text_unnormalized.rb REXML(SAX) [:start_document] [:start_element, nil, "root", "root", {}] [:progress, 6] [:characters, "\n "] [:progress, 9] [:start_element, nil, "A", "A", {}] [:progress, 12] [:characters, "<P>\r <I> <B> Text </B> </I>"] [:progress, 74] [:end_element, nil, "A", "A"] [:progress, 78] [:characters, "\n"] [:progress, 79] [:end_element, nil, "root", "root"] [:progress, 86] [:end_document] ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to d91e124, an XPath variable could be set to
nil, in which case it would be silently converted to an empty string. This is clearly still the intended behaviour of this method, as can be seen from the (redundant)when nilfurther down. But, with the introduction of context support, this case was broken, because trying to do@@context[:node]would raise aNoMethodErrorif there was no context.I also noticed that then
when nilcase was redundant, since without it it'll usenil.to_s, which is already the empty string, so I removed that in a seperate commit.