RFR [15/java.xml] 8238183: SAX2StAXStreamWriter cannot deal with comments prior to the root element
Hi, Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711). JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/ Thanks, Joe
Hi Joe, Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version. Otherwise looks good to me. Naoto On 3/30/20 11:02 AM, Joe Wang wrote:
Hi,
Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711).
JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/
Thanks, Joe
Hi Naoto, Thanks for the review! I refactored the code around getting the XML version and encoding from the locator to get rid of the CCE. The impls with EventWriter and StreamWriter share a base class. All three classes are therefore refactored. No material change to the EventWriter impl. Two fields, xmlVersion and encoding, are added since I expect they will be needed later when we work on fixing the declaration. Note that, as of the current impl, encoding is not referenced in the StreamWriter impl, which is part of the issue in transforming the declaration (JDK-8241711). Here's webrev version 2: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_02/ -Joe On 3/30/20 11:23 AM, naoto.sato@oracle.com wrote:
Hi Joe,
Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version.
Otherwise looks good to me.
Naoto
On 3/30/20 11:02 AM, Joe Wang wrote:
Hi,
Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711).
JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/
Thanks, Joe
HI Joe, This version looks OK Best Lance
On Mar 30, 2020, at 5:56 PM, Joe Wang <huizhe.wang@oracle.com> wrote:
Hi Naoto,
Thanks for the review!
I refactored the code around getting the XML version and encoding from the locator to get rid of the CCE. The impls with EventWriter and StreamWriter share a base class. All three classes are therefore refactored. No material change to the EventWriter impl. Two fields, xmlVersion and encoding, are added since I expect they will be needed later when we work on fixing the declaration. Note that, as of the current impl, encoding is not referenced in the StreamWriter impl, which is part of the issue in transforming the declaration (JDK-8241711).
Here's webrev version 2: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_02/
-Joe
On 3/30/20 11:23 AM, naoto.sato@oracle.com wrote:
Hi Joe,
Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version.
Otherwise looks good to me.
Naoto
On 3/30/20 11:02 AM, Joe Wang wrote:
Hi,
Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711).
JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/
Thanks, Joe
<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
Hi Joe, Much better and cleaner! One nit is that you could remove "docLocator != null &&" as instanceof checks null. Naoto On 3/30/20 2:56 PM, Joe Wang wrote:
Hi Naoto,
Thanks for the review!
I refactored the code around getting the XML version and encoding from the locator to get rid of the CCE. The impls with EventWriter and StreamWriter share a base class. All three classes are therefore refactored. No material change to the EventWriter impl. Two fields, xmlVersion and encoding, are added since I expect they will be needed later when we work on fixing the declaration. Note that, as of the current impl, encoding is not referenced in the StreamWriter impl, which is part of the issue in transforming the declaration (JDK-8241711).
Here's webrev version 2: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_02/
-Joe
On 3/30/20 11:23 AM, naoto.sato@oracle.com wrote:
Hi Joe,
Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version.
Otherwise looks good to me.
Naoto
On 3/30/20 11:02 AM, Joe Wang wrote:
Hi,
Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711).
JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/
Thanks, Joe
On 3/30/20 3:12 PM, naoto.sato@oracle.com wrote:
Hi Joe,
Much better and cleaner! One nit is that you could remove "docLocator != null &&" as instanceof checks null.
Indeed, null check is removed: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_03/ Thanks, Joe
Naoto
On 3/30/20 2:56 PM, Joe Wang wrote:
Hi Naoto,
Thanks for the review!
I refactored the code around getting the XML version and encoding from the locator to get rid of the CCE. The impls with EventWriter and StreamWriter share a base class. All three classes are therefore refactored. No material change to the EventWriter impl. Two fields, xmlVersion and encoding, are added since I expect they will be needed later when we work on fixing the declaration. Note that, as of the current impl, encoding is not referenced in the StreamWriter impl, which is part of the issue in transforming the declaration (JDK-8241711).
Here's webrev version 2: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_02/
-Joe
On 3/30/20 11:23 AM, naoto.sato@oracle.com wrote:
Hi Joe,
Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version.
Otherwise looks good to me.
Naoto
On 3/30/20 11:02 AM, Joe Wang wrote:
Hi,
Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711).
JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/
Thanks, Joe
Looks good. Thanks for the update. Naoto On 3/30/20 3:44 PM, Joe Wang wrote:
On 3/30/20 3:12 PM, naoto.sato@oracle.com wrote:
Hi Joe,
Much better and cleaner! One nit is that you could remove "docLocator != null &&" as instanceof checks null.
Indeed, null check is removed: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_03/
Thanks, Joe
Naoto
On 3/30/20 2:56 PM, Joe Wang wrote:
Hi Naoto,
Thanks for the review!
I refactored the code around getting the XML version and encoding from the locator to get rid of the CCE. The impls with EventWriter and StreamWriter share a base class. All three classes are therefore refactored. No material change to the EventWriter impl. Two fields, xmlVersion and encoding, are added since I expect they will be needed later when we work on fixing the declaration. Note that, as of the current impl, encoding is not referenced in the StreamWriter impl, which is part of the issue in transforming the declaration (JDK-8241711).
Here's webrev version 2: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_02/
-Joe
On 3/30/20 11:23 AM, naoto.sato@oracle.com wrote:
Hi Joe,
Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version.
Otherwise looks good to me.
Naoto
On 3/30/20 11:02 AM, Joe Wang wrote:
Hi,
Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711).
JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/
Thanks, Joe
+1 Good catch Naoto
On Mar 30, 2020, at 6:44 PM, Joe Wang <huizhe.wang@oracle.com> wrote:
On 3/30/20 3:12 PM, naoto.sato@oracle.com wrote:
Hi Joe,
Much better and cleaner! One nit is that you could remove "docLocator != null &&" as instanceof checks null.
Indeed, null check is removed: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_03/
Thanks, Joe
Naoto
On 3/30/20 2:56 PM, Joe Wang wrote:
Hi Naoto,
Thanks for the review!
I refactored the code around getting the XML version and encoding from the locator to get rid of the CCE. The impls with EventWriter and StreamWriter share a base class. All three classes are therefore refactored. No material change to the EventWriter impl. Two fields, xmlVersion and encoding, are added since I expect they will be needed later when we work on fixing the declaration. Note that, as of the current impl, encoding is not referenced in the StreamWriter impl, which is part of the issue in transforming the declaration (JDK-8241711).
Here's webrev version 2: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_02/
-Joe
On 3/30/20 11:23 AM, naoto.sato@oracle.com wrote:
Hi Joe,
Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version.
Otherwise looks good to me.
Naoto
On 3/30/20 11:02 AM, Joe Wang wrote:
Hi,
Please review a fix for the StAXResult impl. The issue was that it output comment prior to the declaration, resulting in an invalid XML document. The patch focuses on fixing this issue, but it does not cover other issues the StAXResult impl may have (e.g. JDK-8241711).
JBS: https://bugs.openjdk.java.net/browse/JDK-8238183 webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/
Thanks, Joe
<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
participants (3)
-
Joe Wang
-
Lance Andersen
-
naoto.sato＠oracle.com