XeroAPI / xero-php-oauth2

Xero PHP SDK for oAuth 2 generated from Xero API OpenAPI Spec 3.0
MIT License
87 stars 64 forks source link

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

Closed PaulB12 closed 5 months ago

PaulB12 commented 7 months ago

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.

github-actions[bot] commented 7 months ago

PETOSS-370

github-actions[bot] commented 7 months ago

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

ravewill commented 7 months ago

We just had an outage because of this!

ChrisB-TL commented 7 months ago

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

glennjacobs commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@pumpkinball bumping

jstnmthw commented 6 months ago

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

bensebborn commented 6 months ago

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

Having to pin version 2.23.1 for now.

PaulB12 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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