refactor(NODE-7313): only use dns.resolve for all types#4846
Merged
PavelSafronov merged 15 commits intomainfrom Mar 16, 2026
Merged
refactor(NODE-7313): only use dns.resolve for all types#4846PavelSafronov merged 15 commits intomainfrom
PavelSafronov merged 15 commits intomainfrom
Conversation
baileympearson
previously requested changes
Feb 9, 2026
PavelSafronov
previously approved these changes
Feb 26, 2026
Contributor
PavelSafronov
left a comment
PavelSafronov
left a comment
There was a problem hiding this comment.
Changes look good.
Contributor
There was a problem hiding this comment.
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 ofresolveSrv. - 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 allresolve()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 breakresolveSRVRecord. Prefer scoping the stub withwithArgs(<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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
nbbeeken
approved these changes
Mar 13, 2026
PavelSafronov
approved these changes
Mar 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactors all dns resolution to only go through
lookupandresolve.Summary of Changes
lookupandresolvesWhat is the motivation for this change?
NODE-7313
Release Highlight
Use
dns.resolvemethod for all types of DNS resolutionThe driver now uses the method
dns.resolve(hostname, rrtype)instead of multipledns.resolveCname/dns.resolvePtrmethods. This reduces the number of methods that need to be supplied to the client.Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript