PATCH Member of a Collection Sends all Fields Instead of Changed Fields

[SpaceCondor]

Issue Description

When modifying a member of a collection property of an object and then issuing a PATCH request, the entire collection and all properties are sent as opposed to just the member and properties that have been updated.

A real world scenario would be updating the sales price of the second line of an order.

Example Code:

// build OData Request
var getDocumentRequest = service.withServicePath("example.com").getOrdersByKey(12345);
var getOrderResponse = getDocumentRequest.execute(....);
var order = getOrderResponse.get();

for (var orderLine : order.getLines()) {

    if(orderLine.getLineNum() == 2) {
          orderLine.setPrice(100);
    }

}

// build OData Request
var updateOrderRequest = service.withServicePath("example.com").updateOrders(order);

In the above scenario I'd expect to see a request like:

PATCH https://example.com/Orders(12345)

{
  "OrderLines": [
    {
      "LineNum": 2,
      "Price": 100
    }
  ]
}

But instead I am getting all order lines and all fields of all order lines.

I understand that I can exclude particular fields from updates and also force include fields, but I don't think I can specify the above behavior?

[Johannes-Schneider]

Hi @SpaceCondor,

thanks for reaching our to us.
The observed behavior is indeed a known shortcoming of the Cloud SDK.

I will reach out to our PO (@newtork) to discuss whether we can find time to implement this feature.
Would you mind elaborating a bit about the urgency from your side?
Is this issue blocking your development or is it more like an inconvenience?

Thanks and best regards,
Johannes

[SpaceCondor]

Hey @Johannes-Schneider

If needed I can construct the request directly as opposed to utilizing the cloud SDK so it is more of an inconvenience.

Within SAP B1, the Service Layer will often produce errors when doing PATCH requests and sending entire objects.

[newtork]

Hi @SpaceCondor,

Currently we are in preparation for the upcoming next major release. This will likely keep us busy within the next weeks.
As long as you have the option to use a temporary workaround, I would like to kindly ask for patience until Q4 2022.

Please let me know in case this request is much more urgent and requires higher priority.

The following request could be used as workaround while leveraging our Generic OData Client:

final String jsonPayload = "{"
        + "  \"OrderLines\": ["
        + "    {"
        + "      \"LineNum\": 2,"
        + "      \"Price\": 100"
        + "    }"
        + "  ]"
        + "}";

final ODataEntityKey entityKey = ODataEntityKey.of(Collections.singletonMap("OrderId", 12345), ODataProtocol.V2);

final ODataRequestUpdate request = new ODataRequestUpdate(
    "/service/path/", // service path
    "Orders", // entity set name
    entityKey,
    jsonPayload,
    UpdateStrategy.MODIFY_WITH_PATCH,
    null, // no version identifier
    ODataProtocol.V2);

final HttpDestination httpDestination = DestinationAccessor.getDestination("MyDestination").asHttp();
final HttpClient httpClient = HttpClientAccessor.getHttpClient(httpDestination);
request.execute(httpClient);

Best regards
Alexander

[SpaceCondor]

@newtork Thank you for the feedback and I completely understand regarding the launch of version 4 of the SDK being a priority.

I will use the provided workaround for now and if it becomes more urgent I will let you know. Thanks again!

[SpaceCondor]

Hey @newtork I just wanted to know if this has been resolved?

[MatKuhr]

Hey, unfortunately we didn't get around to implementing this, and we currently don't have it planned. But as you might have seen, we moved the SDK to open source in the meanwhile. So if you want you could even give this a shot yourself and contribute 😄

However, looking at this again I'm not sure if the exact above example is allowed by OData V4. Because the specification states:

Collection properties and primitive properties provided in the payload corresponding to updatable properties MUST replace the value of the corresponding property in the entity or complex type.

To me this reads like the full list content is required. Furthermore, using PATCH on a collection property directly is explicitly not allowed.

Am I missing something in the spec? Also, I assume the service you are working with does allow this behavior in practice?

---

If the collection is a navigation property, there is the option to update related entities when patching. By default, here also the full list has to be given. But one can also provide a delta like so:

"OrderLines@delta": [
    {
      "@id": 2,
      "Price": 100
    }
  ]

