idempotencyKey parameter introduced in 4.36.0 breaks existing calls to accounting api

itmad-com-au commented 1 year ago

SDK you're using (please complete the following information): xero-node Version 4.36.0

Describe the bug Code which was currently working with version 4.35.0 of xero-node and which is using existing parameters such as summarizeErrors will now stop working (as I experienced today). Many accounting api call signatures (and possible other apis too) have been changed in 4.36.0. The new "optional" idempotencyKey parameter has sometimes been inserted in front of existing parameters

For example with https://xeroapi.github.io/xero-node/accounting/index.html#api-Accounting-updateOrCreateInvoices the idempotencyKey parameter has, in v4.36.0, been inserted in front ot the summarizeErrors parameter

To Reproduce Steps to reproduce the behavior: Install version 4.35.0 xero-node Create a simple call as per 4.35.0 docs (without idempotencyKey parameter) to update invoices along the lines of:

const summarizeErrors = false, unitdp = 4

let xeroResponse, xeroError
try {
  xeroResponse = await xero.accountingApi.updateOrCreateInvoices(xeroTenantId, uploadData, summarizeErrors, unitdp)
  debug('xeroUpdateOrCreateItems: response = ', xeroResponse.body || xeroResponse.response.statusCode)
} catch (err) {
  // copy the err object for analysis by checkForXeroErrors
  xeroError = err
}

Once working, then upgrade to xero-node version 4.36.0

Your api call now fails with

{
  "error": {},
  "responseBody": {
    "Type": null,
    "Title": "BadRequest",
    "Status": 400,
    "Detail": "Idempotency Key: false is used with a different request.",
    "Instance": "4ce039a8-7b0c-4112-xxxx-dd565bbe979d",
    "Extensions": {}
  }
}

Expected behavior The sequence of the call parameters for the accounting api must remain consistent. Any new parameters to existing api interface calls - such as idempotencyKey - must be added to the end of the parameter list, so that existing parameters/code remains unaffected. In version 4.36.0 the idempotencyKey parameter has sometimes been added to the end, sometimes in the middle of existing parameters of the various accounting api calls

github-actions[bot] commented 1 year ago

PETOSS-345

github-actions[bot] commented 1 year ago

Thanks for raising an issue, a ticket has been created to track your request

khross commented 1 year ago

We have run into same problem. We attempted to use null/undefined for the idempotencyKey parameter but that fails as it seems to be casting the value to a string regardless of what is passed in.

madison890000 commented 1 year ago

At first, I tried to proactively fix this issue, as I thought it was a small bug. The specific solution is mentioned in https://github.com/XeroAPI/xero-node/pull/651.

After analyzing the code, I discovered that the root cause of this issue is:

1 The newly added idempotencyKey parameter in version 4.36.0 is an optional parameter, which will be added to the header when sending an HTTP request. It seems that Xero's API does not handle the case when 'Idempotency-Key' is undefined or null, as described in https://github.com/XeroAPI/xero-node/issues/649#issuecomment-1704429964.
2 In version 4.36.0, in the automatically generated code by openApi generator, the idempotencyKey is not uniformly configured as an optional parameter in function calls, as described in the issue. This has caused issues with matching the function call parameters when clients upgraded from version 4.35.0 to 4.36.0.

rootCause 1

At first, I thought that root cause 1 would be relatively easy to fix. If we were to directly modify the code of the node version, it would be simple (though not advisable), just like how I fixed it in the closed PR: https://github.com/XeroAPI/xero-node/pull/651. All we need to do is:

localVarHeaderParams['Idempotency-Key'] = ObjectSerializer.serialize(idempotencyKey, "string");

just change it to the following:

if( idempotencyKey ){
    localVarHeaderParams['Idempotency-Key'] = ObjectSerializer.serialize(idempotencyKey, "string");
}

However, upon closer examination of the code, I realized that it was generated using openApi generator, which complicated things.

I attempted to achieve this by modifying the definition of idempotencyKey in the main project, located at https://github.com/XeroAPI/Xero-OpenAPI.

Definition in version 4.36.0:

    idempotencyKey:
      in: header
      name: Idempotency-Key
      x-snake: idempotency_key
      description: This allows you to safely retry requests without the risk of duplicate processing. 128 character max.
      example: "KEY_VALUE"
      schema:
        type: string

