Reject no root element XML as an invalid XML

[naitoh]

Why?

GitHub: fix #289

We must reject all well-formed XMLs:

https://www.w3.org/TR/2006/REC-xml11-20060816/#proc-types

Validating and non-validating processors alike MUST report violations of this specification's well-formedness constraints in the content of the document entity and any other parsed entities that they read.

No root element XML is not well-formed because document requires one element:

https://www.w3.org/TR/2006/REC-xml11-20060816/#sec-well-formed

[1] document ::= ( prolog element Misc* ) - ( Char* RestrictedChar Char* )

[jrafanie]

First of all, thanks for the work on this project. 🙇

I'm noticing this handles empty string but not nil input.

In 3.4.2, they had the same behavior:

irb(main):003> REXML::Document.new("")
=> <UNDEFINED/>
irb(main):004> REXML::Document.new(nil)
=> <UNDEFINED/>

Now, in 3.4.3, the behavior is changed:

irb(main):002> REXML::Document.new("")
../.gem/ruby/3.3.9/gems/rexml-3.4.3/lib/rexml/parsers/baseparser.rb:271:in `pull_event': Malformed XML: No root element (REXML::ParseException)
Line: 0
Position: 0
Last 80 unconsumed characters:

	from ../.gem/ruby/3.3.9/gems/rexml-3.4.3/lib/rexml/parsers/baseparser.rb:249:in `pull'
	from ../.gem/ruby/3.3.9/gems/rexml-3.4.3/lib/rexml/parsers/treeparser.rb:21:in `parse'
	from ../.gem/ruby/3.3.9/gems/rexml-3.4.3/lib/rexml/document.rb:466:in `build'
	from ../.gem/ruby/3.3.9/gems/rexml-3.4.3/lib/rexml/document.rb:103:in `initialize'
	from (irb):2:in `new'
	from (irb):2:in `<main>'
	from <internal:kernel>:187:in `loop'
	from ../.gem/ruby/3.3.9/gems/bundler-2.6.2/lib/bundler/cli/console.rb:16:in `run'
	from ../.gem/ruby/3.3.9/gems/bundler-2.6.2/lib/bundler/cli.rb:482:in `console'
	from ../.gem/ruby/3.3.9/gems/bundler-2.6.2/lib/bundler/vendor/thor/lib/thor/command.rb:28:in `run'
	from ../.gem/ruby/3.3.9/gems/bundler-2.6.2/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
	from ../.gem/ruby/3.3.9/gems/bundler-2.6.2/lib/bundler/vendor/thor/lib/thor.rb:538:in `dispatch'
	from ../.gem/ruby/3.3.9/gems/bundler-2.6.2/lib/bundler/cli.rb:35:in `dispatch'
	from ../.gem/ruby/3.3.9/gems/bundler-2.6.2/lib/bundler/vendor/thor/lib/thor/base.rb:584:in `start'
	from ../.gem/ruby/3.3.9/gems/bundler-2.6.2/lib/bundler/cli.rb:29:in `start'
	from ../.gem/ruby/3.3.9/gems/bundler-2.6.2/exe/bundle:28:in `block in <top (required)>'
	... 4 levels...
irb(main):003> REXML::Document.new(nil)
=> <UNDEFINED/>

Finally, should this print a warning in a patch release (3.4.x) and raise in the next minor release (3.5.x)? I noticed this because rexml hasn't validated this forever so there's possibly other code that was creating a document and adding elements later, see: https://github.com/search?q=REXML%3A%3ADocument.new%28%22%22%29&type=code

Thanks!

[kou]

Thanks for sharing the information.

How about treating REXML::Document.new("") as REXML::Document.new(nil) for keeping backward compatibility?
(Note that REXML::Document.new(" ") (not an empty input) still rejects the input.)

[jrafanie]

How about treating REXML::Document.new("") as REXML::Document.new(nil) for keeping backward compatibility?

If REXML::Document.new("") is invalid because we have no root element, I would think that REXML::Document.new(nil) is also invalid. Is that correct?

If this is true:
For 3.4.x, I would suggest we print a deprecation warning.
For 3.5.x, I would say we can raise an error for both empty string and nil arguments.

Note, I noticed this because I had some old code that was creating a new rexml document with an empty string and then adding elements afterwards. Maybe it's incorrect but it's always worked. I'd like to fix our code going forward to match what the correct behavior should be for the future.

(Note that REXML::Document.new(" ") (not an empty input) still rejects the input.)

Yes, that makes sense to me.

Thanks again for your quick feedback!

[kou]

If REXML::Document.new("") is invalid because we have no root element, I would think that REXML::Document.new(nil) is also invalid. Is that correct?

No. It's not correct.

REXML::Document.new(nil) (== REXML::Document.new) is the correct way to create an empty document:

rexml/lib/rexml/document.rb

Lines 49 to 53 in 4ffe211

	# When no arguments are given,
	# returns an empty document:
	#
	# d = REXML::Document.new
	# d.to_s # => ""

REXML::Document.new("") is an application bug:

rexml/lib/rexml/document.rb

Lines 55 to 56 in 4ffe211

	# When argument +string+ is given, it must be a string
	# containing a valid XML document:

If this is true: For 3.4.x, I would suggest we print a deprecation warning. For 3.5.x, I would say we can raise an error for both empty string and nil arguments.

In general, I'm negative about runtime deprecation warnings. They may be shown in application/indirect user environment. If they are shown in direct developer's environment, the developer can fix their code. But application/indirect users can't. These deprecation warnings are just annoying...

[jrafanie]

@kou Thank you for the clarification and details on the difference between .new(nil) vs .new(""). It's quite helpful.

In general, I'm negative about runtime deprecation warnings. They may be shown in application/indirect user environment. If they are shown in direct developer's environment, the developer can fix their code. But application/indirect users can't. These deprecation warnings are just annoying...

I'm in agreement there but am concerned about breaking the interface of .new in a patch release, 3.4.3. I'm not expecting breakages upgrading from 3.4.2 to 3.4.3. That was surprising.

My suggestion would be your suggestion:

How about treating REXML::Document.new("") as REXML::Document.new(nil) for keeping backward compatibility?

In 3.4.4:

I suggest .new("") now prints a deprecation warning and forwards to .new(nil). This prevents breaking the previous behavior that existed for several years.

In 3.5.x, you can drop the deprecation warning on .new("") and raise the exception instead.

Thanks again for the feedback and understanding my concerns about the breakages I'm seeing.

[jrafanie]

Also, please let me know if you'd prefer I open an issue with the breakage for 3.4.3. I wasn't sure if it was intentional to break from 3.4.2 to 3.4.3 if you used .new("")

[Fryguy]

How about treating REXML::Document.new("") as REXML::Document.new(nil) for keeping backward compatibility?

This seems very reasonable to me, and probably would alleviate @jrafanie's concern as the patch release for it would put this back to not failing.

[kou]

please let me know if you'd prefer I open an issue with the breakage for 3.4.3

Yes please for better visibility.

[kou]

Opened: #296