Fix #123: Add support for preservation of unknown properties #127

aseovic · 2018-05-21T08:01:35Z

Here is a new PR for this issue, based on earlier discussion.

A few notes:

I did not change how failOnMissingProperty works in the end, because the exception thrown uses fields from the BeanDescriptor class, and I didn't feel it was warranted to make those available to the UnknownPropertyHandler implementations.
Overall, I like the move of the read/write logic into the UnknownPropertyHandler, as it gives users more control over what they can do with it. However, it did make it a bit tricky to support creators, which effectively do a two-pass deserialization, and don't have a bean to deserialize into on first pass.

I've made it work by returning a Consumer<T> from UPH.onUnknownProperty which captures unknown property name and value and applies them to a target bean during second deserialization stage. It works, but I'm not thrilled with the solution, so ideas and suggestions are welcome.

I've added support for unknown/missing classes using option 2 from earlier discussion, javax.json.JsonValue as a way to read unknown properties as opaque JSON values.

It works as expected, but in order to get there I had to make a number of changes within various standard converters, and implement slightly different fix for MetadataFeatureTest.testClassMetadataShouldBeSerializedOnceWhenUsingUntypedConverter, so if you can review those changes and make sure I didn't break some assumptions that are not captured by the existing tests (which are all passing after the changes), I'd appreciate it.

…hed to using JsonValueConverter directly

aseovic · 2018-05-21T21:37:26Z

Made a few changes to clean up reading/consumption of unknown properties and switched to direct usage of JsonValueConverter. I believe we are at the point where this may be ready for merge.

EugenCepoi · 2018-05-30T05:18:31Z

genson/src/main/java/com/owlike/genson/reflect/EvolvableHandler.java

+ * @author Aleksandar Seovic  2018.05.20
+ */
+public class EvolvableHandler implements UnknownPropertyHandler {
+    private static final Converter<JsonValue> CONVERTER = new JSR353Bundle.JsonValueConverter();


The only problem is that this assumes we are in Java EE world and I'd like Genson to work with Java SE :/

Do you see any major problem to just deserialize to untyped Map/List/String/Primitives (outside of an unfriendly untyped api)?

One easy solution would be to move this to a Java EE bundle or something like that. Though everyone else that is not using Java EE won't benefit from it.

There is nothing really that makes this Java EE only. JSON-P spec may be part of Java EE, but both the API and the implementations (in this case Genson itself, which is the only thing I use) can be used in Java SE as well (just like JAX-RS and many other "Java EE" specs).

We can't really deserialize into a plain untyped Map. I use the fact that we are deserializing into JsonValue to turn off handling of @class attributes, in order to read them like any other JSON property.

I believe that makes sense: once you cross JSON-P barrier, everything should be JSON-P compliant, and we shouldn't construct non-JSON-P classes anywhere in the graph, regardless of @class attribute presence.

The JSR353 part is currently provided as a bundle, which means that Genson doesn't have a hard requirement on it in order to work. With this change it means that Genson will need to make this dependency mandatory. I've been trying to make sure Genson is a single jar with no external dependency on other libs (for the core Genson to work). I'm not sure I want this to change...

What is your opinion on that?

Why can't we deserialize into a plain untyped Map? Writing a recursive converter that reads a stream to untyped DOM structures (map/array/primitives) should work. You can then use it directly (without going through genson.deserialize(...)) which would ensure that you don't risk of picking class metadata etc.

Honestly, I don't think not using JSR353 directly would buy us anything -- we'd basically have to replicate the same functionality, for no good reason.

I hear you on the dependency front, but I also think the line is a bit blurry when it comes to dependency on standards, such as JSR353 (JSON-P) and JSR367 (JSON-B, which unfortunately has too many issues in v1 to be usable in real world). I do believe it makes sense to support standards as much as possible, and personally don't see an issue with a hard dependency on the standard API jars.

I'll need to think a bit more about this. I don't like the idea of relying on the JSR in core Genson, but maybe this is not justified.

A tradeoff would be to make a Java EE specific module that comes with goodies related to the Java EE echosystem. In there we could have this default impl that works with JsonValues and the different extensions for Jaxrs/json-p/json-b.

How does it sound?

Personally, I feel you are overthinking the whole Java SE vs Java EE thing ;-)

Many Java EE specs and their implementations are perfectly usable from Java SE, and there is no reason we shouldn't treat them as such. The dependencies we are introducing are tiny (only the standard API JARs, really), and could easily become part of Java SE at some point if enough people care about them (chances are they won't, but the reasons for that are more complex than I care to go into and mostly political...)

