From c742bafa2f79d8a4d9398a3be85fad9a2aa8c7a5 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Wed, 5 Jun 2024 13:00:45 +0200 Subject: [PATCH 1/5] [MNG-8141] Model builder should report problems it finds during building the model And not rely that model was validated, which is not true in some cases. Model builder can still easily detect issues with models while building them. --- https://issues.apache.org/jira/browse/MNG-8141 --- .../maven/model/building/DefaultModelBuilder.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java index 3043a76a0ea6..179be4513f33 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java @@ -431,7 +431,7 @@ private interface InterpolateString { private Map getInterpolatedActivations( Model rawModel, DefaultProfileActivationContext context, DefaultModelProblemCollector problems) { - Map interpolatedActivations = getProfileActivations(rawModel, true); + Map interpolatedActivations = getProfileActivations(rawModel, true, problems); if (interpolatedActivations.isEmpty()) { return Collections.emptyMap(); @@ -753,7 +753,7 @@ private void assembleInheritance( } } - private Map getProfileActivations(Model model, boolean clone) { + private Map getProfileActivations(Model model, boolean clone, ModelProblemCollector problems) { Map activations = new HashMap<>(); for (Profile profile : model.getProfiles()) { Activation activation = profile.getActivation(); @@ -766,7 +766,10 @@ private Map getProfileActivations(Model model, boolean clone activation = activation.clone(); } - activations.put(profile.getId(), activation); + if (activations.put(profile.getId(), activation) != null) { + problems.add(new ModelProblemCollectorRequest(ModelProblem.Severity.FATAL, ModelProblem.Version.BASE) + .setMessage("Duplicate activation for " + profile.getId())); + } } return activations; @@ -787,7 +790,7 @@ private void injectProfileActivations(Model model, Map activ private Model interpolateModel(Model model, ModelBuildingRequest request, ModelProblemCollector problems) { // save profile activations before interpolation, since they are evaluated with limited scope - Map originalActivations = getProfileActivations(model, true); + Map originalActivations = getProfileActivations(model, true, problems); Model interpolatedModel = modelInterpolator.interpolateModel(model, model.getProjectDirectory(), request, problems); From 9490c3b9fef77a562f4f358d77232c3737b8f3ab Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Wed, 5 Jun 2024 13:38:11 +0200 Subject: [PATCH 2/5] Provide an escape hatch --- .../model/building/DefaultModelBuilder.java | 42 ++++++++++++++++--- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java index 179be4513f33..d19dfad587ca 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java @@ -97,6 +97,28 @@ @Named @Singleton public class DefaultModelBuilder implements ModelBuilder { + /** + * Key for "fail on invalid model" property. + */ + private static final String FAIL_ON_INVALID_MODEL = "maven.modelBuilder.failOnInvalidModel"; + + /** + * Checks user and system properties (in this order) for value of {@link #FAIL_ON_INVALID_MODEL} property key, if + * set and returns it. If not set, defaults to {@code true}. + *

