Skip to content

Conversation

gagik
Copy link
Collaborator

@gagik gagik commented Sep 19, 2025

Updates the mongosh service provider. Among other things, this allows us to no longer be broken with mongosh VSCode tests and keep alignment.

Needed for mongodb-js/vscode#1130

Updates the mongosh service provider. Among other things, this allows us to no longer be broken with mongosh VSCode tests and keep alignment.
@gagik gagik requested a review from a team as a code owner September 19, 2025 12:26
@Copilot Copilot AI review requested due to automatic review settings September 19, 2025 12:26
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the MongoDB shell dependencies to newer versions to maintain compatibility with VS Code MongoDB extension tests and keep alignment between projects.

  • Updates @mongosh/arg-parser from version 3.14.0 to 3.19.0
  • Updates @mongosh/service-provider-node-driver from version 3.12.0 to 3.17.0
  • Removes the boolean parameter from the close() method call to match the updated API

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
package.json Updates mongosh dependency versions to newer releases
src/common/connectionManager.ts Updates service provider close method call to match new API signature

@gagik gagik changed the title chore: bump @mongosh dependencies chore: bump @mongosh, mongodb dependencies Sep 19, 2025
@kmruiz
Copy link
Collaborator

kmruiz commented Sep 19, 2025

I see that with this change the OIDC tests are failing with a timeout. Maybe there is a breaking change somewhere in the node provider? I'll take a look too and see if we can get it working.

@gagik gagik changed the title chore: bump @mongosh, mongodb dependencies chore: bump @mongosh dependencies Sep 19, 2025
@gagik
Copy link
Collaborator Author

gagik commented Sep 19, 2025

I see that with this change the OIDC tests are failing with a timeout

That seems to be a flake, so it's possible some change makes it more likely. Oh looks like it's Ubuntu-related, interesting. Will need to look into it too.

I think devtools connect bump should fix it.

@gagik gagik marked this pull request as draft September 19, 2025 12:39
@kmruiz
Copy link
Collaborator

kmruiz commented Sep 19, 2025

Yeah, OIDC tests only run on Linux (because the server only supports it in Linux) so if you see an ubuntu only issue, it's likely OIDC.

I'm debugging and it seems that the issue is that the OIDC flow never finishes, the connection manager stays on "connecting", like when the user never confirms the authentication.

package.json Outdated
"@mongodb-js/devtools-proxy-support": "^0.5.3",
"@mongosh/arg-parser": "^3.19.0",
"@mongosh/service-provider-node-driver": "^3.17.0",
"@vitest/eslint-plugin": "^1.3.4",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

drive by 🙈

@kmruiz
Copy link
Collaborator

kmruiz commented Sep 19, 2025

I've actually found the breaking change. I'm trying to fix it now.

It seems that there has been a change in the node provider that now opens the connection eagerly, instead of lazily, and this breaks our flow.

…ntract

NodeDriverServiceProvider.connect would return a Promise that was
lazily connected. However, now, blocks until there is a ping from the
server, forcing a connection.

This would usually be fine, however, in OIDC it would mean blocking
the whole MCP flow until the OIDC flow is connected. With the previous
version, this was already handled, but now we need to split the logic
a bit more to ensure that OIDC flows stay async.
@kmruiz kmruiz requested a review from Copilot September 19, 2025 16:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

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

Copy link
Collaborator

@kmruiz kmruiz left a comment

Choose a reason for hiding this comment

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

After the fixes in the ConnectionManager, it seems that everything works fine again.

We'll likely want someone else to test this, but I'm approving the PR from my side!

Copy link
Collaborator

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Looks reasonable, though I'm not fully sure why we needed the code rearrangement in connection manager.

name: "switch-connection",
arguments: {
connectionString: "mongodb://localhost:12345",
connectionString: "mangobd://localhost:12345",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the change here? I don't suppose there's anything on port 12345 anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was testing something different: mongodb://localhost:12345 is not an invalid URL, it's just an URL where there is no MongoDB cluster.

Comment on lines 193 to 201
if (isOidcConnection) {
this.changeState("connection-request", {
tag: "connecting",
serviceProvider,
connectedAtlasCluster: settings.atlas,
connectionStringAuthType,
oidcConnectionType: connectionStringAuthType as OIDCConnectionAuthType,
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this was moved up here - it seems like we're returning it immediately a little bit later anyway.

@gagik gagik marked this pull request as ready for review September 23, 2025 08:44
@gagik
Copy link
Collaborator Author

gagik commented Sep 23, 2025

Tested the new connections on VSCode and works well. Thank you @kmruiz for looking into this!

@gagik gagik merged commit 2d3abee into main Sep 23, 2025
16 of 17 checks passed
@gagik gagik deleted the gagik/mongosh-bump branch September 23, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants