JDK 9 RFR of JDK-8149915: enabling validate-annotations feature for xsd schema with annotation causes NPE

[huizhe wang]

Hi Langer,

Thanks for working on the patch.

On 2/17/2016 8:27 AM, Langer, Christoph wrote:
> Hi Joe,
>
> here's my next version, trying to implement your suggestions: http://cr.openjdk.java.net/~clanger/webrevs/8149915.1/

Looks good in general.
>
> I also took the chance to do some cleanups and type check fixes. Hope that's ok?

Yes, it's good to have some cleanups. For the generic initialization, 
it's preferable to have no redundant type arguments, for example, 
instead of:

protected Stack<Entity> fEntityStack = new Stack<Entity>();

do:
protected Stack<Entity> fEntityStack = new Stack<>();


Some of the "cleanup" in the webrev were reversing <> with added type 
argument(s), that would be unnecessary.

For the obsolete Vector, it would be good to replace it with ArrayList.

Copyright year "2016" in XSAttributeChecker needs to be "2016," or else 
the script would complain.

In XSDHandler, note that there's another call to get SECURITY_MANAGER 
towards the end of the reset method (at 3572 in your new version) that 
may be removed. You may also consolidate most of those 
setFeature/Property statements in one try-catch block if you want.

For the test, the @bug tag is still desirable in the general comment 
section. For now, it's @bug 8149915. As we add more test to the class in 
the future, we'd then append more bug ids, @bug 8149915, xxxxxxx.


>
> I'm also wondering why some files have a copyright header like this, e.g. src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/XML11DTDScannerImpl.java:
> /*
>   * reserved comment block
>   * DO NOT REMOVE OR ALTER!
>   */
>
> Shouldn't there be the standard copyright header or is there some reason behind it?

The block is used by the licensing script to swap with license 
information required by licensees. For code that came from Xerces, we 
should keep the block as is with the Apache License. But when we make 
changes, we're required to put in the copyright header. Since the 
changes you made are cleanup / removing unused fields, it's okay to keep 
the block as is.

Best,
Joe

>
> Thanks in advance and best regards
> Christoph
>
>
> -----Original Message-----
> From: huizhe wang [mailto:huizhe.wang at oracle.com]
> Sent: Mittwoch, 17. Februar 2016 00:22
> To: Langer, Christoph <christoph.langer at sap.com>
> Cc: core-libs-dev at openjdk.java.net
> Subject: Re: JDK 9 RFR of JDK-8149915: enabling validate-annotations feature for xsd schema with annotation causes NPE
>
>
> On 2/16/2016 2:13 PM, Langer, Christoph wrote:
>> Hi Joe,
>>
>> thanks for your great suggestions. I'll come up with a new webrev tomorrow.
>>
>> However, I'm still wondering if the change for the default value of XML_SECURITY_PROPERTY_MANAGER in XMLDocumentFragmentScannerImpl should be done anyway as I guess an XMLObject is more expected rather than a String from other places in the code that could make use of such a default...? Is that true?
> You're right, the default value was incorrectly set to the default value
> of a property manager. "null" would be good in this case since there's
> no need to create a new instance here.
>
> Best,
> Joe
>
>> Best
>> Christoph
>>
>> -----Original Message-----
>> From: huizhe wang [mailto:huizhe.wang at oracle.com]
>> Sent: Dienstag, 16. Februar 2016 21:03
>> To: Langer, Christoph <christoph.langer at sap.com>
>> Cc: core-libs-dev at openjdk.java.net
>> Subject: Re: JDK 9 RFR of JDK-8149915: enabling validate-annotations feature for xsd schema with annotation causes NPE
>>
>> Hi Christoph,
>>
>> At initialization of XSDHandler (in reset), note there are two local
>> variables for the security / property manager, change them to instance
>> variables, then when you need to create annotation validator, set both
>> on the validator, e.g.:
>>
>>            fAnnotationValidator.setProperty(SECURITY_MANAGER,
>> fSecurityManager);
>> fAnnotationValidator.setProperty(XML_SECURITY_PROPERTY_MANAGER,
>> fSecurityPropertyMgr);
>>
>> That should fix the issue.
>>
>> There are a bunch of rawtypes/unchecked warnings in this class, bonus
>> but not required :-)
>>
>> For the test, in the past (jaxp standalone), we've created tests for
>> each issue, nowadays we'd prefer to consolidate them. We don't have
>> "SchemaTest" yet, so you may name the test "SchemaTest", then for the
>> @summary, make it general, e.g. "Test Schema creation", and move the
>> specific comment to the test case and make it specific to the issue:
>>
>>       /*
>>        * @bug 8149915
>>        * Verifies that the annotation validator is initialized with the security manager for schema
>>        * creation with http://apache.org/xml/features/validate-annotations=true.
>>        */
>>
>>        @Test
>>        public void testValidation() throws Exception {
>>
>>
>> The test is new, the copyright year would be "2016," instead of "2014,
>> 2016,".
>>
>> Best,
>> Joe
>>
>> On 2/16/2016 4:49 AM, Langer, Christoph wrote:
>>> Hi,
>>>
>>> can you please review the following change for JDK-8149915:
>>>
>>> http://cr.openjdk.java.net/~clanger/webrevs/8149915.0/
>>>
>>> I'm not sure if it is the right thing to set the default for XML_SECURITY_PROPERTY_MANAGER to a new XMLSecurityPropertyManager() object instead of the String "all". But it must not be a String, I think.
>>>
>>> And also I don't know if my approach to add a field "fSecurityManager" to XML11Configuration.
>>>
>>> Please advise and let me know how I can do it better.
>>>
>>> Thanks in advance and best regards
>>> Christoph
>>>