RFR [16/java.xml] 8246816: XMLGregorianCalendar.hashCode() produces far too many identical hashes

[Joe Wang]

Naoto, Roger,

Sorry have to come back with another iteration. It looks like the 
addition of getMillisecond() over the original implementation does 
change things.

In particular, getMillisecond() returns FIELD_UNDEFINED when 
fractionalSecond == null (or not specified). But the normalization 
process will set it to zero after a mathematics adjustment. Now this 
will fail when comparing with a datetime that does not need to be 
normalized and thereby still returns FIELD_UNDEFINED (FIELD_UNDEFINED = 
Integer.MIN_VALUE). This situation is demonstrated in tests 3 at line 56 
and 4 at line 58. 
<http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev_03/test/jaxp/javax/xml/jaxp/unittest/datatype/HashCodeTest.java.html>

The culprit is XMLGregorianCalendarImpl::normalizeToTimezone. In the 
original implementation, XMLGregorianCalendarImpl was calling the 
private normalizeToTimezone while XMLGregorianCalendar the public 
normalize(). The later will do the right thing to set the Millisecond 
back to UNDEFINED.

In webrev_03, XMLGregorianCalendarImpl now calls super.hashCode() since 
the above was the only difference between the hashCode() impls of 
XMLGregorianCalendarImpl and XMLGregorianCalendar.

webrev_03:
http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev_03/

Thanks,
Joe


On 8/6/20 1:51 PM, Joe Wang wrote:
> Thanks Naoto, Roger!  I'll prepare push based on version 01.
>
> Best,
> Joe
>
> On 8/6/20 1:29 PM, Roger Riggs wrote:
>> +1
>>
>> On 8/6/20 4:18 PM, naoto.sato at oracle.com wrote:
>>> Hi Joe, thank you for looking it into further.
>>>
>>> Yes, I agree the chances of collision is very rare, as 
>>> sub-millisecond difference in two objects. So fine with the version 01.
>>>
>>> Naoto
>>>
>>> On 8/6/20 12:18 PM, Joe Wang wrote:
>>>> Thanks Naoto, Roger for the review!
>>>>
>>>> For Naoto's concern about the equals method using EonAndYear and 
>>>> FractionalSecond, I did cut corners using the all int getXXX 
>>>> method. The rational was: it fulfills the equals-hashcode contract 
>>>> just fine, is consistent with the existing implementation, and 
>>>> concise. This API was introduced since 1.5, but I have yet to see 
>>>> any usage of EonAndYear, and very rarely FractionalSecond. The 
>>>> chances collisions occur due to these differences are very low.
>>>>
>>>> But I understand your concern. To be precise or exact as equals(), 
>>>> we would need to call getFractionalSecond and EonAndYear. Here's 
>>>> what that would look like:
>>>> http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev_02/
>>>>
>>>> To me, I like version 1 for the reasons above:
>>>> http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev_01/
>>>>
>>>> What would you think?
>>>>
>>>> Regards,
>>>> Joe
>>>>
>>>> On 8/5/20 6:13 PM, naoto.sato at oracle.com wrote:
>>>>> Hi Joe,
>>>>>
>>>>> For the consistency with equals(), just wondering if the 
>>>>> sub-second element should be obtained with getFractionalSecond() 
>>>>> instead of getMillisecond(), as the equals() subsequently calls it 
>>>>> in internalCompare() method. Also should it call getEonAndYear() 
>>>>> appropriately for the year?
>>>>>
>>>>> Naoto
>>>>>
>>>>> On 8/5/20 4:37 PM, Joe Wang wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review a fix for the hashCode generation.
>>>>>>
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8246816
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev/
>>>>>>
>>>>>> Thanks,
>>>>>> Joe
>>>>>>
>>>>
>>
>