The real question we need to ask is not which umbrella the spec falls under, SE or EE, but whether it's usable without the Java EE app server or not. The specs like EJB are not, and I would avoid them like a plague (although, I'm not sure what we would even do with them), or definitely keep as separate modules (and separate JARs). However, the specs like JAXB, JSON-P and JSON-B are usable in any Java environment, and are something that, in my opinion, we can use and depend on internally. More importantly, I think we should use them, as they allow us to adopt and support those standards which is a good thing.

Where it gets blurry is with things like JAX-RS. In that particular case, I would treat it as a separate, optional module, not because it is not usable from Java SE (it certainly is, I use it that way all the time), but because we are implementing its SPI in order to allow it to use Genson for serialization, not the other way around. In other words, JAX-RS extension is not a true Genson bundle -- it is effectively a JAX-RS "bundle/extension" that allows it to integrate with Genson.

To make it short I think Genson core shouldn't depend on anything outside of what is already part of the jdk. Be it JAXB/JSONP/JSONB and cie. they only make sense in the Java (EE) world, but for ex. someone that is using the Scala extension has no interest in pulling all that stuff. Same goes for someone that uses it on Android.
I'd like Genson core to remain free of dependencies in order to work properly.

EugenCepoi · 2018-05-30T21:19:37Z

genson/src/main/java/com/owlike/genson/convert/ClassMetadataConverter.java

@@ -101,4 +99,8 @@ public T deserialize(ObjectReader reader, Context ctx) throws Exception {
    }
    return wrapped.deserialize(reader, ctx);
  }
+
+  private boolean isJsonValueConverter(Converter<T> converter) {
+    return converter instanceof JSR353Bundle.JsonValueConverter;


This would work only if the class metadata converter wraps the actual final converter, but this is not necessarily always true.

Maybe the right thing would be to:

check if the static type is a super type of the class resolved from metadata, if so then do use the resolved type

otherwise don't

eventually have an option to throw an exception if the type hierarchy mismatches?

The nice thing is that this wouldn't depend on jsr353 at all.

Good point... I wrote this before I fully understood how ChainedFactory works...

Technically, I probably don't need this check anymore, as I switched to using JsonValueConverter directly within EvolvableHandler, but I decided to leave it in because I do think that it should apply in a more general case: if I have JsonValue-typed attribute within a POJO, for example, whatever is read into it should be treated as an opaque value, without any annotation processing.

That said, I don't quite understand what you suggested above, and how type hierarchy would help us here. Can you please clarify? Again, the goal is to disable special @class annotation processing once the JSON-P barrier is crossed, and to treat it just like any other attribute.

So your goal is to ensure that when you do genson.deserialize("", JsonValue.class) and class metadata is enabled, we don't deserialize to the actual @Class value, right?
The tClass attribute of ClassMetadataConverter will be in this case JsonValue.

if (tClass.isAssignableFrom(classFromMetadata)) { genson.deserialize(..., classFromMetadata); } else { wrapped.deserialize(reader, ctx); }

So if we were doing something like the above, we would ensure that we respect the type hierarchy that the user did statically ask for (in this case JsonValue). Of course in some cases we can argue that if the type doesn't match we should throw an error, this could be the configurable part.

But anyway as you directly use the converter this is not needed.

Right, that makes sense. I completely missed that tClass will be JsonValue or one of its subclasses.

That is much better thing to check than the converter chain, I'll make the change.

EugenCepoi · 2018-05-30T21:23:55Z

genson/src/main/java/com/owlike/genson/reflect/EvolvableHandler.java

+    private static final Converter<JsonValue> CONVERTER = new JSR353Bundle.JsonValueConverter();
+
+    @Override
+    public <T> Consumer<T> readUnknownProperty(String propName, ObjectReader reader, Context ctx) {


Shouldn't it be Consumer<?> instead?

No, it would make it impossible to pass the bean we are deserializing into to Consumer.accept method within BeanDescriptor:

[INFO] ------------------------------------------------------------- [ERROR] COMPILATION ERROR : [INFO] ------------------------------------------------------------- [ERROR] com/owlike/genson/reflect/BeanDescriptor.java:[129,82] incompatible types: T cannot be converted to capture#1 of ? [ERROR] com/owlike/genson/reflect/BeanDescriptor.java:[193,59] incompatible types: T cannot be converted to capture#2 of ? [INFO] 2 errors

EugenCepoi · 2018-05-30T21:24:41Z

genson/src/main/java/com/owlike/genson/reflect/EvolvableHandler.java

+                }
+            };
+        } catch (Exception e) {
+            throw new RuntimeException(e);


You could throw a JsonBindingException instead

Yup, good catch.

EugenCepoi · 2018-05-30T21:25:59Z

genson/src/main/java/com/owlike/genson/reflect/EvolvableHandler.java

+                }
+            }
+        } catch (Exception e) {
+            throw new RuntimeException(e);


Same, maybe use JsonBindingException

EugenCepoi · 2018-05-30T21:47:38Z

genson/src/main/java/com/owlike/genson/stream/JsonWriter.java

@@ -62,18 +62,7 @@
  private final int _bufferSize = _buffer.length;
  private int _len = 0;

-  List<MetadataPair> _metadata = new ArrayList<MetadataPair>();


I've used initially a list for speed. Is there a particular reason for replacing it by a map when we don't need lookups by name?

Yes -- that's the "slightly different fix for MetadataFeatureTest.testClassMetadataShouldBeSerializedOnceWhenUsingUntypedConverter" issue I mentioned in the PR description ;-)

Basically, the issue was that the metadata list wasn't cleared in all scenarios (forgot the details, but I remember looking at the issue in debugger), which is why we'd end up with a duplicate @class annotation in the list and subsequently in the JSON document.

The original fix for that problem was this flag in ClassMetadataConverter

skipMetadataSerialization = Wrapper.isOfType(delegate, UntypedConverter.class);

but that didn't play nice with some of the evolvability tests, so I decided to fix the root problem and make it impossible to add duplicate metadata in a first place. Basically, the Map is used to ensure key uniqueness, not for lookups. I honestly don't think it will have any meaningful impact on performance either way -- it's just a one tiny Map, typically containing a single or no entries.

I did read your initial PR description but didn't read at the same time the code, once I started reading the code I forgot about the description :)

The root problem here is that we will have two ClassMetadataConverters for untyped objects. The first one is the one resolved forObject.class, the next one would kick in once the UntypedConverter tries to serialize or deserialize (as it will go again through genson.provideConverter which will in turn resolve a full chain of converters for the given type, including a ClassMetadataConverter).

I think using the map here kind of hides the actual issue. In this PR I'm trying to address the different issues we mentioned.

Sure, addressing the root issue and eliminating the need to do the work twice is certainly a better option than hiding it :-)

