gnodet commented on code in PR #12117:
URL: https://github.com/apache/maven/pull/12117#discussion_r3279175699
##########
impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedConfigurationConverter.java:
##########
@@ -108,6 +111,56 @@ public Object fromConfiguration(
}
}
+ private Class<?> resolveClassForImplementationHint(
+ final Class<?> type, final PlexusConfiguration configuration,
final ClassLoader loader)
+ throws ComponentConfigurationException {
+ try {
+ return super.getClassForImplementationHint(type, configuration,
loader);
+ } catch (final ComponentConfigurationException e) {
+ if (type == null || !type.isSealed()) {
+ throw e;
+ }
+ final String implementation =
configuration.getAttribute("implementation");
+ if (implementation == null || implementation.isEmpty()) {
+ throw e;
+ }
+ return getPermittedSubclass(type, implementation, configuration,
e);
+ }
+ }
+
+ private Class<?> getPermittedSubclass(
+ final Class<?> type,
+ final String implementation,
+ final PlexusConfiguration configuration,
+ final ComponentConfigurationException cause)
+ throws ComponentConfigurationException {
+ final List<Class<?>> matches = new ArrayList<>();
+ for (Class<?> permittedSubclass : type.getPermittedSubclasses()) {
+ if (implementation.equals(permittedSubclass.getName())
+ ||
implementation.equals(permittedSubclass.getCanonicalName())
+ ||
implementation.equals(permittedSubclass.getSimpleName())) {
+ matches.add(permittedSubclass);
+ }
+ }
+
+ if (matches.size() == 1) {
+ return matches.get(0);
+ }
+ matches.sort(Comparator.comparing(Class::getName));
+
+ if (matches.isEmpty()) {
+ throw new ComponentConfigurationException(
+ configuration,
+ "Cannot find permitted subclass '" + implementation + "'
for sealed type " + type.getName(),
+ cause);
+ }
Review Comment:
Minor: sorting before the `isEmpty()` check is harmless but reads oddly.
Moving `isEmpty()` first makes the intent clearer:
```suggestion
if (matches.size() == 1) {
return matches.get(0);
}
if (matches.isEmpty()) {
throw new ComponentConfigurationException(
configuration,
"Cannot find permitted subclass '" + implementation + "'
for sealed type " + type.getName(),
cause);
}
matches.sort(Comparator.comparing(Class::getName));
```
##########
impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedConfigurationConverter.java:
##########
@@ -108,6 +111,56 @@ public Object fromConfiguration(
}
}
+ private Class<?> resolveClassForImplementationHint(
+ final Class<?> type, final PlexusConfiguration configuration,
final ClassLoader loader)
+ throws ComponentConfigurationException {
+ try {
+ return super.getClassForImplementationHint(type, configuration,
loader);
+ } catch (final ComponentConfigurationException e) {
+ if (type == null || !type.isSealed()) {
+ throw e;
+ }
+ final String implementation =
configuration.getAttribute("implementation");
+ if (implementation == null || implementation.isEmpty()) {
+ throw e;
+ }
+ return getPermittedSubclass(type, implementation, configuration,
e);
+ }
+ }
+
+ private Class<?> getPermittedSubclass(
+ final Class<?> type,
+ final String implementation,
+ final PlexusConfiguration configuration,
+ final ComponentConfigurationException cause)
+ throws ComponentConfigurationException {
+ final List<Class<?>> matches = new ArrayList<>();
+ for (Class<?> permittedSubclass : type.getPermittedSubclasses()) {
+ if (implementation.equals(permittedSubclass.getName())
+ ||
implementation.equals(permittedSubclass.getCanonicalName())
+ ||
implementation.equals(permittedSubclass.getSimpleName())) {
+ matches.add(permittedSubclass);
+ }
+ }
+
+ if (matches.size() == 1) {
+ return matches.get(0);
+ }
+ matches.sort(Comparator.comparing(Class::getName));
+
+ if (matches.isEmpty()) {
+ throw new ComponentConfigurationException(
+ configuration,
+ "Cannot find permitted subclass '" + implementation + "'
for sealed type " + type.getName(),
+ cause);
+ }
+
+ throw new ComponentConfigurationException(
+ configuration,
+ "Implementation hint '" + implementation + "' is ambiguous for
sealed type " + type.getName() + ": "
+ + matches.stream().map(Class::getName).toList());
Review Comment:
Minor: the "not found" path includes `cause` but the ambiguity path does
not. Should be consistent so diagnostic context isn't lost:
```suggestion
throw new ComponentConfigurationException(
configuration,
"Implementation hint '" + implementation + "' is ambiguous
for sealed type " + type.getName() + ": "
+ matches.stream().map(Class::getName).toList(),
cause);
```
##########
impl/maven-core/src/test/java/org/apache/maven/configuration/DefaultBeanConfiguratorTest.java:
##########
@@ -108,8 +110,72 @@ void testChildConfigurationElement() throws
BeanConfigurationException {
assertEquals(new File("test"), bean.file);
}
+ @Test
+ void testSealedTypeImplementationHintCanUseSimpleName() throws
BeanConfigurationException {
+ SealedBean bean = new SealedBean();
+
+ Xpp3Dom config = toConfig("<artifact
implementation=\"LocalArtifact\"><name>local</name></artifact>");
+
+ DefaultBeanConfigurationRequest request = new
DefaultBeanConfigurationRequest();
+ request.setBean(bean).setConfiguration(config);
+
+ configurator.configureBean(request);
+
+ LocalArtifact artifact = assertInstanceOf(LocalArtifact.class,
bean.artifact);
+ assertEquals("local", artifact.name);
+ }
+
+ @Test
+ void testSealedTypeImplementationHintCanStillUseClassName() throws
BeanConfigurationException {
+ SealedBean bean = new SealedBean();
+
+ Xpp3Dom config = toConfig("<artifact implementation=\"" +
RemoteArtifact.class.getName()
+ + "\"><url>https://example.invalid/artifact</url></artifact>");
+
+ DefaultBeanConfigurationRequest request = new
DefaultBeanConfigurationRequest();
+ request.setBean(bean).setConfiguration(config);
+
+ configurator.configureBean(request);
+
+ RemoteArtifact artifact = assertInstanceOf(RemoteArtifact.class,
bean.artifact);
+ assertEquals("https://example.invalid/artifact";, artifact.url);
+ }
+
+ @Test
+ void testSealedTypeImplementationHintMustMatchPermittedSubclass() {
+ SealedBean bean = new SealedBean();
+
+ Xpp3Dom config = toConfig("<artifact
implementation=\"MissingArtifact\"><name>missing</name></artifact>");
+
+ DefaultBeanConfigurationRequest request = new
DefaultBeanConfigurationRequest();
+ request.setBean(bean).setConfiguration(config);
+
+ BeanConfigurationException e =
+ assertThrows(BeanConfigurationException.class, () ->
configurator.configureBean(request));
+ assertEquals(
Review Comment:
Minor: there's no test for the **ambiguity** code path — where two permitted
subclasses share the same simple name. The code handles it with a sorted error
message, so it would be good to verify that too. This is admittedly a rare
scenario (e.g., inner classes from different enclosing types) but the code
branch exists.
Also, the `assertEquals(e.getMessage(), ...)` assertion here could be
brittle if `BeanConfigurationException` changes how it wraps the cause message.
Consider `assertTrue(e.getMessage().contains(...))` as a more resilient check.
##########
impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedConfigurationConverter.java:
##########
@@ -108,6 +111,56 @@ public Object fromConfiguration(
}
}
+ private Class<?> resolveClassForImplementationHint(
+ final Class<?> type, final PlexusConfiguration configuration,
final ClassLoader loader)
+ throws ComponentConfigurationException {
+ try {
+ return super.getClassForImplementationHint(type, configuration,
loader);
+ } catch (final ComponentConfigurationException e) {
+ if (type == null || !type.isSealed()) {
+ throw e;
+ }
+ final String implementation =
configuration.getAttribute("implementation");
+ if (implementation == null || implementation.isEmpty()) {
+ throw e;
+ }
+ return getPermittedSubclass(type, implementation, configuration,
e);
+ }
+ }
+
+ private Class<?> getPermittedSubclass(
+ final Class<?> type,
+ final String implementation,
+ final PlexusConfiguration configuration,
+ final ComponentConfigurationException cause)
+ throws ComponentConfigurationException {
+ final List<Class<?>> matches = new ArrayList<>();
+ for (Class<?> permittedSubclass : type.getPermittedSubclasses()) {
+ if (implementation.equals(permittedSubclass.getName())
+ ||
implementation.equals(permittedSubclass.getCanonicalName())
+ ||
implementation.equals(permittedSubclass.getSimpleName())) {
Review Comment:
Missing `cause` in the ambiguity exception — the "not found" path includes
it but this one does not, losing diagnostic context.
```suggestion
throw new ComponentConfigurationException(
configuration,
"Implementation hint '" + implementation + "' is ambiguous
for sealed type " + type.getName() + ": "
+ matches.stream().map(Class::getName).toList(),
cause);
```
--
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]
