MartinKanters commented on a change in pull request #486:
URL: https://github.com/apache/maven/pull/486#discussion_r667495020
##########
File path:
maven-model-transform/src/main/java/org/apache/maven/model/transform/FastForwardFilter.java
##########
@@ -53,75 +53,50 @@
private int domDepth = 0;
- private ContentHandler originalHandler;
-
- FastForwardFilter()
+ FastForwardFilter( XmlPullParser xmlPullParser )
{
- super();
- }
-
- FastForwardFilter( AbstractSAXFilter parent )
- {
- super( parent );
+ super( xmlPullParser );
}
@Override
- public void startElement( String uri, String localName, String qName,
Attributes atts )
- throws SAXException
+ protected boolean accept() throws XmlPullParserException, IOException
{
- super.startElement( uri, localName, qName, atts );
- if ( domDepth > 0 )
+ if ( xmlPullParser.getEventType() == START_TAG )
{
- domDepth++;
- }
- else
- {
- final String key = state.peek() + '.' + localName;
- switch ( key )
+ String localName = xmlPullParser.getName();
+ if ( domDepth > 0 )
{
- case "execution.configuration":
- case "plugin.configuration":
- case "plugin.goals":
- case "profile.reports":
- case "project.reports":
- case "reportSet.configuration":
- domDepth++;
-
- originalHandler = getContentHandler();
-
- ContentHandler outputContentHandler = getContentHandler();
- while ( outputContentHandler instanceof XMLFilter )
- {
- outputContentHandler = ( (XMLFilter)
outputContentHandler ).getContentHandler();
- }
- setContentHandler( outputContentHandler );
- break;
- default:
- break;
+ domDepth++;
}
- state.push( localName );
- }
- }
-
- @Override
- public void endElement( String uri, String localName, String qName )
- throws SAXException
- {
- if ( domDepth > 0 )
- {
- domDepth--;
-
- if ( domDepth == 0 )
+ else
{
- setContentHandler( originalHandler );
+ final String key = state.peek() + '/' + localName;
+ switch ( key )
+ {
+ case "execution/configuration":
+ case "plugin/configuration":
+ case "plugin/goals":
+ case "profile/reports":
+ case "project/reports":
+ case "reportSet/configuration":
+ domDepth++;
+ disable();
+ break;
+ default:
+ break;
+ }
}
+ state.add( localName );
}
- else
+ else if ( xmlPullParser.getEventType() == END_TAG )
{
+ if ( --domDepth == 0 )
Review comment:
Small one, I prefer splitting up decreasing the variable and having this
condition. It might be just me, but my brain takes a couple of extra cycles
remembering whether the return value is before or after the operation.
##########
File path:
maven-model-builder/src/main/java/org/apache/maven/model/io/DefaultModelReader.java
##########
@@ -137,24 +122,66 @@ private TransformerContext getTransformerContext(
Map<String, ?> options )
return (TransformerContext) value;
}
- private Model read( Reader reader, boolean strict, InputSource source )
+ private Model read( Reader reader, Path pomFile, Map<String, ?> options )
throws IOException
{
try
{
- if ( source != null )
+ XmlPullParser parser = new MXParser(
EntityReplacementMap.defaultEntityReplacementMap );
+ parser.setInput( reader );
+
+ TransformerContext context = getTransformerContext( options );
+ if ( context != null )
+ {
+ parser = transformer.transform( parser, pomFile, context );
+ }
+
+ // TODO: avoid or at least cache reflection data
+ InputSource source = getSource( options );
+ boolean strict = isStrict( options );
+ try
{
- return new MavenXpp3ReaderEx().read( reader, strict, source );
+ if ( source != null )
+ {
+ MavenXpp3ReaderEx mr = new MavenXpp3ReaderEx();
+ Method readMethod = mr.getClass().getDeclaredMethod(
"read",
+ XmlPullParser.class, boolean.class,
InputSource.class );
+ readMethod.setAccessible( true );
+ Object model = readMethod.invoke( mr, parser, strict,
source );
+ return (Model) model;
+ }
+ else
+ {
+ MavenXpp3Reader mr = new MavenXpp3Reader();
+ Method readMethod = mr.getClass().getDeclaredMethod(
"read",
+ XmlPullParser.class, boolean.class );
Review comment:
I think this fits on one line
##########
File path:
maven-model-transform/src/main/java/org/apache/maven/model/transform/FastForwardFilter.java
##########
@@ -19,23 +19,23 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayDeque;
import java.util.Deque;
-import org.apache.maven.model.transform.sax.AbstractSAXFilter;
-import org.xml.sax.Attributes;
-import org.xml.sax.ContentHandler;
-import org.xml.sax.SAXException;
-import org.xml.sax.XMLFilter;
+import org.apache.maven.model.transform.pull.BufferingParser;
+import org.codehaus.plexus.util.xml.pull.XmlPullParser;
+import org.codehaus.plexus.util.xml.pull.XmlPullParserException;
/**
* This filter will skip all following filters and write directly to the
output.
* Should be used in case of a DOM that should not be effected by other
filters, even though the elements match
Review comment:
Let's finish the sentence with a dot here, while we are at it.
##########
File path:
maven-model-transform/src/test/java/org/apache/maven/model/transform/ConsumerPomXMLFilterTest.java
##########
@@ -82,10 +69,9 @@ protected AbstractSAXFilter getFilter(
Consumer<LexicalHandler> lexicalHandlerCo
};
- RawToConsumerPomXMLFilter filter =
- new RawToConsumerPomXMLFilterFactory( buildPomXMLFilterFactory
).get( Paths.get( "pom.xml" ) );
- filter.setFeature( "http://xml.org/sax/features/namespaces";, true );
- return filter;
+ parser = new RawToConsumerPomXMLFilterFactory(
buildPomXMLFilterFactory )
+ .get( parser, Paths.get( "pom.xml" ) );
+ return parser;
Review comment:
Same comment here, but in this case you can directly return the
statement: `return new RawToConsumerPomXMLFilterFactory(...`
##########
File path:
maven-model-builder/src/main/java/org/apache/maven/model/io/DefaultModelReader.java
##########
@@ -137,24 +122,66 @@ private TransformerContext getTransformerContext(
Map<String, ?> options )
return (TransformerContext) value;
}
- private Model read( Reader reader, boolean strict, InputSource source )
+ private Model read( Reader reader, Path pomFile, Map<String, ?> options )
throws IOException
{
try
{
- if ( source != null )
+ XmlPullParser parser = new MXParser(
EntityReplacementMap.defaultEntityReplacementMap );
+ parser.setInput( reader );
+
+ TransformerContext context = getTransformerContext( options );
+ if ( context != null )
+ {
+ parser = transformer.transform( parser, pomFile, context );
+ }
+
+ // TODO: avoid or at least cache reflection data
Review comment:
Do you intend solving this TODO in this PR? Otherwise I guess we should
create a ticket on JIRA to solve this somewhere in the future. (and link the
ticket number in this comment)
##########
File path:
maven-model-transform/src/main/java/org/apache/maven/model/transform/BuildToRawPomXMLFilterFactory.java
##########
@@ -44,74 +36,40 @@
{
private final boolean consume;
- private final Consumer<LexicalHandler> lexicalHandlerConsumer;
-
- public BuildToRawPomXMLFilterFactory( Consumer<LexicalHandler>
lexicalHandlerConsumer )
+ public BuildToRawPomXMLFilterFactory()
{
- this( lexicalHandlerConsumer, false );
+ this( false );
}
- public BuildToRawPomXMLFilterFactory( Consumer<LexicalHandler>
lexicalHandlerConsumer, boolean consume )
+ public BuildToRawPomXMLFilterFactory( boolean consume )
{
- this.lexicalHandlerConsumer = lexicalHandlerConsumer;
this.consume = consume;
}
/**
*
* @param projectFile will be used by ConsumerPomXMLFilter to get the
right filter
- * @throws SAXException
- * @throws ParserConfigurationException
- * @throws TransformerConfigurationException
*/
- public final BuildToRawPomXMLFilter get( Path projectFile )
- throws SAXException, ParserConfigurationException,
TransformerConfigurationException
- {
- AbstractSAXFilter parent = new AbstractSAXFilter();
- parent.setParent( getXMLReader() );
- if ( lexicalHandlerConsumer != null )
- {
- lexicalHandlerConsumer.accept( parent );
- }
+ public final XmlPullParser get( XmlPullParser parser, Path projectFile )
+ {
if ( getDependencyKeyToVersionMapper() != null )
{
- ReactorDependencyXMLFilter reactorDependencyXMLFilter =
- new ReactorDependencyXMLFilter(
getDependencyKeyToVersionMapper() );
- reactorDependencyXMLFilter.setParent( parent );
- parent.setLexicalHandler( reactorDependencyXMLFilter );
- parent = reactorDependencyXMLFilter;
+ parser = new ReactorDependencyXMLFilter( parser,
getDependencyKeyToVersionMapper() );
}
if ( getRelativePathMapper() != null )
{
- ParentXMLFilter parentFilter = new ParentXMLFilter(
getRelativePathMapper() );
- parentFilter.setProjectPath( projectFile.getParent() );
- parentFilter.setParent( parent );
- parent.setLexicalHandler( parentFilter );
- parent = parentFilter;
+ parser = new ParentXMLFilter( parser, getRelativePathMapper(),
projectFile.getParent() );
}
- CiFriendlyXMLFilter ciFriendlyFilter = new CiFriendlyXMLFilter(
consume );
+ CiFriendlyXMLFilter ciFriendlyFilter = new CiFriendlyXMLFilter(
parser, consume );
getChangelist().ifPresent( ciFriendlyFilter::setChangelist );
getRevision().ifPresent( ciFriendlyFilter::setRevision );
getSha1().ifPresent( ciFriendlyFilter::setSha1 );
+ parser = ciFriendlyFilter;
- if ( ciFriendlyFilter.isSet() )
- {
- ciFriendlyFilter.setParent( parent );
- parent.setLexicalHandler( ciFriendlyFilter );
- parent = ciFriendlyFilter;
- }
-
- return new BuildToRawPomXMLFilter( parent );
- }
-
- private XMLReader getXMLReader() throws SAXException,
ParserConfigurationException
- {
- XMLReader xmlReader = Factories.newXMLReader();
- xmlReader.setFeature( "http://xml.org/sax/features/namespaces";, true );
- return xmlReader;
+ return parser;
Review comment:
The `parser` param is overwritten a couple of times, which can result
into unexpected results for the calling code. I think creating a new parser
variable at the start of the method which would be returned in the end is more
clear.
##########
File path:
maven-model-transform/src/test/java/org/apache/maven/model/transform/ModulesXMLFilterTest.java
##########
@@ -19,23 +19,19 @@
* under the License.
*/
-import static org.xmlunit.assertj.XmlAssert.assertThat;
-
-import java.util.function.Consumer;
-
+import org.codehaus.plexus.util.xml.pull.XmlPullParser;
import org.junit.jupiter.api.Test;
-import org.xml.sax.ext.LexicalHandler;
+
+import static org.xmlunit.assertj.XmlAssert.assertThat;
public class ModulesXMLFilterTest
extends AbstractXMLFilterTests
{
@Override
- protected ModulesXMLFilter getFilter( Consumer<LexicalHandler>
lexicalHandlerConsumer )
+ protected ModulesXMLFilter getFilter(XmlPullParser parser)
Review comment:
Checkstyle should not like this, the param should be prefixed and
suffixed with spaces
##########
File path:
maven-model-transform/src/main/java/org/apache/maven/model/transform/RawToConsumerPomXMLFilterFactory.java
##########
@@ -41,20 +37,19 @@ public RawToConsumerPomXMLFilterFactory(
BuildToRawPomXMLFilterFactory buildPomX
this.buildPomXMLFilterFactory = buildPomXMLFilterFactory;
}
- public final RawToConsumerPomXMLFilter get( Path projectPath )
- throws SAXException, ParserConfigurationException,
TransformerConfigurationException
+ public final XmlPullParser get( XmlPullParser parser, Path projectPath )
{
- BuildToRawPomXMLFilter parent = buildPomXMLFilterFactory.get(
projectPath );
+ parser = buildPomXMLFilterFactory.get( parser, projectPath );
// Ensure that xs:any elements aren't touched by next filters
- AbstractSAXFilter filter = new FastForwardFilter( parent );
+ parser = new FastForwardFilter( parser );
// Strip modules
- filter = new ModulesXMLFilter( filter );
+ parser = new ModulesXMLFilter( parser );
// Adjust relativePath
- filter = new RelativePathXMLFilter( filter );
+ parser = new RelativePathXMLFilter( parser );
- return new RawToConsumerPomXMLFilter( filter );
+ return parser;
Review comment:
Same comment as above, let's not alter the parameter
##########
File path:
maven-core/src/main/java/org/apache/maven/internal/aether/ConsumerModelSourceTransformer.java
##########
@@ -23,91 +23,30 @@
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.util.function.Consumer;
-import javax.xml.parsers.ParserConfigurationException;
-import javax.xml.stream.XMLInputFactory;
-import javax.xml.stream.XMLStreamException;
-import javax.xml.stream.XMLStreamReader;
-import javax.xml.transform.OutputKeys;
-import javax.xml.transform.Transformer;
-import javax.xml.transform.TransformerConfigurationException;
-import javax.xml.transform.sax.SAXTransformerFactory;
-import javax.xml.transform.sax.TransformerHandler;
-
-import org.apache.maven.model.building.AbstractModelSourceTransformer;
import org.apache.maven.model.building.DefaultBuildPomXMLFilterFactory;
import org.apache.maven.model.building.TransformerContext;
-import org.apache.maven.model.transform.sax.AbstractSAXFilter;
-import org.apache.maven.xml.internal.DefaultConsumerPomXMLFilterFactory;
-import org.xml.sax.SAXException;
-import org.xml.sax.ext.LexicalHandler;
-
-class ConsumerModelSourceTransformer extends AbstractModelSourceTransformer
+import org.apache.maven.model.transform.RawToConsumerPomXMLFilterFactory;
+import org.apache.maven.model.transform.pull.XmlUtils;
+import org.codehaus.plexus.util.ReaderFactory;
+import org.codehaus.plexus.util.xml.XmlStreamReader;
+import org.codehaus.plexus.util.xml.pull.EntityReplacementMap;
+import org.codehaus.plexus.util.xml.pull.MXParser;
+import org.codehaus.plexus.util.xml.pull.XmlPullParser;
+import org.codehaus.plexus.util.xml.pull.XmlPullParserException;
+
+class ConsumerModelSourceTransformer
{
- @Override
- protected AbstractSAXFilter getSAXFilter( Path pomFile,
- TransformerContext context,
- Consumer<LexicalHandler>
lexicalHandlerConsumer )
- throws TransformerConfigurationException, SAXException,
ParserConfigurationException
- {
- return new DefaultConsumerPomXMLFilterFactory( new
DefaultBuildPomXMLFilterFactory( context,
-
lexicalHandlerConsumer, true ) ).get( pomFile );
- }
-
- /**
- * This transformer will ensure that encoding and version are kept.
- * However, it cannot prevent:
- * <ul>
- * <li>attributes will be on one line</li>
- * <li>Unnecessary whitespace before the rootelement will be removed</li>
- * </ul>
- */
- @Override
- protected TransformerHandler getTransformerHandler( Path pomFile )
- throws IOException,
org.apache.maven.model.building.TransformerException
+ public InputStream transform( Path pomFile, TransformerContext context )
+ throws IOException, XmlPullParserException
{
- final TransformerHandler transformerHandler;
-
- final SAXTransformerFactory transformerFactory =
getTransformerFactory();
-
- // Keep same encoding+version
- try ( InputStream input = Files.newInputStream( pomFile ) )
- {
- XMLStreamReader streamReader =
- XMLInputFactory.newFactory().createXMLStreamReader( input );
-
- transformerHandler = transformerFactory.newTransformerHandler();
-
- final String encoding = streamReader.getCharacterEncodingScheme();
- final String version = streamReader.getVersion();
-
- Transformer transformer = transformerHandler.getTransformer();
- transformer.setOutputProperty( OutputKeys.METHOD, "xml" );
- if ( encoding == null && version == null )
- {
- transformer.setOutputProperty(
OutputKeys.OMIT_XML_DECLARATION, "yes" );
- }
- else
- {
- transformer.setOutputProperty(
OutputKeys.OMIT_XML_DECLARATION, "no" );
+ XmlStreamReader reader = ReaderFactory.newXmlReader(
Files.newInputStream( pomFile ) );
+ XmlPullParser parser = new MXParser(
EntityReplacementMap.defaultEntityReplacementMap );
+ parser.setInput( reader );
+ parser = new RawToConsumerPomXMLFilterFactory( new
DefaultBuildPomXMLFilterFactory( context, true ) )
Review comment:
Instead of overwriting the old parser, I think it's clearer if we create
a new variable for it.
##########
File path:
maven-model-transform/src/main/java/org/apache/maven/model/transform/RelativePathXMLFilter.java
##########
@@ -19,90 +19,60 @@
* under the License.
*/
-import org.xml.sax.Attributes;
-import org.xml.sax.SAXException;
+import java.util.List;
-import org.apache.maven.model.transform.sax.AbstractSAXFilter;
+import org.apache.maven.model.transform.pull.NodeBufferingParser;
+import org.codehaus.plexus.util.xml.pull.XmlPullParser;
/**
* Remove relativePath element, has no value for consumer pom
*
* @author Robert Scholte
+ * @author Guillaume Nodet
* @since 4.0.0
*/
-class RelativePathXMLFilter
- extends AbstractEventXMLFilter
+public class RelativePathXMLFilter extends NodeBufferingParser
{
- private boolean parsingParent;
- private String state;
-
- RelativePathXMLFilter()
- {
- super();
- }
-
- RelativePathXMLFilter( AbstractSAXFilter parent )
+ public RelativePathXMLFilter( XmlPullParser xmlPullParser )
{
- super( parent );
+ super( xmlPullParser, "parent" );
}
- @Override
- public void startElement( String uri, final String localName, String
qName, Attributes atts )
- throws SAXException
+ protected void process( List<Event> buffer )
{
- if ( !parsingParent && "parent".equals( localName ) )
+ boolean skip = false;
+ Event prev = null;
+ for ( Event event : buffer )
{
- parsingParent = true;
- }
-
- if ( parsingParent )
- {
- state = localName;
- }
-
- super.startElement( uri, localName, qName, atts );
- }
-
- @Override
- public void endElement( String uri, String localName, String qName )
- throws SAXException
- {
- if ( parsingParent )
- {
- switch ( localName )
+ if ( event.event == START_TAG && "relativePath".equals( event.name
) )
{
- case "parent":
- executeEvents();
-
- parsingParent = false;
- break;
- default:
- break;
+ skip = true;
+ if ( prev != null && prev.event == TEXT && prev.text.matches(
"\\s+" ) )
+ {
+ prev = null;
+ }
+ event = null;
+ }
+ else if ( event.event == END_TAG && "relativePath".equals(
event.name ) )
+ {
+ skip = false;
+ event = null;
+ }
+ else
+ {
+ if ( skip )
Review comment:
This if can combined with the else on line 62
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
