Duplicate field when serializing: bug or not a bug?

[Francis Galiegue]

Hello,

I have had a request for one of my projects to implement Jackson
serialization of my JSON Patch implementation; deserialization was
already working. All files in question are here:

https://github.com/fge/json-patch/tree/master/src/main/java/com/github/fge/jsonpatch

A JSON Patch is an array of individual operations; for instance:

[
{ "op": "add", "path": "/foo", "value": null },
{ "op", "remove", "path": "/bar" },
{ "op": "copy", "path": "/dst", "from": "/src"}
]

The JsonPatchOperation abstract class is the base class for all
operations; it reads:

----
@JsonTypeInfo(use = Id.NAME, include = As.PROPERTY, property = "op")

@JsonSubTypes({
@Type(name = "add", value = AddOperation.class),
@Type(name = "copy", value = CopyOperation.class),
@Type(name = "move", value = MoveOperation.class),
@Type(name = "remove", value = RemoveOperation.class),
@Type(name = "replace", value = ReplaceOperation.class),
@Type(name = "test", value = TestOperation.class)
})

@JsonIgnoreProperties(ignoreUnknown = true)
public abstract class JsonPatchOperation
{
@JsonSerialize
protected final String op;

@JsonSerialize(using = JsonPointerSerializer.class)
protected final JsonPointer path;

protected JsonPatchOperation(final String op, final JsonPointer path)
{
this.op = op;
this.path = path;
}
----

Here there is one problem already: the needed @JsonSerialize
annotation; more on this later.

The JsonPatch class is, essentially, a List<JsonPatchOperation>:

----
public final class JsonPatch
{
private final List<JsonPatchOperation> operations;

@JsonValue
private List<JsonPatchOperation> getOperations()
{
return operations;
}

@JsonCreator
@VisibleForTesting
JsonPatch(final List<JsonPatchOperation> operations)
{
this.operations = ImmutableList.copyOf(operations);
}
----

Here is the second problem: I need the @JsonValue annotation...

The test program (link:
https://github.com/fge/json-patch/blob/master/src/test/java/com/github/fge/jsonpatch/Issue6.java)
shows strange behaviour for deserializing individual operations:

----
Deserialized patch: [op: add; path: "/foo/bar"; value: "baz"]
Serialized Json: [{"op":"add","path":"/foo/bar","value":"baz"}]
Deserialized op: op: add; path: "/foo/bar"; value: "baz"
Serialized Json:
{"op":"add","op":"add","path":"/foo/bar","value":"baz"} <--- PROBLEM
----

The "op" field is doubled. If I remove the @JsonSerialize annotation
from JsonPatchOperation then the output is as follows:

----
Deserialized patch: [op: add; path: "/foo/bar"; value: "baz"]
Serialized Json: [{"path":"/foo/bar","value":"baz"}] <--- PROBLEM
Deserialized op: op: add; path: "/foo/bar"; value: "baz"
Serialized Json: {"op":"add","path":"/foo/bar","value":"baz"}
----

Now the "op" field is no more doubled when deserializing an individual
operation... But it does not appear when serializing a full patch! And
I need it, of course... In fact, prior to having been asked to support
serialization, the JsonPatchOperation did not have the "op" field at
all, I had to add it.

Finally, I am dissatisfied with the fact that I had to add a (private)
getter for "operations" in JsonPatch; is there a cleaner way?

Sorry for the long read,
--
Francis Galiegue, fgal...@gmail.com
JSON Schema in Java: http://json-schema-validator.herokuapp.com

[Francis Galiegue]

Uhm, I forgot to mention: this is Jackson 2.2.3.

[Nate Bauernfeind]

Ok. So first off, when using the sub-type stuff, you typically don't want to have the @JsonDeserialize annotation on the 'op' variable. The sub-type code path adds and removes that for you. The rest of my response assumes you've removed this.

The problem is how it appears @JsonValue is behaving. For example use @JsonSerialize on the getOperations() method. You will notice your code will now return this:

Serialized patch Json: {"operations":[{"op":"add","value":"baz"}]}

Now change getOperations method to this signature: 

private List<?> getOperations() 

Now you will notice that it loses type information:

Serialized patch Json: {"operations":[{"value":"baz"}]}

This is because at serialization time the sub-type mapper only seems to kick in if it can be 100% sure that the type applies on the entire collection. I'm not sure why it doesn't see if it applies for each instance one at a time... but it doesn't.

So anyways, that's your problem. If you're using the mapper directly you can tell it what type you're serializing directly:

        String jsonPatch = "[{\"op\": \"add\", \"value\": \"baz\"}]";

        JsonPatch patch = mapper.readValue(jsonPatch, JsonPatch.class);

        String jsonSer = mapper.writeValueAsString(patch.getOperations());

        System.out.printf("Serialized regular: %s\n", jsonSer);

        jsonSer = mapper.writerWithType(new TypeReference<List<JsonPatchOperation>>() {}).writeValueAsString(patch.getOperations());

        System.out.printf("Serialized writerWithType: %s\n", jsonSer);

Has this result:

Serialized regular: [{"value":"baz"}]

Serialized writerWithType: [{"op":"add","value":"baz"}]

So... I would recommend either make your json patch class a first class object (literally a json object instead of an array of patches). This may have other future benefits like being able to add versioning information that can tag along with your patches. Or the time at which it was created/applied.

Or, if you're exposing a list of patches through a resource method in say Jersey... then instead of returning a JsonPath, just explicitly mark the method as returning a List<JsonPathOperation> and call .getOperations() manually for your intended effect.

I sure hope that helps,

Nate

P.S. You could look further into why List<?> won't include the sub-type information... IMHO I think that any instance of a class that is a sub-class of some type that has the proper sub-type annotations should have the proper mutations during serialization... but that's my opinion. I think there was some reason that made it impossible to do; but I don't remember the answer to that question.

--
You received this message because you are subscribed to the Google Groups "jackson-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jackson-dev...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Francis Galiegue]

Well, I have forgotten an obvious solution: make JsonPatch and
JsonPatchOperation implement JsonSerializable! It is MUCH easier this
way.

The rest of my annotations were used for deserialization, and this
works very well, so no problem there. Now, the code is much more
simple. In JSON Patch:

----
@Override
public void serialize(final JsonGenerator jgen,
final SerializerProvider provider)
throws IOException
{
jgen.writeStartArray();
for (final JsonPatchOperation op: operations)
op.serialize(jgen, provider);
jgen.writeEndArray();
}

@Override
public void serializeWithType(final JsonGenerator jgen,
final SerializerProvider provider, final TypeSerializer typeSer)
throws IOException
{
serialize(jgen, provider);
}
---

Not sure about the serializeWithType implementation, but anyway there
is only one way to serialize a JSON Patch (class is final anyway).

But the end result is there: everything works! I can {de,}serialize
using an ObjectMapper without the need to configure it further.

[Tatu Saloranta]

On Sun, Feb 16, 2014 at 2:09 PM, Francis Galiegue <fgal...@gmail.com> wrote:

Well, I have forgotten an obvious solution: make JsonPatch and
JsonPatchOperation implement JsonSerializable! It is MUCH easier this
way.

The rest of my annotations were used for deserialization, and this
works very well, so no problem there. Now, the code is much more
simple. In JSON Patch:

----
    @Override
    public void serialize(final JsonGenerator jgen,
        final SerializerProvider provider)
        throws IOException
    {
        jgen.writeStartArray();
        for (final JsonPatchOperation op: operations)
            op.serialize(jgen, provider);
        jgen.writeEndArray();
    }

    @Override
    public void serializeWithType(final JsonGenerator jgen,
        final SerializerProvider provider, final TypeSerializer typeSer)
        throws IOException
    {
        serialize(jgen, provider);
    }
---

Not sure about the serializeWithType implementation, but anyway there
is only one way to serialize a JSON Patch (class is final anyway).

serializeWithType() is (only) called when polymorphic type needs to be included.
If this never occurs, it does not matter. If it is needed, the usual implementation calls typeSer() to include type id; and then serializes normally with following exception(s):

- Things serialized as JSON Objects, leave out START_OBJECT/END_OBJECT

- Things serialized as JSON Arrays, leave out START_ARRAY/END_ARRAY

and this is because TypeSerializer will (have to) include those, depending on what kind of inclusion mechanism (as property, as wrapper array/object, external property) is being used.

 

But the end result is there: everything works! I can {de,}serialize
using an ObjectMapper without the need to configure it further.

Yay!

One last thing about @JsonValue: I do not remember 100% for sure, but I think it is currently not applicable for fields. If so, adding support would be an excellent improvement idea. I don't think there is any real reason not to support that.

-+ Tatu +-

[Nate Bauernfeind]

Tatu,

Since we're on the topic, how come serializeWithType only seems to come into play for objects in a collection if and only if the collections generic type is well known? Shouldn't it be on a per instance basis?

And, even then, why would @JsonValue lose the generic type information that is otherwise persisted with the method signature?

Are these just bugs/incomplete implementations?

Nate

[Tatu Saloranta]

On Mon, Feb 17, 2014 at 10:39 PM, Nate Bauernfeind <nate.bau...@gmail.com> wrote:

Tatu,

Since we're on the topic, how come serializeWithType only seems to come into play for objects in a collection if and only if the collections generic type is well known? Shouldn't it be on a per instance basis?

No. This is a fundamental design decision that is baked deeply in handling, and change would be difficult to do, as well as backwards incompatible.

Type id handling relies completely on static types; whereas content serialization (POJO properties) defaults to dynamic handling. This is one area where I wish I had chosen different defaults, and actually chosen static handling for properties as well; and require explicit override to do otherwise.

If determination was made on instance by instance basis, lookups for type id serializer would need to be on value-by-value basis, possibly using different inclusion/id-type mechanisms, and different sub-type hierarchies; some values also would not have known type id inclusion mechanism at all.
In fact, to know if type was to be included could not be determined from Collection at all, but all values of any and all Collections would need to be introspected for possible type id inclusion.

On deserialization, handling would also need to consider possibility of polymorphic type for any and all values, with similar complexity and overhead.

> And, even then, why would @JsonValue lose the generic type information that is
> otherwise persisted with the method signature?

It could either be due to bugs (over time we have fixed numerous probls with handling), or incompatibility between features. The tricky part, and root of most bugs is that of difference between actual type (one annotated with @JsonValue) and proxy type (return value); and it is easy to confuse the two. Especially because serializer is needed for latter, but type handler for former.

Are these just bugs/incomplete implementations?

For @JsonValue, that is an open question. I think there's a good chance it's still just implementation, and could be fixed.

So I am not aware of fundamental reason why this could not be made to work, but I can't rule that out either (very reassuring eh? :) ).

-+ Tatu +-