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
165 stars 56 forks source link

UpdateRequestBuilder sending an empty payload as part of a batch/changeset request #4673

Open seeroush opened 6 months ago

seeroush commented 6 months ago

I'm not sure if this is a bug, or possibly the way I am utilizing the generated OData v2 client. I am attempting to perform a batch request on a particular API, and noticed that the PATCH method is not providing any payload in the changeset body. From the documentation, I'd expect the assignment of the paymentTerms and cashDiscountAmount to be in the payload based on the following statement:

The code above changed the first name of the given business partner. The payload sent to the service with PATCH includes only the first name.

If anyone could guide me on how to properly use the changeset/batch API in addition to updating an entity, it'd be appreciated. I also noticed that etag and csrf functionality of the client was not automatically handled, so I had to manually add the appropriate headers to get to this point. I kept that out of the paired-down sample below, but if CSRF handling should be automatic, guidance around how to get the auto-generated clients to handle it would be appreciated as well. The documentation led me to believe it's automatic.

Here's a sample of how I'm using a combination of batch and changeset to perform the request.

Dependencies:

  "dependencies": {
    "@nestjs/axios": "3.0.2",
    "@nestjs/common": "10.2.7",
    "@nestjs/platform-express": "10.2.7",
    "@nestjs/testing": "10.2.7",
    "@sap-cloud-sdk/connectivity": "3.15.0",
    "@sap-cloud-sdk/http-client": "3.15.0",
    "@sap-cloud-sdk/odata-v2": "3.15.0",
    "@sap-cloud-sdk/util": "3.15.0",
    "axios": "1.6.8",
    "bignumber.js": "9.1.2",
    "class-transformer": "0.5.1",
    "class-validator": "0.14.0",
    "moment": "2.29.4",
    "rxjs": "7.8.1"
  },

Code:

const {
  entryViewItemsApi,
  operations: { setEditable, setReadOnly },
} = facFinancialDocumentSrv01();

const entry = await entryViewItemsApi
  .requestBuilder()
  .getByKey('<accounting_document>', '<accounting_document_item>', '<company_code>', '2024', '')
  .execute({ destinationName: 'DESTINATION' });

// --- Batch Requests ---
const editableRequestBuilder = setEditable({
  accountingDocument: '<accounting_document>',
  companyCode: '<company_code>',
  fiscalYear: '2024',
});

const readonlyRequestBuilder = setReadOnly({
  accountingDocument: '<accounting_document>',
  companyCode: '<company_code>',
  fiscalYear: '2024',
});

// Set fields on item
entry.paymentTerms = 'CUSTOM';
entry.cashDiscountAmount = new BigNumber(5.00);

const updateRequest = entryViewItemsApi.requestBuilder().update(entry).addCustomHeaders({
  'if-match': entry.versionIdentifier,
});

const responses = await batch([
  changeset(
    editableRequestBuilder,
    updateRequest,
    readonlyRequestBuilder,
  ),
]).execute({ destinationName: 'DESTINATION' });

I am getting 415 responses from the server, so I added some middleware to output the request body to console, and noticed that the PATCH request body is set to {} when I would've expected it to be:

{"paymentTerms": "CUSTOM", "cashDiscountAmount": "5"}
--batch_a5e4fe59-20d8-4bed-ad1a-4c1b605182d6
Content-Type: multipart/mixed; boundary=changeset_d7a5a14d-b3b4-4ed6-ae30-0c07b96eeaeb

--changeset_d7a5a14d-b3b4-4ed6-ae30-0c07b96eeaeb
Content-Type: application/http
Content-Transfer-Encoding: binary
Content-Id: 11b46233-3ac5-419f-b8f4-0a8fe105826a

POST /sap/opu/odata/sap/FAC_FINANCIAL_DOCUMENT_SRV_01/SetEditable?FiscalYear='2024'&CompanyCode='<company_cod>'&AccountingDocument='<accounting_document>' HTTP/1.1
Content-Type: application/json
Accept: application/json

--changeset_d7a5a14d-b3b4-4ed6-ae30-0c07b96eeaeb
Content-Type: application/http
Content-Transfer-Encoding: binary
Content-Id: 6ffb551b-a4ee-4d53-998a-74cc63713a8e

PATCH /sap/opu/odata/sap/FAC_FINANCIAL_DOCUMENT_SRV_01/EntryViewItems(AccountingDocument='<accounting_document>',AccountingDocumentItem='<accounting_document_item>',CompanyCode='<company_cod>',FiscalYear='2024',Ledger='') HTTP/1.1
Content-Type: application/json
Accept: application/json
If-Match: W/"'6FB5547479391F921246BEFC9E5092D09E81DC1F'"

{}

--changeset_d7a5a14d-b3b4-4ed6-ae30-0c07b96eeaeb
Content-Type: application/http
Content-Transfer-Encoding: binary
Content-Id: c3e43f42-cc1a-455e-b9b0-e1974fca1dd2

POST /sap/opu/odata/sap/FAC_FINANCIAL_DOCUMENT_SRV_01/SetReadOnly?FiscalYear='2024'&CompanyCode='<company_cod>'&AccountingDocument='<accounting_document>' HTTP/1.1
Content-Type: application/json
Accept: application/json

--changeset_d7a5a14d-b3b4-4ed6-ae30-0c07b96eeaeb--
--batch_a5e4fe59-20d8-4bed-ad1a-4c1b605182d6--

Thanks in advance!

tomfrenken commented 5 months ago

Hey @seeroush thank's for reaching out.

