gnodet commented on code in PR #12117:
URL: https://github.com/apache/maven/pull/12117#discussion_r3279247605
##########
impl/maven-core/src/test/java/org/apache/maven/configuration/DefaultBeanConfiguratorTest.java:
##########
@@ -31,6 +31,8 @@
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
Review Comment:
Add `assertTrue` import for the assertion change below:
```suggestion
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
```
##########
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(
+ "Cannot find permitted subclass 'MissingArtifact' for sealed
type " + SealedArtifact.class.getName(),
+ e.getMessage());
+ }
+
static class SomeBean {
File file;
}
+
+ static class SealedBean {
+
+ SealedArtifact artifact;
+ }
+
+ public sealed interface SealedArtifact permits LocalArtifact,
RemoteArtifact {}
+
+ public static final class LocalArtifact implements SealedArtifact {
+
+ String name;
+ }
+
+ public static final class RemoteArtifact implements SealedArtifact {
+
+ String url;
+ }
}
Review Comment:
Consider adding a test for the **ambiguity** code path (two permitted
subclasses sharing the same simple name). This also requires adding the helper
types:
```suggestion
}
@Test
void testSealedTypeAmbiguousSimpleNameThrowsError() {
AmbiguousBean bean = new AmbiguousBean();
Xpp3Dom config = toConfig("<value implementation=\"Ambiguous\"/>");
DefaultBeanConfigurationRequest request = new
DefaultBeanConfigurationRequest();
request.setBean(bean).setConfiguration(config);
BeanConfigurationException e =
assertThrows(BeanConfigurationException.class, () ->
configurator.configureBean(request));
assertTrue(e.getMessage().contains("is ambiguous for sealed type"));
}
static class SomeBean {
File file;
}
static class SealedBean {
SealedArtifact artifact;
}
static class AmbiguousBean {
AmbiguousSealedType value;
}
public sealed interface SealedArtifact permits LocalArtifact,
RemoteArtifact {}
public sealed interface AmbiguousSealedType permits Holder1.Ambiguous,
Holder2.Ambiguous {}
public static final class LocalArtifact implements SealedArtifact {
String name;
}
public static final class RemoteArtifact implements SealedArtifact {
String url;
}
public static final class Holder1 {
public static final class Ambiguous implements AmbiguousSealedType {}
}
public static final class Holder2 {
public static final class Ambiguous implements AmbiguousSealedType {}
}
}
```
##########
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(
+ "Cannot find permitted subclass 'MissingArtifact' for sealed
type " + SealedArtifact.class.getName(),
+ e.getMessage());
Review Comment:
Using `assertEquals` on the full message can be brittle if
`BeanConfigurationException` changes how it wraps the cause. A `contains` check
is more resilient:
```suggestion
assertTrue(
e.getMessage().contains("Cannot find permitted subclass
'MissingArtifact' for sealed type "
+ SealedArtifact.class.getName()));
```
--
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]
