On Jul 19, 2016, at 12:29 PM, Toby Crawley <tcrawley(a)redhat.com&gt; wrote: I've done some work to pretty-print XML validation errors that occur when parsing (standalone|domain|host).xml, and wanted to get some feedback on what I have so far to see if: 1) there is interest in seeing this completed 2) the current approach is the best way to integrate with WildFly 3) this same approach could be used to pretty-print issues with other xml parsed by WildFly (web.xml, jboss-deployment-structure.xml, etc) # Background Inspired by error reporting in the Elm language[1] and improvements in configuration feedback for Clojure tooling[2], I looked at what it would take to provide better feedback when parsing XML configuration. My goals were: * Give users clear feedback that can be used to correct the configuration error without the user having to context-switch to documentation, and, in most cases, enable the user to quickly identify and understand the issue before looking away from the validation output, by: * Showing (instead of telling) where in the XML the error occurred * Providing richer feedback than the native validation error provides (detect potential misspellings, provide alternate locations, etc) * Showing documentation for the element/attribute where possible (pulled from the XSD) * Use what we already produce (XSDs), without having to create additional context-specific schema. I've partially implemented a library (vdx)[3] that, given a validation error, the source document, and the relevant schemas, generates and prints a friendly error message. I've also made minimal changes to WildFly to report validation errors to vdx (see below). # Examples Below is some examples of the current startup output from WildFly when a validation error occurs: ## Detecting a misspelled attribute 13:18:03,798 INFO [org.jboss.modules] (main) JBoss Modules version 1.5.2.Final 13:18:03,949 INFO [org.jboss.msc] (main) JBoss MSC version 1.2.6.Final 13:18:04,010 INFO [org.jboss.as] (MSC service thread 1-6) WFLYSRV0049: WildFly Core 2.2.0.CR5-SNAPSHOT "Kenny" starting 13:18:04,751 ERROR [org.jboss.as.controller] (Controller Boot Thread) ====================== Validation Error in standalone.xml ====================== 28: <extension module="org.wildfly.extension.request-controller"/> 29: <extension module="org.wildfly.extension.security.manager"/> 30: <extension modue="org.wildfly.extension.undertow"/> ^ 'modue' isn't an allowed attribute for the 'extension' element 31: </extensions> 32: <management> 33: <security-realms> Did you mean 'module'? ================================================================================ 13:18:04,753 ERROR [org.jboss.as.server] (Controller Boot Thread) WFLYSRV0055: Caught exception during boot: org.jboss.as.controller.persistence.ConfigurationPersistenceException: WFLYCTL0085: Failed to parse configuration at org.jboss.as.controller.persistence.XmlConfigurationPersister.load(XmlConfigurationPersister.java:175) at org.jboss.as.server.ServerService.boot(ServerService.java:357) at org.jboss.as.controller.AbstractControllerService$1.run(AbstractControllerService.java:299) at java.lang.Thread.run(Thread.java:745) 13:18:04,754 FATAL [org.jboss.as.server] (Controller Boot Thread) WFLYSRV0056: Server boot has failed in an unrecoverable manner; exiting. See previous messages for details. 13:18:04,762 INFO [org.jboss.as] (MSC service thread 1-3) WFLYSRV0050: WildFly Core 2.2.0.CR5-SNAPSHOT "Kenny" stopped in 4ms ## Detecting a misplaced attribute ... 14:32:23,842 ERROR [org.jboss.as.controller] (Controller Boot Thread) ====================== Validation Error in standalone.xml ====================== 89: </console-handler> 90: <periodic-rotating-file-handler name="FILE" autoflush="true" 91: category="WARN"> ^ 'category' isn't an allowed attribute for the 'periodic-rotating-file-handler' element 92: <formatter> 93: <named-formatter name="PATTERN"/> 94: </formatter> 'category' is allowed on elements: subsystem > logger, subsystem > logging-profiles > logging-profile > logger Did you intend to put it on one of those elements? ================================================================================ ... ## Detecting a misplaced element ... 10:54:27,359 ERROR [org.jboss.as.controller] (Controller Boot Thread) ====================== Validation Error in standalone.xml ====================== 81: </management> 82: <profile> 83: <extension/> ^ 'extension' isn't an allowed element here 84: <subsystem xmlns="urn:jboss:domain:logging:3.0"> 85: <console-handler name="CONSOLE"> 86: <level name="INFO"/> 'extension' is allowed in elements: domain > extensions, domain > host-excludes > host-exclude > excluded-extensions, host > extensions, host > servers > server > extensions, server > extensions Did you intend to put it in one of those elements? ================================================================================ ... # Changes to WildFly To support this, I've made some small changes changes to wildfly-core[4], but anticipate more before this is complete. The current changes are: * Modifications to ParseUtils to wrap the XMLStreamExceptions with an exception that can convey more context to vdx * Modifications to XmlConfigurationPersistor to: * catch the wrapped exception * see if ${jboss.home.dir}/docs/schema/ is available. If so, pretty-print the error. If not, throw the wrapped exception (which is the behavior before my changes). # Current issues * Only the first validation issue is reported, but this is unavoidable, since the subsystem parsers throw on the first error encountered * This uses xmlschema-walker from Apache XmlSchema[5], which has a couple of bugs that will need to be fixed and released (or forked) * Only errors reported by throwing Exceptions returned by ParseUtils are pretty-printed. Exceptions that come from within STAX reader aren't yet handled (for example, a misplaced element in the logging parser causes reader.handleAny() to be called[6], which triggers an unwrapped exception) * vdx itself is far from complete[7] - that work is pending the outcome of this discussion # Next steps As I said above, the first question to answer is: is this an interesting feature that you would like to see completed? If so, I'm willing to continue working on this, and would be happy to discuss here or on JIRA/HipChat as appropriate. [1]: http://elm-lang.org/blog/compilers-as-assistants [2]: http://rigsomelight.com/2016/05/17/good-configuration-feedback-is-essenti... [3]: https://github.com/projectodd/vdx [4]: https://github.com/tobias/wildfly-core/commit/b4d03897a6ea1b8c786d983da3b... [5]: https://ws.apache.org/xmlschema/ [6]: https://github.com/wildfly/wildfly-core/blob/2.x/logging/src/main/java/or... [7]: https://github.com/projectodd/vdx/issues _______________________________________________ wildfly-dev mailing list wildfly-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/wildfly-dev

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.

