gnodet-bot commented on code in PR #13118: URL: https://github.com/apache/maven/pull/13118#discussion_r4010990800
########## api/maven-api-spi/src/main/java/org/apache/maven/api/spi/SettingsParser.java: ########## @@ -0,0 +1,79 @@ +/* + * 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. + */ + String STRICT = "strict"; Review Comment: ⚠️ **Missing value-type documentation on `STRICT` constant** `ModelParser` has the same pattern and documents it correctly: ```java /** * Option that can be specified in the options map. The value should be a Boolean. */ String STRICT = "strict"; ``` Here you only say `"Boolean parsing option"` — which is easy to miss. An implementor reading this Javadoc needs to know to fetch the value as `Boolean`, not `String`. Align the wording with `ModelParser`: ```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); + } + Review Comment: ⚠️ **No-arg and cause-only constructors silently allow null message** `SettingsParserException()` and `SettingsParserException(Throwable cause)` both delegate to `this(null, null)` / `this(null, cause)`, setting `message = null`. In `DefaultSettingsBuilder.readSettings()`, the warning and fatal problem messages include `e.getMessage()` directly — so a parser that throws `new SettingsParserException(cause)` (no message) will produce a problem like `"Non-parseable settings settings.yaml: null"`, which is useless for diagnostics. The `@param` on `SettingsParserException(Throwable cause)` should either: 1. Require a non-null message (match the 2-arg constructor pattern), or 2. At least document that message may be null, so callers of `getMessage()` know to guard. Prefer option 1 — make it impossible to create a message-less `SettingsParserException` from a cause: ```suggestion public SettingsParserException(String message, Throwable cause) { this(message != null ? message : (cause != null ? cause.getMessage() : "unknown error"), -1, -1, cause); } ``` (And drop the no-arg constructor or at least give it a default message.) ########## 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: ⚠️ **Original strict-parse exception is silently discarded if lenient parse also fails** When the strict parse throws `SettingsParserException e`, the inner catch retries with `strict=false` (line 174). If that retry *also* throws a `SettingsParserException`, it escapes the inner `catch` block and is caught by the outer catch on line 183 as a FATAL error. Two consequences: 1. The WARNING problem for the original strict failure is **never recorded** (line 175-181 are never reached). 2. The outer FATAL records the *lenient*-parse exception, losing the original strict one entirely — no cause chain linking them. The old code had the same structure, so this is not a regression, but the new code is a good opportunity to fix it. The simplest fix is to record the strict-parse warning first, before retrying: ```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)); ``` With this ordering, if the lenient parse succeeds, the WARNING is recorded. If the lenient parse throws, the WARNING is still recorded before the FATAL, giving the user both data points. -- 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]
