RFR (JAXP): 8033980 : Xerces Update: datatype XMLGregorianCalendarImpl and DurationImpl

[Daniel Fuchs]

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
>>>
>>
>