> > On Jul 19, 2016, at 2:38 PM, Toby Crawley <tcrawley(a)redhat.com&gt; wrote: > >> On Tue, Jul 19, 2016 at 3:15 PM, Brian Stansberry >> <brian.stansberry(a)redhat.com&gt; 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.)

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.

>> 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.

Most of my concerns about this ware already raised by Brian (l18n, extra size of distro, inconsistencies between subsystems, ...) Also on top of this, having something like that would glorify configuration of server via XML, which is something we are trying to discourage in favor of CLI / mgmt api.

> On Jul 20, 2016, at 8:59 AM, Aleksandar Kostadinov > <akostadi(a)redhat.com <mailto:akostadi@redhat.com>> wrote: > > Tomaž Cerar wrote on 07/20/16 16:38: >> Most of my concerns about this ware already raised by Brian (l18n, extra >> size of distro, inconsistencies between subsystems, ...) >> >> Also on top of this, having something like that would glorify >> configuration of server via XML, >> which is something we are trying to discourage in favor of CLI / >> mgmt api. > > Excuse my ignorance if this is already taken care of. But last time I > looked, using whatever management API was not very nice for container > runtime. i.e. configuration changed through API was not persisted. > I am wondering if this is already fixed somehow or is at least somebody > considering it as an important use case (I think it should). Persisting server configuration changes made via the supported management APIs back to the config file has been done in AS 7.0 and later. Its a fundamental requirement.

On Jul 20, 2016, at 5:49 PM, Aleksandar Kostadinov <akostadi(a)redhat.com&gt; wrote: Brian Stansberry wrote on 07/20/16 20:23: > >> On Jul 20, 2016, at 8:59 AM, Aleksandar Kostadinov >> <akostadi(a)redhat.com <mailto:akostadi@redhat.com>> wrote: >> >> Tomaž Cerar wrote on 07/20/16 16:38: >>> Most of my concerns about this ware already raised by Brian (l18n, extra >>> size of distro, inconsistencies between subsystems, ...) >>> >>> Also on top of this, having something like that would glorify >>> configuration of server via XML, >>> which is something we are trying to discourage in favor of CLI / >>> mgmt api. >> >> Excuse my ignorance if this is already taken care of. But last time I >> looked, using whatever management API was not very nice for container >> runtime. i.e. configuration changed through API was not persisted. >> I am wondering if this is already fixed somehow or is at least somebody >> considering it as an important use case (I think it should). > > Persisting server configuration changes made via the supported > management APIs back to the config file has been done in AS 7.0 and > later. Its a fundamental requirement. I should have explained better. The config is saved on container. But container being stateless loses changes on restart. At least for OpenShift v2 that has been an issue. Shame on me I didn't look at OpenShift v3 integration. I'm not sure how it is handled there. Perhaps I should have done my homework before hijacking the thread. Sorry about that. Hopefully I can check wildfly on v3 next week.

