Polymorphic JSON merge patch models may cause properties to be included in update when deserializing

[alzimmermsft]

While regenerating an SDK to use stream-style serialization I found that polymorphic properties aren't being handled correctly during deserialization if they support JSON merge patch.

The current design of JSON stream-style deserialization uses the model setter if the property is defined by a super type. JSON merge patch uses the setter to determine if a property should be included in serialization when the model is used in a patch operation. These together result in the parent properties always being included in serialization, and possibly in an incorrect way if the value is null and something else sets it in-between deserialization and using the model in a patch operation.

Code sample

public class Parent {
  private String property;

  public Parent setProperty(String property) {
    this.property = property;
    // Track property for patch serialization
    return this;
  }
}

public class Child extends Parent {
  public Child setProperty(String property) {
    super.setProperty(property);
    // Track property for patch serialization
    return this;
  }

  public static Child fromJson(JsonReader jsonReader) throws IOException {
    // logic
    if ("property".equals(fieldName) {
      childInstance.setProperty(jsonReader.getString());
    }
    // logic
  }
}

Solutions

At this time there are two solutions I can think of:

1. The models within the polymorphic hierarchy have internal setters that don't track the property's inclusion into patch serialization.
2. The parent model defines the property as package-private and all children models change deserialization to set the property directly.

My preference is for option #2 as option #1 would require each child model within the hierarchy to add an internal setter as well. Additionally, option #2 can be used universally, even when the model isn't used in JSON merge patch operations and would allow for the removal of calls like:

public String getProperty() {
  return super.getProperty();
}

public Type setProperty(String property) {
  super.setProperty(property);
  return this;
}

and allow for direct access to the field:

public String getProperty() {
  return property;
}

public Type setProperty(String property) {
  this.property = property;
  return this;
}

[haolingdong-msft]

Thanks @alzimmermsft for raising this. Do you mean using deserialized model to do patch operation could lead to problem? Is it similar as this one: https://github.com/Azure/autorest.java/pull/2576#issuecomment-1966144039 For this issue, we previously discussed with @srnagar, and we decided to ignore it first as we assume users do not use deserialized model to do patch. But this can be discussed furthur.

[weidongxu-microsoft]

We'd expect user do this with a new class (and only set the property that to be modified -- the right way to do a json-merge-patch on a resource)

client.patch(name, new Child().setProperty(newValue);

instead of

Child child = client.getByName(name);
child.setProperty(newValue);
client.patch(name, child);

However, user may confuse about it (for instance, when the lib is used with other libs that only support PUT to update or simple PATCH).

And it is possible that user use a de-serialized model that nested in the resource. e,g,

Child child = client.getByName(name);
Property prop = child.getProperty();
client.patch(name2, new Child().setProperty(prop));

Then the updatedProperties in Property class could carry-over to the new Child class.

[alzimmermsft]

Thanks @alzimmermsft for raising this. Do you mean using deserialized model to do patch operation could lead to problem? Is it similar as this one: #2576 (comment) For this issue, we previously discussed with @srnagar, and we decided to ignore it first as we assume users do not use deserialized model to do patch. But this can be discussed furthur.

Yeah, this is that comment, I also talked with @srnagar yesterday about it as I didn't realize this was discussed before.

We'd expect user do this with a new class (and only set the property that to be modified -- the right way to do a json-merge-patch on a resource)

client.patch(name, new Child().setProperty(newValue);

instead of

Child child = client.getByName(name);
child.setProperty(newValue);
client.patch(name, child);

However, user may confuse about it (for instance, when the lib is used with other libs that only support PUT to update or simple PATCH).

And it is possible that user use a de-serialized model that nested in the resource. e,g,

Child child = client.getByName(name);
Property prop = child.getProperty();
client.patch(name2, new Child().setProperty(prop));

Then the updatedProperties in Property class could carry-over to the new Child class.

I think we should fully support both newed up instances for patching and roundtripping scenarios as if only newing up is supported this may result in a lot of confusion. For example, though the service doesn't support it the idea is generalizable, Storage has metadata on Blobs and you may have data like last-change which is a date and if it's older than a certain threshold you had a will-delete-on value, such as rotating logs stored in Storage. Overall, the idea is there are likely scenarios where the service value will need inspection before the patching can be done and forcing a path where a new model needs to be newed up is a bit confusing:

ServiceValue serviceValue = service.get(value);
if (serviceValue.propertyToCheck() == needsUpdate) {
  serivceValue.setValueToUpdate(newState);
  service.patch(serviceValue);
}

is much nicer and clearer than

ServiceValue serviceValue = service.get(value);
if (serviceValue.propertyToCheck() == needsUpdate) {
  ServiceValue updateValue = new ServiceValue().setValueToUpdate(newState);
  service.patch(updateValue);
}

especially in polymorphic scenarios where the update value may be something supported by all types, because the latter codesnippet would require you to also type check to make sure you update with the right type.

[weidongxu-microsoft]

We'd expect user do this with a new class (and only set the property that to be modified -- the right way to do a json-merge-patch on a resource)

client.patch(name, new Child().setProperty(newValue);

instead of

Child child = client.getByName(name);
child.setProperty(newValue);
client.patch(name, child);

However, user may confuse about it (for instance, when the lib is used with other libs that only support PUT to update or simple PATCH). And it is possible that user use a de-serialized model that nested in the resource. e,g,

Child child = client.getByName(name);
Property prop = child.getProperty();
client.patch(name2, new Child().setProperty(prop));

Then the updatedProperties in Property class could carry-over to the new Child class.

I think we should fully support both newed up instances for patching and roundtripping scenarios as if only newing up is supported this may result in a lot of confusion. For example, though the service doesn't support it the idea is generalizable, Storage has metadata on Blobs and you may have data like last-change which is a date and if it's older than a certain threshold you had a will-delete-on value, such as rotating logs stored in Storage. Overall, the idea is there are likely scenarios where the service value will need inspection before the patching can be done and forcing a path where a new model needs to be newed up is a bit confusing:

One point is that there may be intrinsic ambiguity on "what is intended by user".

e.g. in below, user likely only want to modify "property", as they get and update on same resource. On this, user would want to send a json-merge-patch with only one property.

Child child = client.getByName(name);
child.setProperty(newValue);
client.createOrUpdate(name, child);

But here

Child child = client.getByName(name);
child.setProperty(newValue);  // with or without this line
client.createOrUpdate(newName, child);  // via json-merge-patch

user likely want to keep every property in "name" resource and copy it to a new resource. And here user would want to send a json-merge-patch with all properties existed in "name" resource.