Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
durran wants to merge 12 commits into
base: main
Choose a base branch
from
+150 −162
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4e89a87
refactor(NODE-7313): only use dns.resolve for all types
durran Jan 22, 2026
e024ec6
test: stub only resolve
durran Feb 4, 2026
3ade98f
test: fix gssapi tests
durran Feb 4, 2026
38b46c1
test: fix initial dns seedlist test
durran Feb 4, 2026
9b5b0a8
update sinon stubs
tadjik1 Feb 6, 2026
e7e43d3
make sure to check stubs with arguments
tadjik1 Feb 6, 2026
52aa2e9
run kerberos tests only
tadjik1 Feb 6, 2026
68008ba
fix kerberos tests
tadjik1 Feb 6, 2026
bb16669
run all tests
tadjik1 Feb 9, 2026
481b7b1
address typescript type comment
tadjik1 Feb 17, 2026
9a631da
fix typo in test
tadjik1 Feb 18, 2026
c342496
avoid ts casting
tadjik1 Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/cmap/auth/gssapi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ export async function performGSSAPICanonicalizeHostName(

try {
// Perform a reverse ptr lookup on the ip address.
const results = await dns.promises.resolvePtr(address);
const results = await dns.promises.resolve(address, 'PTR');
// If the ptr did not error but had no results, return the host.
return results.length > 0 ? results[0] : host;
} catch {
Expand All @@ -185,7 +185,7 @@ export async function performGSSAPICanonicalizeHostName(
export async function resolveCname(host: string): Promise<string> {
// Attempt to resolve the host name
try {
const results = await dns.promises.resolveCname(host);
const results = await dns.promises.resolve(host, 'CNAME');
// Get the first resolved host id
return results.length > 0 ? results[0] : host;
} catch {
Expand Down
20 changes: 12 additions & 8 deletions src/connection_string.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,26 +41,30 @@ const LB_REPLICA_SET_ERROR = 'loadBalanced option not supported with a replicaSe
const LB_DIRECT_CONNECTION_ERROR =
'loadBalanced option not supported when directConnection is provided';

function retryDNSTimeoutFor(api: 'resolveSrv'): (a: string) => Promise<dns.SrvRecord[]>;
function retryDNSTimeoutFor(api: 'resolveTxt'): (a: string) => Promise<string[][]>;
function retryDNSTimeoutFor(rrtype: 'SRV'): (lookupAddress: string) => Promise<dns.SrvRecord[]>;
function retryDNSTimeoutFor(rrtype: 'TXT'): (lookupAddress: string) => Promise<string[][]>;
function retryDNSTimeoutFor(
api: 'resolveSrv' | 'resolveTxt'
): (a: string) => Promise<dns.SrvRecord[] | string[][]> {
rrtype: 'SRV' | 'TXT'
): (lookupAddress: string) => Promise<dns.SrvRecord[] | string[][]> {
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 dns.promises[api](lookupAddress);
return await resolve(lookupAddress);
} catch (firstDNSError) {
if (firstDNSError.code === dns.TIMEOUT) {
return await dns.promises[api](lookupAddress);
return await resolve(lookupAddress);
} else {
throw firstDNSError;
}
}
};
}
Comment on lines 53 to 64
Copy link
Contributor

@baileympearson baileympearson Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tadjik1 tadjik1 Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@baileympearson baileympearson Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@tadjik1 tadjik1 Feb 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@baileympearson baileympearson Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tadjik1 tadjik1 Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const resolveSrv = retryDNSTimeoutFor('resolveSrv');
const resolveTxt = retryDNSTimeoutFor('resolveTxt');
const resolveSrv = retryDNSTimeoutFor('SRV');
const resolveTxt = retryDNSTimeoutFor('TXT');

/**
* Lookup a `mongodb+srv` connection string, combine the parts and reparse it as a normal
Expand Down
2 changes: 1 addition & 1 deletion src/sdam/srv_polling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export class SrvPoller extends TypedEventEmitter<SrvPollerEvents> {
let srvRecords;

try {
srvRecords = await dns.promises.resolveSrv(this.srvAddress);
srvRecords = await dns.promises.resolve(this.srvAddress, 'SRV');
} catch {
this.failure();
return;
Expand Down
79 changes: 27 additions & 52 deletions test/integration/initial-dns-seedlist-discovery/dns_seedlist.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ describe('DNS timeout errors', () => {

beforeEach(async function () {
client = new MongoClient(CONNECTION_STRING, { serverSelectionTimeoutMS: 2000, tls: false });
stub = sinon.stub(dns.promises, 'resolve').callThrough();
});

afterEach(async function () {
Expand All @@ -31,134 +32,108 @@ describe('DNS timeout errors', () => {
await client.close();
});

const restoreDNS =
api =>
async (...args) => {
sinon.restore();
return await dns.promises[api](...args);
};
const restoreDNS = rrtype => async hostname => {
stub.restore();
return await dns.promises.resolve(hostname, rrtype);
};

describe('when SRV record look up times out', () => {
beforeEach(() => {
stub = sinon
.stub(dns.promises, 'resolveSrv')
stub
.withArgs(sinon.match.string, 'SRV')
.onFirstCall()
.rejects(new DNSTimeoutError())
.onSecondCall()
.callsFake(restoreDNS('resolveSrv'));
});

afterEach(async function () {
sinon.restore();
.callsFake(restoreDNS('SRV'));
});

it('retries timeout error', metadata, async () => {
await client.connect();
expect(stub).to.have.been.calledTwice;
expect(stub.withArgs(sinon.match.string, 'SRV')).to.have.been.calledTwice;
});
});

describe('when TXT record look up times out', () => {
beforeEach(() => {
stub = sinon
.stub(dns.promises, 'resolveTxt')
stub
.withArgs(sinon.match.string, 'TXT')
.onFirstCall()
.rejects(new DNSTimeoutError())
.onSecondCall()
.callsFake(restoreDNS('resolveTxt'));
});

afterEach(async function () {
sinon.restore();
.callsFake(restoreDNS('TXT'));
});

it('retries timeout error', metadata, async () => {
await client.connect();
expect(stub).to.have.been.calledTwice;
expect(stub.withArgs(sinon.match.string, 'TXT')).to.have.been.calledTwice;
});
});

describe('when SRV record look up times out twice', () => {
beforeEach(() => {
stub = sinon
.stub(dns.promises, 'resolveSrv')
stub
.withArgs(sinon.match.string, 'SRV')
.onFirstCall()
.rejects(new DNSTimeoutError())
.onSecondCall()
.rejects(new DNSTimeoutError());
});

afterEach(async function () {
sinon.restore();
});

it('throws timeout error', metadata, async () => {
const error = await client.connect().catch(error => error);
expect(error).to.be.instanceOf(DNSTimeoutError);
expect(stub).to.have.been.calledTwice;
expect(stub.withArgs(sinon.match.string, 'SRV')).to.have.been.calledTwice;
});
});

describe('when TXT record look up times out twice', () => {
beforeEach(() => {
stub = sinon
.stub(dns.promises, 'resolveTxt')
stub
.withArgs(sinon.match.string, 'TXT')
.onFirstCall()
.rejects(new DNSTimeoutError())
.onSecondCall()
.rejects(new DNSTimeoutError());
});

afterEach(async function () {
sinon.restore();
});

it('throws timeout error', metadata, async () => {
const error = await client.connect().catch(error => error);
expect(error).to.be.instanceOf(DNSTimeoutError);
expect(stub).to.have.been.calledTwice;
expect(stub.withArgs(sinon.match.string, 'TXT')).to.have.been.calledTwice;
});
});

describe('when SRV record look up throws a non-timeout error', () => {
beforeEach(() => {
stub = sinon
.stub(dns.promises, 'resolveSrv')
stub
.withArgs(sinon.match.string, 'SRV')
.onFirstCall()
.rejects(new DNSSomethingError())
.onSecondCall()
.callsFake(restoreDNS('resolveSrv'));
});

afterEach(async function () {
sinon.restore();
.callsFake(restoreDNS('SRV'));
});

it('throws that error', metadata, async () => {
const error = await client.connect().catch(error => error);
expect(error).to.be.instanceOf(DNSSomethingError);
expect(stub).to.have.been.calledOnce;
expect(stub.withArgs(sinon.match.string, 'SRV')).to.have.been.calledOnce;
});
});

describe('when TXT record look up throws a non-timeout error', () => {
beforeEach(() => {
stub = sinon
.stub(dns.promises, 'resolveTxt')
stub
.withArgs(sinon.match.string, 'TXT')
.onFirstCall()
.rejects(new DNSSomethingError())
.onSecondCall()
.callsFake(restoreDNS('resolveTxt'));
});

afterEach(async function () {
sinon.restore();
.callsFake(restoreDNS('TXT'));
});

it('throws that error', metadata, async () => {
const error = await client.connect().catch(error => error);
expect(error).to.be.instanceOf(DNSSomethingError);
expect(stub).to.have.been.calledOnce;
expect(stub.withArgs(sinon.match.string, 'TXT')).to.have.been.calledOnce;
});
});
});
35 changes: 22 additions & 13 deletions test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
beforeEach(async function () {
// this fn stubs DNS resolution to always pass - so we are only checking pre-DNS validation

sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
const stub = sinon.stub(dns.promises, 'resolve');
stub.withArgs(sinon.match.string, 'SRV').callsFake(async () => {
return [
{
name: 'resolved.mongo.localhost',
Expand All @@ -34,7 +35,7 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
];
});

sinon.stub(dns.promises, 'resolveTxt').callsFake(async () => {
stub.withArgs(sinon.match.string, 'TXT').callsFake(async () => {
throw { code: 'ENODATA' };
});

Expand Down Expand Up @@ -84,9 +85,11 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
* - the SRV mongodb+srv://blogs.mongodb.com resolving to blogs.evil.com
* Remember, the domain of an SRV with one or two . separated parts is the SRVs entire hostname.
*/
let stub;

beforeEach(async function () {
sinon.stub(dns.promises, 'resolveTxt').callsFake(async () => {
stub = sinon.stub(dns.promises, 'resolve');
stub.withArgs(sinon.match.string, 'TXT').callsFake(async () => {
throw { code: 'ENODATA' };
});
});
Expand All @@ -96,7 +99,7 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
});

it('an SRV with one domain level causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
stub.withArgs(sinon.match.string, 'SRV').callsFake(async () => {
return [
{
name: 'localhost.mongodb',
Expand All @@ -115,7 +118,7 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
});

it('an SRV with two domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
stub.withArgs(sinon.match.string, 'SRV').callsFake(async () => {
return [
{
name: 'test_1.evil.local', // this string only ends with part of the domain, not all of it!
Expand All @@ -134,7 +137,7 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
});

it('an SRV with three or more domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
stub.withArgs(sinon.match.string, 'SRV').callsFake(async () => {
return [
{
name: 'blogs.evil.com',
Expand Down Expand Up @@ -166,8 +169,11 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
context(
'when given a host from DNS resolution that is identical to the original SRVs hostname',
function () {
let stub;

beforeEach(async function () {
sinon.stub(dns.promises, 'resolveTxt').callsFake(async () => {
stub = sinon.stub(dns.promises, 'resolve');
stub.withArgs(sinon.match.string, 'TXT').callsFake(async () => {
throw { code: 'ENODATA' };
});
});
Expand All @@ -177,7 +183,7 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
});

it('an SRV with one domain level causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
stub.withArgs(sinon.match.string, 'SRV').callsFake(async () => {
return [
{
name: 'localhost',
Expand All @@ -198,7 +204,7 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
});

it('an SRV with two domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
stub.withArgs(sinon.match.string, 'SRV').callsFake(async () => {
return [
{
name: 'mongo.local',
Expand Down Expand Up @@ -233,8 +239,11 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
context(
'when given a returned address that does NOT share the domain name of the SRV record because its missing a `.`',
function () {
let stub;

beforeEach(async function () {
sinon.stub(dns.promises, 'resolveTxt').callsFake(async () => {
stub = sinon.stub(dns.promises, 'resolve');
stub.withArgs(sinon.match.string, 'TXT').callsFake(async () => {
throw { code: 'ENODATA' };
});
});
Expand All @@ -244,7 +253,7 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
});

it('an SRV with one domain level causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
stub.withArgs(sinon.match.string, 'SRV').callsFake(async () => {
return [
{
name: 'test_1.cluster_1localhost',
Expand All @@ -263,7 +272,7 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
});

it('an SRV with two domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
stub.withArgs(sinon.match.string, 'SRV').callsFake(async () => {
return [
{
name: 'test_1.my_hostmongo.local',
Expand All @@ -282,7 +291,7 @@ describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
});

it('an SRV with three domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
stub.withArgs(sinon.match.string, 'SRV').callsFake(async () => {
return [
{
name: 'cluster.testmongodb.com',
Expand Down
Loading