feat: add subtype module (2.17) #229

black-06 · 2023-11-11T06:55:55Z

Adds a module that allows subtype to be registered without annotating the parent class.

It is implemented on SPI.

Its original version is https://github.com/black-06/jackson-modules-dynamic-subtype.

I removed the custom ServiceLoader and always use the standard library,
which means that pojo always needs a no-parameter constructor.

cowtowncoder · 2023-11-12T04:37:09Z

pom.xml

@@ -33,6 +33,7 @@ not datatype, data format, or JAX-RS provider modules.
    <module>mrbean</module>
    <module>osgi</module>
    <module>paranamer</module>
+    <module>subtype</module>


I'll probably want to change the name, "subtype" is too generic. But let me think about better name for a while...

It's a difficult thing for me to give it a name...

True🥲🥲

cowtowncoder · 2023-11-12T04:37:43Z

subtype/README.md

+Registering modules.
+
+```
+ObjectMapper mapper = new ObjectMapper().registerModule(new DynamicSubtypeModule());


I think current name is SubtypeModule.

cowtowncoder · 2023-11-12T04:38:38Z

subtype/README.md

+2. provide a non-argument constructor (SPI require it).
+
+```java
+import io.github.black.jackson.JsonSubType;


Needs to change to new package.

cowtowncoder · 2023-11-12T04:44:55Z

subtype/src/test/java/com/fasterxml/jackson/module/subtype/WithJsonSubTypesTest.java

+@RunWith(value = Parameterized.class)
+public class WithJsonSubTypesTest<T extends WithJsonSubTypesTest.Parent> {
+
+    private final ObjectMapper mapper = new ObjectMapper().findAndRegisterModules();


Let's not use this method but instead explicitly register module.

cowtowncoder · 2023-11-12T04:45:48Z

subtype/src/test/java/com/fasterxml/jackson/module/subtype/WithJsonSubTypesTest.java

+            this.foo = foo;
+        }
+
+        @SuppressWarnings("unused") // jackson require it


We could instead just use public String foo to avoid having to add getter and setter

cowtowncoder · 2023-11-12T04:48:10Z

subtype/src/test/java/com/fasterxml/jackson/module/subtype/WithoutJsonSubTypesTest.java

