Quantcast

RFR (JAXP) 8176405 : Catalog circular reference check did not work in certain scenarios

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (JAXP) 8176405 : Catalog circular reference check did not work in certain scenarios

huizhe wang
Hi,

Please review a fix to the circular reference check that incorrectly
treated duplicate entries as circular.

JBS: https://bugs.openjdk.java.net/browse/JDK-8176405
Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/webrev/

Thanks,
Joe

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (JAXP) 8176405 : Catalog circular reference check did not work in certain scenarios

huizhe wang

On 3/23/2017 11:08 AM, Roger Riggs wrote:
> Hi Joe,
>
>  - javax.xml.catalog.GroupEntry.set(catalog):183
>    - rename the method to setCatalog(catalog) to be a bit more expressive.

Done.
>
>  - GroupEntry: 460:461;  correct the @param for "catalogId" ->
> "catalogURI"
>    Check all the @param catalogId -> catalogURI in the file
>    and there are extra blank lines between the @param tags

Fixed.
>
> - CatalogImpl.java: line 432: the @param name "path" should be "uri"
> to match the method signature

Fixed, and plus a couple of methods where @param was missing.
>
> - CatalogMessages.properties:
>   Is it intentional that the OtherError JAXP09000002: Unexpected error
>   has the same number as FormatFailed: JAXP09000002?

Fixed, JAXP09000003 as it should be.
>
>   And should CircularReference = JAXP0901001 be JAXP09030004?

It's parsing error but also a restriction imposed by the impl. I moved
it to it's own category "Implementation restriction".
>
> - CatalogTest: Line 574:  The Assert at 573 prints the expected and
> actual.
>   Adding a reason as a third arg makes it easier to debug.
>   The println at 574 will only be printed if the test succeeds, so is
> of little value

Changed that to assertTrue(msg.contains(expectedMsgId)). It's safer, in
case the test is run on a non-English system.

Updated:
JBS: https://bugs.openjdk.java.net/browse/JDK-8176405
Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/webrev/

Thanks,
Joe

>
> Regards, Roger
>
On 3/17/2017 1:22 PM, huizhe wang wrote:

> Hi,
>
> Please review a fix to the circular reference check that incorrectly
> treated duplicate entries as circular.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8176405
> Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/webrev/
>
> Thanks,
> Joe
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (JAXP) 8176405 : Catalog circular reference check did not work in certain scenarios

roger riggs
Looks good.

Thanks, Roger


On 3/23/2017 4:37 PM, huizhe wang wrote:

>
> On 3/23/2017 11:08 AM, Roger Riggs wrote:
>> Hi Joe,
>>
>>  - javax.xml.catalog.GroupEntry.set(catalog):183
>>    - rename the method to setCatalog(catalog) to be a bit more
>> expressive.
>
> Done.
>>
>>  - GroupEntry: 460:461;  correct the @param for "catalogId" ->
>> "catalogURI"
>>    Check all the @param catalogId -> catalogURI in the file
>>    and there are extra blank lines between the @param tags
>
> Fixed.
>>
>> - CatalogImpl.java: line 432: the @param name "path" should be "uri"
>> to match the method signature
>
> Fixed, and plus a couple of methods where @param was missing.
>>
>> - CatalogMessages.properties:
>>   Is it intentional that the OtherError JAXP09000002: Unexpected error
>>   has the same number as FormatFailed: JAXP09000002?
>
> Fixed, JAXP09000003 as it should be.
>>
>>   And should CircularReference = JAXP0901001 be JAXP09030004?
>
> It's parsing error but also a restriction imposed by the impl. I moved
> it to it's own category "Implementation restriction".
>>
>> - CatalogTest: Line 574:  The Assert at 573 prints the expected and
>> actual.
>>   Adding a reason as a third arg makes it easier to debug.
>>   The println at 574 will only be printed if the test succeeds, so is
>> of little value
>
> Changed that to assertTrue(msg.contains(expectedMsgId)). It's safer,
> in case the test is run on a non-English system.
>
> Updated:
> JBS: https://bugs.openjdk.java.net/browse/JDK-8176405
> Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/webrev/
>
> Thanks,
> Joe
>
>>
>> Regards, Roger
>>
> On 3/17/2017 1:22 PM, huizhe wang wrote:
>> Hi,
>>
>> Please review a fix to the circular reference check that incorrectly
>> treated duplicate entries as circular.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8176405
>> Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/webrev/
>>
>> Thanks,
>> Joe
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (JAXP) 8176405 : Catalog circular reference check did not work in certain scenarios

Lance Andersen
In reply to this post by huizhe wang
Hi Joe,

The current version, including our offline discussion, looks good.

> On Mar 17, 2017, at 4:22 PM, huizhe wang <[hidden email]> wrote:
>
> Hi,
>
> Please review a fix to the circular reference check that incorrectly treated duplicate entries as circular.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8176405
> Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/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
[hidden email] <mailto:[hidden email]>



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (JAXP) 8176405 : Catalog circular reference check did not work in certain scenarios

huizhe wang
Thanks Roger, Lance!

Re-running tests, I'll push the patch once they pass.

-Joe

On 3/23/2017 1:59 PM, Lance Andersen wrote:

> Hi Joe,
>
> The current version, including our offline discussion, looks good.
>
>> On Mar 17, 2017, at 4:22 PM, huizhe wang <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> Hi,
>>
>> Please review a fix to the circular reference check that incorrectly
>> treated duplicate entries as circular.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8176405
>> Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8176405/webrev/ 
>> <http://cr.openjdk.java.net/%7Ejoehw/jdk9/8176405/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
> [hidden email] <mailto:[hidden email]>
>
>
>

Loading...