I still think using List vs Map won't help much performance-wise, considering what it's used for, but am happy to revert the change if that's what you prefer, once #133 is in.

EugenCepoi · 2018-05-30T21:49:00Z

genson/src/main/java/com/owlike/genson/Genson.java

@@ -609,6 +615,10 @@ public RuntimePropertyFilter runtimePropertyFilter() {
    return runtimePropertyFilter;
  }

+  public UnknownPropertyHandler unknownPropertyHandler() {


Worth updating the user doc with this new c onfig option. Also some javadoc here would be nice.

Agree on both points, but I was waiting for us to agree on what it is that should be documented ;-)

I think we are getting closer, I can add some docs.

I'd document the simpler public API. So basically how to register an UnknownPropertyHandler and what it is. What are the existing defaults and how to use them. I wouldn't go in the details on how to implement one.

Yup, that's the plan :-)

aseovic · 2018-05-31T08:11:07Z

One more thing: I think it makes sense to move Evolvable (interface) and EvolvableObject (base class) from com.owlike.genson.reflect to top-level com.owlike.genson package, as those are the classes people will use frequently.

What do you think?

EugenCepoi · 2018-06-01T19:40:34Z

genson/src/main/java/com/owlike/genson/reflect/EvolvableHandler.java

+ * @author Aleksandar Seovic  2018.05.20
+ */
+public class EvolvableHandler implements UnknownPropertyHandler {
+    private static final Converter<JsonValue> CONVERTER = new JSR353Bundle.JsonValueConverter();


I'll need to think a bit more about this. I don't like the idea of relying on the JSR in core Genson, but maybe this is not justified.

A tradeoff would be to make a Java EE specific module that comes with goodies related to the Java EE echosystem. In there we could have this default impl that works with JsonValues and the different extensions for Jaxrs/json-p/json-b.

How does it sound?

EugenCepoi · 2018-06-01T19:41:04Z

genson/src/main/java/com/owlike/genson/reflect/EvolvableHandler.java

+            throw new JsonBindingException(e);
+        }
+    }
+}


New line according to github ;P

EugenCepoi · 2018-06-01T22:11:50Z

genson/src/main/java/com/owlike/genson/stream/JsonWriter.java

@@ -62,18 +62,7 @@
  private final int _bufferSize = _buffer.length;
  private int _len = 0;

-  List<MetadataPair> _metadata = new ArrayList<MetadataPair>();


I did read your initial PR description but didn't read at the same time the code, once I started reading the code I forgot about the description :)

The root problem here is that we will have two ClassMetadataConverters for untyped objects. The first one is the one resolved forObject.class, the next one would kick in once the UntypedConverter tries to serialize or deserialize (as it will go again through genson.provideConverter which will in turn resolve a full chain of converters for the given type, including a ClassMetadataConverter).

