RFR: 8248695: HostLocaleProviderAdapterImpl provides invalid date-only

[Joe Wang]

On 7/13/2020 9:04 PM, naoto.sato at oracle.com wrote:
> Hi Joe,
>
> On 7/13/20 7:28 PM, Joe Wang wrote:
>>
>>
>> On 7/13/2020 7:01 PM, naoto.sato at oracle.com wrote:
>>> Hi Joe,
>>>
>>> Thank you for your review.
>>>
>>> On 7/13/20 3:55 PM, Joe Wang wrote:
>>>> Hi Naoto,
>>>>
>>>> Would it make sense to provide an additional test using the public 
>>>> APIs similar to the one provided in the bug report? I'm sure yours 
>>>> is correct and covers more cases than the original, but it would be 
>>>> nice to have an actual use case and use the public APIs. The report 
>>>> showed it was failed somewhere down the stream than when it is run 
>>>> against the current build, which produces IAE "Too many pattern 
>>>> letters: a" instead of what's reported.
>>>
>>> I am not quite sure I got your suggestion, but the test case uses 
>>> the same API as in the bug report (LocaleProviders.java, line 
>>> 421-428) that are supposed to catch two cases, i.e., 
>>> ofLocalizedDate()/ofLocalizedTime() tests catch "unsupported 
>>> temporal field" exception, such as HourOfDay, and 
>>> ofLocalizedDateTime() catches "Too many pattern letters: aa" which 
>>> you saw in (possibly) en_US locale. To make it clearer, I added some 
>>> comments to it.
>>
>> Yes, the test covered the cases. What I was suggesting was an 
>> additional test that uses only public APIs similar to that in the bug 
>> report, that is what users would do and how they may encounter this 
>> issue, e.g.
>>      System.setProperty("java.locale.providers", "HOST");
>>      DateTimeFormatter formatter = 
>> DateTimeFormatter.ofLocalizedDate(FormatStyle.FULL);
>>
>> It's unlikely for an user application to refer to a private 
>> package/class like sun.util.locale.provider.LocaleProviderAdapter.
>>
>> I understand it's been that way for these test. But to me, a real 
>> world use case is always nice to have. Your call whether you want to 
>> add it or not. There's no missing coverage.
>
> Now I got what you are talking about. The way the bug report is doing 
> is in fact discouraged, as once it is read at the startup, it won't be 
> read again with later setProperty() call. Apps are supposed to specify 
> the property as -D launcher parameter. (And the test case is doing it, 
> at line 184 of LocaleProvidersRun.java) Internal interface is used 
> only to tell whether HOST provider is actually used or not, so apps 
> should not depend on it.

I see. Thanks for the clarification. Also, nice comments to the test, 
helpful for understanding the details if ever it's read again.

Best,
Joe

>
>>
>>>
>>>>
>>>> HostLocaleProviderAdapter_md:849 - 865: may be compacted into one 
>>>> if statement, (bCal && getCalendarInfoWrapper(...) || 
>>>> getLocaleInfoWrapper(...)).
>>>
>>> Quite right. Modified.
>>>
>>> The updated webrev is located at:
>>>
>>> https://cr.openjdk.java.net/~naoto/8248695/webrev.01/
>>
>> Looks good to me.
>
> Thanks!
>
> Naoto
>
>>
>> Best,
>> Joe
>>
>>>
>>> Naoto
>>>
>>>>
>>>> Regards,
>>>> Joe
>>>>
>>>> On 7/13/2020 5:54 AM, naoto.sato at oracle.com wrote:
>>>>> Ping.
>>>>>
>>>>> On 7/7/20 3:55 PM, naoto.sato at oracle.com wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the fix to the following issue:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8248695
>>>>>>
>>>>>> The proposed changeset is located at:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~naoto/8248695/webrev.00/
>>>>>>
>>>>>> There were two causes that resulted in throwing exceptions. One 
>>>>>> was that the Host adapter for Windows always produced Date and 
>>>>>> Time combined patterns, so formatting a LocalDate ended up with 
>>>>>> unsupported temporal field for HourOfDay (reported in the bug), 
>>>>>> and the other cause was the pattern for am/pm was "aa", which was 
>>>>>> not valid as a DateTimeFormatter pattern.
>>>>>>
>>>>>> Besides these issues, localized DayOfWeek/AM_PM names have not 
>>>>>> been correctly implemented in the host adapter. Now those names 
>>>>>> are correctly returned from Windows.
>>>>>>
>>>>>> Naoto
>>>>
>>