Cannot merge polymorphic objects

[matthew-altman]

Referring to #2336 because there was a similar issue with polymorphic maps that was addressed there, and at the end of that issue it mentions:

If attempts to provide some form of risky merging for polymorphic values are still desired, a new issue should be created (with reference to this issue for back story).

We are on version 2.10.0
I have some classes defined similarly to:

public class MyRequest {
  @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = As.PROPERTY, property = "type")
  @JsonSubTypes({
      @Type(value = ThingyAA.class, name = "ThingyAA"),
      @Type(value = ThingyBB.class, name = "ThingyBB")
  })
  public BaseThingy<?> thingy;

  @JacksonConstructor
  public MyRequest() {
  }

  public MyRequest(BaseThingy<?> thingy) {
    this.thingy = thingy;
  }
}

@JsonIgnoreProperties(value = "type", allowGetters = true, allowSetters = false)
public abstract class BaseThingy<D extends BaseThingyConfig> {
  public Map<Integer, D> config = new HashMap<>();
  public String name;
}

public abstract class BaseThingyConfig {
  public final Map<String, Object> data = new HashMap<>();
}

public class ThingyAAConfig extends BaseThingyConfig {
  @InternalJSONColumn
  public String foo;
}

public class ThingyBBConfig extends BaseThingyConfig {
  @InternalJSONColumn
  public String bar;
}

public class ThingyAA extends BaseThingy<ThingyAAConfig> {
  @InternalJSONColumn
  public String stuff;
}

public class ThingyBB extends BaseThingy<ThingyBBConfig> {
  @InternalJSONColumn
  public String otherStuff;
}

The problem we're seeing is the incoming request completely overwrites the existing object instead of merging.

If we force a merge using @JsonMerge then an exception is thrown:
Cannot merge polymorphic property 'thingy'

There are a few ways we're thinking of trying to get around this. One is to create a custom deserializer. And another is to manually merge the json via a deep node merge before passing to the reader similar to:

ObjectReader jsonNodeReader = objectMapper.readerFor(JsonNode.class);
JsonNode existingNode = jsonNodeReader.readValue(objectMapper.writeValueAsBytes(currentValue));
JsonNode incomingNode = jsonNodeReader.readValue(request.getInputStream());
JsonNode merged = merge(existingNode, incomingNode);
ObjectReader patchReader = objectMapper.readerForUpdating(currentValue);
patchReader.readValue(merged);

public static JsonNode merge(JsonNode mainNode, JsonNode updateNode) {
    Iterator<String> fieldNames = updateNode.fieldNames();

    while (fieldNames.hasNext()) {
      String updatedFieldName = fieldNames.next();
      JsonNode valueToBeUpdated = mainNode.get(updatedFieldName);
      JsonNode updatedValue = updateNode.get(updatedFieldName);

      // If the node is an @ArrayNode
      if (valueToBeUpdated != null && valueToBeUpdated.isArray() &&
          updatedValue.isArray()) {
        // running a loop for all elements of the updated ArrayNode
        for (int i = 0; i < updatedValue.size(); i++) {
          JsonNode updatedChildNode = updatedValue.get(i);
          // Create a new Node in the node that should be updated, if there was no corresponding node in it
          // Use-case - where the updateNode will have a new element in its Array
          if (valueToBeUpdated.size() <= i) {
            ((ArrayNode) valueToBeUpdated).add(updatedChildNode);
          }
          // getting reference for the node to be updated
          JsonNode childNodeToBeUpdated = valueToBeUpdated.get(i);
          merge(childNodeToBeUpdated, updatedChildNode);
        }
        // if the Node is an @ObjectNode
      } else if (valueToBeUpdated != null && valueToBeUpdated.isObject()) {
        merge(valueToBeUpdated, updatedValue);
      } else {
        if (mainNode instanceof ObjectNode) {
          ((ObjectNode) mainNode).replace(updatedFieldName, updatedValue);
        }
      }
    }
    return mainNode;
  }

Can some type of deep node merge occur in Jackson for this polymorphic scenario to alleviate us having to maintain this json functionality ourselves?

[matthew-altman]

The workaround we ended up going with was writing a custom Deserializer that extends StdDeserializer reads the request into a JsonNode, get the Thingy from the request, and then use a readForUpdating reader to deserialize that normally and put the deserialized object back on the request.
Like so:

public class ThingyDeserializer extends StdDeserializer<MyRequest> {
  public ThingyDeserializer() {
    super(MyRequest.class);
  }

