Added support for 'standalone' attribute in StaxEventItemWriter #3764
Conversation
| if (getStandalone()) { | ||
| writer.add(factory.createStartDocument(getEncoding(), getVersion(), getStandalone())); | ||
| } | ||
| else { | ||
| writer.add(factory.createStartDocument(getEncoding(), getVersion())); | ||
| } |
There was a problem hiding this comment.
This means we will have either standalone='yes' or no standalone attribute at all. Is there any reason for that choice? There is no way to output standalone='no' with this approach. I know that the absence of the standalone attribute is equivalent to standalone='no' which is the default, but there is a section from the w3c spec that concerns me:
The standalone document declaration must have the value "no" if any external markup declarations contain declarations of:
...
So unless I'm misunderstanding this spec, there are some circumstances where the standalone attribute must have the value no (Now I'm not sure if this means the attribute must be present and set to no or completely absent which is also interpreted as no..). Since we are not sure how XML parsers would behave regrading this situation, we prefer to give the user the ability to write the standalone attribute with either yes or no, or omit it completely. This can be done using an enum (for example with YES, NO, NONE (the default) values) instead of a boolean.
@parikshitdutta Wdyt? If you agree, please update the PR accordingly. Otherwise let me know and I can take care of the changes.
There was a problem hiding this comment.
I knew standalone='no' is the default value, and it is better to omit any attribute value that is optional and default (I had this knowledge), So:
-
Standalone attribute is optional, and default value is no.
-
As I looked into
stax2-api-4.2.1code, I found they too only writing the attribute if it'syes(true), please check here: https://github.com/FasterXML/stax2-api/blob/8f10bb50124cb70048a3ec8dd2dca4421a04733c/src/main/java/org/codehaus/stax2/ri/Stax2EventWriterImpl.java#L86-L87
That's why, I went with that notion, and didn't see any reason to offer different options.
But then I also understand, giving yes/no/none would make it less ambiguous, more flexible, and that then does not depend on any particular parser or underlying implementation. But I am not very sure if that would be redundant or not.
I am okay to make the necessary changes, please share your final thought.
There was a problem hiding this comment.
Thank you for your feedback!
But then I also understand, giving yes/no/none would make it less ambiguous, more flexible, and that then does not depend on any particular parser or underlying implementation.
I agree, I think we need to give the user the ability to explicitly output standalone='no' even if omitting it is equivalent (this might be a requirement for some users and it is currently impossible with this PR). However, in hindsight, we do not need an enum, we can use the wrapper Boolean type (and not the primitive boolean) and use null as default:
null: no standalone attribute at all (the default)- Boolean.FALSE: standalone='no'
- Boolean.TRUE: standalone='yes'
If you agree, please update the PR accordingly and it should be good to merge. Thank you upfront!
fmbenhassine
added
the
status: waiting-for-reporter
parikshitdutta
force-pushed
the
BATCH-758
branch
from
ceda883 to
a45721d
Compare
|
Hi @benas, Thank you. |
parikshitdutta
force-pushed
the
BATCH-758
branch
from
a45721d to
73cd735
Compare
parikshitdutta
changed the title
fmbenhassine
removed
the
status: waiting-for-reporter
|
@parikshitdutta LGTM now, rebased and merged as d65203e. Thank you for your contribution 👍 |
Closes BATCH-2856 [#758 ]