refactor(NODE-7313): only use dns.resolve for all types by durran · Pull Request #4846 · mongodb/node-mongodb-native

durran · 2026-01-22T13:50:55Z

Description

Refactors all dns resolution to only go through lookup and resolve.

Summary of Changes

Refactors dns resolution to only use lookup and resolves
Fixes all tests

What is the motivation for this change?

NODE-7313

Release Highlight

Release notes highlight

Double check the following

Lint is passing (npm run check:lint)
Self-review completed using the steps outlined here
PR title follows the correct format: type(NODE-xxxx)[!]: description
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

baileympearson · 2026-02-09T23:09:55Z

src/connection_string.ts

  return async function dnsReqRetryTimeout(lookupAddress: string) {
    try {
-      return await dns.promises[api](lookupAddress);
+      return (await dns.promises.resolve(lookupAddress, rrtype)) as dns.SrvRecord[] | string[][];
    } catch (firstDNSError) {
      if (firstDNSError.code === dns.TIMEOUT) {
-        return await dns.promises[api](lookupAddress);
+        return (await dns.promises.resolve(lookupAddress, rrtype)) as dns.SrvRecord[] | string[][];
      } else {
        throw firstDNSError;
      }
    }
  };
 }


Can we choose an approach that doesn't necessitate casting? One approach would be something like:

Suggested change

return async function dnsReqRetryTimeout(lookupAddress: string) {

try {

return await dns.promises[api](lookupAddress);

return (await dns.promises.resolve(lookupAddress, rrtype)) as dns.SrvRecord[] | string[][];

} catch (firstDNSError) {

if (firstDNSError.code === dns.TIMEOUT) {

return await dns.promises[api](lookupAddress);

return (await dns.promises.resolve(lookupAddress, rrtype)) as dns.SrvRecord[] | string[][];

} else {

throw firstDNSError;

}

}

};

}

const resolve =

rrtype === 'SRV'

? (address: string) => dns.promises.resolve(address, 'SRV')

: (address: string) => dns.promises.resolve(address, 'TXT');

return async function dnsReqRetryTimeout(lookupAddress: string) {

try {

return await resolve(lookupAddress);

} catch (firstDNSError) {

if (firstDNSError.code === dns.TIMEOUT) {

return await resolve(lookupAddress);

} else {

throw firstDNSError;

}

}

};

Another would be modifying the function to instead accept a lookup function, which it then calls.

@PavelSafronov I have updated this block using generics (basically map rrtype to response). please let me know if this works

There's still a cast, it's just been moved to a different place

In my view it's essentially the same as you suggested, but with the different mechanism: generics vs. runtime if-else:

const resolve = rrtype === 'SRV' ? (address: string) => dns.promises.resolve(address, 'SRV') : (address: string) => dns.promises.resolve(address, 'TXT'); // is effectively the same as (from the Typescript point of view) interface DNSLookupMap { SRV: dns.SrvRecord[]; TXT: string[][]; } const resolve = () => dns.promises.resolve(lookupAddress, rrtype) as Promise<DNSLookupMap[T]>;

In other words, I don't want to add runtime check for the compilation step (it just feels wrong, however I'm not strongly against it).

Would you agree? Or is it casting in general that you would like to avoid?

Casting in general. I don't feel terribly strongly, but if we have the capability to write this in way that doesn't require casting, I guess I don't see why we wouldn't?

Runtime checks to have a perf cost (although in general I think an if-statement is effectively negligable). But regardless, this is a factory, so the perf cost is once per module instantiation per function.

agree! switched implementation back (with the resolve being defined with ternary condition)

test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts

PavelSafronov

Changes look good.

Copilot

Pull request overview

Refactors the driver’s DNS resolution paths so SRV/TXT/CNAME/PTR lookups consistently route through dns.resolve(...) (and dns.lookup(...) where appropriate), aligning runtime behavior and test stubbing with the unified API.

Changes:

