From 9db546ddb905392c6c4df9484895bf11182ab303 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 6 Jun 2024 18:51:39 +0200 Subject: [PATCH] [MNG-8142] Hidden bug: JDK profile activator throw NumberFormatEx (#1557) If property `java.version` is in unexpected format, the activator throws `NumberFormatEx`, that in turn, is caught and reported by `DefaultProfileSelector` w/o any cause. These should be cleanly reported instead: report that `java.version` property is in "unexpected format", and also report why was there are failure to evaluate a property activation. Note 1: Maven allows `-Djava.version` override (!!!), this is exactly what IT MNG-3746 does, but the `NumberFormatEx` went unnoticed, was swallowed, no cause reported. Note 2: This bug was revealed by #1555 as it reported the issue, and later "asserted error free log" which was not error-free. Hence, this bug was simply revealed by improved logging on unrelated issue. --- https://issues.apache.org/jira/browse/MNG-8142 --- .../impl/model/DefaultProfileSelector.java | 2 +- .../profile/JdkVersionProfileActivator.java | 12 +++- .../model/profile/DefaultProfileSelector.java | 3 +- .../JdkVersionProfileActivator.java | 10 ++- .../profile/DefaultProfileSelectorTest.java | 72 +++++++++++++++++++ .../JdkVersionProfileActivatorTest.java | 25 +++++++ 6 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 maven-model-builder/src/test/java/org/apache/maven/model/profile/DefaultProfileSelectorTest.java diff --git a/maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultProfileSelector.java b/maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultProfileSelector.java index e451956e5034..c15513e1ec10 100644 --- a/maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultProfileSelector.java +++ b/maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultProfileSelector.java @@ -106,7 +106,7 @@ private boolean isActive(Profile profile, ProfileActivationContext context, Mode problems.add( Severity.ERROR, Version.BASE, - "Failed to determine activation for profile " + profile.getId(), + "Failed to determine activation for profile " + profile.getId() + ": " + e.getMessage(), profile.getLocation(""), e); return false; diff --git a/maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/profile/JdkVersionProfileActivator.java b/maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/profile/JdkVersionProfileActivator.java index 62e288e2949e..2a417a3bc650 100644 --- a/maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/profile/JdkVersionProfileActivator.java +++ b/maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/profile/JdkVersionProfileActivator.java @@ -70,7 +70,17 @@ public boolean isActive(Profile profile, ProfileActivationContext context, Model activation.getLocation("jdk")); return false; } - return isJavaVersionCompatible(jdk, version); + try { + return isJavaVersionCompatible(jdk, version); + } catch (NumberFormatException e) { + problems.add( + BuilderProblem.Severity.WARNING, + ModelProblem.Version.BASE, + "Failed to determine JDK activation for profile " + profile.getId() + " due invalid JDK version: '" + + version + "'", + profile.getLocation("jdk")); + return false; + } } public static boolean isJavaVersionCompatible(String requiredJdkRange, String currentJavaVersion) { diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/profile/DefaultProfileSelector.java b/maven-model-builder/src/main/java/org/apache/maven/model/profile/DefaultProfileSelector.java index b2d0e9a8015c..3ccfb261c78d 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/profile/DefaultProfileSelector.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/profile/DefaultProfileSelector.java @@ -119,7 +119,8 @@ private boolean isActive(Profile profile, ProfileActivationContext context, Mode } } catch (RuntimeException e) { problems.add(new ModelProblemCollectorRequest(Severity.ERROR, Version.BASE) - .setMessage("Failed to determine activation for profile " + profile.getId()) + .setMessage("Failed to determine activation for profile " + profile.getId() + ": " + + e.getMessage()) .setLocation(profile.getLocation("")) .setException(e)); return false; diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/profile/activation/JdkVersionProfileActivator.java b/maven-model-builder/src/main/java/org/apache/maven/model/profile/activation/JdkVersionProfileActivator.java index 23f43208660b..378550fc56c0 100644 --- a/maven-model-builder/src/main/java/org/apache/maven/model/profile/activation/JdkVersionProfileActivator.java +++ b/maven-model-builder/src/main/java/org/apache/maven/model/profile/activation/JdkVersionProfileActivator.java @@ -69,7 +69,15 @@ public boolean isActive(Profile profile, ProfileActivationContext context, Model .setLocation(activation.getLocation("jdk"))); return false; } - return isJavaVersionCompatible(jdk, version); + try { + return isJavaVersionCompatible(jdk, version); + } catch (NumberFormatException e) { + problems.add(new ModelProblemCollectorRequest(Severity.WARNING, Version.BASE) + .setMessage("Failed to determine JDK activation for profile " + profile.getId() + + " due invalid JDK version: '" + version + "'") + .setLocation(activation.getLocation("jdk"))); + return false; + } } public static boolean isJavaVersionCompatible(String requiredJdkRange, String currentJavaVersion) { diff --git a/maven-model-builder/src/test/java/org/apache/maven/model/profile/DefaultProfileSelectorTest.java b/maven-model-builder/src/test/java/org/apache/maven/model/profile/DefaultProfileSelectorTest.java new file mode 100644 index 000000000000..9b86ef8401c8 --- /dev/null +++ b/maven-model-builder/src/test/java/org/apache/maven/model/profile/DefaultProfileSelectorTest.java @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.model.profile; + +import java.util.Collections; +import java.util.List; + +import org.apache.maven.model.Activation; +import org.apache.maven.model.Profile; +import org.apache.maven.model.building.ModelProblemCollector; +import org.apache.maven.model.building.SimpleProblemCollector; +import org.apache.maven.model.profile.activation.ProfileActivator; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests {@link DefaultProfileSelector}. + */ +public class DefaultProfileSelectorTest { + private Profile newProfile(String id) { + Activation activation = new Activation(); + Profile profile = new Profile(); + profile.setId(id); + profile.setActivation(activation); + return profile; + } + + @Test + void testThrowingActivator() { + DefaultProfileSelector selector = new DefaultProfileSelector(); + selector.addProfileActivator(new ProfileActivator() { + @Override + public boolean isActive(Profile profile, ProfileActivationContext context, ModelProblemCollector problems) { + throw new RuntimeException("BOOM"); + } + + @Override + public boolean presentInConfig( + Profile profile, ProfileActivationContext context, ModelProblemCollector problems) { + return true; + } + }); + + List profiles = Collections.singletonList(newProfile("one")); + DefaultProfileActivationContext context = new DefaultProfileActivationContext(); + SimpleProblemCollector problems = new SimpleProblemCollector(); + List active = selector.getActiveProfiles(profiles, context, problems); + assertTrue(active.isEmpty()); + assertEquals(1, problems.getErrors().size()); + assertEquals( + "Failed to determine activation for profile one: BOOM", + problems.getErrors().get(0)); + } +} diff --git a/maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/JdkVersionProfileActivatorTest.java b/maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/JdkVersionProfileActivatorTest.java index 74d6f19478bf..5d982478dd1c 100644 --- a/maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/JdkVersionProfileActivatorTest.java +++ b/maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/JdkVersionProfileActivatorTest.java @@ -22,9 +22,13 @@ import org.apache.maven.api.model.Activation; import org.apache.maven.api.model.Profile; +import org.apache.maven.model.building.SimpleProblemCollector; +import org.apache.maven.model.profile.ProfileActivationContext; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.*; + /** * Tests {@link JdkVersionProfileActivator}. * @@ -169,4 +173,25 @@ void testVersionRangeExclusiveUpperBound() throws Exception { assertActivation(false, profile, newContext(null, newProperties("1.6.0_09"))); assertActivation(false, profile, newContext(null, newProperties("1.6.0_09-b03"))); } + + @Test + void testRubbishJavaVersion() { + Profile profile = newProfile("[1.8,)"); + + assertActivationWithProblems(profile, newContext(null, newProperties("Pūteketeke")), "invalid JDK version"); + assertActivationWithProblems(profile, newContext(null, newProperties("rubbish")), "invalid JDK version"); + assertActivationWithProblems(profile, newContext(null, newProperties("1.a.0_09")), "invalid JDK version"); + assertActivationWithProblems(profile, newContext(null, newProperties("1.a.2.b")), "invalid JDK version"); + } + + private void assertActivationWithProblems( + Profile profile, ProfileActivationContext context, String warningContains) { + SimpleProblemCollector problems = new SimpleProblemCollector(); + + assertFalse(activator.isActive(new org.apache.maven.model.Profile(profile), context, problems)); + + assertEquals(0, problems.getErrors().size()); + assertEquals(1, problems.getWarnings().size()); + assertTrue(problems.getWarnings().get(0).contains(warningContains)); + } }