But this feature is only supported with OData 4.01, not 4.0. And I think this only applies to navigation properties, not collections of complex types. Thus adding this into the SDK would require some form of feature toggle.

---

[SpaceCondor]

@MatKuhr You are correct that this technically does not abide by the specification, however this is behavior implemented by the SAP Business One Service Layer. There is a blog post from SAP here about it: https://community.sap.com/t5/enterprise-resource-planning-blogs-by-sap/sap-business-one-service-layer-entity-crud-update/ba-p/13171993

The important bit is the following:

According to the OData standard, since collection members have no individual identity, PATCH is not supported for collection properties.
However, this rule does not suit the B1 data model well, as the collection members of B1 business objects have individual identity. Considering this fact, Service Layer extends the PATCH to support collection properties.

[MatKuhr]

Interesting, okay. In that case we anyway should be careful with implementing anything in the SDK, as we aim to only offer OOTB functionality for things that are conforming to the spec.

Maybe an easier workaround is also to just do something like:

Function<List<LinteItem>, List<LinteItem>> update; // some update logic

var updatedList = update.apply(order.getOrderLines());
var updatesOnlyList = updatedList.stream().filter(it -> !it.getChangedFields().isEmpty()).toList();

order.setOrderLines(updatesOnlyList);
order = orderService.updateOrder(order);

With the last line assuming the service responds with the entity and the full list. If not, one could manually reset the object to contain the full list.

Didn't try it, but the getChangedFields() method is public, so from the top of my head this might just work 😄

[SpaceCondor]

@MatKuhr Unfortunately that will not work, that would send only lines with changed fields, but for those lines it would send all fields.

I was thinking about adding an override here:

https://github.com/SAP/cloud-sdk-java/blob/e5d1143a6bb79e89ec9b942f57ee9fff8f7c9671/datamodel/odata-v4/odata-v4-core/src/main/java/com/sap/cloud/sdk/datamodel/odatav4/core/UpdateRequestHelperPatch.java#L133

From:

if( input instanceof VdmComplex ) {
   changedFields = ((VdmObject<?>) input).toMapOfFields();
   changedFields.putAll(((VdmObject<?>) input).getCustomFields());
} else {
   changedFields = ((VdmObject<?>) input).getChangedFields();
}

To:

if( input instanceof VdmComplex && !sendOnlyChangedVdmComplexFields /* <-----Override flag */) {
   changedFields = ((VdmObject<?>) input).toMapOfFields();
   changedFields.putAll(((VdmObject<?>) input).getCustomFields());
} else {
   changedFields = ((VdmObject<?>) input).getChangedFields();
}

Not sure if this is something that would be considered, but it is needed for my use case.

[MatKuhr]

it would send all fields

and that is not supported by the B1 service? Naively, I'd think updating all properties is just a special case of updating some properties 😄

Anyhow, what might actually be an alternative solution is to create a small custom implementation of the LinteItem complex type, overriding the protected Map<String, Object> toMapOfFields(), only returning the changed fields. That should do the trick (and be arguably simpler than changing the rather complex serialization code 😄 ). Maybe worth a shot?

[SpaceCondor]

@MatKuhr

and that is not supported by the B1 service? Naively, I'd think updating all properties is just a special case of updating some properties 😄

Unfortunately no, the presence of certain fields in DocumentLine will cause the PATCH to fail, since the B1 Service attempts to update them (even if they are the same value) and there are many fields which cannot be updated.

And that is a great idea for overriding the toMapOfFields() code! That greatly simplifies what I am trying to do.

[SpaceCondor]

@MatKuhr

@Nonnull
@Override
public class DocumentLinePatch extends DocumentLine {

    @Nonnull
    @Override
    protected Map<String, Object> toMapOfFields() {

        Map<String, Object> mapOfFields = super.toMapOfFields();
        mapOfFields.keySet().retainAll(this.changedOriginalFields.keySet());
        return mapOfFields;

    }
}

Did the trick! Not sure why I didn't think of that.

I'll close this issue now. Thanks so much for the help. 👍

[MatKuhr]

Happy that it worked 🥳

Also, I didn't know about the retainAll you used above before, that's pretty handy, will use it myself if I can remember 😄