Implement JSON-B 3 Polymorphism #100

jungm · 2023-04-12T14:26:27Z

related discussion: https://lists.apache.org/thread/vok6wprktky7qkvwk79x68gz6mjcp7rb

rmannibucau

Did a full review now we agreed upon the design, it is mainly a few enhancements of the runtime and get back the user contract on some points but looks very close to be mergeable to me. Very good work and kudo to have enhanced the default polymorphism support which was not as great as we aim at!

rmannibucau · 2023-04-17T07:14:44Z

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JohnzonBuilder.java

@@ -31,6 +31,7 @@
 import org.apache.johnzon.mapper.Mapper;


guess except the setMappingsFactory the rest of the diff is no more needed?

rmannibucau · 2023-04-17T07:20:56Z

johnzon-mapper/src/main/java/org/apache/johnzon/mapper/access/Meta.java

@@ -78,21 +78,28 @@ private static <T extends Annotation> T getIndirectAnnotation(final Class<T> api
        return null;
    }

+    public static <T extends Annotation> T getDirectAnnotation(final Class<?> clazz, final Class<T> api) {


not sure why there is a diff in this class, it is already on master and should stay private while the usage should be getAnnotation in all the rest of the diff 🤔

rmannibucau · 2023-04-17T07:22:40Z

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbPolymorphismHandler.java

+        return clazz.isAnnotationPresent(JsonbTypeInfo.class) || !getParentClassesWithTypeInfo(clazz).isEmpty();
+    }
+
+    public List<Map.Entry<String, String>> getPolymorphismPropertiesToSerialize(Class<?> clazz, Collection<String> otherProperties) {


looks like the returned type should be an array of pairs (or Map.Entry this part is a detail) instead of a list to make the iteration faster at runtime, wdyt?

rmannibucau · 2023-04-17T07:24:48Z

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbPolymorphismHandler.java

+    }
+
+    public List<Map.Entry<String, String>> getPolymorphismPropertiesToSerialize(Class<?> clazz, Collection<String> otherProperties) {
+        List<Map.Entry<String, String>> entries = new ArrayList<>();


why not a plain LinkedHashMap which will enable to simplify the "found more than once" check and keep the ordering (will just need to sorted(reverse) when converting to an array), wdyt? Overall idea is to make the intent to provide a partial object (generator usage) more explicit.

The "TypeInfo key only once" check can actually be computed in advance, so I kept it as a List.
list.add(0, ...) is way more convenient than converting a LinkedHashMap to a reversed Map.Entry<>[] imho

rmannibucau · 2023-04-17T07:25:58Z

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbPolymorphismHandler.java

+     * @throws JsonbException validation failed
+     */
+    private void validateOnlyOneParentWithTypeInfo(Class<?> classToValidate) {
+        if (getParentClassesWithTypeInfo(classToValidate).size() > 1) {


this looks called too often (at least twice per type if I'm not mistaken, maybe reuse the same instance for both needs?)

rmannibucau · 2023-04-17T07:32:36Z

src/site/markdown/index.md

@@ -497,57 +497,6 @@ in `MessageDecoder`.
    }


-
-### JSON-B Extra


maybe keep it and explain:

very high level what is in the spec

how to migrate from extras module to the standard impl

don't know if migrating from jsonb-extras to jsonb3 will even be something users want to do - generated JSON format is different ({"_type": "bla", "_value": {"someValue": 1}} vs {"_type": "bla", "someValue": 1})

Don't really want to add support for this format in jsonb module directly. It's out of spec and jsonb-extras was portable and is probably used on top of other jsonb impls
Maybe jsonb-extras shouldn't actually be deleted? It was pretty leightweight anyways

:), fine to revert the deletion, while the feature is there all good for me.

reverted the deletion + updated docs that extras polymorphism shouldn't be used anymore

rmannibucau · 2023-04-17T07:35:45Z

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbPolymorphismHandler.java

+    }
+
+    public Class<?> getTypeToDeserialize(JsonObject jsonObject, Class<?> clazz) {
+        validateJsonbTypeInfo(clazz);


this should be done one per class somehow since it is a runtime class (compared to getPolymorphismPropertiesToSerialize which is a classmapping init computation only) so maybe use ClassMapping to do the validation only one, can likely become a Supplier<Class<?>> using the supplier "pre" part to do the validation and supplier#get to impl the runtime only (current line 91+).

rmannibucau · 2023-04-17T07:36:18Z

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbPolymorphismHandler.java

+        }
+
+        JsonValue typeValue = jsonObject.get(typeInfo.key());
+        if (!(typeValue instanceof JsonString)) {


typeValue != null && typeValue.getValueType() != STRING is faster due to JVM instanceof impl (to avoid when possible)

rmannibucau · 2023-04-17T07:36:36Z

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbPolymorphismHandler.java

+        }
+
+        String typeValueString = ((JsonString) typeValue).getString();
+        for (JsonbSubtype subtype : typeInfo.value()) {


should be a map for runtime probably

rmannibucau · 2023-04-17T07:39:23Z

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbPolymorphismHandler.java

+     * @param clazz base class
+     * @return List of classes with JsonbTypeInfo annotation
+     */
+    private List<Class<?>> getParentClassesWithTypeInfo(Class<?> clazz) {


wonder if the following pattern isnt more relevant (pseudo code):

final var superClass = clazz.getSuperclass(); if (superClass != null && hasTypeInfo(superClass)) { return superClass; } return Stream.of(clazz.getInterfaces()).filter(it -> hasTypeInfo(it)).findFirst().orElse(null);

Overall idea is to avoid to create a list to keep only the first element. For interface case it will not change a lot but for parent case it will make it way more efficient. wdyt?

tandraschko · 2023-04-18T11:24:49Z

great work Markus, looking forward for this contribution!

This reverts commit 594083a.

rmannibucau

a small autoformat fix to do and getTypeToDeserialize to make runtime friendly and we are good to me

rmannibucau · 2023-04-19T07:03:27Z

src/site/markdown/index.md

@@ -512,7 +512,9 @@ This module provides some extension to JSON-B.

 #### Polymorphism

-This extension provides a way to handle polymorphism:
+This extension shouldn't be used anymore if you don't absolutely rely on the JSON format it generates/parses.


What about "If your aim is only to be polymorphic you can use the built-in JSON-B feature but some cases remain unhandled and this extension enable them (mainly JSON-B to JSON-B cases like configurations and service to service calls but unlikely javascript to java cases)".

rmannibucau · 2023-04-19T07:03:40Z

johnzon-mapper/src/main/java/org/apache/johnzon/mapper/Mappings.java

@@ -18,11 +18,16 @@
 */
 package org.apache.johnzon.mapper;

-import static java.util.Arrays.asList;


oops (autoformat)?

rmannibucau · 2023-04-19T07:06:42Z

johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbPolymorphismHandler.java

+        return result.toArray(Map.Entry[]::new);
+    }
+
+    public Class<?> getTypeToDeserialize(JsonObject jsonObject, Class<?> clazz) {


implemented like that this must not be called at runtime, overall consider any reflection is forbidden at runtime except to discover the passed instance type so this impl should be limited to (pseudo code) typeInfos.get(jsonObject.getString("...")) so the reflection should have been stored in the handler earlier (likely in validate or getPolymorphismPropertiesToSerialize to avoid to do it > once) and the lookup shouldnt be a loop but a map access

reworked and reflection free now. Except for Class#getName, is using it okay? Or should we possibly completely move away from having the class as a parameter?

this one is only in error case and not so costly so no worries for me

rmannibucau

Mappings diff can drop (autoformat?) then it looks good to me (early approval to avoid to need another pass just for style).
Very good work, thanks a lot for the effort!

jungm · 2023-04-19T12:05:36Z

@rmannibucau dropped mappings diff

jungm added 7 commits April 7, 2023 22:32

Implement JSON-B 3 polymorphism (WIP)

12a188a

Implement more JSON-B 3 polymorphism validations

ed12c76

JSON-B 3 polymorphism tests

751a504

Allow JsonbSubtype for type=annotated type

af650bd

removed johnzon-jsonb-extras in favor of json-b 3

594083a

MapperBuilder#setPolymorphismHandler javadoc

6257eb5

use JsonbRule in tests

75c5d15

jungm force-pushed the jsonb3-polymorphism branch from bbac29e to 75c5d15 Compare April 12, 2023 17:28

jungm added 7 commits April 14, 2023 21:21

Revert mapper API changes

1a98191

Allow custom Mappings to be used

b10a3b5

Implement JsonbMappings for polymorphism

c4fded5

create Mappings via Function instead of using reflection

8c9a9b7

use Meta.getAnnotation instead of Class.getAnnotation

e606955

restore old ClassMapping constructor for better backwards compatibility

78bcca2

fix codestyle (import order)

afcc6c0

rmannibucau requested changes Apr 17, 2023

View reviewed changes

reduce diff

bfd707d

reduce diff

bf546fc

jungm force-pushed the jsonb3-polymorphism branch from 9911f6e to bf546fc Compare April 18, 2023 14:40

jungm added 4 commits April 19, 2023 08:37

optimized jsonb polymorphism impl

1a64e46

Revert "removed johnzon-jsonb-extras in favor of json-b 3"

b052c44

This reverts commit 594083a.

update docs on jsonb-extras polymorphism

2e7c33e

restore MapperConfig constructor

ed67336

rmannibucau requested changes Apr 19, 2023

View reviewed changes

cache JsonbTypeInfos to avoid reflections

cce3ed4

rmannibucau approved these changes Apr 19, 2023

View reviewed changes

reduce diff

4214a4c

rmannibucau merged commit 9080e29 into apache:master Apr 20, 2023

jungm deleted the jsonb3-polymorphism branch April 20, 2023 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement JSON-B 3 Polymorphism #100

Implement JSON-B 3 Polymorphism #100

jungm commented Apr 12, 2023 •

edited

Loading

rmannibucau left a comment

rmannibucau Apr 17, 2023

rmannibucau Apr 17, 2023

rmannibucau Apr 17, 2023

rmannibucau Apr 17, 2023

jungm Apr 19, 2023

rmannibucau Apr 17, 2023

rmannibucau Apr 17, 2023

jungm Apr 18, 2023

rmannibucau Apr 18, 2023

jungm Apr 19, 2023

rmannibucau Apr 17, 2023

rmannibucau Apr 17, 2023

rmannibucau Apr 17, 2023

rmannibucau Apr 17, 2023

tandraschko commented Apr 18, 2023

rmannibucau left a comment

rmannibucau Apr 19, 2023

rmannibucau Apr 19, 2023

rmannibucau Apr 19, 2023

jungm Apr 19, 2023

rmannibucau Apr 19, 2023

rmannibucau left a comment

jungm commented Apr 19, 2023

Implement JSON-B 3 Polymorphism #100

Implement JSON-B 3 Polymorphism #100

Conversation

jungm commented Apr 12, 2023 • edited Loading

rmannibucau left a 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

tandraschko commented Apr 18, 2023

rmannibucau left a 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

rmannibucau left a comment

Choose a reason for hiding this comment

jungm commented Apr 19, 2023

jungm commented Apr 12, 2023 •

edited

Loading