Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
On 2/17/14 11:17 PM, huizhe wang wrote:
Thanks Daniel.
I updated the test to call class.getResourceAsStream (see fromFile
method) instead at the runtime. The filePath now is only used for the
helper method to generate test file. Also added readObject() to
XMLGregorianCalendarImpl (at the bottom). The method calls the save()
method to initialize the orig_* fields as done in the constructors.
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Hi Joe,
The changes look good.
In the serialization test I wonder if it would be better
to simply replace:
53 {
54 filePath =
SerializationTest.class.getResource(SerializationTest.java).getPath();
55 int pos = filePath.lastIndexOf('/');
56 filePath = filePath.substring(0, pos + 1);
57 }
with:
{
filePath = System.getProperty(test.src, .);
}
best regards,
-- daniel
Thanks,
Joe
On 2/17/2014 8:23 AM, Daniel Fuchs wrote:
Hi Joe,
Sorry for the late reply...
On 2/14/14 6:35 PM, huizhe wang wrote:
Hi All,
I added a SerializationTest. The test contains a helper that can
generate serialization files for XMLGregorianCalendar and Duration. I've
created such files for jdk6,7,8 and 9, and manually run the test, that
is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the
JDK9(or 10 in the future) repo and for an auto-run, it would use the
current JDK9/10 build and test against JDK6, 7, 8 and 9. Past JDK10, we
could consider add serialization files for JDK10.
The new fields did not affect serialization compatibility. The above
tests passed with/without the new fields being transient. But I added
transient since it's the right thing to do.
Adding fields is a compatible change in accordance with Java Object
Serialization Spec
http://docs.oracle.com/javase/7/docs/platform/serialization/spec/version.html#6678.
Concerning the new test I think it would be much more
reliable to open the .ser files as a resource, using
something like:
InputStream streamIn = SerializationTest.class.getResourceAsStream(
javaVersion + FILENAME_CAL);
ObjectInputStream objectinputstream = new ObjectInputStream(streamIn);
The reason is that URL.getPath() is an URL path - not a file path, so it
can contain encoded characters (such as %20) that FileInputStream will
not understand. I suspect that if there's a white space or some other
special character in the path somewhere your current mechanism
will not work.
Also it would be good to verify what is the value of the transient
fields after deserialization: I see that XMLGregorianCalendarImpl
initializes them with non-default values - and you may need to
write a readObject() to reinitialize those fields to these non
default values: the deserialization framework may simply leave
them with the global default for fields of that type (null
for objects, 0 for numerical values, false for boolean etc...)
[I am not 100% sure but it will not hurt to check]
best regards,
-- daniel
Thanks,
Joe
On 2/13/2014 6:23 AM, Alan Bateman wrote:
On 13/02/2014 08:18, huizhe wang wrote:
Hi Alan, Lance, and Daniel,
The Xerces serialization revision meant to create a serialization
form that would help maintain future serialization compatibility. But
in reality it itself is causing significant incompatibility as Alan
pointed out below and we discussed previously. I've removed the
revision from the patch as a result.
Please see the new webrev here:
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Thanks for dropping the serialization change as it was just not going
to work the way you had intended.
I agree with Daniel's comment about all the new fields added to
XMLGregorianCalendarImpl as it's not clear why they aren't transient.
I have not studied the rest of the changes but I think Daniel and
Lance are.
-Alan
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
On 2/18/2014 3:39 AM, Daniel Fuchs wrote:
On 2/17/14 11:17 PM, huizhe wang wrote:
Thanks Daniel.
I updated the test to call class.getResourceAsStream (see fromFile
method) instead at the runtime. The filePath now is only used for the
helper method to generate test file. Also added readObject() to
XMLGregorianCalendarImpl (at the bottom). The method calls the save()
method to initialize the orig_* fields as done in the constructors.
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Hi Joe,
The changes look good.
In the serialization test I wonder if it would be better
to simply replace:
53 {
54 filePath =
SerializationTest.class.getResource(SerializationTest.java).getPath();
55 int pos = filePath.lastIndexOf('/');
56 filePath = filePath.substring(0, pos + 1);
57 }
with:
{
filePath = System.getProperty(test.src, .);
}
Ok, I changed that to:
54 filePath = System.getProperty(test.src);
55 if (filePath == null) {
56 //current directory
57 filePath = System.getProperty(user.dir);
58 }
59 filePath += File.separator;
Thanks,
Joe
best regards,
-- daniel
Thanks,
Joe
On 2/17/2014 8:23 AM, Daniel Fuchs wrote:
Hi Joe,
Sorry for the late reply...
On 2/14/14 6:35 PM, huizhe wang wrote:
Hi All,
I added a SerializationTest. The test contains a helper that can
generate serialization files for XMLGregorianCalendar and Duration.
I've
created such files for jdk6,7,8 and 9, and manually run the test, that
is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the
JDK9(or 10 in the future) repo and for an auto-run, it would use the
current JDK9/10 build and test against JDK6, 7, 8 and 9. Past
JDK10, we
could consider add serialization files for JDK10.
The new fields did not affect serialization compatibility. The above
tests passed with/without the new fields being transient. But I added
transient since it's the right thing to do.
Adding fields is a compatible change in accordance with Java Object
Serialization Spec
http://docs.oracle.com/javase/7/docs/platform/serialization/spec/version.html#6678.
Concerning the new test I think it would be much more
reliable to open the .ser files as a resource, using
something like:
InputStream streamIn = SerializationTest.class.getResourceAsStream(
javaVersion + FILENAME_CAL);
ObjectInputStream objectinputstream = new
ObjectInputStream(streamIn);
The reason is that URL.getPath() is an URL path - not a file path,
so it
can contain encoded characters (such as %20) that FileInputStream will
not understand. I suspect that if there's a white space or some other
special character in the path somewhere your current mechanism
will not work.
Also it would be good to verify what is the value of the transient
fields after deserialization: I see that XMLGregorianCalendarImpl
initializes them with non-default values - and you may need to
write a readObject() to reinitialize those fields to these non
default values: the deserialization framework may simply leave
them with the global default for fields of that type (null
for objects, 0 for numerical values, false for boolean etc...)
[I am not 100% sure but it will not hurt to check]
best regards,
-- daniel
Thanks,
Joe
On 2/13/2014 6:23 AM, Alan Bateman wrote:
On 13/02/2014 08:18, huizhe wang wrote:
Hi Alan, Lance, and Daniel,
The Xerces serialization revision meant to create a serialization
form that would help maintain future serialization compatibility.
But
in reality it itself is causing significant incompatibility as Alan
pointed out below and we discussed previously. I've removed the
revision from the patch as a result.
Please see the new webrev here:
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Thanks for dropping the serialization change as it was just not going
to work the way you had intended.
I agree with Daniel's comment about all the new fields added to
XMLGregorianCalendarImpl as it's not clear why they aren't transient.
I have not studied the rest of the changes but I think Daniel and
Lance are.
-Alan
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
Thanks Joe,
+1
There's a small alignment issue in SerializationTest.java line 152.
Also the
if (condition) {
}
else {
}
formatting in the source files is bizarre, but if it can help with
future merges I have no issues.
No need to regenerate a webrev.
-- daniel
On 2/18/14 6:42 PM, huizhe wang wrote:
On 2/18/2014 3:39 AM, Daniel Fuchs wrote:
On 2/17/14 11:17 PM, huizhe wang wrote:
Thanks Daniel.
I updated the test to call class.getResourceAsStream (see fromFile
method) instead at the runtime. The filePath now is only used for the
helper method to generate test file. Also added readObject() to
XMLGregorianCalendarImpl (at the bottom). The method calls the save()
method to initialize the orig_* fields as done in the constructors.
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Hi Joe,
The changes look good.
In the serialization test I wonder if it would be better
to simply replace:
53 {
54 filePath =
SerializationTest.class.getResource(SerializationTest.java).getPath();
55 int pos = filePath.lastIndexOf('/');
56 filePath = filePath.substring(0, pos + 1);
57 }
with:
{
filePath = System.getProperty(test.src, .);
}
Ok, I changed that to:
54 filePath = System.getProperty(test.src);
55 if (filePath == null) {
56 //current directory
57 filePath = System.getProperty(user.dir);
58 }
59 filePath += File.separator;
Thanks,
Joe
best regards,
-- daniel
Thanks,
Joe
On 2/17/2014 8:23 AM, Daniel Fuchs wrote:
Hi Joe,
Sorry for the late reply...
On 2/14/14 6:35 PM, huizhe wang wrote:
Hi All,
I added a SerializationTest. The test contains a helper that can
generate serialization files for XMLGregorianCalendar and Duration.
I've
created such files for jdk6,7,8 and 9, and manually run the test, that
is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the
JDK9(or 10 in the future) repo and for an auto-run, it would use the
current JDK9/10 build and test against JDK6, 7, 8 and 9. Past
JDK10, we
could consider add serialization files for JDK10.
The new fields did not affect serialization compatibility. The above
tests passed with/without the new fields being transient. But I added
transient since it's the right thing to do.
Adding fields is a compatible change in accordance with Java Object
Serialization Spec
http://docs.oracle.com/javase/7/docs/platform/serialization/spec/version.html#6678.
Concerning the new test I think it would be much more
reliable to open the .ser files as a resource, using
something like:
InputStream streamIn = SerializationTest.class.getResourceAsStream(
javaVersion + FILENAME_CAL);
ObjectInputStream objectinputstream = new
ObjectInputStream(streamIn);
The reason is that URL.getPath() is an URL path - not a file path,
so it
can contain encoded characters (such as %20) that FileInputStream will
not understand. I suspect that if there's a white space or some other
special character in the path somewhere your current mechanism
will not work.
Also it would be good to verify what is the value of the transient
fields after deserialization: I see that XMLGregorianCalendarImpl
initializes them with non-default values - and you may need to
write a readObject() to reinitialize those fields to these non
default values: the deserialization framework may simply leave
them with the global default for fields of that type (null
for objects, 0 for numerical values, false for boolean etc...)
[I am not 100% sure but it will not hurt to check]
best regards,
-- daniel
Thanks,
Joe
On 2/13/2014 6:23 AM, Alan Bateman wrote:
On 13/02/2014 08:18, huizhe wang wrote:
Hi Alan, Lance, and Daniel,
The Xerces serialization revision meant to create a serialization
form that would help maintain future serialization compatibility.
But
in reality it itself is causing significant incompatibility as Alan
pointed out below and we discussed previously. I've removed the
revision from the patch as a result.
Please see the new webrev here:
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Thanks for dropping the serialization change as it was just not going
to work the way you had intended.
I agree with Daniel's comment about all the new fields added to
XMLGregorianCalendarImpl as it's not clear why they aren't transient.
I have not studied the rest of the changes but I think Daniel and
Lance are.
-Alan
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
On 2/18/2014 10:05 AM, Daniel Fuchs wrote:
Thanks Joe,
+1
There's a small alignment issue in SerializationTest.java line 152.
That's a debug I added a moment ago, removed.
Also the
if (condition) {
}
else {
}
formatting in the source files is bizarre, but if it can help with
future merges I have no issues.
That's from Xerces (they use Eclipse). I've seen them do this in pure
format changesets, that is, they'd change } else { to the above.
NetBeans format would have been:
if (condition) {
} else {
}
I personally like The NetBeans format. But it will take a while before
we could get jaxp code formatted properly since in many cases the
relevant files are very big (over 3000 lines in this case). Hitting
NetBeans format can generate a lot of noises to the webrev.
No need to regenerate a webrev.
Thanks,
Joe
-- daniel
On 2/18/14 6:42 PM, huizhe wang wrote:
On 2/18/2014 3:39 AM, Daniel Fuchs wrote:
On 2/17/14 11:17 PM, huizhe wang wrote:
Thanks Daniel.
I updated the test to call class.getResourceAsStream (see fromFile
method) instead at the runtime. The filePath now is only used for the
helper method to generate test file. Also added readObject() to
XMLGregorianCalendarImpl (at the bottom). The method calls the save()
method to initialize the orig_* fields as done in the constructors.
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Hi Joe,
The changes look good.
In the serialization test I wonder if it would be better
to simply replace:
53 {
54 filePath =
SerializationTest.class.getResource(SerializationTest.java).getPath();
55 int pos = filePath.lastIndexOf('/');
56 filePath = filePath.substring(0, pos + 1);
57 }
with:
{
filePath = System.getProperty(test.src, .);
}
Ok, I changed that to:
54 filePath = System.getProperty(test.src);
55 if (filePath == null) {
56 //current directory
57 filePath = System.getProperty(user.dir);
58 }
59 filePath += File.separator;
Thanks,
Joe
best regards,
-- daniel
Thanks,
Joe
On 2/17/2014 8:23 AM, Daniel Fuchs wrote:
Hi Joe,
Sorry for the late reply...
On 2/14/14 6:35 PM, huizhe wang wrote:
Hi All,
I added a SerializationTest. The test contains a helper that can
generate serialization files for XMLGregorianCalendar and Duration.
I've
created such files for jdk6,7,8 and 9, and manually run the test,
that
is, read them back with JDK6, 7, 8 and 9. The test worked fine.
In the
JDK9(or 10 in the future) repo and for an auto-run, it would use the
current JDK9/10 build and test against JDK6, 7, 8 and 9. Past
JDK10, we
could consider add serialization files for JDK10.
The new fields did not affect serialization compatibility. The above
tests passed with/without the new fields being transient. But I
added
transient since it's the right thing to do.
Adding fields is a compatible change in accordance with Java Object
Serialization Spec
http://docs.oracle.com/javase/7/docs/platform/serialization/spec/version.html#6678.
Concerning the new test I think it would be much more
reliable to open the .ser files as a resource, using
something like:
InputStream streamIn =
SerializationTest.class.getResourceAsStream(
javaVersion + FILENAME_CAL);
ObjectInputStream objectinputstream = new
ObjectInputStream(streamIn);
The reason is that URL.getPath() is an URL path - not a file path,
so it
can contain encoded characters (such as %20) that FileInputStream
will
not understand. I suspect that if there's a white space or some other
special character in the path somewhere your current mechanism
will not work.
Also it would be good to verify what is the value of the transient
fields after deserialization: I see that XMLGregorianCalendarImpl
initializes them with non-default values - and you may need to
write a readObject() to reinitialize those fields to these non
default values: the deserialization framework may simply leave
them with the global default for fields of that type (null
for objects, 0 for numerical values, false for boolean etc...)
[I am not 100% sure but it will not hurt to check]
best regards,
-- daniel
Thanks,
Joe
On 2/13/2014 6:23 AM, Alan Bateman wrote:
On 13/02/2014 08:18, huizhe wang wrote:
Hi Alan, Lance, and Daniel,
The Xerces serialization revision meant to create a serialization
form that would help maintain future serialization compatibility.
But
in reality it itself is causing significant incompatibility as
Alan
pointed out below and we discussed previously. I've removed the
revision from the patch as a result.
Please see the new webrev here:
http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
Thanks for dropping the serialization change as it was just not
going
to work the way you had intended.
I agree with Daniel's comment about all the new fields added to
XMLGregorianCalendarImpl as it's not
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
Hi Joe, Sorry for the late reply... On 2/14/14 6:35 PM, huizhe wang wrote: Hi All, I added a SerializationTest. The test contains a helper that can generate serialization files for XMLGregorianCalendar and Duration. I've created such files for jdk6,7,8 and 9, and manually run the test, that is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the JDK9(or 10 in the future) repo and for an auto-run, it would use the current JDK9/10 build and test against JDK6, 7, 8 and 9. Past JDK10, we could consider add serialization files for JDK10. The new fields did not affect serialization compatibility. The above tests passed with/without the new fields being transient. But I added transient since it's the right thing to do. Adding fields is a compatible change in accordance with Java Object Serialization Spec http://docs.oracle.com/javase/7/docs/platform/serialization/spec/version.html#6678. Concerning the new test I think it would be much more reliable to open the .ser files as a resource, using something like: InputStream streamIn = SerializationTest.class.getResourceAsStream( javaVersion + FILENAME_CAL); ObjectInputStream objectinputstream = new ObjectInputStream(streamIn); The reason is that URL.getPath() is an URL path - not a file path, so it can contain encoded characters (such as %20) that FileInputStream will not understand. I suspect that if there's a white space or some other special character in the path somewhere your current mechanism will not work. Also it would be good to verify what is the value of the transient fields after deserialization: I see that XMLGregorianCalendarImpl initializes them with non-default values - and you may need to write a readObject() to reinitialize those fields to these non default values: the deserialization framework may simply leave them with the global default for fields of that type (null for objects, 0 for numerical values, false for boolean etc...) [I am not 100% sure but it will not hurt to check] best regards, -- daniel Thanks, Joe On 2/13/2014 6:23 AM, Alan Bateman wrote: On 13/02/2014 08:18, huizhe wang wrote: Hi Alan, Lance, and Daniel, The Xerces serialization revision meant to create a serialization form that would help maintain future serialization compatibility. But in reality it itself is causing significant incompatibility as Alan pointed out below and we discussed previously. I've removed the revision from the patch as a result. Please see the new webrev here: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Thanks for dropping the serialization change as it was just not going to work the way you had intended. I agree with Daniel's comment about all the new fields added to XMLGregorianCalendarImpl as it's not clear why they aren't transient. I have not studied the rest of the changes but I think Daniel and Lance are. -Alan
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
Thanks Daniel. I updated the test to call class.getResourceAsStream (see fromFile method) instead at the runtime. The filePath now is only used for the helper method to generate test file. Also added readObject() to XMLGregorianCalendarImpl (at the bottom). The method calls the save() method to initialize the orig_* fields as done in the constructors. http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Thanks, Joe On 2/17/2014 8:23 AM, Daniel Fuchs wrote: Hi Joe, Sorry for the late reply... On 2/14/14 6:35 PM, huizhe wang wrote: Hi All, I added a SerializationTest. The test contains a helper that can generate serialization files for XMLGregorianCalendar and Duration. I've created such files for jdk6,7,8 and 9, and manually run the test, that is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the JDK9(or 10 in the future) repo and for an auto-run, it would use the current JDK9/10 build and test against JDK6, 7, 8 and 9. Past JDK10, we could consider add serialization files for JDK10. The new fields did not affect serialization compatibility. The above tests passed with/without the new fields being transient. But I added transient since it's the right thing to do. Adding fields is a compatible change in accordance with Java Object Serialization Spec http://docs.oracle.com/javase/7/docs/platform/serialization/spec/version.html#6678. Concerning the new test I think it would be much more reliable to open the .ser files as a resource, using something like: InputStream streamIn = SerializationTest.class.getResourceAsStream( javaVersion + FILENAME_CAL); ObjectInputStream objectinputstream = new ObjectInputStream(streamIn); The reason is that URL.getPath() is an URL path - not a file path, so it can contain encoded characters (such as %20) that FileInputStream will not understand. I suspect that if there's a white space or some other special character in the path somewhere your current mechanism will not work. Also it would be good to verify what is the value of the transient fields after deserialization: I see that XMLGregorianCalendarImpl initializes them with non-default values - and you may need to write a readObject() to reinitialize those fields to these non default values: the deserialization framework may simply leave them with the global default for fields of that type (null for objects, 0 for numerical values, false for boolean etc...) [I am not 100% sure but it will not hurt to check] best regards, -- daniel Thanks, Joe On 2/13/2014 6:23 AM, Alan Bateman wrote: On 13/02/2014 08:18, huizhe wang wrote: Hi Alan, Lance, and Daniel, The Xerces serialization revision meant to create a serialization form that would help maintain future serialization compatibility. But in reality it itself is causing significant incompatibility as Alan pointed out below and we discussed previously. I've removed the revision from the patch as a result. Please see the new webrev here: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Thanks for dropping the serialization change as it was just not going to work the way you had intended. I agree with Daniel's comment about all the new fields added to XMLGregorianCalendarImpl as it's not clear why they aren't transient. I have not studied the rest of the changes but I think Daniel and Lance are. -Alan
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
Hi All, I added a SerializationTest. The test contains a helper that can generate serialization files for XMLGregorianCalendar and Duration. I've created such files for jdk6,7,8 and 9, and manually run the test, that is, read them back with JDK6, 7, 8 and 9. The test worked fine. In the JDK9(or 10 in the future) repo and for an auto-run, it would use the current JDK9/10 build and test against JDK6, 7, 8 and 9. Past JDK10, we could consider add serialization files for JDK10. The new fields did not affect serialization compatibility. The above tests passed with/without the new fields being transient. But I added transient since it's the right thing to do. Adding fields is a compatible change in accordance with Java Object Serialization Spec http://docs.oracle.com/javase/7/docs/platform/serialization/spec/version.html#6678. Thanks, Joe On 2/13/2014 6:23 AM, Alan Bateman wrote: On 13/02/2014 08:18, huizhe wang wrote: Hi Alan, Lance, and Daniel, The Xerces serialization revision meant to create a serialization form that would help maintain future serialization compatibility. But in reality it itself is causing significant incompatibility as Alan pointed out below and we discussed previously. I've removed the revision from the patch as a result. Please see the new webrev here: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Thanks for dropping the serialization change as it was just not going to work the way you had intended. I agree with Daniel's comment about all the new fields added to XMLGregorianCalendarImpl as it's not clear why they aren't transient. I have not studied the rest of the changes but I think Daniel and Lance are. -Alan
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
Hi Alan, Lance, and Daniel, The Xerces serialization revision meant to create a serialization form that would help maintain future serialization compatibility. But in reality it itself is causing significant incompatibility as Alan pointed out below and we discussed previously. I've removed the revision from the patch as a result. Please see the new webrev here: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Thanks, Joe On 2/12/2014 11:45 AM, Alan Bateman wrote: On 12/02/2014 04:26, huizhe wang wrote: Hi Lance, Daniel, I suggest we take this incompatibility and document it in the release notes (we'll likely have more). I reversed DurationImpl and then realized why the Xerces engineers made the incompatibility change when I started on XMLGregorianCalendarImpl. It was because XMLGregorianCalendarImpl did not implement what was recommended, using the lexical form for serialization. In the original implementation therefore, the impl between XMLGregorianCalendarImpl and DurationImpl was not consistent. Keeping this Xerces revision makes the serialization consistent in both classes and adds the benefit of having the same source as that of Xerces. I think we might need to hold the horses and understand this one and compatibility impact a bit more. As I understand, XMLGregorianCalendar is not Serializable but the Xerces implementation (XMLGregorianCalendarImpl) is. If someone is using the JDK and hasn't configured an alternative DatatypeFactory then it's an instance of com.sun.org.apache.xerces.internal.jaxp.datatype.XMLGregorianCalendarImpl that is serialized today. Do I have this right? I assume there is no serialization interop between this and someone using vanilla Xerces as the other end will be dealing with org.apache classes. With the proposed change then you've added a writeReplace so that going forward it will write a com.sun.org.apache.xerces.internal.jaxp.datatype.SerializedDuration into the stream. This isn't going to work if you've got a JDK 8 or older on the other end, right? Also I assume this doesn't help the Xerces interop because the fully qualified class names are still different. XMLGregorianCalendarImpl doesn't appear to have a readObject or other serialization methods so I assume that if you don't change the serialVersionUID then it would at least be possible to deserialize something that was serialized by an older JDK. As you've added a writeReplace then it makes me wonder why the serialVersionUID of XMLGregorianCalendarImpl has changed. Maybe that part could be reversed and leave the long standing value? One thing that might help is to develop a number serialization tests to help better understand the impact of the change. -Alan.
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
Hi Joe, Couldn't all the new orig_* fields in XMLGregorianCalendarImpl be made transient? It looks as if they shouldn't be serialized anyway. Should they? Not making them transient would change the serialization form, and I'm not sure where that would take us... best regards, -- daniel On 2/13/14 9:18 AM, huizhe wang wrote: Hi Alan, Lance, and Daniel, The Xerces serialization revision meant to create a serialization form that would help maintain future serialization compatibility. But in reality it itself is causing significant incompatibility as Alan pointed out below and we discussed previously. I've removed the revision from the patch as a result. Please see the new webrev here: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Thanks, Joe On 2/12/2014 11:45 AM, Alan Bateman wrote: On 12/02/2014 04:26, huizhe wang wrote: Hi Lance, Daniel, I suggest we take this incompatibility and document it in the release notes (we'll likely have more). I reversed DurationImpl and then realized why the Xerces engineers made the incompatibility change when I started on XMLGregorianCalendarImpl. It was because XMLGregorianCalendarImpl did not implement what was recommended, using the lexical form for serialization. In the original implementation therefore, the impl between XMLGregorianCalendarImpl and DurationImpl was not consistent. Keeping this Xerces revision makes the serialization consistent in both classes and adds the benefit of having the same source as that of Xerces. I think we might need to hold the horses and understand this one and compatibility impact a bit more. As I understand, XMLGregorianCalendar is not Serializable but the Xerces implementation (XMLGregorianCalendarImpl) is. If someone is using the JDK and hasn't configured an alternative DatatypeFactory then it's an instance of com.sun.org.apache.xerces.internal.jaxp.datatype.XMLGregorianCalendarImpl that is serialized today. Do I have this right? I assume there is no serialization interop between this and someone using vanilla Xerces as the other end will be dealing with org.apache classes. With the proposed change then you've added a writeReplace so that going forward it will write a com.sun.org.apache.xerces.internal.jaxp.datatype.SerializedDuration into the stream. This isn't going to work if you've got a JDK 8 or older on the other end, right? Also I assume this doesn't help the Xerces interop because the fully qualified class names are still different. XMLGregorianCalendarImpl doesn't appear to have a readObject or other serialization methods so I assume that if you don't change the serialVersionUID then it would at least be possible to deserialize something that was serialized by an older JDK. As you've added a writeReplace then it makes me wonder why the serialVersionUID of XMLGregorianCalendarImpl has changed. Maybe that part could be reversed and leave the long standing value? One thing that might help is to develop a number serialization tests to help better understand the impact of the change. -Alan.
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
On 13/02/2014 08:18, huizhe wang wrote: Hi Alan, Lance, and Daniel, The Xerces serialization revision meant to create a serialization form that would help maintain future serialization compatibility. But in reality it itself is causing significant incompatibility as Alan pointed out below and we discussed previously. I've removed the revision from the patch as a result. Please see the new webrev here: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Thanks for dropping the serialization change as it was just not going to work the way you had intended. I agree with Daniel's comment about all the new fields added to XMLGregorianCalendarImpl as it's not clear why they aren't transient. I have not studied the rest of the changes but I think Daniel and Lance are. -Alan
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
On 12/02/2014 04:26, huizhe wang wrote: Hi Lance, Daniel, I suggest we take this incompatibility and document it in the release notes (we'll likely have more). I reversed DurationImpl and then realized why the Xerces engineers made the incompatibility change when I started on XMLGregorianCalendarImpl. It was because XMLGregorianCalendarImpl did not implement what was recommended, using the lexical form for serialization. In the original implementation therefore, the impl between XMLGregorianCalendarImpl and DurationImpl was not consistent. Keeping this Xerces revision makes the serialization consistent in both classes and adds the benefit of having the same source as that of Xerces. I think we might need to hold the horses and understand this one and compatibility impact a bit more. As I understand, XMLGregorianCalendar is not Serializable but the Xerces implementation (XMLGregorianCalendarImpl) is. If someone is using the JDK and hasn't configured an alternative DatatypeFactory then it's an instance of com.sun.org.apache.xerces.internal.jaxp.datatype.XMLGregorianCalendarImpl that is serialized today. Do I have this right? I assume there is no serialization interop between this and someone using vanilla Xerces as the other end will be dealing with org.apache classes. With the proposed change then you've added a writeReplace so that going forward it will write a com.sun.org.apache.xerces.internal.jaxp.datatype.SerializedDuration into the stream. This isn't going to work if you've got a JDK 8 or older on the other end, right? Also I assume this doesn't help the Xerces interop because the fully qualified class names are still different. XMLGregorianCalendarImpl doesn't appear to have a readObject or other serialization methods so I assume that if you don't change the serialVersionUID then it would at least be possible to deserialize something that was serialized by an older JDK. As you've added a writeReplace then it makes me wonder why the serialVersionUID of XMLGregorianCalendarImpl has changed. Maybe that part could be reversed and leave the long standing value? One thing that might help is to develop a number serialization tests to help better understand the impact of the change. -Alan.
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
Hi joe It looks like you changed the serialversionuid in Durationimpl, did it get changed incorrectly previously? Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Sent from my iPad On Feb 10, 2014, at 6:53 PM, huizhe wang huizhe.w...@oracle.com wrote: Hi, This is an update from Xerces for two datatype classes XMLGregorianCalendarImpl and DurationImpl. For details, please refer to: https://bugs.openjdk.java.net/browse/JDK-8033980 webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Tests included for: 1243, 1416, 1097 (Note that JDK did not have this bug) No test: 1343: remove unused code Existing tests: JAXP SQE and Unit tests passed. Thanks, Joe
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
On 2/11/14 5:55 PM, Lance @ Oracle wrote: Hi joe It looks like you changed the serialversionuid in Durationimpl, did it get changed incorrectly previously? I noticed that as well, but I'm not sure it matters since Duration uses writeReplace to serialize itself as an instance of SerializedDuration. Is that correct Joe? If I'm not mistaken it also means that the serialization forms (before after the change) are not compatible - but is that an issue for such internal classes? best regards, -- daniel Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Sent from my iPad On Feb 10, 2014, at 6:53 PM, huizhe wang huizhe.w...@oracle.com wrote: Hi, This is an update from Xerces for two datatype classes XMLGregorianCalendarImpl and DurationImpl. For details, please refer to: https://bugs.openjdk.java.net/browse/JDK-8033980 webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Tests included for: 1243, 1416, 1097 (Note that JDK did not have this bug) No test: 1343: remove unused code Existing tests: JAXP SQE and Unit tests passed. Thanks, Joe
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
On 2/11/2014 9:05 AM, Daniel Fuchs wrote: On 2/11/14 5:55 PM, Lance @ Oracle wrote: Hi joe It looks like you changed the serialversionuid in Durationimpl, did it get changed incorrectly previously? I noticed that as well, but I'm not sure it matters since Duration uses writeReplace to serialize itself as an instance of SerializedDuration. Is that correct Joe? Yes, it's serialized as an instance of SerializedDuration, similar to the calendar where it's SerializedXMLGregorianCalendar. If I'm not mistaken it also means that the serialization forms (before after the change) are not compatible - but is that an issue for such internal classes? Indeed, it's probably an issue since the binary is not compatible. It looks like this change should be rolled back. -Joe best regards, -- daniel Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Sent from my iPad On Feb 10, 2014, at 6:53 PM, huizhe wang huizhe.w...@oracle.com wrote: Hi, This is an update from Xerces for two datatype classes XMLGregorianCalendarImpl and DurationImpl. For details, please refer to: https://bugs.openjdk.java.net/browse/JDK-8033980 webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Tests included for: 1243, 1416, 1097 (Note that JDK did not have this bug) No test: 1343: remove unused code Existing tests: JAXP SQE and Unit tests passed. Thanks, Joe
Re: RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl
Hi Lance, Daniel, I suggest we take this incompatibility and document it in the release notes (we'll likely have more). I reversed DurationImpl and then realized why the Xerces engineers made the incompatibility change when I started on XMLGregorianCalendarImpl. It was because XMLGregorianCalendarImpl did not implement what was recommended, using the lexical form for serialization. In the original implementation therefore, the impl between XMLGregorianCalendarImpl and DurationImpl was not consistent. Keeping this Xerces revision makes the serialization consistent in both classes and adds the benefit of having the same source as that of Xerces. Thanks, Joe On 2/11/2014 10:56 AM, huizhe wang wrote: On 2/11/2014 9:05 AM, Daniel Fuchs wrote: On 2/11/14 5:55 PM, Lance @ Oracle wrote: Hi joe It looks like you changed the serialversionuid in Durationimpl, did it get changed incorrectly previously? I noticed that as well, but I'm not sure it matters since Duration uses writeReplace to serialize itself as an instance of SerializedDuration. Is that correct Joe? Yes, it's serialized as an instance of SerializedDuration, similar to the calendar where it's SerializedXMLGregorianCalendar. If I'm not mistaken it also means that the serialization forms (before after the change) are not compatible - but is that an issue for such internal classes? Indeed, it's probably an issue since the binary is not compatible. It looks like this change should be rolled back. -Joe best regards, -- daniel Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Sent from my iPad On Feb 10, 2014, at 6:53 PM, huizhe wang huizhe.w...@oracle.com wrote: Hi, This is an update from Xerces for two datatype classes XMLGregorianCalendarImpl and DurationImpl. For details, please refer to: https://bugs.openjdk.java.net/browse/JDK-8033980 webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/ Tests included for: 1243, 1416, 1097 (Note that JDK did not have this bug) No test: 1343: remove unused code Existing tests: JAXP SQE and Unit tests passed. Thanks, Joe