Updated SRV polling to use dns.promises.resolve(host, 'SRV') instead of resolveSrv.
Updated SRV/TXT connection-string resolution retry logic to use dns.promises.resolve(host, rrtype).
Updated GSSAPI canonicalization to use dns.promises.resolve(host, 'PTR' | 'CNAME') and adjusted related unit/manual/integration tests.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/sdam/srv_polling.ts`	Switch SRV polling to `dns.promises.resolve(..., 'SRV')`.
`src/connection_string.ts`	Refactor DNS timeout retry helper to use `resolve(..., 'SRV'
`src/cmap/auth/gssapi.ts`	Replace `resolvePtr/resolveCname` with `resolve(..., 'PTR'
`test/unit/sdam/srv_polling.test.ts`	Update SRV polling DNS stubs/assertions for `resolve`.
`test/unit/connection_string.test.ts`	Update SRV/TXT stubbing to a single `dns.promises.resolve` stub with rrtype matching.
`test/unit/cmap/auth/gssapi.test.ts`	Update canonicalization tests to use `dns.resolve(..., rrtype)` spies/stubs.
`test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts`	Update SRV polling prose test to stub `dns.promises.resolve`.
`test/manual/kerberos.test.ts`	Update kerberos DNS assertions/stubs to use `dns.resolve(..., rrtype)` and `dns.lookup`.
`test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts`	Update seedlist discovery prose test to stub `dns.promises.resolve` for SRV/TXT.
`test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts`	Update DNS timeout tests to stub `dns.promises.resolve` with rrtype matching.

Comments suppressed due to low confidence (1)

test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts:298

sinon.stub(dns.promises, 'resolve') here intercepts all resolve() calls, including the driver's TXT lookup (resolve(host, 'TXT')) during SRV connection string processing. Since this stub always returns SRV records, TXT resolution will return the wrong shape and can break resolveSRVRecord. Prefer scoping the stub with withArgs(<hostname>, 'SRV') (and let 'TXT' call through or reject with { code: 'ENODATA' }).

      // first call is for the driver initial connection
      // second call will check the poller
      resolveSrvStub = sinon.stub(dns.promises, 'resolve').callsFake(async address => {
        expect(address).to.equal(`_${srvServiceName}._tcp.test.mock.test.build.10gen.cc`);
        if (initialDNSLookup) {
          initialDNSLookup = false;
          return initialRecords;
        }
        return replacementRecords;
      });

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T15:57:07Z

test/unit/sdam/srv_polling.test.ts


-        expect(dns.promises.resolveSrv).to.have.been.calledOnce.and.to.have.been.calledWith(
+        expect(dns.promises.resolve).to.have.been.calledOnce.and.to.have.been.calledWith(
          `_mongodb._tcp.${SRV_HOST}`


SrvPoller._poll() now calls dns.promises.resolve(this.srvAddress, 'SRV'), but this test asserts the call only received the hostname. Update the expectation (and optionally the stub) to include the 'SRV' rrtype argument so the assertion matches the real invocation.

Suggested change

`_mongodb._tcp.${SRV_HOST}`

`_mongodb._tcp.${SRV_HOST}`,

'SRV'

Copilot · 2026-03-05T15:57:08Z

test/unit/cmap/auth/gssapi.test.ts

                expect(dns.lookup).to.be.calledOnceWith(hostName);
-                expect(dns.resolvePtr).to.be.calledOnceWith(lookedUp.address);
-                expect(dns.resolveCname).to.not.be.called;
+                expect(dns.resolve).to.be.calledOnceWith(lookedUp.address, 'PTR');
+                expect(dns.resolve).to.not.be.calledOnceWith(lookedUp.address, 'CNAME');
              });


This assertion uses to.not.be.calledOnceWith(...), which can still pass even if dns.resolve was called with the forbidden args multiple times (it only asserts the call count isn't exactly 1). To guarantee no CNAME lookup happened here, assert dns.resolve was not called with (lookedUp.address, 'CNAME') (e.g., not.be.calledWith) or explicitly assert the only call was the PTR one.

Copilot · 2026-03-05T15:57:08Z

test/unit/cmap/auth/gssapi.test.ts

                expect(host).to.equal(resolved);
                expect(dns.lookup).to.be.calledOnceWith(hostName);
-                expect(dns.resolvePtr).to.be.calledOnceWith(lookedUp.address);
-                expect(dns.resolveCname).to.not.be.called;
+                expect(dns.resolve).to.be.calledOnceWith(lookedUp.address, 'PTR');
+                expect(dns.resolve).to.not.be.calledOnceWith(sinon.match.string, 'CNAME');
              });


Same issue as above: to.not.be.calledOnceWith(...) does not prove CNAME was never queried, only that it wasn't queried exactly once with those args. Use not.be.calledWith(..., 'CNAME') (or assert the call set explicitly) to make the test robust.

Copilot · 2026-03-05T15:57:08Z

test/manual/kerberos.test.ts

        );
        await client.connect();
-        expect(dns.resolveCname).to.be.calledOnceWith(host);
+        expect(resolveStub.withArgs(sinon.match.any, 'CNAME')).to.be.calledOnceWith(host);


resolveStub.withArgs(..., 'CNAME') matches calls like (host, 'CNAME'), so the assertion should include the rrtype argument (or assert on resolveStub directly). As written, calledOnceWith(host) will fail because the actual call includes a second 'CNAME' argument.

Suggested change

expect(resolveStub.withArgs(sinon.match.any, 'CNAME')).to.be.calledOnceWith(host);

expect(resolveStub.withArgs(sinon.match.any, 'CNAME')).to.be.calledOnceWith(host, 'CNAME');

Copilot · 2026-03-05T15:57:09Z

test/manual/kerberos.test.ts

          beforeEach(function () {
-            resolveCnameSpy.restore();
-            sinon.stub(dns, 'resolveCname').resolves([]);
+            resolveStub.withArgs(sinon.match.string, 'CNAME').rejects([]);


In the "cname lookup is empty" setup, the stub uses rejects([]), but an empty CNAME result should be a resolved value (resolves([])), not a rejected promise. Using rejects([]) changes the code path to the error/fallback behavior and also rejects with a non-Error value.

Suggested change

resolveStub.withArgs(sinon.match.string, 'CNAME').rejects([]);

resolveStub.withArgs(sinon.match.string, 'CNAME').resolves([]);

refactor(NODE-7313): only use dns.resolve for all types

4e89a87

durran force-pushed the NODE-7313 branch from c5c685e to 4e89a87 Compare February 3, 2026 12:19

durran and others added 8 commits February 4, 2026 13:21

test: stub only resolve

e024ec6

test: fix gssapi tests

3ade98f

test: fix initial dns seedlist test

38b46c1

update sinon stubs

9b5b0a8

make sure to check stubs with arguments

e7e43d3

run kerberos tests only

52aa2e9

fix kerberos tests

68008ba

run all tests

bb16669

tadjik1 marked this pull request as ready for review February 9, 2026 12:08

tadjik1 requested a review from a team as a code owner February 9, 2026 12:08

baileympearson requested changes Feb 9, 2026

View reviewed changes

tadjik1 added 3 commits February 17, 2026 12:56

address typescript type comment

481b7b1

fix typo in test

9a631da

avoid ts casting

c342496

tadjik1 requested review from PavelSafronov and baileympearson February 19, 2026 08:48

PavelSafronov approved these changes Feb 26, 2026

View reviewed changes

Merge branch 'main' into NODE-7313

d992590

dariakp requested a review from Copilot March 5, 2026 15:52

Copilot started reviewing on behalf of dariakp March 5, 2026 15:53 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

	`_mongodb._tcp.${SRV_HOST}`
	`_mongodb._tcp.${SRV_HOST}`,
	'SRV'

	expect(resolveStub.withArgs(sinon.match.any, 'CNAME')).to.be.calledOnceWith(host);
	expect(resolveStub.withArgs(sinon.match.any, 'CNAME')).to.be.calledOnceWith(host, 'CNAME');

	resolveStub.withArgs(sinon.match.string, 'CNAME').rejects([]);
	resolveStub.withArgs(sinon.match.string, 'CNAME').resolves([]);

Conversation

durran commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary of Changes

What is the motivation for this change?

Release Highlight

Release notes highlight

Double check the following

Uh oh!

baileympearson Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

tadjik1 Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

baileympearson Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

tadjik1 Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baileympearson Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

tadjik1 Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PavelSafronov left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

durran commented Jan 22, 2026 •

edited

Loading

tadjik1 Feb 18, 2026 •

edited

Loading