+import static org.junit.Assert.assertTrue;
+
+/**
+ * test work without {@link JsonSubTypes}


I am not sure I understand -- why would we test functioning without module? And isn't module actually register with the mapper. Or what is being tested here? Could probably use longer explanation of reasoning.

Probably test against Jackson polymorphic type handling working as expected. If so, does not seem necessary

I actually wanted to test whether @JsonSubType worked with @JsonSubTypes

JooHyukKim · 2023-11-12T05:02:18Z

subtype/src/main/java/com/fasterxml/jackson/module/subtype/SubtypeModule.java

+                List<NamedType> list1 = SubtypeModule.findSubtypes(a.getRawType(), a::getAnnotation);
+                List<NamedType> list2 = subtypes.getOrDefault(a.getRawType(), Collections.emptyList());


Can we rename these list1 and list2 to something less generic?

Here is a simple modification of https://github.com/FasterXML/jackson-databind/blob/3a271be870821301f73054e43e32f657e4753234/src/main/java/com/fasterxml/jackson/databind/introspect/AnnotationIntrospectorPair.java#L280-L291

JooHyukKim · 2023-11-12T05:05:14Z

subtype/src/main/java/com/fasterxml/jackson/module/subtype/SubtypeModule.java

+                if (list1.isEmpty()) return list2;
+                if (list2.isEmpty()) return list1;


If the two lists are null-safe, no need for empty checking.

JooHyukKim · 2023-11-12T05:05:28Z

subtype/src/main/java/com/fasterxml/jackson/module/subtype/SubtypeModule.java

+                List<NamedType> list = new ArrayList<>(list1.size() + list2.size());
+                list.addAll(list1);
+                list.addAll(list2);
+                return list;


Suggested change

List<NamedType> list = new ArrayList<>(list1.size() + list2.size());

list.addAll(list1);

list.addAll(list2);

return list;

List<NamedType> list = new ArrayList<>(list1.size() + list2.size());

list1.addAll(list2);

return list;

JooHyukKim · 2023-11-12T05:07:00Z

subtype/src/main/java/com/fasterxml/jackson/module/subtype/SubtypeModule.java

+                List<NamedType> list = new ArrayList<>(list1.size() + list2.size());
+                list.addAll(list1);
+                list.addAll(list2);
+                return list;


Is it possible to have duplication? If so, can we have these as set first, then return as list?

JooHyukKim · 2023-11-12T05:09:12Z

subtype/src/test/java/com/fasterxml/jackson/module/subtype/WithoutJsonSubTypesTest.java

+import static org.junit.Assert.assertTrue;
+
+/**
+ * test work without {@link JsonSubTypes}


Probably test against Jackson polymorphic type handling working as expected. If so, does not seem necessary

JooHyukKim · 2023-11-12T05:10:35Z

subtype/src/main/java/com/fasterxml/jackson/module/subtype/SubtypeModule.java

+        subtypes.remove(parent);
+    }
+
+    private static <S> List<NamedType> findSubtypes(Class<S> clazz, Function<Class<JsonSubType>, JsonSubType> getter) {


Is there a reason for keeping it static? if not maybe like...

Suggested change

private static <S> List<NamedType> findSubtypes(Class<S> clazz, Function<Class<JsonSubType>, JsonSubType> getter) {

private <S> List<NamedType> _findSubtypes(Class<S> clazz, Function<Class<JsonSubType>, JsonSubType> getter) {

JooHyukKim · 2023-11-12T05:13:57Z

subtype/src/main/java/com/fasterxml/jackson/module/subtype/JsonSubType.java

+@Target(ElementType.TYPE)
+@Retention(RetentionPolicy.RUNTIME)
+@JacksonAnnotation
+public @interface JsonSubType {


Name sounds like it's part of @JsonSubTypes of annotations module. This may confuse users quite. If module name changes as per https://github.com/FasterXML/jackson-modules-base/pull/229/files#r1390329649, can we change name here also?

JooHyukKim · 2023-11-12T05:14:35Z

subtype/src/main/java/com/fasterxml/jackson/module/subtype/JsonSubType.java

+     * more than one type name should be associated with the same type.
+     *
+     * @return subtype name array
+     * @since 2.12


Version mismatch, can remove.

Suggested change

* @since 2.12

... then add @since 2.16 to the class

JooHyukKim · 2023-11-12T05:15:20Z

subtype/src/main/java/com/fasterxml/jackson/module/subtype/SubtypeModule.java

+ * Therefore, we can {@link #unregisterType} a class and then module will reload this class's subclasses.
+ */
+public class SubtypeModule extends Module {


Suggested change

* Therefore, we can {@link #unregisterType} a class and then module will reload this class's subclasses.

*/

public class SubtypeModule extends Module {

* Therefore, we can {@link #unregisterType} a class and then module will reload this class's subclasses.

*

* @since 2.16

*/

public class SubtypeModule extends Module {

cowtowncoder · 2023-11-16T02:24:18Z

Quick note: due to timing, decided that I will not try to get this in Jackson 2.16 -- so will need to be re-based to 2.17.
This because there's a bit of work to be done here.

For example, I think that naming of @JsonSubType should change as the general rule is that only jackson-annotations has annotations with name starting with @Json (and for legacy reasons, some in jackson-databind).
Modules can provide annotations that start with @Jackson prefix (or anything else that makes sense).

But I need to read the PR in bit more thought to make sure my feedback is relevant.

feat: add subtype module

30da2f3

black-06 mentioned this pull request Nov 11, 2023

Contribute to jackson-modules-base black-06/jackson-modules-dynamic-subtype#1

Closed

cowtowncoder reviewed Nov 12, 2023

View reviewed changes

JooHyukKim reviewed Nov 12, 2023

View reviewed changes

fix

5152c26

cowtowncoder changed the title ~~feat: add subtype module~~ feat: add subtype module (2.17) Nov 16, 2023

cowtowncoder added the 2.17 label Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add subtype module (2.17) #229

feat: add subtype module (2.17) #229

black-06 commented Nov 11, 2023

cowtowncoder Nov 12, 2023

black-06 Nov 13, 2023

JooHyukKim Nov 13, 2023

cowtowncoder Nov 12, 2023

cowtowncoder Nov 12, 2023

cowtowncoder Nov 12, 2023

cowtowncoder Nov 12, 2023

cowtowncoder Nov 12, 2023

JooHyukKim Nov 12, 2023

black-06 Nov 13, 2023

JooHyukKim Nov 12, 2023

black-06 Nov 13, 2023

JooHyukKim Nov 12, 2023

JooHyukKim Nov 12, 2023

JooHyukKim Nov 12, 2023

JooHyukKim Nov 12, 2023

JooHyukKim Nov 12, 2023

JooHyukKim Nov 12, 2023

JooHyukKim Nov 12, 2023

JooHyukKim Nov 12, 2023

cowtowncoder commented Nov 16, 2023

		List<NamedType> list1 = SubtypeModule.findSubtypes(a.getRawType(), a::getAnnotation);
		List<NamedType> list2 = subtypes.getOrDefault(a.getRawType(), Collections.emptyList());

		if (list1.isEmpty()) return list2;
		if (list2.isEmpty()) return list1;

	private static <S> List<NamedType> findSubtypes(Class<S> clazz, Function<Class<JsonSubType>, JsonSubType> getter) {
	private <S> List<NamedType> _findSubtypes(Class<S> clazz, Function<Class<JsonSubType>, JsonSubType> getter) {

feat: add subtype module (2.17) #229

Are you sure you want to change the base?

feat: add subtype module (2.17) #229

Conversation

black-06 commented Nov 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Nov 16, 2023