But no matter whether I try to add nullable as false or true, I can't make the generated code change.

if( idempotencyKey ){
    localVarHeaderParams['Idempotency-Key'] = ObjectSerializer.serialize(idempotencyKey, "string");
}

I realized that it is impossible to achieve this by modifying the definition of the YAML file, and it may require modifying some template files mustache.

Therefore, my suggestion is to fix the backend API of Xero so that the 'Idempotency-Key' in the http header can still function properly even if it is null/undefined. This can be resolved without having to upgrade the version of xero-node SDK, which will solve rootCause 1.

rootCause 2

In the xero-node SDK, some methods have added an optional parameter called 'idempotencyKey,' which disrupts the calling code of the original SDK, such as the updateOrCreateInvoices function.

The updateOrCreateInvoices method in version 4.35.0 supports five parameters, which are as follows:

updateOrCreateInvoices (xeroTenantId: string, invoices: Invoices,  summarizeErrors?: boolean, unitdp?: number, options: {headers: {[name: string]: string}} = {headers: {}})

The method for version 4.36.0 has been changed to:

updateOrCreateInvoices (xeroTenantId: string, invoices: Invoices, idempotencyKey?: string, summarizeErrors?: boolean, unitdp?: number, options: {headers: {[name: string]: string}} = {headers: {}})

This causes the calling method of version 4.35.0 to recognize "summarizeErrors" as "idempotencyKey" in version 4.36.0. This results in a breaking change.

Since this code is generated by the OpenAPI generator, we need you to consider whether the current way of calling optional parameters is appropriate and if it needs to be changed in the next version.

updateOrCreateInvoices (xeroTenantId: string, config: {
    invoices: Invoices;
    idempotencyKey?: string;
    summarizeErrors?: boolean;
    unitdp?: number;
}, options: {headers: {[name: string]: string}} = {headers: {}})

The order and number of parameters like this will not cause a break change.

madison890000 commented 1 year ago

@itmad-com-au @khross

I just fix this issue manually updated the code in https://github.com/madison890000/xero-node.

Here is the npm link I published: https://www.npmjs.com/package/fix-xero-node.

You can use it by replacing

"xero-node": "4.35.0"

to

"fix-xero-node": "4.36.1"

to upgrading you dependencies of xero-node from 4.35.0 to 4.36.0.

I have only made two changes:

first is to move the new parameter "idempotencyKey" to the second-to-last position, just before the headers parameter. this means no changes need to be done when updating from 4.35.0 to 4.36.0.
second is deleting the Idempotency-Key in header if it is null/undefined. this means no need to add idempotencyKey if you don't want to. and it will ignore it as well if it is set to be null/undefined.

If the official updates to a new version and fixes this bug, you can switch back 'xero-node' again.

gierschv commented 11 months ago

I have a similar issue when using createInvoiceAttachmentByFileName, it now randomly fails with Idempotency Key: [object Object] is used with a different request..

I assume it tries to serialize the latest options object as a string and using it as idempotency key in the request. First request succeeds because it's using [object Object] as idempotency key.

I'm using the same options as the sample from the README for this function, with latest release 4.36.0: https://github.com/XeroAPI/xero-node#accounting-api

I will try to downgrade for now until this is fixed.

Thanks!

madison890000 commented 11 months ago

I have a similar issue when using createInvoiceAttachmentByFileName, it now randomly fails with Idempotency Key: [object Object] is used with a different request..

I assume it tries to serialize the latest options object as a string and using it as idempotency key in the request. First request succeeds because it's using [object Object] as idempotency key.

I'm using the same options as the sample from the README for this function, with latest release 4.36.0: https://github.com/XeroAPI/xero-node#accounting-api

I will try to downgrade for now until this is fixed.

Thanks!

This is the definition of createInvoiceAttachmentByFileName.

