Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding in idempotency_key is a breaking change and not backward compatible #337

Closed
PaulB12 opened this issue Dec 1, 2023 · 14 comments · Fixed by #346
Closed

Adding in idempotency_key is a breaking change and not backward compatible #337

PaulB12 opened this issue Dec 1, 2023 · 14 comments · Fixed by #346

Comments

@PaulB12
Copy link

PaulB12 commented Dec 1, 2023

SDK you're using (please complete the following information):

Describe the bug
With the new addition of idempotency_key for any PUT/POST/PATCH requests is a breaking change to existing applications.
As idempotency_key isn't last in the constructor it is replacing values that were previously there when named parameters have not been used.

To Reproduce

Take a previous version where the function for createInvoicesWithHttpInfo used to be:
createInvoicesWithHttpInfo($xero_tenant_id, $invoices, $summarize_errors = false, $unitdp = null)
It has now become:
createInvoicesWithHttpInfo($xero_tenant_id, $invoices, $idempotency_key = null, $summarize_errors = false, $unitdp = null)

Code that used to call the previous version may look something like this:
$this->api->createInvoicesWithHttpInfo( $tenantId, $invoices, $this->summarizeErrors, $this->unitdp )

Because idempotency_key is now the third slot, the idempotency_key will be set to either 1 or 0 (depending if your previous summarizeErrors is true or false) and requests will end up failing due to duplicate key.

Expected behavior
To have idempotency_key at the end of the constructor so existing applications that do not use name parameters aren't broken upon upgrading.

Additional context
Using named parameters in our own code would prevent this issue however we still have to support legacy applications, if changing the constructor so idempotency_key is last isn't possible I do think it's a good idea to notify that between versions there is a breaking change for users who are upgrading.

Copy link

github-actions bot commented Dec 1, 2023

PETOSS-370

Copy link

github-actions bot commented Dec 1, 2023

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

@ravewill
Copy link

ravewill commented Dec 6, 2023

We just had an outage because of this!

@ChrisB-TL
Copy link

This also broke our application, which we had to rollback...

@glennjacobs
Copy link

Same here, this broke our application. I don't understand why this change was made on a patch release, it should have been a minor release IMO and of course, should not have broken backwards compatibility.

Rolling back to 2.31.1 resolved the issue.

@mbardelmeijer
Copy link

We ran into this breaking change as well, fixing to 2.23.1 What's the upcoming plan for this SDK? Will this change be reverted, or should we modify our code to handle these changes?

@ravewill
Copy link

@mbardelmeijer for what it's worth, we switched to using named arguments for all Xero SDK functions so that this doesn't bite us again.

@PaulB12
Copy link
Author

PaulB12 commented Dec 19, 2023

@pumpkinball bumping

@jstnmthw
Copy link

jstnmthw commented Jan 4, 2024

Broke our integration. No notice of breaking changes or updates.

@bensebborn
Copy link

Broken our integration too - this should be a major version bump with upgrade advice.

Having to pin version 2.23.1 for now.

@PaulB12
Copy link
Author

PaulB12 commented Jan 9, 2024

Xero support advising to post an issue here but no response, is quite concerning from Xero.

Think it'll just be the case of sorting your own applications out either using named parameters or just swapping the order you're putting the arguments in.

@wobinb
Copy link
Contributor

wobinb commented Jan 9, 2024

Hi all, sorry for the radio silence on this.

Whilst it's a fairly simple fix within this SDK, it has knock on effects within the other SDKs which we have to consider. For context the SDK is autogenerated from our OpenAPI Specification here: https://github.com/XeroAPI/Xero-OpenAPI so the solution is to adjust the order within that, however the change will also impact the other SDKs that are generated from the same file.

Please bear with us whilst we look in to it.

@Raghunath-S-S-J
Copy link
Contributor

Hi all, Apologies for the delay. We have fixed the idempotency_key parameter issue in the SDK version 3.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!

@Raghunath-S-S-J
Copy link
Contributor

Closing the issue. Reopen or create new issue incase facing any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants