Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally enforce required elements in XSDs #263

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ public class ModelloParameterConstants
*/
public static final String EXTENDED_CLASSNAME_SUFFIX = "modello.xpp3.extended.suffix";

/**
* Boolean flag relaxing XSD generation with regards to mandatory elements.
* If set to {@code true} will not require mandatory elements in the XML which can be useful if the XML is post processed (e.g. POM merging with parents)
* @since 2.1
*/
public static final String XSD_MANDATORY_ELEMENTS_NOT_ENFORCED = "modello.xsd.mandatory.element.not.enforced";
private ModelloParameterConstants()
{
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,15 @@ public class ModelloXsdMojo
@Parameter( defaultValue = "${project.build.directory}/generated-site/resources/xsd", required = true )
private File outputDirectory;

/**
* Boolean flag relaxing XSD generation with regards to mandatory elements.
* If set to {@code true} will not require mandatory elements in the XML which can be useful if the XML is post processed (e.g. POM merging with parents).
* The default value is true for backwards compatibility reasons, but should be set to false for most cases.
* @since 2.1.0
*/
@Parameter( defaultValue = "true")
private boolean areMandatoryElementsNotEnforced;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look very usual to have a boolean property starting with is or are.
Usually, the getters are written isXxx but the field is most often named without the is/are prefix.
I would propose to invert it and name it enforceMandatoryElements and default it to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rename but the default should be to enforce mandatory elements. The POM case is really an unusual edge case IMHO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have missed something, but the current default value is true for not enforcing mandatory elements. I'm fine with changing the default value though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The javadoc should be changed accordingly if the default value is to be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my bad. I will think again about defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as you proposed @gnodet in 9ba65d8.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets start with the default being no enforcement for backwards-compatibility.


/**
*
* @since 1.0-alpha-21
Expand All @@ -65,6 +74,7 @@ protected void customizeParameters( Properties parameters )
{
parameters.put( ModelloParameterConstants.OUTPUT_XSD_FILE_NAME, xsdFileName );
}
parameters.put( ModelloParameterConstants.XSD_MANDATORY_ELEMENTS_NOT_ENFORCED, areMandatoryElementsNotEnforced );
}

protected boolean producesCompilableResult()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ private void generateXsd( Properties parameters )

// we assume parameters not null
String xsdFileName = parameters.getProperty( ModelloParameterConstants.OUTPUT_XSD_FILE_NAME );
boolean areMandatoryElementsEnforced = !Boolean.valueOf( parameters.getProperty( ModelloParameterConstants.XSD_MANDATORY_ELEMENTS_NOT_ENFORCED ) );

File f = new File( directory, objectModel.getId() + "-" + getGeneratedVersion() + ".xsd" );

Expand Down Expand Up @@ -137,7 +138,8 @@ private void generateXsd( Properties parameters )
// Element descriptors
// Traverse from root so "abstract" models aren't included
int initialCapacity = objectModel.getClasses( getGeneratedVersion() ).size();
writeComplexTypeDescriptor( w, objectModel, root, new HashSet<ModelClass>( initialCapacity ) );
writeComplexTypeDescriptor( w, objectModel, root, new HashSet<ModelClass>( initialCapacity ),
areMandatoryElementsEnforced );

w.endElement();
}
Expand Down Expand Up @@ -184,7 +186,7 @@ private static void writeDocumentation( XMLWriter w, String version, String desc
}

private void writeComplexTypeDescriptor( XMLWriter w, Model objectModel, ModelClass modelClass,
Set<ModelClass> written )
Set<ModelClass> written, boolean areMandatoryElementsEnforced )
{
written.add( modelClass );

Expand Down Expand Up @@ -242,9 +244,12 @@ private void writeComplexTypeDescriptor( XMLWriter w, Model objectModel, ModelCl
{
w.startElement( "xs:element" );

// Usually, would only do this if the field is not "required", but due to inheritance, it may be
// present, even if not here, so we need to let it slide
w.addAttribute( "minOccurs", "0" );
if ( !areMandatoryElementsEnforced || !field.isRequired() )
{
// Usually, would only do this if the field is not "required", but due to inheritance, it may be
// present, even if not here, so we need to let it slide
w.addAttribute( "minOccurs", "0" );
}
}

String xsdType = getXsdType( field.getType() );
Expand Down Expand Up @@ -428,7 +433,7 @@ else if ( xsdType == null )
{
if ( !written.contains( fieldModelClass ) )
{
writeComplexTypeDescriptor( w, objectModel, fieldModelClass, written );
writeComplexTypeDescriptor( w, objectModel, fieldModelClass, written, areMandatoryElementsEnforced );
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,19 @@ public void testXsdGenerator()
catch ( SAXParseException e )
{
// ok, expected exception
assertTrue( String.valueOf( e.getMessage() ).contains( "invalidElement" ) );
assertTrue( e.getMessage().contains( "invalidElement" ) );
}

try
{
validator.validate( new StreamSource( getClass().getResourceAsStream( "/features-missing-required.xml" ) ) );
fail( "parsing of features-invalid.xml should have failed" );
}
catch ( SAXParseException e )
{
// ok, expected exception
assertTrue( e.getMessage().contains( "description" ) );
assertTrue( e.getMessage().endsWith(" is expected."));
}

try
Expand All @@ -91,7 +103,7 @@ public void testXsdGenerator()
catch ( SAXParseException e )
{
// ok, expected exception
assertTrue( String.valueOf( e.getMessage() ).contains( "transientString" ) );
assertTrue( e.getMessage().contains( "transientString" ) );
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/

import org.codehaus.modello.AbstractModelloGeneratorTest;
import org.codehaus.modello.ModelloParameterConstants;
import org.codehaus.modello.core.ModelloCore;
import org.codehaus.modello.model.Model;
import org.xml.sax.SAXException;
Expand Down Expand Up @@ -55,6 +56,7 @@ public void testXsdGenerator()
ModelloCore modello = (ModelloCore) lookup( ModelloCore.ROLE );

Properties parameters = getModelloParameters( "1.4.0" );
parameters.setProperty( ModelloParameterConstants.XSD_MANDATORY_ELEMENTS_NOT_ENFORCED, "true" );

Model model = modello.loadModel( getTestFile( "../../src/main/mdo/modello.mdo" ) );

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?xml version="1.0"?>

<features-demo xmlns="http://codehaus-plexus.github.io/FEATURES/1.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://codehaus-plexus.github.io/FEATURES/1.0.0 http://codehaus-plexus.github.io/features-1.0.0.xsd">
</features-demo>
2 changes: 1 addition & 1 deletion modello-test/src/main/resources/features.xml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<features-demo xmlns="http://codehaus-plexus.github.io/FEATURES/1.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://codehaus-plexus.github.io/FEATURES/1.0.0 http://codehaus-plexus.github.io/features-1.0.0.xsd">
<versionField>1.0.0</versionField>
<!--required></required--><!-- TODO: this field is marked required but not set in the XML, should fail... -->
<required></required>
<identifier>id</identifier>
<identifierPart2><id>reference</id></identifierPart2>

Expand Down