BatchResponse Handling: Changeset always successfull but isn't

[drvup]

Describe the bug
We do send multiple requests in a batch to a SAP CAP application. Within, we do upsert different entities on the CAP app.
It's working fine, but since some weeks, we discovered missing requests sent to our CAP app.
We checked our logs and were wondering why there is nothing discovered.
Today, we did debug this locally and identified, if a request inside a changeset as part of the batch send to CAP is rejected (e.g. the payload had a property who is not valid / existing), the SAP Cloud SDK is setting the changeset to isSuccess, instead of isError.

To Reproduce
Steps to reproduce the behavior:

1. Send a batch request having 1 changeset and 1 update request.
2. Inside the update request, add a non-existing property (in our example, it's "callbackStatus") within a value
3. Add a custom header: "prefer": "odata.continue-on-error". This will prevent the batch-request from failing completely (only the changesets / requests can fail, and it will not stop other changesets from fulfillment).
4. Send the request to the CAP app
5. On the ".then()" block, check the status of the changeset.
   Example:

        await batchRequest
            .execute(this.destination)
            .then((batchResponse) => {
                // batchResponse[] -> changesets [] -> request []
                for (const changesetResponse of batchResponse) {
                    if (!changesetResponse.isSuccess()) {
                        // >>> No Success! Damn!!! <<<
                        [...]
                    } // if
                } // for batchResponse
            }) // .then

Expected behavior
The changeset is not successful, as the response from CAP is showing http status 400 for the request within the changeset.
A changeset is always atomic. So, if one request inside a changeset is failing, the complete changeset is failing.
We also do send a custom header: "prefer": "odata.continue-on-error"
Having this customizing, the complete batch request is NOT failing, but the request due to the not-existing-property-thing, is failing. You can see the response for the request on the CAP app on the screenshot.

Screenshots

Used Versions:

• node version v18.19.1
• npm version v10.2.4
• SAP Cloud SDK version: 3.22.2

Additional Information
While debugging, I saw on file odata-common/request-builder/batch/batch-response-deserializer.js, as soon as it's a changeset, the deserialize method deserializeChangeSet() does set the isSuccess to true. From my POV, this should not be the case and there should be a check on the sub method, if the response is failed.
I do wonder if this was never there and just discovered ( after 4 years running on production ) or if there was a code change in place.

Impact / Priority
Affected development phase: Production
Impact: Inconvenience / Impaired

Best regards,
Cedric

[marikaner]

Hey @drvup, this has not changed in a long time and it is intended. When sending requests as batches you can send multiple changesets or retrieve requests together. When one of them fails, the total response will still be successful, but the individual responses underneath might indicate errors, much like Promise.allSettled(). Ergo, if a changeset failed you will only see it in the subresponse for this changeset.
This also corresponds to the batch behavior on HTTP level (you will likely get a 202 status code on the batch request but an error code on the individual changeset).

I recommend to take a look at the docs for details on response handling with batch.

Please also note that changesets should be atomic, but every service owner could implement this differently...

[drvup]

Hi @marikaner

thanks for coming back to me about it.

We debugged this deeper and identified the original reason for it. CAP v8 has adjusted the batch-response approach in comparison to the CAP v7 version. It's now part of the changeset-response, instead of the direct batch-response:

// CAP 8
--batch_25837852-1e7d-4031-bb8b-b7910c736812
content-type: multipart/mixed;boundary=changeset_04cad1cb-fe1b-492e-b9a7-72b1ebfa2eda

--changeset_04cad1cb-fe1b-492e-b9a7-72b1ebfa2eda
content-type: application/http
content-transfer-encoding: binary
content-id: 968dd495-ea0e-4f91-b653-9bddd95b72d5

HTTP/1.1 400 Bad Request
odata-version: 4.0
content-type: application/json;odata.metadata=minimal

{"error":{"code":"400","message":"Property \\"iDoNotExist\\" does not exist in SalesOrder","@Core.ContentID":"968dd495-ea0e-4f91-b653-9bddd95b72d5"}}
--changeset_04cad1cb-fe1b-492e-b9a7-72b1ebfa2eda--

--batch_25837852-1e7d-4031-bb8b-b7910c736812--


// CAP 7
--batch_55ce50c9-71e0-4e16-9ee1-b6a6e7de415b
content-type: application/http
content-transfer-encoding: binary
content-id: fa590e90-d9da-4c90-902d-0addd7b1791e

HTTP/1.1 400 Bad Request
odata-version: 4.0
content-type: application/json;odata.metadata=minimal

{"error":{"code":"400","message":"Deserialization Error: 'iDoNotExist' does not exist in type 'ServiceName.SalesOrder.","@Core.ContentID":"fa590e90-d9da-4c90-902d-0addd7b1791e"}}
--batch_55ce50c9-71e0-4e16-9ee1-b6a6e7de415b--

(this was the same request, send to 2 different CAP Apps)

I am with you about the handling of
batch-response -> changeset-response -> changeset-request-response.

But, if an atomic changset ist failing, because one changeset-request-response is failed, the SDK should set the isSuccess() => false and isError() => true for the changeset.
From my POV, the official spec is marking the changeset as atomic by default. So, if this is the case, it would be appreciated if the SDK does follow this approach and does the same. Example modification of odata-common/request-builder/batch/batch-response-deserializer.js:

    deserializeChangeSet(changesetData) {
        const requestOfChangesetFailed = changesetData.some(subResponseData => !batch_response_parser_1.isHttpSuccessCode(subResponseData.httpCode));
        return {
            responses: changesetData.map(subResponseData => this.deserializeChangeSetSubResponse(subResponseData)),
            isSuccess: () => !requestOfChangesetFailed,
            isError: () => requestOfChangesetFailed,
            isReadResponse: () => false,
            isWriteResponses: () => true
        };
    }

However, the SDK could also provide a capability to enhance it on the consumer site?

Let me know what you think & thanks for taking the time, it's really appreciated :)

BR,
Cedric