On Jul 20, 2016, at 5:49 PM, Aleksandar Kostadinov <akostadi(a)redhat.com&gt; wrote: Brian Stansberry wrote on 07/20/16 20:23: > >> On Jul 20, 2016, at 8:59 AM, Aleksandar Kostadinov >> <akostadi(a)redhat.com <mailto:akostadi@redhat.com>> wrote: >> >> Tomaž Cerar wrote on 07/20/16 16:38: >>> Most of my concerns about this ware already raised by Brian (l18n, extra >>> size of distro, inconsistencies between subsystems, ...) >>> >>> Also on top of this, having something like that would glorify >>> configuration of server via XML, >>> which is something we are trying to discourage in favor of CLI / >>> mgmt api. >> >> Excuse my ignorance if this is already taken care of. But last time I >> looked, using whatever management API was not very nice for container >> runtime. i.e. configuration changed through API was not persisted. >> I am wondering if this is already fixed somehow or is at least somebody >> considering it as an important use case (I think it should). > > Persisting server configuration changes made via the supported > management APIs back to the config file has been done in AS 7.0 and > later. Its a fundamental requirement. I should have explained better. The config is saved on container. But container being stateless loses changes on restart. At least for OpenShift v2 that has been an issue.

Shame on me I didn't look at OpenShift v3 integration. I'm not sure how it is handled there. Perhaps I should have done my homework before hijacking the thread. Sorry about that. Hopefully I can check wildfly on v3 next week.

> On Jul 20, 2016, at 5:49 PM, Aleksandar Kostadinov > <akostadi(a)redhat.com <mailto:akostadi@redhat.com>> wrote: > > Brian Stansberry wrote on 07/20/16 20:23: >> >>> On Jul 20, 2016, at 8:59 AM, Aleksandar Kostadinov >>> <akostadi(a)redhat.com <mailto:akostadi@redhat.com> >>> <mailto:akostadi@redhat.com>> wrote: >>> >>> Tomaž Cerar wrote on 07/20/16 16:38: >>>> Most of my concerns about this ware already raised by Brian (l18n, >>>> extra >>>> size of distro, inconsistencies between subsystems, ...) >>>> >>>> Also on top of this, having something like that would glorify >>>> configuration of server via XML, >>>> which is something we are trying to discourage in favor of CLI / >>>> mgmt api. >>> >>> Excuse my ignorance if this is already taken care of. But last time I >>> looked, using whatever management API was not very nice for container >>> runtime. i.e. configuration changed through API was not persisted. >>> I am wondering if this is already fixed somehow or is at least somebody >>> considering it as an important use case (I think it should). >> >> Persisting server configuration changes made via the supported >> management APIs back to the config file has been done in AS 7.0 and >> later. Its a fundamental requirement. > > I should have explained better. The config is saved on container. But > container being stateless loses changes on restart. At least for > OpenShift v2 that has been an issue. > Thanks; I figured I was missing something. :) > Shame on me I didn't look at OpenShift v3 integration. I'm not sure > how it is handled there. Perhaps I should have done my homework before > hijacking the thread. Sorry about that. Hopefully I can check wildfly > on v3 next week. Jason has already replied with what I was going to say about ways to prepare the config other than editing xml. I’ll just follow up a bit on what he said by mentioning that I presented at a meeting of the folks who prepare middleware images for use on OpenShift and showed them the offline CLI, which they were interested in since it eases scripting of configuration. I don’t know how much it’s been adapted since then though. I recognize that end users are going to want to edit xml, and that’s fine, people can use techniques that best fit their preferences. I think it’s mistaken though for *tools* to be based on manipulating our xml configs. We work hard to ensure long term compatibility in our management APIs, but we have no such guarantees about our xml formats. We encourage normal users to prefer our management APIs over xml for the same reason, but an individual having to check the xsd again if our format changes is a smaller problem than a tool having to be rewritten.

On Aug 9, 2016, at 3:13 PM, Toby Crawley <tcrawley(a)redhat.com&gt; 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.

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.

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?

* 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)

- 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.

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.

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&gt; wrote: > On Tue, Jul 19, 2016 at 4:07 PM, Brian Stansberry > <brian.stansberry(a)redhat.com&gt; 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&gt; wrote: >>> >>>> On Tue, Jul 19, 2016 at 3:15 PM, Brian Stansberry >>>> <brian.stansberry(a)redhat.com&gt; 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

On Aug 17, 2016, at 11:50 AM, Toby Crawley <tcrawley(a)redhat.com&gt; wrote: On Mon, Aug 15, 2016 at 8:08 PM, Brian Stansberry <brian.stansberry(a)redhat.com&gt; wrote: > >> On Aug 9, 2016, at 3:13 PM, Toby Crawley <tcrawley(a)redhat.com&gt; wrote: >> * 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. That failure generates https://gist.github.com/tobias/dbf1fc6bb1e264273c0c96453eb9295d. Since VDX doesn't have further context about the error, it can't point directly to the offending attribute; it just uses the provided [row,col] from the error. Any XMLStreamExceptions that don't come from ParseUtils should look the same.