SAP / cloud-sdk-java

Use the SAP Cloud SDK for Java 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
22 stars 13 forks source link

ODataHealthyResponseValidator Loses Failed Batch Request #542

Closed SpaceCondor closed 1 month ago

SpaceCondor commented 2 months ago

In the ODataHealthyResponseValidator class there is this bit under requireHealthyResponse:

https://github.com/SAP/cloud-sdk-java/blob/845ff8349c9a85afad0986f860d3ef6956459f74/datamodel/odata-client/src/main/java/com/sap/cloud/sdk/datamodel/odata/client/request/ODataHealthyResponseValidator.java#L59C1-L72C33

        final ODataResponseException preparedException =
            new ODataResponseException(
                batchFailedRequest == null ? request : batchFailedRequest,
                httpResponse,
                msg,
                null);

        final Try<ODataServiceError> odataError = Try.of(() -> loadErrorFromResponse(result));
        if( odataError.isSuccess() ) {
            final String msgError = msg + " The OData service responded with an error message.";
            throw new ODataServiceErrorException(request, httpResponse, msgError, null, odataError.get());
        }

        throw preparedException;

The issue is that if odataError.isSuccess() is successful, the preparedException is lost, along with the failed batch request.

Should preparedException be sent as the cause to ODataServiceErrorException? The constructor parameter is currently null:

throw new ODataServiceErrorException(request, httpResponse, msgError, preparedException , odataError.get());

MatKuhr commented 2 months ago

From a first look: Agreed, if possible we should also for batch return ODataServiceErrorException with the correct request object. I raised a PR #543 to see if we can improve this 👍🏼

MatKuhr commented 1 month ago

Forgot to update here: This fix has also been shipped with 5.12.0, thus closing the issue. As usual, ping us if it doesn't work 😉