SAP / cloud-sdk-js

Use the SAP Cloud SDK for JavaScript / TypeScript to reduce development effort when building applications on SAP Business Technology Platform that communicate with SAP solutions and services such as SAP S/4HANA Cloud, SAP SuccessFactors, and many others.
Apache License 2.0
164 stars 56 forks source link

BatchResponse Handling: Changeset always successfull but isn't #5106

Open drvup opened 1 week ago

drvup commented 1 week ago

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 20241021_BatchChangesetResponse

Used Versions:

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 commented 1 week ago

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...