gnodet commented on code in PR #2333:
URL: https://github.com/apache/maven/pull/2333#discussion_r3439145839
##########
api/maven-api-settings/src/main/mdo/settings.mdo:
##########
@@ -529,6 +529,18 @@
Extra configuration for the transport layer.
</description>
</field>
+ <field>
+ <name>aliases</name>
+ <version>1.3.0+</version>
+ <description>List of additional server aliases. For each alias, an
additional server will be generated.
+ Genearte servers items will have the same configuration as the
original one, but with the ID specified in this list
+ and with an empty aliases list.
+ This is useful when the same credentials should be used for
multiple servers.</description>
Review Comment:
Typo: "Genearte" → "Generated". Also minor wording improvement for clarity.
```suggestion
<description>List of additional server aliases. For each alias, an
additional server entry will be generated
with the same configuration as the original one, but with the ID
replaced by the alias
and with an empty aliases list.
This is useful when the same credentials should be used for
multiple servers.</description>
```
##########
compat/maven-settings-builder/src/main/java/org/apache/maven/settings/validation/DefaultSettingsValidator.java:
##########
@@ -93,6 +93,20 @@ public void validate(Settings settings,
SettingsProblemCollector problems) {
"must be unique but found duplicate server with id
" + server.getId());
}
}
+
+ for (int i = 0; i < servers.size(); i++) {
+ Server server = servers.get(i);
+ for (String alias : server.getAliases()) {
+ if (!serverIds.add(alias)) {
+ addViolation(
+ problems,
+ Severity.WARNING,
+ "servers.server[" + i + "].aliases",
+ server.getId(),
+ "must be unique for all servers id but found
duplicate alias " + alias);
Review Comment:
Same wording fix as the impl validator.
```suggestion
"must be unique across all server ids and
aliases but found duplicate alias " + alias);
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsValidator.java:
##########
@@ -123,6 +126,20 @@ public void validate(Settings settings, boolean
isProjectSettings, ProblemCollec
"must be unique but found duplicate server with id
" + server.getId());
}
}
+
+ for (int i = 0; i < servers.size(); i++) {
+ Server server = servers.get(i);
+ for (String alias : server.getAliases()) {
Review Comment:
Consider adding validation for empty or blank alias strings before the
uniqueness check. An `<alias></alias>` or `<alias> </alias>` entry would
silently produce a server with an empty/blank ID, which would cause
hard-to-diagnose issues downstream.
Something like:
```java
validateStringNotEmpty(problems, "servers.server[" + i + "].aliases", alias,
server.getId());
```
before the `serverIds.add` check.
Same applies to the compat validator.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsValidator.java:
##########
@@ -123,6 +126,20 @@ public void validate(Settings settings, boolean
isProjectSettings, ProblemCollec
"must be unique but found duplicate server with id
" + server.getId());
}
}
+
+ for (int i = 0; i < servers.size(); i++) {
+ Server server = servers.get(i);
+ for (String alias : server.getAliases()) {
+ if (!serverIds.add(alias)) {
+ addViolation(
+ problems,
+ BuilderProblem.Severity.WARNING,
+ "servers.server[" + i + "].aliases",
+ server.getId(),
+ "must be unique for all servers id but found
duplicate alias " + alias);
Review Comment:
Minor grammar: "for all servers id" reads awkwardly.
```suggestion
"must be unique across all server ids and
aliases but found duplicate alias " + alias);
```
##########
compat/maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java:
##########
@@ -182,6 +184,7 @@ private Settings readSettings(
}
settingsValidator.validate(settings, problems);
+ settings.setServers(serversByIds(settings.getServers()));
Review Comment:
`serversByIds()` uses `Stream.toList()` which returns an unmodifiable list.
The Modello-generated `setServers()` stores the reference directly, so any
downstream code calling `getServers().add(...)` would throw
`UnsupportedOperationException`.
This is likely fine since settings aren't mutated after building, but if
safety is preferred, consider wrapping: `new ArrayList<>(serversByIds(...))`
(would need the import).
--
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]