The usage of the SDK does look correct, and the empty patch request is rather suspicious. Also, the fact that CSRF fetching as well as the ETag matching don't seem to work out of the box for you (it should be fetched by default for non-get requests) hint at multiple issues.

Are you certain the entry variable actually contains anything when you create the batch request at runtime?

seeroush commented 5 months ago

Hi @tomfrenken, yes. I validated that the values were non-empty via a debugger. I'm currently having to work around the issue by explicitly setting the payload like so:

  updateRequest.requestConfig.payload = {
    "PaymentTerms": 'CUSTOM',
    "CashDiscountAmount": new BigNumber(5.00),
  }

Do the handling of the out-of-the-box features depend on the destination being configured a certain way? One thing I should clarify is that I am hitting an on-premesis SAP instance and am not connecting through an SAP cloud proxy. Could that have something to do with the strange behavior around CSRF/ETag fetching?

tomfrenken commented 5 months ago

Hmm, let's try a few things.

1) We read the etag from the response. The selection process for the appropriate etag looks like this:

          const eTag =
            extractEtagFromHeader(response.headers) ||
            this.extractODataEtag(response.data) ||
            this.requestConfig.eTag;

We can see in the logs that the entity's .versionIdentifier property is populated, which means the requestConfig's etag property already contains this value.

If you don't set the if-match header manually, what value does it contain instead? I'm assuming the response's headers or response's data already contained a different header.

2) To fetch a CSRF token, we make up to two requests, looking like this:

Request:             HEAD /resource/path/
Request Header:      X-CSRF-Token=fetch

and

Request:             HEAD /resource/path
Request Header:      X-CSRF-Token=fetch

if this doesn't work, the target system is likely the cause, meaning it doesn't follow the usual default. To deviate from the default, we have documented how to change the SDK's default behavior here.

  1. The payload should contain the correct values regardless of if you target an on-premise system through a cloud connector or not, however, when you target onpremise systems we do various authentication flows in the backend. If you do not go through the cloud connector, you should make it a cloud destination. (You can read more about the implications of onpremise destinations here)
seeroush commented 5 months ago

Thanks, Tom! The solution I currently landed with does look similar to what you've outlined. Because I'm doing batch operations, I had to manually build out the middleware as you suggested to extract both CSRF and etag tokens. The psuedocode looks something like:

// Fetch CSRF token and cookie via middleware on a GET request
const {token, cookies} = getCsrfToken();

// entries are returned with a single HTTP batch request
// etags are returned as a map<document_id, etag>. The etag identifiers are plucked from response.body.d.__metadata.etag
const {entries, etags} = await fetchViaBatch(myEntryIdentifiers);

// Build changeset request for all entries which includes setEditable, updateEntryValue, setReadOnly
const changesets = entries.map((entry) => getChangeSet(entry))

// Perform batch update request with all changesets
const batchRequest = batch(changesets);
const batchReqId = batchRequest.build().config.boundary;
const responses = await batchRequest.addCustomHeaders({
   'x-csrf-token': resp.csrfToken,

   // Not entirely sure why, but I had to manually fetch the boundary value and set the top-level header for it to work.
   'content-type': `multipart/mixed;boundary=${batchReqId}`,
   'cookie: `${resp.cookie};`,
}).execute({ destinationName: 'DESTINATION' });

While this works in practice for a single SAP instance, I'm trying to identify exceptions so that when we start interfacing with more than one SAP instance, the code will be as portable as possible. I suppose I'm just having a hard time understanding what conditions (destination configuration, request building, etc...) are ideal so that I don't need to manually fetch and build my requests. It sounds like the client is supposed to handle it, but I'm not sure what the exceptions/conditions are that requires a custom solution.

I will talk to the team to see if it's possible for us to use a cloud connector for our test SAP deployment.

tomfrenken commented 5 months ago

The issues you are currently facing should be unrelated to the cloud connector.

Can you share for which service you've generated the client? That way I can generate it myself to further debug it.

Ideally the target system's endpoint returns the csrf token so you don't have to build your own middleware and create the csrf token & cookie headers.

I'm surprised you had to set 'content-type': multipart/mixed;boundary=${batchReqId}, given that in the logs that header is already set

Content-Type: multipart/mixed; boundary=changeset_d7a5a14d-b3b4-4ed6-ae30-0c07b96eeaeb

seeroush commented 5 months ago

The name of the service is FAC_FINANCIAL_DOCUMENT_SRV_01. I've attached the .edmx file (as a .txt) with the redacted domain URI (fake.domain.com). Let me know if that helps.

FAC_FINANCIAL_DOCUMENT_SRV_01_anonymous.edmx.txt

tomfrenken commented 5 months ago

Alright, I've generated a client and tried to replicate the issue, however, for me, the correct payload was created.

My current assumption is, that the remote state has been updated in between your requests, which would lead to an empty payload, as the remote state already contains the change.

Can you verify what the remote state contains at runtime? For this, add a breakpoint for either one of the two:

entry.remoteState
entry["remoteState"]

here you can check if the entry already contained the desired change.

tomfrenken commented 5 months ago

Alternatively, you can also try to use a PUT request instead, as documented here. This should definitely contain your change, as it doesn't depend on the remote state.

seeroush commented 5 months ago

Thanks Tom! I'll look into the remoteState field to see what its contents are prior to the update batch request. I'm going to be out-of-office for the next week, so there'll be a bit of delay in the response. I appreciate the feedback and all your effort in helping out.

marikaner commented 5 months ago

Hey @seeroush, were you able to identify the issue with @tomfrenken's suggestions?