Hi Toby,
Sorry for the delay. People I wanted to talk to have been traveling and on vacation etc.
I’ll reply in-line. Bottom line is it looks like you’ve made good progress and this seems
worth pursuing. Thanks for digging into it!
On Aug 9, 2016, at 3:13 PM, Toby Crawley <tcrawley(a)redhat.com>
wrote:
I've done some more work on VDX, and have addressed most of the
technical concerns about it from this thread, namely:
* I18N - VDX is now i18n ready, but currently just has a default
message set in english.
* Displaying the default message with code - the original message is
now displayed as part of the output, see
https://gist.github.com/tobias/ec2846a13b6ff656d8e47cbc85071cb1 for
an example.
* Handling errors that aren't created by ParseUtils - For any
XMLStreamExceptions that don't get wrapped by ParseUtils, VDX now
shows that error the best it can. This still doesn't handle errors
that aren't XMLStreamExceptions, but is progress. This should also
handle errors thrown from AttributeDefinitions, since they are
XMLStreamExceptions.
What does the failure message look like when AttributeDefinition rejects an attribute? I
want to make sure this is helping, not obscuring.
A simple test would be to change the ‘port’ attribute in a socket-binding element to -1.
https://gist.github.com/tobias/551e712892b9326096ecc80bb78bc132
is an example of handling a non-wrapped exception thrown by the
reader.
* Size of dependencies - VDX now has no dependencies, and is currently
61K. Removing the dependencies on Apache XMLSchema and Apache
Commons Lang reduces the total size by 552K.
Great!
Existing issues:
* I18N - though it is set up for i18n, it doesn't yet have any
translations. How many/what languages do we support for WildFly
currently? What is the process for getting translations done?
For WildFly itself, English is fine. But for things that end up in EAP (which this would)
we need the ability for the Red Hat localization team to do translations and get the
properties files into VDX source. We do this via a tool called Zanata, available at
https://translate.jboss.org, with integration via a maven plugin. We can help you get this
set up once things are near done. The biggest thing is that your code is i18n which means
it can be done.
* non-XMLStreamException errors - it's currently possible to
trigger
errors that aren't XMLStreamExceptions when parsing
standalone.xml. A couple of examples are:
- adding a duplicate extension under server > extensions causes an
IllegalStateException that occurs when trying to generate the
XMLStreamException
(
https://gist.github.com/tobias/59d155afe0c88e268b83cb75734353eb)
This one looks to be the parser author (aka me) being lazy and incorrectly using a util
method instead of throwing a more custom exception.
I filed
https://issues.jboss.org/browse/WFCORE-1717 for this and listed you as the
reporter. Thanks!
- Creating a file-handler with no attributes under server >
management > audit-log > handlers results in an error about there
being no add operation at address [], with no other context
(
https://gist.github.com/tobias/34b7d7791e20148004d883238adf16ac). If
you add a name attribute, you get a different, more helpful
management operation error
(
https://gist.github.com/tobias/6d47539aa40de90ec45cfaaec83ea15e),
but neither error points to the xml configuration as the problem.
This sounds like the parser not doing validation that it could, and deferring to the
management op handling.
Nothing can really be done with VDX to help with these issues, but
I
would be happy to provide PR's to fix them as I find them if there
is interest.
Parser improvements are welcome. Somewhat. ;) This gets back a bit to something I said in
my first post in this thread:
"Personally I think it’s ok to have this for only some failures. Others may disagree
though and start filing bug reports, leading to demands that we fix said “bugs”, leading
to a shift of resource away from other tasks.”
So if we get some simple, no risk patches, great, thank you! I just don’t want to start
having people insist that we beef up the level of validation in our parsers so we can get
pretty reports. But then not contributing the parser improvements.
WFCORE-1717 is different; there the parser is just wrong.
The question now is - should I keep working on this? Is there enough
interest in this for me to continue it to the point where it can be
used by wildfly-core?
Is there interest in seeing this applied to xml that is provided as
part of a deployment (web.xml, jboss-web.xml,
jboss-deployment-structure.xml, etc) in addition to (or instead of
standalone.xml)? I haven't yet looked at what that would take.
For reference, the source for VDX is available at
https://github.com/projectodd/vdx, and my changes so far to
wildfly-core are in this commit:
https://github.com/tobias/wildfly-core/commit/2654406bfcfed7cbf255db819f0...
- Toby
On Tue, Jul 19, 2016 at 7:28 PM, Toby Crawley <tcrawley(a)redhat.com> wrote:
> On Tue, Jul 19, 2016 at 4:07 PM, Brian Stansberry
> <brian.stansberry(a)redhat.com> wrote:
>> Comments in-line, except for something I just thought of.
>>
>> All exception and log messages produced will likely need to follow
>> WildFly’s/EAP's i18n standards. Message prefixed with a code, text produced
>> in a way in an i18n manner with a reasonable way to get localized text into
>> the software.
>
> Ah, good point. It shouldn't be difficult to add localized text to the
> output, the biggest cost there would be the time to translate the
> messages, but I'm not familiar with how i18n is done in our projects.
> For the message code, we could print the original exception message at
> the bottom or top of the validation block. That would then use the
> same code as we provide now, and would provide the same message we use
> now with every error. Would that satisfy the message code requirement?
>
>>>
>>> On Jul 19, 2016, at 2:38 PM, Toby Crawley <tcrawley(a)redhat.com> wrote:
>>>
>>>> On Tue, Jul 19, 2016 at 3:15 PM, Brian Stansberry
>>>> <brian.stansberry(a)redhat.com> wrote:
>>>>
>>>> The only big concern I have about this is that we’ll get this behavior
for
>>>> some failures but not all. And I don’t want to go down the path of trying
to
>>>> force every parser to work in a manner such that we consistently get
this.
>>>>
>>>
>>> I haven't looked at it too deeply, but it may be straightforward to
>>> alter staxmapper to allow providing an exception generator that would
>>> allow catching more of the cases that the parsers miss.
>>>
>>
>> I’m not sure how big of a problem staxmapper-thrown exceptions are. (I
>> haven’t really thought.)
>
> That's just the first place I saw errors from outside of ParseUtils,
> but I haven't yet started playing with attribute values.
>
>>
>> What I was thinking more about when I wrote my previous post was parsers not
>> using ParseUtils, or sometimes not using it.
>>
>> Also, a lot of XmlStreamException cases are generated from implementations
>> of org.jboss.as.controller.AttributeDefinition, e.g.
>>
>>
https://github.com/wildfly/wildfly-core/blob/master/controller/src/main/j...
>>
>> Parsers are encouraged to invoke methods on AttributeDefinition to validate
>> attribute values. Perhaps though those are better left alone, as the
>> validators are meant to produce useful exception messages.
>>
>
> If we let these fall back to just pointing out the error location with
> the original message, this may be ok (assuming by "the alidators are
> meant to produce useful exception messages" you mean the messages
> produced by the AttributeDefinitions).
>
> <snip>
>
>>>> A minor concern is how big the added dependencies are. (I don’t know.)
We
>>>> want to keep WildFly Core small in footprint.
>>>>
>>>
>>> Right now, the only dependencies vdx (31k) has are commons-lang (which
>>> is already a module in WildFly, but not core-feature-pack),
>>> xmlschema-walker (100k), and xmlschema-core (168k). For the rest of
>>> the work, I don't currently see needing any more dependencies.
>>>
>>
>> Thanks. So about 583K including 284K for commons-lang. The current
>> wildly-core-dist-3.0.0.Alpha3.zip is about 16.9MB, so this is fairly
>> substantial.
>>
>
> I'm only using commons-lang for a Levenshtein distance implementation
> - that could certainly be pulled in, and we could drop the 284k for
> commons-lang. It certainly would be nice to keep core under 17MB
> though.
>
> - Toby
--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat