gnodet-bot commented on code in PR #13881:
URL: https://github.com/apache/maven/pull/13881#discussion_r4502278829
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsParserTest.java:
##########
@@ -509,17 +508,23 @@ void unnamedParserWorksInNativeContainer() throws
Exception {
}
@Test
- void unnamedParserConflictIsReported() throws Exception {
- var parsers = new HashMap<String, SettingsParser>();
- parsers.put(null, new PropertiesSettingsParser());
- parsers.put("properties", new PropertiesSettingsParser());
- var error =
- assertThrows(SettingsBuilderException.class, () ->
build(source("settings.properties", ""), parsers));
+ void parserConflictIsReported() throws Exception {
+ var parser1 = mock(SettingsParser.class);
+ when(parser1.id()).thenReturn("parser1");
Review Comment:
๐งน **Stale mock stubs โ still present.**
`parser1.id()` is stubbed to `"parser1"` on line 513, then immediately
overwritten to `"parser-a"` on line 516 โ Mockito uses the last stub, so line
513 is dead. Same for `parser2.id()`: stubbed `"parser2"` on line 515, then
`"parser-b"` on line 518.
Remove lines 513 and 515 (the two overridden stubs).
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsParserTest.java:
##########
@@ -265,35 +265,31 @@ 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)
+ private void assertSupportsFailureStopsSelectionAndParsing(String
parserId, boolean matchingFirst)
throws Exception {
SettingsParser matching = mock(SettingsParser.class);
+ when(matching.id()).thenReturn("matching");
Review Comment:
๐งน **Redundant/overwritten mock stubs โ still present.**
Three dead stubs in this block:
- `matching.id()` stubbed `"matching"` on line 276, then identically again
on line 281 โ Mockito uses the last stub, so line 276 is dead.
- `broken.id()` stubbed `"broken"` on line 278, then overwritten with
`parserId` on line 283 โ line 278 is dead.
- `unvisited.id()` stubbed `"unvisited"` on line 280, then identically again
on line 286 โ line 280 is dead.
Remove lines 276, 278, and 280 (the three dead first stubs, before the
effective ones are set at 281/283/286).
##########
its/core-it-suite/src/test/resources/mng-7836-alternative-pom-syntax/maven-hocon-extension/src/main/java/org/apache/maven/hocon/HoconModelReader.java:
##########
@@ -41,6 +41,10 @@
@Priority(1)
public class HoconModelReader implements ModelParser {
+ public String id() {
Review Comment:
๐งน **Missing `@Override` โ still present in IT fixtures.**
`id()` implements `ModelParser.id()` from the SPI but `@Override` is absent
here. The same issue applies to `DumbModelParser1`โ`4` and
`PropertiesSettingsParser` (mng-8655) โ all IT fixture implementations in this
PR add `id()` without the annotation.
All other implementations in the main codebase (e.g. `XmlSettingsParser`,
`DiTest` inner classes, `DefaultModelDialectManager`) include `@Override`
correctly.
```suggestion
@Override
public String id() {
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -92,21 +93,32 @@ protected KeyComputer<Repository> getRepositoryKey() {
*/
public DefaultSettingsBuilder(
SettingsXmlFactory settingsXmlFactory, Interpolator interpolator,
Map<String, Dispatcher> dispatchers) {
- this(settingsXmlFactory, interpolator, dispatchers, Map.of());
+ this(settingsXmlFactory, interpolator, dispatchers, List.of());
}
@Inject
public DefaultSettingsBuilder(
SettingsXmlFactory settingsXmlFactory,
Interpolator interpolator,
Map<String, Dispatcher> dispatchers,
- @Nullable Map<String, SettingsParser> settingsParsers) {
+ @Nullable List<SettingsParser> settingsParsers) {
this.xmlSettingsParser = new XmlSettingsParser(settingsXmlFactory);
- this.settingsParsers = settingsParsers != null ? settingsParsers :
Map.of();
+ this.settingsParsers = deduplicate(settingsParsers);
this.interpolator = interpolator;
this.dispatchers = dispatchers;
}
+ private static List<SettingsParser> deduplicate(@Nullable
List<SettingsParser> parsers) {
+ if (parsers == null || parsers.isEmpty()) {
+ return List.of();
+ }
+ Map<String, SettingsParser> seen = new LinkedHashMap<>();
+ for (SettingsParser parser : parsers) {
+ seen.putIfAbsent(parser.id(), parser);
+ }
Review Comment:
๐ **Silent deduplication โ id() collision produces no diagnostic.**
`deduplicate()` calls `putIfAbsent()` and silently drops a parser when two
implementations share the same `id()`. The Javadoc on `SettingsParser.id()`
says the id "must be globally unique across all extensions" and that `id()` is
used to "detect conflicts between extensions" โ but when a collision occurs,
the user gets no warning, no `BuilderProblem`, and no log entry. The duplicate
parser is silently discarded.
Add a `LOGGER.warn()` when `putIfAbsent` discards a registration:
```suggestion
private static List<SettingsParser> deduplicate(@Nullable
List<SettingsParser> parsers) {
if (parsers == null || parsers.isEmpty()) {
return List.of();
}
Map<String, SettingsParser> seen = new LinkedHashMap<>();
for (SettingsParser parser : parsers) {
SettingsParser previous = seen.putIfAbsent(parser.id(), parser);
if (previous != null) {
LOGGER.warn(
"Duplicate settings parser id '{}': {} already
registered, ignoring {}",
parser.id(),
previous.getClass().getName(),
parser.getClass().getName());
}
}
```
Same issue in `DefaultModelProcessor.deduplicate()` โ apply the same fix
there.
--
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]