+ * This is only meant to provide "escape hatch" for those builds, that are for some reason stuck with invalid models. + */ + private static boolean isFailOnInvalidModel(ModelBuildingRequest request) { + String val = request.getUserProperties().getProperty(FAIL_ON_INVALID_MODEL); + if (val == null) { + val = request.getSystemProperties().getProperty(FAIL_ON_INVALID_MODEL); + } + if (val != null) { + return Boolean.parseBoolean(val); + } + return true; + } + @Inject private ModelProcessor modelProcessor; @@ -253,6 +275,7 @@ public ModelBuildingResult build(ModelBuildingRequest request) throws ModelBuild protected ModelBuildingResult build(ModelBuildingRequest request, Collection importIds) throws ModelBuildingException { // phase 1 + boolean failOnInvalidModel = isFailOnInvalidModel(request); DefaultModelBuildingResult result = new DefaultModelBuildingResult(); DefaultModelProblemCollector problems = new DefaultModelProblemCollector(result); @@ -306,7 +329,7 @@ protected ModelBuildingResult build(ModelBuildingRequest request, Collection interpolatedActivations = - getInterpolatedActivations(rawModel, profileActivationContext, problems); + getInterpolatedActivations(rawModel, profileActivationContext, failOnInvalidModel, problems); injectProfileActivations(tmpModel, interpolatedActivations); List activePomProfiles = @@ -430,8 +453,12 @@ private interface InterpolateString { } private Map getInterpolatedActivations( - Model rawModel, DefaultProfileActivationContext context, DefaultModelProblemCollector problems) { - Map interpolatedActivations = getProfileActivations(rawModel, true, problems); + Model rawModel, + DefaultProfileActivationContext context, + boolean failOnInvalidModel, + DefaultModelProblemCollector problems) { + Map interpolatedActivations = + getProfileActivations(rawModel, true, failOnInvalidModel, problems); if (interpolatedActivations.isEmpty()) { return Collections.emptyMap(); @@ -753,7 +780,8 @@ private void assembleInheritance( } } - private Map getProfileActivations(Model model, boolean clone, ModelProblemCollector problems) { + private Map getProfileActivations( + Model model, boolean clone, boolean failOnInvalidModel, ModelProblemCollector problems) { Map activations = new HashMap<>(); for (Profile profile : model.getProfiles()) { Activation activation = profile.getActivation(); @@ -767,7 +795,8 @@ private Map getProfileActivations(Model model, boolean clone } if (activations.put(profile.getId(), activation) != null) { - problems.add(new ModelProblemCollectorRequest(ModelProblem.Severity.FATAL, ModelProblem.Version.BASE) + problems.add(new ModelProblemCollectorRequest( + failOnInvalidModel ? Severity.FATAL : Severity.WARNING, ModelProblem.Version.BASE) .setMessage("Duplicate activation for " + profile.getId())); } } @@ -790,7 +819,8 @@ private void injectProfileActivations(Model model, Map activ private Model interpolateModel(Model model, ModelBuildingRequest request, ModelProblemCollector problems) { // save profile activations before interpolation, since they are evaluated with limited scope - Map originalActivations = getProfileActivations(model, true, problems); + // at this stage we already failed if wanted to + Map originalActivations = getProfileActivations(model, true, false, problems); Model interpolatedModel = modelInterpolator.interpolateModel(model, model.getProjectDirectory(), request, problems); From 2c38ba4bcb90fa2253c1f2adb50d5d8249baa363 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 6 Jun 2024 12:09:42 +0200 Subject: [PATCH 3/5] Add UT --- .../model/building/DefaultModelBuilder.java | 2 +- .../building/DefaultModelBuilderTest.java | 21 ++++++++ .../resources/poms/building/badprofiles.xml | 54 +++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 maven-model-builder/src/test/resources/poms/building/badprofiles.xml diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java index d19dfad587ca..25863ca24f2d 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java @@ -797,7 +797,7 @@ private Map getProfileActivations( if (activations.put(profile.getId(), activation) != null) { problems.add(new ModelProblemCollectorRequest( failOnInvalidModel ? Severity.FATAL : Severity.WARNING, ModelProblem.Version.BASE) - .setMessage("Duplicate activation for " + profile.getId())); + .setMessage("Duplicate activation for profile " + profile.getId())); } } diff --git a/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java b/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java index 59fb44750003..94d17ca391e0 100644 --- a/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java +++ b/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java @@ -19,6 +19,7 @@ package org.apache.maven.model.building; import org.apache.maven.model.Dependency; +import org.apache.maven.model.Model; import org.apache.maven.model.Parent; import org.apache.maven.model.Repository; import org.apache.maven.model.resolution.InvalidRepositoryException; @@ -26,7 +27,10 @@ import org.apache.maven.model.resolution.UnresolvableModelException; import org.junit.Test; +import java.io.File; + import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; /** * @author Guillaume Nodet @@ -87,6 +91,23 @@ public void testCycleInImports() throws Exception { builder.build(request); } + @Test + public void testBadProfiles() { + ModelBuilder builder = new DefaultModelBuilderFactory().newInstance(); + assertNotNull(builder); + + DefaultModelBuildingRequest request = new DefaultModelBuildingRequest(); + request.setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL); + request.setModelSource(new FileModelSource(new File("src/test/resources/poms/building/badprofiles.xml"))); + request.setModelResolver(new BaseModelResolver()); + + try { + builder.build(request); + } catch (ModelBuildingException e) { + assertTrue(e.getMessage().contains("Duplicate activation for profile badprofile")); + } + } + static class CycleInImportsResolver extends BaseModelResolver { @Override public ModelSource resolveModel(Dependency dependency) throws UnresolvableModelException { diff --git a/maven-model-builder/src/test/resources/poms/building/badprofiles.xml b/maven-model-builder/src/test/resources/poms/building/badprofiles.xml new file mode 100644 index 000000000000..e098a70fb946 --- /dev/null +++ b/maven-model-builder/src/test/resources/poms/building/badprofiles.xml @@ -0,0 +1,54 @@ + + + + + + 4.0.0 + + test + test + 0.1-SNAPSHOT + pom + + + + badprofile + + true + + + UTF-8 + + + + badprofile + + + simple.xml + + + + activated + + + + From 680e45a064a309da23d86ce48b6cf72f99cf3f42 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 6 Jun 2024 12:11:10 +0200 Subject: [PATCH 4/5] Reformat --- .../apache/maven/model/building/DefaultModelBuilderTest.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java b/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java index 94d17ca391e0..01ab83d4b32b 100644 --- a/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java +++ b/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java @@ -18,8 +18,9 @@ */ package org.apache.maven.model.building; +import java.io.File; + import org.apache.maven.model.Dependency; -import org.apache.maven.model.Model; import org.apache.maven.model.Parent; import org.apache.maven.model.Repository; import org.apache.maven.model.resolution.InvalidRepositoryException; @@ -27,8 +28,6 @@ import org.apache.maven.model.resolution.UnresolvableModelException; import org.junit.Test; -import java.io.File; - import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; From 784ca54cb7c1b16c943edd474a4a119614a64fa6 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 6 Jun 2024 12:15:48 +0200 Subject: [PATCH 5/5] Add UT --- .../model/building/DefaultModelBuilder.java | 4 +++- .../building/DefaultModelBuilderTest.java | 18 +++++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java index 25863ca24f2d..04a6a9e62295 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java @@ -99,8 +99,10 @@ public class DefaultModelBuilder implements ModelBuilder { /** * Key for "fail on invalid model" property. + *

+ * Visible for testing. */ - private static final String FAIL_ON_INVALID_MODEL = "maven.modelBuilder.failOnInvalidModel"; + static final String FAIL_ON_INVALID_MODEL = "maven.modelBuilder.failOnInvalidModel"; /** * Checks user and system properties (in this order) for value of {@link #FAIL_ON_INVALID_MODEL} property key, if diff --git a/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java b/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java index 01ab83d4b32b..13a8ac0e476b 100644 --- a/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java +++ b/maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java @@ -30,6 +30,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * @author Guillaume Nodet @@ -101,12 +102,27 @@ public void testBadProfiles() { request.setModelResolver(new BaseModelResolver()); try { - builder.build(request); + builder.build(request); // throw, making "pom not available" + fail(); } catch (ModelBuildingException e) { assertTrue(e.getMessage().contains("Duplicate activation for profile badprofile")); } } + @Test + public void testBadProfilesCheckDisabled() throws Exception { + ModelBuilder builder = new DefaultModelBuilderFactory().newInstance(); + assertNotNull(builder); + + DefaultModelBuildingRequest request = new DefaultModelBuildingRequest(); + request.getUserProperties().setProperty(DefaultModelBuilder.FAIL_ON_INVALID_MODEL, "false"); + request.setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL); + request.setModelSource(new FileModelSource(new File("src/test/resources/poms/building/badprofiles.xml"))); + request.setModelResolver(new BaseModelResolver()); + + builder.build(request); // does not throw, old behaviour (but result may be fully off) + } + static class CycleInImportsResolver extends BaseModelResolver { @Override public ModelSource resolveModel(Dependency dependency) throws UnresolvableModelException {