Adding support for element name in SimpleTypeException when a Validation Error occurs

[awagle]

This fixes an issue reported by multiple customers where the element name was not being populated when validation error occurred. Testing performed - Built the jars using maven and used in BW installation to test customer issue and see that element name is now printed along with the value. ValidationKernel has multiple lines where a new SimpleTypeException is thrown. If this fix is valid, I can submit changes for all the calls to new SimpleTypeException since in most calls we have the elementdeclaration with us.

[awagle]

@aaletal can you please review this request and let me know if it's the right fix for one scenario? This seems to be the most common one customers are running into.

[aalewis-tibco]

Hmmm. Added constructor for SimpleTypeException looks okay, but let's change the existing one to:

this(null, initialValue, type, cause);

that'll prevent accidental desynchronization; there's still only one constructor (ultimately) in use.

Honestly, though, I'd rather have the added argument at the end. And ... why not just pass the ElementDefinition?

this(initialValue, type, cause, null);

And the new constructor: (final String initialValue, final SimpleType type, final SchemaException cause, final ElementDefinition element)

Keep the member as a QName. ElementDefinition, if non-null, must have a non-null QName return from getName().

this.elementName = (element == null) ? null : element.getName(); // initialize in constructor

Here:

if(elementName == null || !elementName.toString().isEmpty()){

Ummm. I suppose that's a perfectly brilliant and cromulent style. Elsewhere. It doesn't match the rest of the code (which admittedly isn't consistent, but I don't have any desire to add yet another style, and one that I don't much like, either). Whitespace and parentheses aren't in short supply:

if ( (elementName == null) || !elementName.toString().isEmpty() ) {

But with that clarified, it appears to be a mistake, no? if ( (elementName != null) && !elementName.toString().isEmpty() ) seems to be what's desired, at this point, because it's guarding against NPEs or empty string inserted: "for element null' is not valid" "for element ' is not valid". I see in your "fixing the condition" update that the == has been replaced with !=, but you've still got an ||, so you'll test for null, and if it's null, you'll then test whether after dereferencing it (generating an NPE) it has an empty string. So, no.

Also, please note that the closing single-quote ' is misplaced there; it should follow the initial value, not the element name: + "' for element " + elementName + " is not valid ..."

That's SimpleTypeException. As for the ValidationKernel:

if(declaration != null && declaration.getName() != null && !declaration.getName().toString().isEmpty()){

Ugh. This isn't helpful or complete, it's just obfuscation. There's only one implementation of the schema model at present, and it implements the invariant that named components must have names. There's no reason to presume that some future implementation is going to (exist at all, and if it does, will) violate that constraint. And that means that the test isn't even needed, especially with the SimpleType constructor modified:

m_errors.error(new SimpleTypeException(m_atomBridge.getC14NString(initialValue), simpleType, e, declaration));

declaration may be null but the constructor as modified above will handle that.

The advantage of the above suggested changes is that the old constructor can then be temporarily commented out, and the errors that then appear in the IDE will indicate where the code can be changed to use the new constructor, either by passing the ElementDefinition as the final parameter or by passing null (or by leaving it as is, with a comment mentioning that there's no ElementDefinition in scope to pass in to enhance the message). That's a useful way to achieve coverage.

Outstanding research, though; finding a single place (the constructor for simpletype and its message) to make an enhancement is thoroughly impressive synthesis and analysis of the error reports. Wish I'd done it. :-/

[aalewis-tibco]

We're not going to pull this one, but we're doing something that's based on it. The refactored commit has additional changes. It retains the QName argument (which makes a little more work for callers of the constructor, but allows us to use the new constructor in a couple of places where there isn't an elementdefinition that can be supplied), and keeps the old constructor (because the schema parser's simple type exceptions mostly don't have the context available). It's also more consistent stylistically with what we've got (for whatever that's worth).