gnodet commented on code in PR #2333:
URL: https://github.com/apache/maven/pull/2333#discussion_r3369570646
##########
api/maven-api-settings/src/main/mdo/settings.mdo:
##########
@@ -529,6 +529,16 @@
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 by copying the entire configuration, but
using a different ID.
Review Comment:
Minor: The description mentions "an additional server will be generated by
copying the entire configuration" -- it may be worth noting that the
`<aliases>` list itself is NOT copied to the generated server entries (they get
an empty aliases list). This is important for users to understand: aliases are
not transitive.
##########
compat/maven-settings-builder/src/test/java/org/apache/maven/settings/building/DefaultSettingsBuilderFactoryTest.java:
##########
@@ -32,17 +38,101 @@ private File getSettings(String name) {
return new File("src/test/resources/settings/factory/" + name +
".xml").getAbsoluteFile();
}
- @Test
- void testCompleteWiring() throws Exception {
+ SettingsBuildingResult execute(String settingsName) throws Exception {
+ Properties properties = new Properties();
+ properties.setProperty("user.home", "/home/user");
+
SettingsBuilder builder = new
DefaultSettingsBuilderFactory().newInstance();
assertNotNull(builder);
DefaultSettingsBuildingRequest request = new
DefaultSettingsBuildingRequest();
- request.setSystemProperties(System.getProperties());
- request.setUserSettingsFile(getSettings("simple"));
+ request.setSystemProperties(properties);
+ request.setUserSettingsFile(getSettings(settingsName));
SettingsBuildingResult result = builder.build(request);
assertNotNull(result);
- assertNotNull(result.getEffectiveSettings());
+ return result;
+ }
+
+ @Test
+ void testCompleteWiring() throws Exception {
+ Settings settings = execute("simple").getEffectiveSettings();
+
+ String localRepository = settings.getLocalRepository();
+ assertTrue(localRepository.equals("/home/user/.m2/repository")
+ || localRepository.endsWith("\\home\\user\\.m2\\repository"));
+ }
+
+ @Test
+ void testSettingsWithServers() throws Exception {
+ Settings settings =
execute("settings-servers-1").getEffectiveSettings();
+
+ List<Server> servers = settings.getDelegate().getServers();
+ assertEquals(2, servers.size());
+
+ Server server1 = getServerById(servers, "server-1");
+ assertEquals("username1", server1.getUsername());
+ assertEquals("password1", server1.getPassword());
+
+ Server server2 = getServerById(servers, "server-2");
+ assertEquals("username2", server2.getUsername());
+ assertEquals("password2", server2.getPassword());
+ }
+
+ @Test
+ void testSettingsWithServersAndAliases() throws Exception {
+ Settings settings =
execute("settings-servers-2").getEffectiveSettings();
+
+ List<Server> servers = settings.getDelegate().getServers();
+ assertEquals(6, servers.size());
+
+ Server server1 = getServerById(servers, "server-1");
+ assertEquals("username1", server1.getUsername());
+ assertEquals("password1", server1.getPassword());
+ assertEquals(List.of("server-11", "server-12"), server1.getAliases());
+
+ Server server11 = getServerById(servers, "server-11");
+ assertEquals("username1", server11.getUsername());
+ assertEquals("password1", server11.getPassword());
+ assertTrue(server11.getAliases().isEmpty());
+
+ Server server12 = getServerById(servers, "server-12");
+ assertEquals("username1", server12.getUsername());
+ assertEquals("password1", server12.getPassword());
+ assertTrue(server11.getAliases().isEmpty());
Review Comment:
Bug: copy-paste error -- this asserts `server11.getAliases()` but should
assert `server12.getAliases()`. The test still passes because both
alias-expanded servers have empty aliases, but it is verifying the wrong
variable.
```suggestion
assertTrue(server12.getAliases().isEmpty());
```
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsBuilderFactoryTest.java:
##########
@@ -53,23 +61,106 @@ void setup() {
.thenReturn(new DefaultSettingsXmlFactory());
}
- @Test
- void testCompleteWiring() {
+ SettingsBuilderResult execute(String settingsName) {
SettingsBuilder builder =
new DefaultSettingsBuilder(new DefaultSettingsXmlFactory(),
new DefaultInterpolator(), Map.of());
assertNotNull(builder);
SettingsBuilderRequest request = SettingsBuilderRequest.builder()
.session(session)
-
.userSettingsSource(Sources.buildSource(getSettings("settings-simple")))
+
.userSettingsSource(Sources.buildSource(getSettings(settingsName)))
.build();
SettingsBuilderResult result = builder.build(request);
assertNotNull(result);
- assertNotNull(result.getEffectiveSettings());
+ return result;
+ }
+
+ @Test
+ void testCompleteWiring() {
+ Settings settings = execute("settings-simple").getEffectiveSettings();
+
+ String localRepository = settings.getLocalRepository();
+ assertTrue(localRepository.equals("${user.home}/.m2/repository")
+ ||
localRepository.endsWith("\\${user.home}\\.m2\\repository"));
+ }
+
+ @Test
+ void testSettingsWithServers() {
+ Settings settings =
execute("settings-servers-1").getEffectiveSettings();
+
+ List<Server> servers = settings.getServers();
+ assertEquals(2, servers.size());
+
+ Server server1 = getServerById(servers, "server-1");
+ assertEquals("username1", server1.getUsername());
+ assertEquals("password1", server1.getPassword());
+
+ Server server2 = getServerById(servers, "server-2");
+ assertEquals("username2", server2.getUsername());
+ assertEquals("password2", server2.getPassword());
+ }
+
+ @Test
+ void testSettingsWithServersAndAliases() {
+ Settings settings =
execute("settings-servers-2").getEffectiveSettings();
+
+ assertEquals("${user.home}/.m2/repository",
settings.getLocalRepository());
+
+ List<Server> servers = settings.getServers();
+ assertEquals(6, servers.size());
+
+ Server server1 = getServerById(servers, "server-1");
+ assertEquals("username1", server1.getUsername());
+ assertEquals("password1", server1.getPassword());
+ assertEquals(List.of("server-11", "server-12"), server1.getAliases());
+
+ Server server11 = getServerById(servers, "server-11");
+ assertEquals("username1", server11.getUsername());
+ assertEquals("password1", server11.getPassword());
+ assertTrue(server11.getAliases().isEmpty());
+
+ Server server12 = getServerById(servers, "server-12");
+ assertEquals("username1", server12.getUsername());
+ assertEquals("password1", server12.getPassword());
+ assertTrue(server11.getAliases().isEmpty());
Review Comment:
Same copy-paste error as in the compat test -- should be `server12` instead
of `server11`.
```suggestion
assertTrue(server12.getAliases().isEmpty());
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -228,6 +232,17 @@ private Settings readSettings(
return settings;
}
+ private List<Server> serversByIds(List<Server> servers) {
Review Comment:
Consider adding validation here (or at least in the validator) that an alias
does not equal the server's own id, and that the same alias string is not used
by multiple servers. The existing duplicate-id check in
`DefaultSettingsValidator` will catch both cases, but the resulting error
message ("duplicate server with id X") does not help the user understand
whether the problem is in their `<aliases>` configuration. A more specific
message during alias expansion would be more actionable.
For example:
```java
private List<Server> serversByIds(List<Server> servers) {
Set<String> allIds = new HashSet<>();
return servers.stream()
.flatMap(server -> {
if (!allIds.add(server.getId())) {
// will be caught by validator
}
return Stream.concat(
Stream.of(server),
server.getAliases().stream().map(alias -> {
if (alias.equals(server.getId())) {
// report: alias must differ from server id
}
if (!allIds.add(alias)) {
// report: alias clashes with another
server/alias
}
return serverAlias(server, alias);
}));
})
.toList();
}
```
This is a suggestion for a potential follow-up rather than a blocker.
--
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]
