gnodet-bot commented on code in PR #13118: URL: https://github.com/apache/maven/pull/13118#discussion_r4012967056
########## impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsParserTest.java: ########## @@ -0,0 +1,539 @@ +/* + * 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.impl; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Properties; + +import org.apache.maven.api.Constants; +import org.apache.maven.api.Session; +import org.apache.maven.api.services.BuilderProblem; +import org.apache.maven.api.services.Interpolator; +import org.apache.maven.api.services.SettingsBuilderException; +import org.apache.maven.api.services.SettingsBuilderRequest; +import org.apache.maven.api.services.SettingsBuilderResult; +import org.apache.maven.api.services.Source; +import org.apache.maven.api.services.Sources; +import org.apache.maven.api.services.xml.SettingsXmlFactory; +import org.apache.maven.api.settings.Server; +import org.apache.maven.api.settings.Settings; +import org.apache.maven.api.spi.SettingsParser; +import org.apache.maven.api.spi.SettingsParserException; +import org.apache.maven.di.Injector; +import org.apache.maven.impl.model.DefaultInterpolator; +import org.codehaus.plexus.components.secdispatcher.Dispatcher; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +class DefaultSettingsParserTest { + @TempDir + Path directory; + + @Test + void customSettings() throws Exception { + Path source = directory.resolve("settings.properties"); + Files.writeString(source, propertiesSettings() + "\n"); + var result = build(Sources.fromPath(source), Map.of("properties", new PropertiesSettingsParser())); + assertEquals( + directory.resolve("repository").toString(), + result.getEffectiveSettings().getLocalRepository()); + } + + @Test + void sourceWithoutBackingFile() throws Exception { + var result = build( + source("memory.properties", propertiesSettings()), + Map.of("properties", new PropertiesSettingsParser())); + assertEquals( + directory.resolve("repository").toString(), + result.getEffectiveSettings().getLocalRepository()); + } + + @Test + void xmlFallbackDoesNotRequireXmlExtension() throws Exception { + var result = build( + source("settings.conf", "<settings><offline>true</offline></settings>"), + Map.of("properties", new PropertiesSettingsParser())); + assertTrue(result.getEffectiveSettings().isOffline()); + } + + @Test + void xmlStrictFailureBecomesWarningAfterLenientParsing() throws Exception { + var result = build(source("settings.xml", "<settings>\n<unknown/>\n</settings>"), Map.of()); + var problems = + result.getProblems().problems(BuilderProblem.Severity.WARNING).toList(); + assertEquals(1, problems.size()); + assertEquals(2, problems.get(0).getLineNumber()); + assertTrue(problems.get(0).getColumnNumber() > 0); + } + + @Test + void malformedXmlIsFatal() throws Exception { + var error = assertThrows( + SettingsBuilderException.class, () -> build(source("settings.xml", "<settings>\n<offline>"), Map.of())); + assertTrue(error.getMessage().contains("Non-parseable settings settings.xml")); + assertTrue(error.getProblemCollector() + .problems(BuilderProblem.Severity.FATAL) + .allMatch(problem -> problem.getLineNumber() > 0)); + } + + @Test + void customStrictFailureRetriesSameParser() throws Exception { + var calls = new ArrayList<Boolean>(); + SettingsParser parser = new PropertiesSettingsParser() { + @Override + public Settings parse(Source source, Map<String, ?> options) { + calls.add((Boolean) options.get(STRICT)); + if (!Boolean.FALSE.equals(options.get(STRICT))) { + throw new SettingsParserException("Unknown setting", 3, 7, null); + } + return Settings.newInstance().withOffline(true); + } + }; + var result = build(source("settings.properties", "unknown=value"), Map.of("properties", parser)); + assertEquals(List.of(true, false), calls); + assertTrue(result.getEffectiveSettings().isOffline()); + var problem = result.getProblems() + .problems(BuilderProblem.Severity.WARNING) + .findFirst() + .orElseThrow(); + assertEquals("Unknown setting", problem.getMessage()); + assertEquals(3, problem.getLineNumber()); + assertEquals(7, problem.getColumnNumber()); + } + + @Test + void selectedParserFailureDoesNotFallBackToXml() throws Exception { + SettingsParser parser = mock(SettingsParser.class); + when(parser.supports(any())).thenReturn(true); + when(parser.parse(any(), any())).thenThrow(new SettingsParserException("Invalid custom settings", 4, 2, null)); + var error = assertThrows( + SettingsBuilderException.class, + () -> build(source("settings.properties", "<settings/>"), Map.of("properties", parser))); + assertTrue(error.getMessage().contains("Invalid custom settings")); + var problem = error.getProblemCollector() + .problems(BuilderProblem.Severity.FATAL) + .findFirst() + .orElseThrow(); + assertEquals(4, problem.getLineNumber()); + assertEquals(2, problem.getColumnNumber()); + verify(parser, times(2)).parse(any(), any()); + } + + @Test + void strictWarningSurvivesFailedLenientParsing() throws Exception { + SettingsParser parser = mock(SettingsParser.class); + when(parser.supports(any())).thenReturn(true); + var strict = new SettingsParserException("Unknown setting", 3, 7, null); + var lenient = new SettingsParserException("Invalid value", 5, 2, null); + when(parser.parse(any(), any())).thenThrow(strict).thenThrow(lenient); + var error = assertThrows( + SettingsBuilderException.class, + () -> build(source("settings.properties", ""), Map.of("properties", parser))); + var warnings = error.getProblemCollector() + .problems(BuilderProblem.Severity.WARNING) + .toList(); + assertEquals(1, warnings.size()); + var warning = warnings.get(0); + assertEquals("Unknown setting", warning.getMessage()); + assertEquals("settings.properties", warning.getSource()); + assertEquals(3, warning.getLineNumber()); + assertEquals(7, warning.getColumnNumber()); + assertSame(strict, warning.getException()); + var fatals = error.getProblemCollector() + .problems(BuilderProblem.Severity.FATAL) + .toList(); + assertEquals(1, fatals.size()); + var fatal = fatals.get(0); + assertEquals("Non-parseable settings settings.properties: Invalid value", fatal.getMessage()); + assertEquals(5, fatal.getLineNumber()); + assertEquals(2, fatal.getColumnNumber()); + assertSame(lenient, fatal.getException()); + verify(parser).parse(any(), eq(Map.of(SettingsParser.STRICT, true))); + verify(parser).parse(any(), eq(Map.of(SettingsParser.STRICT, false))); + } + + @Test + void strictWarningSurvivesUnreadableLenientInput() throws Exception { + SettingsParser parser = mock(SettingsParser.class); + when(parser.supports(any())).thenReturn(true); + var strict = new SettingsParserException("Unknown setting", 3, 7, null); + var unreadable = new IOException("Read failed"); + when(parser.parse(any(), any())).thenThrow(strict).thenThrow(unreadable); + var error = assertThrows( + SettingsBuilderException.class, + () -> build(source("settings.properties", ""), Map.of("properties", parser))); + var warnings = error.getProblemCollector() + .problems(BuilderProblem.Severity.WARNING) + .toList(); + assertEquals(1, warnings.size()); + assertSame(strict, warnings.get(0).getException()); + var fatals = error.getProblemCollector() + .problems(BuilderProblem.Severity.FATAL) + .toList(); + assertEquals(1, fatals.size()); + assertEquals( + "Non-readable settings settings.properties: Read failed", + fatals.get(0).getMessage()); + assertSame(unreadable, fatals.get(0).getException()); + verify(parser, times(2)).parse(any(), any()); + } + + @Test + void causeOnlyParserFailureHasUsefulDiagnostic() throws Exception { + SettingsParser parser = mock(SettingsParser.class); + when(parser.supports(any())).thenReturn(true); + when(parser.parse(any(), any())) + .thenThrow(new SettingsParserException(new IllegalArgumentException("Invalid value"))); + var error = assertThrows( + SettingsBuilderException.class, + () -> build(source("settings.properties", ""), Map.of("properties", parser))); + var fatal = error.getProblemCollector() + .problems(BuilderProblem.Severity.FATAL) + .findFirst() + .orElseThrow(); + assertEquals("Non-parseable settings settings.properties: Invalid value", fatal.getMessage()); + } + + @Test + void conflictingParsersAreReportedBeforeParsing() throws Exception { + SettingsParser first = mock(SettingsParser.class); + SettingsParser second = mock(SettingsParser.class); + when(first.supports(any())).thenReturn(true); + when(second.supports(any())).thenReturn(true); + var error = assertThrows( + SettingsBuilderException.class, + () -> build(source("settings.properties", "<settings/>"), Map.of("second", second, "first", first))); + var fatals = error.getProblemCollector() + .problems(BuilderProblem.Severity.FATAL) + .toList(); + assertEquals(1, fatals.size()); + var fatal = fatals.get(0); + assertEquals("Multiple settings parsers support this source: first, second", fatal.getMessage()); + assertEquals("settings.properties", fatal.getSource()); + assertEquals(-1, fatal.getLineNumber()); + assertEquals(-1, fatal.getColumnNumber()); + assertEquals(0, error.getProblemCollector().problemsReportedFor(BuilderProblem.Severity.WARNING)); + verify(first, never()).parse(any(), any()); + verify(second, never()).parse(any(), any()); + } + + @Test + void supportsFailurePreventsXmlFallback() throws Exception { + assertSupportsFailureStopsSelectionAndParsing("broken", false); + } + + @Test + void unnamedSupportsFailurePreventsXmlFallback() throws Exception { + assertSupportsFailureStopsSelectionAndParsing(null, false); + } + + @Test + void supportsFailurePreventsPreviouslyMatchedParser() throws Exception { + assertSupportsFailureStopsSelectionAndParsing("broken", true); + } + + @Test + void unnamedSupportsFailurePreventsPreviouslyMatchedParser() throws Exception { + assertSupportsFailureStopsSelectionAndParsing(null, true); + } + + private void assertSupportsFailureStopsSelectionAndParsing(String providerName, boolean matchingFirst) + throws Exception { + SettingsParser matching = mock(SettingsParser.class); + SettingsParser broken = mock(SettingsParser.class); + SettingsParser unvisited = mock(SettingsParser.class); + when(matching.supports(any())).thenReturn(true); + var failure = new IllegalStateException(); + when(broken.supports(any())).thenThrow(failure); + var parsers = new LinkedHashMap<String, SettingsParser>(); + if (matchingFirst) { + parsers.put("matching", matching); + } + parsers.put(providerName, broken); + parsers.put("unvisited", unvisited); + var xmlFactory = mock(SettingsXmlFactory.class); + var builder = new DefaultSettingsBuilder(xmlFactory, new DefaultInterpolator(), Map.of(), parsers); + var source = source("settings.xml", "<settings/>"); + var error = assertThrows( + SettingsBuilderException.class, + () -> builder.build(SettingsBuilderRequest.builder() + .session(mock(Session.class)) + .userSettingsSource(source) + .build())); + var fatals = error.getProblemCollector() + .problems(BuilderProblem.Severity.FATAL) + .toList(); + assertEquals(1, fatals.size()); + var fatal = fatals.get(0); + assertEquals( + "Settings parser '" + (providerName != null ? providerName : "<unnamed>") + + "' failed to determine support for this source", + fatal.getMessage()); + assertEquals("settings.xml", fatal.getSource()); + assertEquals(-1, fatal.getLineNumber()); + assertEquals(-1, fatal.getColumnNumber()); + assertSame(failure, fatal.getException()); + assertEquals(0, error.getProblemCollector().problemsReportedFor(BuilderProblem.Severity.WARNING)); + verify(broken).supports(source); + verify(matching, never()).parse(any(), any()); Review Comment: 💡 **Nit: vacuous assertion when `matchingFirst=false`** When `matchingFirst` is `false`, `matching` is never added to the `parsers` map, so `verify(matching, never()).parse(any(), any())` passes trivially — Mockito never saw an interaction to record. The assertion says nothing in that branch. If the intent is to prove that stopping on the broken provider also prevents any previously-seen match from being parsed, the meaningful assertion is only valid when `matchingFirst=true`. For the `false` case, a more useful assertion would be that the `broken` provider's `supports()` was actually called: ```suggestion verify(broken).supports(source); if (matchingFirst) { verify(matching, never()).parse(any(), any()); } verify(broken, never()).parse(any(), any()); ``` (Or just leave the `verify(matching, never()).parse(...)` call guarded by `if (matchingFirst)` — it is harmless as-is but misleading.) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
