gnodet-bot commented on code in PR #13118: URL: https://github.com/apache/maven/pull/13118#discussion_r4011032683
########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/SettingsParser.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.api.spi; + +import java.io.IOException; +import java.util.Map; + +import org.apache.maven.api.annotations.Consumer; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.annotations.Nullable; +import org.apache.maven.api.di.Named; +import org.apache.maven.api.services.Source; +import org.apache.maven.api.settings.Settings; + +/** + * Parses settings in an additional syntax. Maven selects a parser for each settings source, + * then performs interpolation, decryption, validation and merging on the returned settings. + * If no parser supports a source, Maven uses its XML settings reader. Multiple parsers + * supporting the same source are an error. + * <p> + * Parsers must be available in the container building the settings. In particular, a parser + * supplied by a core extension cannot read the bootstrap settings needed to resolve that + * extension. This SPI does not change settings file discovery or extension loading. + * + * @since 4.1.0 + */ +@Experimental +@Consumer +@Named +public interface SettingsParser extends SpiService { + /** + * Boolean parsing option indicating whether unknown input should be rejected. + */ + /** + * Option that can be specified in the options map. The value should be a {@code Boolean}; + * when {@code true} or absent, unknown input is rejected. + */ + String STRICT = "strict"; Review Comment: ⚠️ **[NOT ADDRESSED] Duplicate Javadoc block on `STRICT`** The field still has two consecutive Javadoc comments — lines 48–50 (`Boolean parsing option...`) and lines 51–54 (`Option that can be specified...`). Only the second block is shown by IDE tooling; the first is silently discarded by the Javadoc compiler. Drop the first block: ```suggestion /** * Option that can be specified in the options map. The value should be a {@code Boolean}; * when {@code true} or absent, unknown input is rejected. */ String STRICT = "strict"; ``` ########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/SettingsParserException.java: ########## @@ -0,0 +1,71 @@ +/* + * 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.api.spi; + +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.services.MavenException; + +/** + * A syntax error in a settings source, with optional one-based line and column numbers. + * + * @since 4.1.0 + */ +@Experimental +public class SettingsParserException extends MavenException { + + /** + * The one-based index of the line containing the error. + */ + private final int lineNumber; + + /** + * The one-based index of the column containing the error. + */ + private final int columnNumber; + + public SettingsParserException() { + this(null, null); + } + + public SettingsParserException(String message) { + this(message, null); + } + + public SettingsParserException(String message, Throwable cause) { + this(message, -1, -1, cause); + } + + public SettingsParserException(String message, int lineNumber, int columnNumber, Throwable cause) { + super(message, cause); + this.lineNumber = lineNumber; + this.columnNumber = columnNumber; + } + + public SettingsParserException(Throwable cause) { + this(null, cause); + } Review Comment: ⚠️ **[NOT ADDRESSED] Null-message constructors produce useless diagnostics** `SettingsParserException()` (line 42) and `SettingsParserException(Throwable cause)` (line 60) both set message to `null`. In `DefaultSettingsBuilder.readSettings()`, `e.getMessage()` is used directly in the warning (line 180) and fatal (line 189) problem messages — so a parser that throws `new SettingsParserException(cause)` produces: ``` Non-parseable settings settings.yaml: null ``` That is useless for diagnostics. Fall back to the cause's message: ```suggestion public SettingsParserException(String message, Throwable cause) { this(message != null ? message : (cause != null ? cause.getMessage() : "unknown error"), -1, -1, cause); } public SettingsParserException(Throwable cause) { this(cause != null ? cause.getMessage() : "unknown error", cause); } ``` ########## impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java: ########## @@ -159,35 +167,24 @@ private Settings readSettings( Settings settings; try { - try (InputStream is = settingsSource.openStream()) { - settings = settingsXmlFactory.read(XmlReaderRequest.builder() - .inputStream(is) - .location(settingsSource.getLocation()) - .strict(true) - .build()); - } catch (XmlReaderException e) { - try (InputStream is = settingsSource.openStream()) { - settings = settingsXmlFactory.read(XmlReaderRequest.builder() - .inputStream(is) - .location(settingsSource.getLocation()) - .strict(false) - .build()); - Location loc = e.getCause() instanceof XMLStreamException xe ? xe.getLocation() : null; - problems.reportProblem(new DefaultBuilderProblem( - settingsSource.getLocation(), - loc != null ? loc.getLineNumber() : -1, - loc != null ? loc.getColumnNumber() : -1, - e, - e.getMessage(), - BuilderProblem.Severity.WARNING)); - } + SettingsParser parser = selectParser(settingsSource); + try { + settings = parser.parse(settingsSource, Map.of(SettingsParser.STRICT, true)); + } catch (SettingsParserException e) { + settings = parser.parse(settingsSource, Map.of(SettingsParser.STRICT, false)); + problems.reportProblem(new DefaultBuilderProblem( + settingsSource.getLocation(), + e.getLineNumber(), + e.getColumnNumber(), + e, + e.getMessage(), + BuilderProblem.Severity.WARNING)); Review Comment: ⚠️ **[NOT ADDRESSED] WARNING is lost when lenient retry also fails** Current flow: strict parse throws `SettingsParserException e` (line 173) → lenient retry is attempted (line 174) → if the retry *also* throws, it escapes the inner catch, hits the outer `catch (SettingsParserException)` at line 183 as a FATAL, and `reportProblem(WARNING, e)` on lines 175–181 is **never reached**. The user gets a FATAL with the retry's exception, losing the original strict-parse error and all its location info. Record the WARNING *before* retrying so it survives either outcome: ```suggestion } catch (SettingsParserException e) { problems.reportProblem(new DefaultBuilderProblem( settingsSource.getLocation(), e.getLineNumber(), e.getColumnNumber(), e, e.getMessage(), BuilderProblem.Severity.WARNING)); settings = parser.parse(settingsSource, Map.of(SettingsParser.STRICT, false)); ``` ########## impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java: ########## @@ -159,35 +167,24 @@ private Settings readSettings( Settings settings; try { - try (InputStream is = settingsSource.openStream()) { - settings = settingsXmlFactory.read(XmlReaderRequest.builder() - .inputStream(is) - .location(settingsSource.getLocation()) - .strict(true) - .build()); - } catch (XmlReaderException e) { - try (InputStream is = settingsSource.openStream()) { - settings = settingsXmlFactory.read(XmlReaderRequest.builder() - .inputStream(is) - .location(settingsSource.getLocation()) - .strict(false) - .build()); - Location loc = e.getCause() instanceof XMLStreamException xe ? xe.getLocation() : null; - problems.reportProblem(new DefaultBuilderProblem( - settingsSource.getLocation(), - loc != null ? loc.getLineNumber() : -1, - loc != null ? loc.getColumnNumber() : -1, - e, - e.getMessage(), - BuilderProblem.Severity.WARNING)); - } + SettingsParser parser = selectParser(settingsSource); + try { + settings = parser.parse(settingsSource, Map.of(SettingsParser.STRICT, true)); + } catch (SettingsParserException e) { + settings = parser.parse(settingsSource, Map.of(SettingsParser.STRICT, false)); + problems.reportProblem(new DefaultBuilderProblem( + settingsSource.getLocation(), + e.getLineNumber(), + e.getColumnNumber(), + e, + e.getMessage(), + BuilderProblem.Severity.WARNING)); } - } catch (XmlReaderException e) { - Location loc = e.getCause() instanceof XMLStreamException xe ? xe.getLocation() : null; + } catch (SettingsParserException e) { problems.reportProblem(new DefaultBuilderProblem( settingsSource.getLocation(), - loc != null ? loc.getLineNumber() : -1, - loc != null ? loc.getColumnNumber() : -1, + e.getLineNumber(), + e.getColumnNumber(), e, "Non-parseable settings " + settingsSource.getLocation() + ": " + e.getMessage(), BuilderProblem.Severity.FATAL)); Review Comment: ⚠️ **[NEW] `selectParser` conflict error misformatted as "Non-parseable settings"** `selectParser()` is called at line 170, inside the outer `try`. When multiple parsers match, it throws `SettingsParserException("Multiple settings parsers support this source: ...")`, which propagates here and produces: ``` Non-parseable settings settings.properties: Multiple settings parsers support this source: first, second ``` That message is wrong — the source was never parsed; the error is a configuration conflict between registered parsers. A user seeing "Non-parseable settings" will look for a syntax error in their file, not a duplicated extension registration. The test at `conflictingParsersAreReportedBeforeParsing` only asserts `contains("Multiple settings parsers...")` so it misses this misleading prefix. Cleanest fix: have `selectParser` throw a `RuntimeException` or dedicated non-`SettingsParserException` type that bypasses this catch entirely. Alternatively, move the `selectParser()` call outside of the `try` block. -- 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]
