xero-node icon indicating copy to clipboard operation
xero-node copied to clipboard

`Cannot read properties of undefined (reading 'defaultPort')` when used with `nock`

Open TiddoLangerak opened this issue 1 year ago • 6 comments

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

  • Version 5.1.0

Describe the bug When using Nock to mock requests to Xero then error responses result in a Cannot read properties of undefined (reading 'defaultPort') error triggered in the ApiError class.

This appears to be caused by xero-node using an undocumented http.ClientRequest.agent property to fetch the port, which doesn't exist when using Nock (and possibly other mocking frameworks, too).

To Reproduce

  1. Setup a test using nock to generate some kind of error response. For example:
     nock(/xero/)
         .get(/Accounts/)
         .reply(429);
     client.accountingApi.getAccounts(/*...*/);
    
  2. Observe that this triggers the above error, instead of throwing an ApiError

Expected behavior I expect an ApiError to be thrown, instead of a cannot read properties of undefined

Screenshots

Additional context http.ClientRequest.agent is an undocumented property, it's likely not intended as a public facing API. There's a documented alternative available to get the port: request.socket.localPort. So replacing

protocol: axiosError.request.protocol,

with

protocol: axiosError.request.socket.localPort

should give the same information, using a documented public api, and compatible with Nock.

TiddoLangerak avatar Mar 04 '24 08:03 TiddoLangerak

PETOSS-396

github-actions[bot] avatar Mar 04 '24 08:03 github-actions[bot]

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

github-actions[bot] avatar Mar 04 '24 08:03 github-actions[bot]

Have you had the chance to consider this issue? I'd be happy to submit a PR if you agree with the suggested change?

TiddoLangerak avatar Mar 25 '24 07:03 TiddoLangerak

Same issue here

QuentinLemCode avatar Mar 25 '24 11:03 QuentinLemCode

@TiddoLangerak The PR can maybe accelerate the fix :)

QuentinLemCode avatar Mar 25 '24 11:03 QuentinLemCode

@sangeet-joy-tw Can we have update on this please ?

QuentinLemCode avatar Mar 29 '24 16:03 QuentinLemCode

Hey, @sangeet-joy-tw Could we have an update on this, please? Thanks in advance 🙏

c4c1us avatar Apr 08 '24 08:04 c4c1us

Hello @BenGDev @GraceYBR @JamesColemanXero @MattR-Api @MJMortimer @TheRegan This issue blocks us to update the xero-node dependency. We rely on nock to test our business services using Xero API.

Can you review and approve this PR please ? https://github.com/XeroAPI/xero-node/pull/682

QuentinLemCode avatar Apr 09 '24 07:04 QuentinLemCode

@manishT72 @manishT72x Could you take a look at this issue and the PR linked, please ?

QuentinLemCode avatar May 16 '24 11:05 QuentinLemCode

Hey @TiddoLangerak @QuentinLemCode @c4c1us

We have addressed the port issue in our latest xero-node v7.0.0.

Please install the latest version and kindly let us know if you guys are facing any more "undefined" issues with API Error class.

sangeet-joy-tw avatar Jun 24 '24 06:06 sangeet-joy-tw