I think using the map here kind of hides the actual issue. In this PR I'm trying to address the different issues we mentioned.

EugenCepoi · 2018-06-01T22:17:20Z

@aseovic in regards to moving Evolvable(Object) to com.owlike.genson.
I have no objection in moving it under the same package, but I think it might be nice actually to have a new module that groups all the Java EE specific APIs/integrations, and move in there the Evolvable(Object).

BTW WDYT of avoiding to leave global comments that would trigger a discussion in the PR but rather place them on code, that way we have context of the previous messages? I wish github was doing better on that :/

aseovic · 2018-06-03T05:31:40Z

As I said in another comment, I don't think having a Java EE module would buy us anything, and I definitely don't think Evolvable and related classes are only applicable and should only work in Java EE environment.

As for discussion, we seem to be doing all right ;-), but I hear you. I'll try to associate comments with the code where appropriate.

EugenCepoi · 2018-06-08T00:37:50Z

I've been thinking a bit more about the jsonp related dependency of the default impl of UnknwonPropertyHandler. As we are planning to split genson into several modules, we will likely have a jsonp module. I think this is where the default impl that uses JsonValue should go. In the meanwhile it could be moved to the jsonp extension package.
I mostly want this because I want to keep Genson core with 0 dependencies and ensure that what it does makes sense cross JVM languages. If in the future we find that it would be definitely nicer to have jsonp as part of Genson core we could reconsider things.

Let me know if you have a strong objection to this.

On another note, I hope I didn't sound too abrupt in some of my last comments. I've been quite busy with every day work and lots of context switching, which made me less thoughtful :)

aseovic added 2 commits May 20, 2018 03:08

Fix for owlike#123: Initial refactoring based on PR feedback

fa5ab83

Fix for owlike#123: Added support for missing classes via JsonValue

1617dbf

aseovic mentioned this pull request May 21, 2018

Fix for #123 - Add support for preservation of unknown properties #124

Closed

Fix for owlike#123: Refactored unknown property consumption and switc…

7028dc7

…hed to using JsonValueConverter directly

EugenCepoi reviewed May 30, 2018

View reviewed changes

Switched to JsonBindingException

2e05077

EugenCepoi reviewed Jun 1, 2018

View reviewed changes

EugenCepoi mentioned this pull request Jun 6, 2018

Better control for class metadata ser/de and JsonP extension #133

Merged

aseovic added 2 commits June 6, 2018 14:20

Made Evolvable public

5c6eea5

Made Evolvable public

f334ab2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #123: Add support for preservation of unknown properties #127

Fix #123: Add support for preservation of unknown properties #127

aseovic commented May 21, 2018

aseovic commented May 21, 2018

EugenCepoi May 30, 2018 •

edited

Loading

aseovic May 30, 2018

EugenCepoi May 30, 2018

aseovic May 31, 2018

EugenCepoi Jun 1, 2018

aseovic Jun 3, 2018 •

edited

Loading

EugenCepoi Jun 6, 2018

EugenCepoi May 30, 2018 •

edited

Loading

aseovic May 31, 2018

EugenCepoi Jun 1, 2018

aseovic Jun 3, 2018

EugenCepoi May 30, 2018

aseovic May 31, 2018 •

edited

Loading

EugenCepoi May 30, 2018

aseovic May 31, 2018

EugenCepoi May 30, 2018

aseovic May 31, 2018

EugenCepoi May 30, 2018

aseovic May 31, 2018

EugenCepoi Jun 1, 2018

aseovic Jun 3, 2018

EugenCepoi May 30, 2018

aseovic May 31, 2018

EugenCepoi Jun 1, 2018

aseovic Jun 3, 2018

aseovic commented May 31, 2018

EugenCepoi Jun 1, 2018

EugenCepoi Jun 1, 2018

EugenCepoi Jun 1, 2018

EugenCepoi commented Jun 1, 2018

aseovic commented Jun 3, 2018

EugenCepoi commented Jun 8, 2018

Fix #123: Add support for preservation of unknown properties #127

Are you sure you want to change the base?

Fix #123: Add support for preservation of unknown properties #127

Conversation

aseovic commented May 21, 2018

aseovic commented May 21, 2018

EugenCepoi May 30, 2018 • edited Loading

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

aseovic Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EugenCepoi May 30, 2018 • edited Loading

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

aseovic May 31, 2018 • edited Loading

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

aseovic commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EugenCepoi commented Jun 1, 2018

aseovic commented Jun 3, 2018

EugenCepoi commented Jun 8, 2018

EugenCepoi May 30, 2018 •

edited

Loading

aseovic Jun 3, 2018 •

edited

Loading

EugenCepoi May 30, 2018 •

edited

Loading

aseovic May 31, 2018 •

edited

Loading