Details
-
Type:
New Feature
-
Status:
Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 1.3.1
-
Fix Version/s: 1.4
-
Labels:None
-
Environment:All
-
Number of attachments :2
Description
AbstractSchemagenMojo sneakily hard-codes the resulting filename for the generated schema file.
This should really be configurable; provided a parameter to set the resulting file name, as well as an integration test.
For reasons of somewhat misguided backwards compatibility, I set the default value of the parameter to its currently hardcoded value, "schema1.xsd".
-
- fileRenamePatch_v14.txt
- 13/Jan/12 4:47 AM
- 170 kB
- Lennart Jorelid
-
- diagramAdditions_1.png
- 30 kB
- 28/Nov/11 11:52 PM
Issue Links
- depends upon
-
MOJO-1786
Switch to maven-fluido-skin as default skin
-
Activity
Unfortunately the solution is not this simple. The SchemaGen produces one XML Schema file per namespace. So if there are more than one namespace, "schema1.xsd", "schema2.xsd", etc is created. Your code does not handle that, nor does the test check this.
I still have my doubts if we should include this. Like Anders already discovered: it's not so straightforward.
Jaxb will always generate at least the schema1.xsd file, that's the reason for the current implementation.
Users can always change the outputDirectory to a temp dir and copy/rename file to a final output directory (just google)
The solution is rather straightforward, gentlemen; I have created a decently complete implementation and test case for the scenario where several namespaces are used within several JAXB-annotated model files.
Moreover - I believe the renaming feature within the plugin to be very valuable in any decently large project (and not as an afterthought).
Since generated schema files contain references to other schema file names within the same JAXB generation unit, it is important that all renaming is done at the same time.
Typical scenario:
a) [fact] JAXB requires the schema file/stream to enable validation upon serialization
b) [fact] JAXB validation requires you to match the marshaled entity with its generated schema
c) [scenario] Your deliverable contains 40 maven projects, each holding several JAXB-annotated entity classes within separate namespaces
d) [fact] Generated schema files refer between their own namespace and the generated filename of each imported namespace (see below). This is complex and messy if all schema file names are called schemaX.xsd.
e) [fact] Maven's pom should be as simple as possible. Noone wants to mess around with several plugins to achieve the effect that one plugin should provide. In this case, the JAXB maven plugin should provide filename alteration and consistent schema transformation (i.e. xs:import namespace="http://another/namespace" schemaLocation="whateverIWannaCallTheFile.xsd").
Consider the generated multi-namespace schema below (note the import statement referring to another namespace/generated schema file).
Multiply the complexity of renaming all generated schema files with the number of projects... and I hope you reach the same conclusion as me.
Renaming is the job of our plugin, not a set of other plugins working in concert. Simplicity is king.
<?xml version="1.0" encoding="UTF-8" standalone="yes"?> <xs:schema version="1.0" targetNamespace="http://some/namespace" xmlns:ns1="http://another/namespace" xmlns:xs="http://www.w3.org/2001/XMLSchema"> <xs:import namespace="http://another/namespace" schemaLocation="schema3.xsd"/> <xs:element name="anOptionalElementInSomeNamespace" type="xs:string"/> <xs:complexType name="fooBar"> <xs:sequence> <xs:element name="requiredElement" type="xs:string" default="requiredElementValue"/> <xs:element ref="ns1:aRequiredElementInAnotherNamespace" default="requiredElementValue"/> <xs:element name="optionalElement" type="xs:string" minOccurs="0"/> </xs:sequence> <xs:attribute name="requiredAttribute" type="xs:string" use="required"/> <xs:attribute name="optionalAttribute" type="xs:string"/> </xs:complexType> </xs:schema>
How about trying to get this support into JAXB RI first (maybe it already exists in v2.2? I haven't checked) and then we can use it? It exists for the JAXB RI v2.1 Ant task, but we're not using the same code as it is.
I agree that the best way of achieving this support would be to fix the problem within the SchemaGenerator.
(The infamous "There is no way to control the name of the generated schema files at this time." sentence seems to set the level here).
If such a thing is done one needs not only enable supplying proper file names for generated schema,
but also provide namespace prefix control to avoid the "ns1:", "ns2:" syndrome. However, we can rather quickly implement
both these things within the JAXB plugin; what are your time estimates for a process to make these changes within the
SchemaGenerator?
I think I suggest doing both these things in parallel - we can easily switch the transformation to arguments supplied to SchemaGenerator when we have such opportunities.
The integration tests should still be useable in both scenarios.
- http://jaxb.java.net/nonav/2.2.1/docs/schemagen.html
- http://jaxb.java.net/nonav/2.2.1/docs/schemagenTask.html
As you could see, generation and renaming is done in two steps, the input and outputdirectory are separated.
To stay close the ant-syntax we could add:
<schemas> <schema> <namespace/> <file/> </schema> </schemas>
we've already created a jaxb workingdirectory.
If there are schemas specified, use this workingdirectory, otherwise generate does sources directly under target/generated-resources/jaxb
The configuration for the schema definition is as per below.
Due to Maven's implicit reflection rules, we cannot arrive closer to the ant syntax.
<configuration> <schemas> <schema> <uri>http://some/namespace</uri> <prefix>some</prefix> <file>some_schema</file> </schema> <schema> <uri>http://another/namespace</uri> <prefix>another</prefix> <file>another_schema</file> </schema> <schema> <uri>http://yet/another/namespace</uri> <prefix>yetAnother</prefix> <file>yet_another_schema</file> </schema> </schemas> ... </configuration>
Attached a newer patch, providing support for configuring multiple schema namespaces. For each namespace, the following can be set:
- changing the name of the output schema file
- The "schemaX.xsd" file name can be replaced by an arbitrary name. The ".xsd" suffix is automatically appended, so not necessary to supply within the plugin configuration.
- All schemaLocation attributes are altered within schema import elements to refer to the new file names, as needed to keep consistency.
- changing the xmlns prefix within the XSD for each namespace
- The "nsX" namespace can be replaced by an arbitrary value. This yields appropriate xmlns:[whatever]="[namespace uri]" definitions within the schema header
- All namespace prefixes in elements are altered to keep consistency.
- All ref attribute values are altered to keep consistency.
Couldn't find the IDEA codestyle definition used by yourselves; please point me in the right direction to find it.
Until found, you will need to reformat all source code within your IDEs.
The patch is diff'ed from trunk.
New full patch, containing improved site documentation.
Separated the usage of the schemagen mojo from the usage of the xjc mojo. Also attached a separate image archive, since SVN diffs does not treat binary content well.
Can you please take a look at the provided functionality, and relay some feedback gentlemen?
We are now able to change both namespace prefix and generated schema filename in this plugin, which seems to provide more features than the JAXB RI Ant task.
fileRenamePatch, version 4: Added XML normalization and proper validation of transformed results within the verify.bsh.
it-55 now validates the actual generated XSDs against expected ones.
Added it/mjaxb-55-partialdefaults to validate that default values for optional configuration properties are treated correctly.
Re-added some files which went missing in the former diff/patch version.
Moreover, since one of the documentation files was renamed, you will need to hit 'y' at the patch prompt:
... patching file src/site/apt/index.apt.vm The next patch would delete the file src/site/apt/usage_xjc.apt.vm, which does not exist! Assume -R? [n] y patching file src/site/apt/usage_xjc.apt.vm ...
The patch still works correctly, yielding a working plugin build:
... [INFO] Building: mjaxb-55/pom.xml [INFO] ..SUCCESS (5.1 s) [INFO] Building: mjaxb-55-partialdefaults/pom.xml [INFO] ..SUCCESS (5.1 s) [INFO] Building: schemagen-main/pom.xml [INFO] ..SUCCESS (4.9 s) [INFO] Building: schemagen-test/pom.xml [INFO] ..SUCCESS (4.9 s) [INFO] Building: xjc-multiple-xsd-specified/pom.xml [INFO] ..SUCCESS (6.1 s) [INFO] --------------------------------------- [INFO] Execution Summary: [INFO] Builds Passing: 13 [INFO] Builds Failing: 0 [INFO] Builds Skipped: 0 [INFO] --------------------------------------- ...
Changed packages at the request of Anders.
Feedback would be appreciated.
New patch version (version 7) uploaded. Feedback, guys!
Updated site to use the maven-fluido-skin, in order to get line numbering on all code listings.
This implies that the site images are not required any more, removing them.
Reformatted source code in accordance with the rest of the project.
And ... feedback on this please.
I unassigned as I'm jammed with other stuff and will not have time to review this in detail right now.
However, I think you should remove the branding stuff ("JGuru Europe AB") in the author tags. In fact, drop the author line all together.
Also, the pom.xml is included in your patch without any functional changes (just space changes). It should not be included.
Not sure about the skin change though. We should probably discuss that at mojo-dev first. Possibly, we should also handle that in a separate ticket.
Robert: Do you have any thoughts on this? If we can handle it correctly I think it would be a nice feature to include. Obviously, there is at least one person that needs it. ![]()
I'll pick it up from here, but it will take some time to verify the patch, it's quite huge. I agree with Anders' comments. The skinning-part should be discussed at a higher level. Please focus on the issue you're trying to fix.
Great with the feedback, gentlemen.
I see three comments above. Here are my thoughts regarding them:
a) Skin.
I am quite certain that there is a need to discuss a site skin change between all the mojo projects
within codehaus. Feel free to start/carry on that discussion as you see fit.
The skin is needed by this patch since I refer to specific lines within the code shown in the
faq and examples. AFAIK the only skin currently providing line numbering within code snippets
is the fluido skin - if you know of any other means to achieve this effect, please
point me in that direction.
I would guess that the need for line numbers/syntax coloring is not isolated to the jaxb2-maven-plugin project.
Most other projects could (or should) document their usage with examples in the same manner as I have tried
to do for the jaxb2-maven-plugin. I don't have any specific requirements regarding the look of the skin - only
the line numbering/syntax coloring in code snippets is relevant. (C.f. the comments within the "faq"
and "usage - schemagen").
b) Focus.
The focus of this patch has been 3 things - 2 features enhancements (assigning file name, assigning prefix), and
1 documentation upgrade (there was no/insufficient documentation on schemagen usage within the plugin site
before this patch). I would argue that the documentation improvements are indeed within the focus of delivering
the patch.
In its current form, Doxia does not have a core mechanism to indicate that a code block should be rendered with
line numbers or syntax coloring (although I am working on a patch for Doxia to handle those mechanics).
c) Author tags.
I can certainly remove the company from the author tag - but not the author tag itself.
All/most other files within the plugin have author tags, so I see no reason why the files I
supplied should be without them. In fact, I think it is good practice to credit the authors
in open source projects within the @author tag.
d) Patch size.
The patch is in total 5 classes and 1 interface, but the addition into the existing framework consist
of only 1 property and 4 statements. The rest of the code is unit tests, integration tests and
documentation. I have focused on readability and documentation within the added classes, rather
than achieving the smallest patch size possible.
WRT verifying the patch, I recommend to start - as always - within the ITs.
Will fix the remaining issues tomorrow, and supply a new patch.
Attached the additions to the plugin, in UML form. Short explanation follows:
The SchemagenHelper utility class holds the algorithms to alter the generated schema files.
Each operation requiring that we loop over the XML within a document and perform some change or data harvest is realized with a NodeProcessor implementation. The JavaDoc of each NodeProcessor implementation supplies an explanation of what change it performs within its process method.
Added dependency to the switch to the fluido skin for all Mojo projects.
@Lennart: Feel free to keep the author tag as I see that most other classes have them (I don't think we should have them though, but that's a separate discussion). However, I strongly feel that we should not have company branding in these tags (except possibly by the email address domain). There is no such thing in existing tags.
I agree, Anders - the reference to company within author tags was removed with fileRenamePatch_v9.txt (done 2011-11-28).
It should never have made it into the patch in the first place - it was a "standard file template setup in the IDE" kind of thing.
Added cobertura coverage report to the site.
Added unit test cases to the helper classes.
I've already patched some parts which already improve current code.
The name schemas is misleading, it should be something like transformSchemas.
I think we should rename 2 properties from the Schema-class: change file to toFile and prefix to toPrefix to make it clear these are the output parameters.
The staleSourceScanner now doesn't work anymore for these schema's. I'd really prefer to keep does original files for 2 reasons: easy staleSource detection and just to be sure the plugin didn't broke the files.
So ... you would like the configuration for the plugin to be
<configuration> <transformSchemas> <transformSchema> <uri>http://some/namespace</uri> <toPrefix>some</toPrefix> <toFile>some_schema</toFile> </transformSchema> <transformSchema> <uri>http://another/namespace</uri> <toPrefix>another</toPrefix> <toFile>another_schema</toFile> </transformSchema> <transformSchema> <uri>http://yet/another/namespace</uri> <toPrefix>yetAnother</toPrefix> <toFile>yet_another_schema</toFile> </transformSchema> </schemas> ... </configuration>
instead of
<configuration> <schemas> <schema> <uri>http://some/namespace</uri> <prefix>some</prefix> <file>some_schema</file> </schema> <schema> <uri>http://another/namespace</uri> <prefix>another</prefix> <file>another_schema</file> </schema> <schema> <uri>http://yet/another/namespace</uri> <prefix>yetAnother</prefix> <file>yet_another_schema</file> </schema> </schemas> ... </configuration>
Doable, but with more typing - and I'm not certain that it becomes more user friendly or less misleading.
(What is the toPrefix of a transformSchema, as opposed to the prefix of a Schema?)
I have changed the code according to your suggestion - but I don't think that the result is better.
If people are in doubt about how to use the plugin, the plugin documentation should be well-written (and conclusive) enough to scatter any doubts.
Regarding keeping the original files:
a) The plugin should be well-written and well-tested enough that we are certain it does not work incorrectly. ("break the files" as you write).
If we find that the plugin is buggy, we should be swift to provide a bug fix and new release. I don't think we should be keeping the original
files on that account. If people do not want to transform the resulting schema, they simply do not provide a
<transformSchema>...</transformSchema> configuration element - and all files generated by SchemaGen will be kept in their original state.
b) The stale file detection could be augmented to take the new/transformed filenames into consideration. I will provide a fix for this.
It would make no sense, I believe, not to redo the transformation after a plugin configuration change (within one of the <transformSchema>
elements for instance).
Changed name of Schema class to TransformSchema. Also changed properties file --> toFile and prefix --> toPrefix.
Updated ITs and site documentation accordingly.
Ran schemagen code through the maven_checks.xml checkstyle.
Formatted according to specification on the maven site.
I've review it again. I've got a few request:
- Could you use xmlunit to compare xml documents? I've already added the dependency. My problem is org.codehaus.mojo.jaxb2.helpers.SchemagenHelperTest.validateProcessingNodes() which seems to fail when using assertXMLEqual( changedDocument, document )
- TransformSchema should be a pojo, so return the toFile as is.
- Use org.codehaus.plexus.util.StringUtils.isEmpty(String) and org.codehaus.plexus.util.StringUtils.isNotEmpty(String), these are null-safe methods.
- The validator is too strict. People can use expressions for certain values, which can be empty. Don't throw an exception, just skip it.
- Add a workDirectory, where the schemaN.xsd can be placed. From here the mojo can pick them up, transform it a write them to the outputDirectory.
- Add yourself as contributer in the pom
Could you use xmlunit to compare xml documents? I've already added the dependency.
My problem is org.codehaus.mojo.jaxb2.helpers.SchemagenHelperTest.validateProcessingNodes()
which seems to fail when using assertXMLEqual( changedDocument, document )
Added and fixed, with a few caveats:
- We might ask the XMLUnit folks to publish their source code on the maven repo1.
At the moment, one has to download the source manually to debug
XMLUnit classes, which seems unnecessarily inconvenient. - The error/diff printouts in XMLUnit Diffs (or DetailedDiffs) WRT namespace inconsistencies
felt really confusing. This is most likely what tripped yourself.
TransformSchema should be a pojo, so return the toFile as is.
TransformSchema is a pojo, with some smarts in one getter aimed at saving unnecessary
typing in the configuration. I suggest we leave it as it is.
The validator is too strict. People can use expressions for certain values, which can be empty.
Don't throw an exception, just skip it.
In the case of a schema transformation, we need at least 2 configuration properties with
non-empty values - namespace URL and either toPrefix or toFile. Any schemagen configuration
not complying with this rule is incorrect, irrespective of how the incorrect configuration
was generated.
The question, then, is "how do we handle incorrect plugin configuration".
My answer is clear, and different from your suggestion:
Plugins given incorrect configuration should not try to "enhance" or "correct" the configuration by guessing what should be appropriate in the general case. Instead, all plugins should fail with an exception message clearly describing what was wrong and how to correct the problem. Plugins can otherwise cause big problems for larger real-life projects (in an example from real life, around 400 maven projects combine to create one enterprise application) where some incorrect plugin configuration can generate major problems while being quite hard to identify/detect.
The method to rectify such difficult-to-detect build problems is that each
plugin throws exceptions as described above.
Therefore, I disagree that the validator is too strict.
Let us release the plugin with exactly the provided level of assistance to the users.
Add a workDirectory, where the schemaN.xsd can be placed.
From here the mojo can pick them up, transform it a write them to the outputDirectory.
Done.
Add yourself as contributer in the pom
Done. Also, please do not remove the cobertura inclusion within the pom.
We need our coverage report.
Could we please get a vote for a 1.4 release of this plugin now?
Last commit on this branch is almost 3 months ago; the patch has been submitted since awhile.
Could you please grant me commit privs for this mojo, so I can assist in getting the tempo up a little?
We need this patch to be submitted to a larger audience.
The patch has improved, but there are still some thing I'd like to be solved differently. From here on I can pick it up myself. Give me some time to adjust your patch.
Fixed in rev. 15920
I've deployed a SNAPSHOT to our snapshot-repository ready for test.
Thanks.
Could we please release version 1.4 of the plugin?
We are still waiting for a stable release with the augmented functionality,
and this plugin has been deployed to the snapshot repo since February.
@selckin: Please file e new (bug) ticket if you've found some error! Also, please include more details so that it is possible to knwo where the error is.
If you provide a patch (with a test) I'll review and commit it.