fix: use query params for GET address-book methods#9084
Conversation
- Replace .send(params) with .query(params) in getConnections, getListingEntryContacts, and getListingEntryDirectory so filters are sent as URL query params rather than request body GN-2899 #9084
c2e224f to
f8046c6
Compare
|
Tests failing |
- Replace .send(params) with .query(params) in getConnections, getListingEntryContacts, and getListingEntryDirectory so filters are sent as URL query params rather than request body GN-2899 #9084
f8046c6 to
1b395d7
Compare
- Replace .send(params) with .query(params) in getConnections, getListingEntryContacts, and getListingEntryDirectory so filters are sent as URL query params rather than request body GN-2899 #9084
1b395d7 to
e3b92e2
Compare
- Replace .send(params) with .query(params) in getConnections, getListingEntryContacts, and getListingEntryDirectory so filters are sent as URL query params rather than request body GN-2899 #9084
e3b92e2 to
6ad97d5
Compare
- Replace .send(params) with .query(params) in getConnections, getListingEntryContacts, and getListingEntryDirectory so filters are sent as URL query params rather than request body GN-2899 #9084
6ad97d5 to
9fd3f2c
Compare
lokesh-bitgo
left a comment
There was a problem hiding this comment.
The core fix is correct — .send(params) on GET was silently dropping all filter params since most servers ignore the request body on GET requests. .query(params ?? {}) is the right approach. CI is green and the logic is sound.
Leaving a few non-blocking suggestions inline.
There was a problem hiding this comment.
Nit (follow-up): getListing() (line ~82) also calls .send() on a GET with no arguments — same anti-pattern as the methods fixed here. Less harmful since there are no params to drop, but it's inconsistent in the same class. Consider cleaning it up in a follow-up: just remove the .send() call entirely since .get().set(...).result() is sufficient for a parameterless GET.
| function makeGetStub() { | ||
| const queryStub = sinon.stub().returns({ result: sinon.stub().resolves({}) }); | ||
| const setStub = sinon.stub().returns({ query: queryStub }); | ||
| mockBitGo.get.returns({ set: setStub }); |
There was a problem hiding this comment.
Low: mockBitGo.get is stubbed to accept any URL — a typo in the endpoint path (e.g. /connections → /connection) would go undetected. Consider asserting the URL too:
sinon.assert.calledWith(mockBitGo.get, 'https://app.bitgo.com/api/address-book/v1/connections');| sinon.assert.calledOnce(queryStub); | ||
| sinon.assert.calledWith(queryStub, params); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Low: getConnections has a no-params test verifying undefined → {} (the ?? {} path), but getListingEntryContacts and getListingEntryDirectory only have one test each (with params). Consider adding a no-params case for both:
it('should work with no params', async function () {
const { queryStub } = makeGetStub();
await addressBook.getListingEntryContacts();
sinon.assert.calledWith(queryStub, {});
});| return this.bitgo | ||
| .get(url) | ||
| .set('enterprise-id', this.enterpriseId) | ||
| .query(params ?? {}) |
There was a problem hiding this comment.
Nit: GetAddressBookConnectionsParams includes array fields (ownerWalletId?: string[], targetWalletId?: string[]). Superagent's default .query() serializes arrays as repeated keys — ?ownerWalletId=a&ownerWalletId=b. Worth verifying the address-book service parses that format (vs bracket notation ownerWalletId[]=a or comma-separated). If the format doesn't match, these filters would silently not work.
Summary
getConnections,getListingEntryContacts, andgetListingEntryDirectorywere using.send(params)on GET requests, which puts params in the request body — ignored by the server.query(params)so filters are correctly sent as URL query paramsquery:, notbody:Test plan
.query()is called with the provided paramsFixes GN-2899
🤖 Generated with Claude Code