  @Override
  public MyRequest deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
    return p.readValueAs(MyRequest.class);
  }

  @Override
  public MyRequest deserialize(JsonParser p, DeserializationContext ctxt, MyRequest req) throws IOException {
    JsonNode incomingNode = p.getCodec().readTree(p);
    JsonNode thingyNode = incomingNode.get("thingy");
    BaseThingy thingy = req.thingy;
    if (idp == null) {
      idp = getThingy(thingyNode);
    }

    if (idp != null) {
      ObjectReader reader = ((ObjectMapper) p.getCodec()).readerForUpdating(thingy);
      reader.readValue(thingyNode);
    }

    req.thingy = thingy;
    return req;
  }

  private BaseThingy getThingy(JsonNode thingyNode) {
    if (thingyNode == null) {
      return null;
    }

    String type = thingyNode.get("type").asText();

    switch (type) {
      case "ThingyAA":
        return new ThingyAA();
      case "ThingyBB":
        return new ThingyBB();
    }
  }
}

[jameswangz]

Do we have any plan to support merging polymorphic objects? From my point of view, a polymorphic object which needs to be merged should not change its type, if this is the precondition then the merge process should be easy, just read the original object and set its fields.

We have lots of polymorphic models in our project and heavily depend on this feature, it would be nice to support this in the new version.

[cowtowncoder]

Yes, it often appears things are easy when you are not familiar with the details. This may be one of these cases.
A fundamental problem is that the polymorphic type information must be present in case of data merging and figuring out how to handle difference between metadata (type info) and data (properties) is non trivial.
This is partly since merging is just a slightly special case of regular deserialization, and not separate of its own (if it was things would be easier here although would also require a lot of duplicated code).

I'd be happy to help anyone who can implement it, but at this point do not have time to try to tackle the problem myself.

[jameswangz]

Thanks for providing the detail information, I'll try if I can track the problem and send a pull request later.

[cowtowncoder]

Good luck!

[jameswangz]

Hi Tatu,

I have added the merging polymorphic property logic in the code and tested, it works as expected now, the limitation is we can only merge the same sub type property, and I think it should be the case for most scenarios, changing the type of a polymorphic object doesn't make sense and it's not accepted by JPA as well, below is my implementation, please advice if there are any problems:

  SettableBeanProperty.deserializeWith(JsonParser p, DeserializationContext ctxt,
                                    Object toUpdate)

        // 20-Oct-2016, tatu: Also tricky -- for now, report an error
        if (_valueTypeDeserializer != null) {
            // 25-Oct-2021 Added by James to support merging polymorphic property
            // https://github.com/FasterXML/jackson-databind/issues/2541
            // Please note we only support merging same type polymorphic property for now,
            // merging different type is hard and usually doesn't make sense
            JavaType subType = ctxt.getTypeFactory().constructType(toUpdate.getClass());
            JsonDeserializer<Object> subTypeValueDeserializer = ctxt.findContextualValueDeserializer(subType, this);
            return subTypeValueDeserializer.deserialize(p, ctxt, toUpdate);
        }

[jameswangz]

Any suggestions on the modification? Can I send a pull request for this?

[cowtowncoder]

Would this actually work, for all kinds of polymorphic type embedding? Since this ignores _valueTypeDeserializer (which handles type id use/transformations), wouldn't it fail for many cases?

You can definitely submit a PR, with tests to show expected working usage. I hope to have to look into it, although right now have bit of a time crunch.
But maybe we can find others to have a look at proposed changes as well; as well as find likely edge cases.

[jameswangz]

It only supports merging the same type polymorphic property, say if we have an old value which type is A, we can't change it to B in the merging process, like you said, changing the polymorphic type is very hard, and it usually doesn't make sense.

I'll try to add unit tests to show the usage and submit a PR later, do you know who else can look at this?

[cowtowncoder]

My concern is beyond different types -- yes, type cannot change, but more important, Type Id information should be incoming (json) content so who should handle that and how?

If you submit PR, I can have a look & you can ask others on the mailing list. There are a few active collaborators but they mostly work on specific modules.

[jameswangz]

Sorry for the late response, I have submitted a PR which includes the test cases, please review.

[jameswangz]

Hi, Tatu, can you have a look at the pull request?

[cowtowncoder]

Yes, I hope to look into this in near future. Right now I am severely overloaded unfortunately.

Thank you for the PR!

[cowtowncoder]

Apologies again for slow progress here: I am finally back to reviewing this.

I realized that my concerns about cases where deserialization cannot succeed (and the nasty part of having to ignore unknown properties) can be easiest alleviated with just adding a new MapperFeature; something like ALLOW_MERGE_FOR_POLYMORPHIC (and adding a reference from exception message if not enabled).
The reason here is that I want this potentially only partially working feature to be behind explicit enable -- so developers are more likely to maybe check out Javadocs for feature to learn about limitations.
And perhaps in future there could be a way to only automatically ignore type identifier property (if that is the inclusion mechanism) as well.

So: I think I will merge this in and do some post-processing.

[cowtowncoder]

Will proceed once I get CLA, stay tuned!