Skip to content

feat(devtools-connect): make dns lookup configurable COMPASS-9793 #602

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

Merged
nbbeeken merged 2 commits into from
Dec 3, 2025
Merged

feat(devtools-connect): make dns lookup configurable COMPASS-9793 #602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc35af0
feat(devtools-connect): make lookup configurable
nbbeeken Dec 3, 2025
37c69c9
chore: ignore depcheck on ts-node
nbbeeken Dec 3, 2025
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
1 change: 1 addition & 0 deletions packages/devtools-connect/.depcheckrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ ignores:
# this is explicitly called as a dependency to make sure that is bootstrapped
# before mongodb-runner, since it is required via mongodb
- '@mongodb-js/saslprep'
- 'ts-node' # used by mocha
Copy link
Collaborator Author

@nbbeeken nbbeeken Dec 3, 2025

Choose a reason for hiding this comment

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

I guess this relates to the -r change, but not sure how.

ignore-patterns:
- 'dist'
2 changes: 1 addition & 1 deletion packages/devtools-connect/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
"depcheck": "depcheck",
"check": "npm run typecheck && npm run lint && npm run depcheck",
"check-ci": "npm run check",
"testonly": "nyc mocha --colors -r ts-node/register src/**/*.spec.ts",
"testonly": "nyc mocha --colors --register ts-node/register src/**/*.spec.ts",
Copy link
Collaborator Author

@nbbeeken nbbeeken Dec 3, 2025

Choose a reason for hiding this comment

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

Assertions were not pointing to the correct line in the tests without this change and the debugger would not stop on the breakpoint lines. -r is mocha's "require", not sure if that's the same as node's 🤔

"test": "npm run lint && npm run compile && npm run testonly",
"test-cov": "nyc -x \"**/*.spec.*\" --reporter=lcov --reporter=text --reporter=html npm run test",
"test-watch": "npm run test -- --watch",
Expand Down
24 changes: 24 additions & 0 deletions packages/devtools-connect/src/connect.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,30 @@ describe('devtools connect', function () {
expect(result.client).to.equal(mClient);
});

it('allows lookup to be configured', async function () {
const uri = 'localhost:27017';
const mClient = stubConstructor(FakeMongoClient);
const mClientType = sinon.stub().returns(mClient);
mClient.connect.onFirstCall().resolves(mClient);
const lookupSpy = sinon.spy();
await connectMongoClient(
uri,
{ ...defaultOpts, useSystemCA: false, lookup: lookupSpy },
bus,
mClientType as any,
);

// get options passed to mongo client
const finalClientOptions = mClientType.getCalls()[0].args[1];
expect(finalClientOptions.lookup).to.not.equal(lookupSpy); // lookup is always wrapped
finalClientOptions.lookup('localhost', 27017, () => {}); // invoke it the way it would be by net/tls
expect(lookupSpy.getCalls()).to.have.lengthOf(1); // verify our input lookup was called instead of the dns package
Comment on lines +431 to +432
Copy link
Collaborator Author

@nbbeeken nbbeeken Dec 3, 2025

Choose a reason for hiding this comment

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

bit of an odd test but since we're mocking things we don't actually get to the point where the driver would cause lookup to be invoked, by calling it here I'm checking that our input is wrapped rather than used directly.

expect(lookupSpy.getCalls()[0].args[1]).to.have.property(
'verbatim',
false,
); // but we still always set verbatim to false
});

describe('retryable TLS errors', function () {
it('retries TLS errors without system CA integration enabled -- MongoClient error', async function () {
const uri = 'localhost:27017';
Expand Down
7 changes: 6 additions & 1 deletion packages/devtools-connect/src/connect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,10 +588,15 @@ async function connectMongoClientImpl({
ca ? { ca } : {},
);

const customLookup = mongoClientOptions.lookup;
// Adopt dns result order changes with Node v18 that affected the VSCode extension VSCODE-458.
// Refs https://github.com/microsoft/vscode/issues/189805
mongoClientOptions.lookup = (hostname, options, callback) => {
return dns.lookup(hostname, { verbatim: false, ...options }, callback);
return (customLookup ?? dns.lookup)(
hostname,
{ verbatim: false, ...options },
callback,
);
Comment on lines +591 to +599
Copy link
Collaborator Author

@nbbeeken nbbeeken Dec 3, 2025

Choose a reason for hiding this comment

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

Suggested change
const customLookup = mongoClientOptions.lookup;
// Adopt dns result order changes with Node v18 that affected the VSCode extension VSCODE-458.
// Refs https://github.com/microsoft/vscode/issues/189805
mongoClientOptions.lookup = (hostname, options, callback) => {
return dns.lookup(hostname, { verbatim: false, ...options }, callback);
return (customLookup ?? dns.lookup)(
hostname,
{ verbatim: false, ...options },
callback,
);
// Adopt dns result order changes with Node v18 that affected the VSCode extension VSCODE-458.
// Refs https://github.com/microsoft/vscode/issues/189805
mongoClientOptions.lookup ??= (hostname, options, callback) => {
return dns.lookup(hostname, { verbatim: false, ...options }, callback);

This could also just be ??= if we want to use whatever lookup we are given, but it seems more correct to me that this package owns its opinion on the verbatim flag

Copy link
Collaborator

@addaleax addaleax Dec 3, 2025

Choose a reason for hiding this comment

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

Maybe also link to NODE-6437 given that passing the flag is a way to work around shortcomings of the driver, not something we should be doing here

};

delete (mongoClientOptions as any).useSystemCA; // can be removed once no product uses this anymore
Expand Down
Loading