public async createInvoiceAttachmentByFileName (xeroTenantId: string, invoiceID: string, fileName: string, body: fs.ReadStream, idempotencyKey?: string, includeOnline?: boolean, options: {headers: {[name: string]: string}} = {headers: {}}) : Promise<{ response: http.IncomingMessage; body: Attachments;  }> {
        const localVarPath = this.basePath + '/Invoices/{InvoiceID}/Attachments/{FileName}'
        ...

The example in readme is incorrect. You need change to blow to make it works in 4.36.0.

const accountAttachmentsResponse = await xero.accountingApi.createInvoiceAttachmentByFileName(activeTenantId, invoiceId, filename, readStream,idempotencyKey,true, {
  headers: {
    'Content-Type': contentType
  }
});

gierschv commented 11 months ago

I have a similar issue when using createInvoiceAttachmentByFileName, it now randomly fails with Idempotency Key: [object Object] is used with a different request..

I assume it tries to serialize the latest options object as a string and using it as idempotency key in the request. First request succeeds because it's using [object Object] as idempotency key.

I'm using the same options as the sample from the README for this function, with latest release 4.36.0: https://github.com/XeroAPI/xero-node#accounting-api

I will try to downgrade for now until this is fixed.

Thanks!

This is the definition of createInvoiceAttachmentByFileName.
public async createInvoiceAttachmentByFileName (xeroTenantId: string, invoiceID: string, fileName: string, body: fs.ReadStream, idempotencyKey?: string, includeOnline?: boolean, options: {headers: {[name: string]: string}} = {headers: {}}) : Promise<{ response: http.IncomingMessage; body: Attachments;  }> {

        const localVarPath = this.basePath + '/Invoices/{InvoiceID}/Attachments/{FileName}'

        ...
The example in readme is incorrect.

You need change to blow to make it works in 4.36.0.
const accountAttachmentsResponse = await xero.accountingApi.createInvoiceAttachmentByFileName(activeTenantId, invoiceId, filename, readStream,idempotencyKey,true, {

  headers: {

    'Content-Type': contentType

  }

});

Thanks for the info, we will use TS for this part of our code in the future that will avoid future issues like that with Xero.

The problems is see here:

Why breaking changes were introduced as a minor version release?
Why the docs / samples are not up to date for that release.

I guess that I will need to be more careful with Xero updates in the future 😬

madison890000 commented 11 months ago

I have a similar issue when using createInvoiceAttachmentByFileName, it now randomly fails with Idempotency Key: [object Object] is used with a different request..

I assume it tries to serialize the latest options object as a string and using it as idempotency key in the request. First request succeeds because it's using [object Object] as idempotency key.

I'm using the same options as the sample from the README for this function, with latest release 4.36.0: https://github.com/XeroAPI/xero-node#accounting-api

I will try to downgrade for now until this is fixed.

Thanks!

This is the definition of createInvoiceAttachmentByFileName.
public async createInvoiceAttachmentByFileName (xeroTenantId: string, invoiceID: string, fileName: string, body: fs.ReadStream, idempotencyKey?: string, includeOnline?: boolean, options: {headers: {[name: string]: string}} = {headers: {}}) : Promise<{ response: http.IncomingMessage; body: Attachments;  }> {

        const localVarPath = this.basePath + '/Invoices/{InvoiceID}/Attachments/{FileName}'

        ...
The example in readme is incorrect. You need change to blow to make it works in 4.36.0.
const accountAttachmentsResponse = await xero.accountingApi.createInvoiceAttachmentByFileName(activeTenantId, invoiceId, filename, readStream,idempotencyKey,true, {

  headers: {

    'Content-Type': contentType

  }

});
Thanks for the info, we will use TS for this part of our code in the future that will avoid future issues like that with Xero.

The problems is see here:

Why breaking changes were introduced as a minor version release?

Why the docs / samples are not up to date for that release.

I guess that I will need to be more careful with Xero updates in the future 😬

Yeah, and I don't know why.

if you are have issues when you trying to upgrade from 4.35.0 to 4.36.0 to avoid breaking changes, you can try this: https://github.com/XeroAPI/xero-node/issues/649#issuecomment-1742613821

Raghunath-S-S-J commented 8 months ago

Hi all, Apologies for the delay. We have fixed the idempotency_key parameter issue in the SDK version 5.0.0.

The idempotency_key is now the last optional parameter in the method definitions Latest release also includes other changes (check out the release notes for details).

Do revert back to us if you are still facing any issues.

Thank you for your patience!

XeroAPI / xero-node