-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #123: Add support for preservation of unknown properties #127
base: master
Are you sure you want to change the base?
Changes from 3 commits
fa5ab83
1617dbf
7028dc7
2e05077
5c6eea5
f334ab2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
|
||
import com.owlike.genson.*; | ||
import com.owlike.genson.annotation.HandleClassMetadata; | ||
import com.owlike.genson.convert.DefaultConverters.UntypedConverterFactory.UntypedConverter; | ||
import com.owlike.genson.ext.jsr353.JSR353Bundle; | ||
import com.owlike.genson.reflect.TypeUtil; | ||
import com.owlike.genson.stream.ObjectReader; | ||
import com.owlike.genson.stream.ObjectWriter; | ||
|
@@ -65,26 +65,24 @@ protected Converter<?> create(Type type, Genson genson, Converter<?> nextConvert | |
|
||
private final boolean classMetadataWithStaticType; | ||
private final Class<T> tClass; | ||
private final boolean skipMetadataSerialization; | ||
|
||
public ClassMetadataConverter(Class<T> tClass, Converter<T> delegate, boolean classMetadataWithStaticType) { | ||
super(delegate); | ||
this.tClass = tClass; | ||
this.classMetadataWithStaticType = classMetadataWithStaticType; | ||
skipMetadataSerialization = Wrapper.isOfType(delegate, UntypedConverter.class); | ||
} | ||
|
||
public void serialize(T obj, ObjectWriter writer, Context ctx) throws Exception { | ||
if (!skipMetadataSerialization && obj != null && | ||
(classMetadataWithStaticType || (!classMetadataWithStaticType && !tClass.equals(obj.getClass())))) { | ||
if (obj != null && !isJsonValueConverter(wrapped) && | ||
(classMetadataWithStaticType || !tClass.equals(obj.getClass()))) { | ||
writer.beginNextObjectMetadata() | ||
.writeMetadata("class", ctx.genson.aliasFor(obj.getClass())); | ||
} | ||
wrapped.serialize(obj, writer, ctx); | ||
} | ||
|
||
public T deserialize(ObjectReader reader, Context ctx) throws Exception { | ||
if (ValueType.OBJECT.equals(reader.getValueType())) { | ||
if (ValueType.OBJECT.equals(reader.getValueType()) && !isJsonValueConverter(wrapped)) { | ||
String className = reader.nextObjectMetadata().metadata("class"); | ||
if (className != null) { | ||
try { | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
The nice thing is that this wouldn't depend on jsr353 at all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point... I wrote this before I fully understood how Technically, I probably don't need this check anymore, as I switched to using 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So your goal is to ensure that when you do
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, that makes sense. I completely missed that That is much better thing to check than the converter chain, I'll make the change. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
package com.owlike.genson.reflect; | ||
|
||
import javax.json.JsonValue; | ||
import java.util.Map; | ||
|
||
/** | ||
* An interface that can be implemented by data classes | ||
* in order to support schema evolution. | ||
* <p> | ||
* This interface is used in combination with {@link EvolvableHandler} | ||
* in order to prevent data loss during serialization across different | ||
* versions of data classes. | ||
* | ||
* @author Aleksandar Seovic 2018.05.20 | ||
*/ | ||
interface Evolvable { | ||
/** | ||
* Add unknown property to this instance. | ||
* | ||
* @param propName property name | ||
* @param propValue property value | ||
*/ | ||
void addUnknownProperty(String propName, JsonValue propValue); | ||
|
||
/** | ||
* Return a map of unknown properties. | ||
* | ||
* @return a map of unknown properties | ||
*/ | ||
Map<String, JsonValue> unknownProperties(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
package com.owlike.genson.reflect; | ||
|
||
import com.owlike.genson.Context; | ||
import com.owlike.genson.Converter; | ||
import com.owlike.genson.ext.jsr353.JSR353Bundle; | ||
import com.owlike.genson.stream.ObjectReader; | ||
import com.owlike.genson.stream.ObjectWriter; | ||
|
||
import javax.json.JsonValue; | ||
|
||
import java.util.Map; | ||
import java.util.function.Consumer; | ||
|
||
/** | ||
* An implementation of an {@link UnknownPropertyHandler} that supports | ||
* evolution of data classes via {@link Evolvable} interface. | ||
* <p> | ||
* If the target object we are deserializing into is {@link Evolvable}, | ||
* this handler will add any unknown properties encountered during | ||
* deserialization into {@link Evolvable#unknownProperties()} map, | ||
* and will write them out along with all known properties during | ||
* subsequent serialization. | ||
* <p> | ||
* This prevents data loss when serializing and deserializing the same | ||
* JSON payload using different versions of Java data classes. | ||
* | ||
* @author Aleksandar Seovic 2018.05.20 | ||
*/ | ||
public class EvolvableHandler implements UnknownPropertyHandler { | ||
private static final Converter<JsonValue> CONVERTER = new JSR353Bundle.JsonValueConverter(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
@Override | ||
public <T> Consumer<T> readUnknownProperty(String propName, ObjectReader reader, Context ctx) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't it be Consumer<?> instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it would make it impossible to pass the bean we are deserializing into to
|
||
try { | ||
JsonValue propValue = CONVERTER.deserialize(reader, ctx); | ||
|
||
return objTarget -> { | ||
if (objTarget instanceof Evolvable) { | ||
((Evolvable) objTarget).addUnknownProperty(propName, propValue); | ||
} | ||
}; | ||
} catch (Exception e) { | ||
throw new RuntimeException(e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could throw a JsonBindingException instead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, good catch. |
||
} | ||
} | ||
|
||
@Override | ||
public <T> void writeUnknownProperties(T bean, ObjectWriter writer, Context ctx) { | ||
try { | ||
if (bean instanceof Evolvable) { | ||
Map<String, JsonValue> props = ((Evolvable) bean).unknownProperties(); | ||
if (props != null) { | ||
for (String propName : props.keySet()) { | ||
writer.writeName(propName); | ||
CONVERTER.serialize(props.get(propName), writer, ctx); | ||
} | ||
} | ||
} | ||
} catch (Exception e) { | ||
throw new RuntimeException(e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same, maybe use JsonBindingException There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup. |
||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New line according to github ;P |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
package com.owlike.genson.reflect; | ||
|
||
import com.owlike.genson.annotation.JsonIgnore; | ||
|
||
import javax.json.JsonValue; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
/** | ||
* Convenience base class for {@link Evolvable} data classes. | ||
* | ||
* @author Aleksandar Seovic 2018.05.20 | ||
*/ | ||
public abstract class EvolvableObject implements Evolvable { | ||
@JsonIgnore | ||
private Map<String, JsonValue> unknownProperties; | ||
|
||
@Override | ||
public void addUnknownProperty(String propName, JsonValue propValue) { | ||
if (unknownProperties == null) { | ||
unknownProperties = new HashMap<>(); | ||
} | ||
unknownProperties.put(propName, propValue); | ||
} | ||
|
||
@Override | ||
public Map<String, JsonValue> unknownProperties() { | ||
return unknownProperties; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth updating the user doc with this new c onfig option. Also some javadoc here would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that's the plan :-)