You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/07/05 14:52:02 UTC
[GitHub] [maven] gnodet opened a new pull request #486: Use the MX xpp parser instead of a STAX transformation
gnodet opened a new pull request #486:
URL: https://github.com/apache/maven/pull/486
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MNG-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MNG-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #486: Use the MX xpp parser instead of a STAX transformation
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-874185595
I think this needs a JIRA issue.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-876277470
It is a bit weird that this IT is using an external plugin for verification. First step would be to update the version of the flatten-maven-plugin. Maybe in the future we need to reconsider rewriting this IT.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-933189288
Re `Caused by: java.lang.NoSuchMethodError: org.apache.maven.plugin.PluginParameterExpressionEvaluator.<init>` see here https://github.com/apache/maven/pull/562#discussion_r720683592
Do you have some ancient enforcer? Or comment is wrong and there is something else (ancient) using this ctor?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] hboutemy commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-981067843
@rfscholte do you see any drawback to using plexus-utils' XPP3?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-933284411
> > Hm, but something is wrong, it WAS ancient and is already upped [apache/maven-integration-testing#115](https://github.com/apache/maven-integration-testing/pull/115)
>
> Eh, update your fork please https://github.com/gnodet/maven-integration-testing (master on apache/maven-integration-testing contains fixes for you: upped enforcer)
Thx, I've rebased both PRs on top of master, which looks much better indeed !
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-933190909
> Re `Caused by: java.lang.NoSuchMethodError: org.apache.maven.plugin.PluginParameterExpressionEvaluator.<init>` see here [#562 (comment)](https://github.com/apache/maven/pull/562#discussion_r720683592) Do you have some ancient enforcer? Or comment is wrong and there is something else (ancient) using this ctor?
Hm, but something is wrong, it WAS ancient and is already upped https://github.com/apache/maven-integration-testing/pull/115
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-923700698
> > Just ran the branch on Jenkins, it is unstable: https://ci-maven.apache.org/blue/organizations/jenkins/Maven%2Fmaven-box%2Fmaven/detail/MNG-7182/1/tests
> > One thing I notice is that it looks like the linebreaks around the license are better compared to what I could do. Please also provide a PR for that.
>
> Yes, it took me a bit of time to get that right ;-)
> I'll have a look at those tests, I thought I did modify them to pass.
I fixed and squashed the PRs.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-921010038
@rfscholte could you please review, I think that one is important to include in master asap to avoid the performance loss in the first alpha.
There's still this small problem on windows + jdk8 (file lock still in place) which I've worked around by using a fork + timeout in the tests, but It does not appear on any other combination and I've been unable to find a place where a file lock would be kept. In addition, I haven't been able to reproduce it in a real environment.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-921919306
> Just ran the branch on Jenkins, it is unstable: https://ci-maven.apache.org/blue/organizations/jenkins/Maven%2Fmaven-box%2Fmaven/detail/MNG-7182/1/tests
> One thing I notice is that it looks like the linebreaks around the license are better compared to what I could do. Please also provide a PR for that.
Yes, it took me a bit of time to get that right ;-)
I'll have a look at those tests, I thought I did modify them to pass.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-876247449
>
>
> There's a single test failing on windows + jdk 1.8. Does that ring any bell @rfscholte ?
> https://github.com/apache/maven/pull/486/checks?check_run_id=3016896271#step:8:6012
Bad code, concurrent write access, Windows file locking.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-921907834
Just ran the branch on Jenkins, it is unstable: https://ci-maven.apache.org/blue/organizations/jenkins/Maven%2Fmaven-box%2Fmaven/detail/MNG-7182/1/tests
One thing I notice is that it looks like the linebreaks around the license are better compared to what I could do. Please also provide a PR for that.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a change in pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #486:
URL: https://github.com/apache/maven/pull/486#discussion_r674625338
##########
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:
I really don't get the problem, but once again, I've added an intermediary variable.
Some people also prefer not returning instances created directly to ease debugging.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas edited a comment on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-933196485
> Hm, but something is wrong, it WAS ancient and is already upped [apache/maven-integration-testing#115](https://github.com/apache/maven-integration-testing/pull/115)
Eh, update your fork please https://github.com/gnodet/maven-integration-testing (master on apache/maven-integration-testing contains fixes for you: upped enforcer)
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-876237822
There's a single test failing on windows + jdk 1.8. Does that ring any bell @rfscholte ?
https://github.com/apache/maven/pull/486/checks?check_run_id=3016896271#step:8:6012
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-969134862
I'd like to get that in `master` to avoid the big performance loss. Anyone could take a look ?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-876260420
That would be my guess too
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #486: Use the MX xpp parser instead of a STAX transformation
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-874185595
I think this needs a JIRA issue.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #486:
URL: https://github.com/apache/maven/pull/486#issuecomment-933196485
> Hm, but something is wrong, it WAS ancient and is already upped [apache/maven-integration-testing#115](https://github.com/apache/maven-integration-testing/pull/115)
Eh, update your fork please https://github.com/gnodet/maven-integration-testing
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet merged pull request #486: [MNG-7182] Use a pull parser during the build/consumer transformation
Posted by GitBox <gi...@apache.org>.
gnodet merged pull request #486:
URL: https://github.com/apache/maven/pull/486
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org