VSCODE-311: new data service and connection model #377

alenakhineika · 2021-12-16T15:45:08Z

Description

Switch to a new connection info format. The new data service is using a new connection info object that should be adopted by the extension and used to connect and save connections.

VSCode should start using a new connection info object for the saved connections including migration of previously saved connections to a new format and using the connection-secrets module from Compass to protect all secrets.

VSCODE-312 - Switch to a new data service and connection model
VSCODE-313 - Use the connection-secrets module to protect all secrets
VSCODE-314 - Transform previously saved connections to a new format

I also removed LSP inspector since the webview extension is buggy and doesn't work almost all the time.

It also fixes #383.

Checklist

New tests and/or benchmarks are included
Documentation is changed or added

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Dependents

Dependent on mongodb-js/compass#2650

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

…l.from

addaleax

Nice work! Took a first look at everything, but I think I’ll need to spend a bit more time before I can say that I fully follow the data flow everywhere :)

src/commands/launchMongoShell.ts

src/connectionController.ts

src/explorer/indexListTreeItem.ts

src/language/languageServerController.ts

src/language/mongoDBService.ts

…WithSsh

addaleax

Looks good! I left a few more comments, but they are mostly non-blocking, and in any case, nothing larger.

One thing I’m curious about: Is there a specfic reason for keeping the connections in memory in ConnectionController._connections, instead of always accessing them through the storage API? I’m not saying we should change this, I’m just wondering 🙂

src/storage/storageController.ts

src/telemetry/connectionTelemetry.ts

src/connectionController.ts

addaleax · 2021-12-20T17:29:21Z

src/connectionController.ts

-      newConnectionModel
-    );
+  parseNewConnection(rawConnectionModel: RawConnectionModel): ConnectionInfo {
+    const connectionModel = new ConnectionModel(rawConnectionModel);


I guess this is the part where we’re not sure about whether it’s alright to keep going with ConnectionModel? I would think it’s okay, yeah. We should probably just leave a ticket about eventually removing it. (But it might be nice if @Anemy could confirm this 🙂)

I think it's fine yup. It's probably not worth adding all the logic to build the connectionInfo from the connection configuration object the form in the webview creates. Would be nice to remove this usage of connection-model eventually, and this would definitely be something that sharing the connect form would help make go away as well.

src/views/webviewController.ts

src/connectionController.ts

Anemy · 2021-12-20T22:17:56Z

src/connectionController.ts

-            return 'MongoDB connection strings begin with "mongodb://" or "mongodb+srv://"';
+          try {
+            // eslint-disable-next-line no-new
+            new ConnectionString(uri);


I like this change, but I'm wondering if the old one, although not as good of a validation check, might give a user unsure what to enter in something more information to help their case.
Mostly if they input something like a url but is invalid they'd get an Invalid connection string error message which doesn't hint at what might go there. https://github.com/mongodb-js/mongodb-connection-string-url/blob/main/src/index.ts#L106
Maybe something for that package instead of here. We do have a placeholder here that should give those users something to go off of so this may be a non-issue.

@addaleax what do you think?

I can definitely see the concern here – if you want to keep the previous check additionally (i.e. add

if (!uri.startsWith('mongodb://') && !uri.startsWith('mongodb+srv://')) { return 'MongoDB connection strings begin with "mongodb://" or "mongodb+srv://"';

before the new ConnectionString check), I think that’s okay and won’t make anything worse, but I also honestly think that this isn’t something we need to worry about a ton, and I do agree with @Anemy that it would be a bit nicer to improve the error message inside the connection string url package as a long-term solution 🙂

alenakhineika · 2021-12-20T23:05:19Z

@addaleax Answering your question why we keep connections in memory: first of all, I didn't change this logic in this pr, this worked like this before. But was thinking about it while refactoring and kept like this because of two reasons:

to fetch all connections at once with secrets to migrate them to a new format and then use them to seamlessly switch between connections without accessing a keychain, and allow users to copy URLs for connections on the sidebar also without accessing the keychain.
In the scope, we defined a goal that Compass and VSCodeshould use a single shared entry in the OS keychain for all saved connections. It means that the password to the keychain will be requested only once and then used for all saved connections. So in the future, we also will fetch everything at once and I believe store it in memory to not reenter the password for the keychain, therefore it felt like a good idea to keep them in memory for now as well.

We also discusses it with Maurizio and as I remember he was also up to pulling them all together. I am open here for a discussion if you have a strong opinion to do it differently!

Anemy

nice work - big changes that touch almost all the extension. Solid cleanup on the language server.

Anemy · 2022-01-07T13:12:06Z

src/explorer/connectionTreeItem.ts

@@ -106,44 +80,18 @@ export default class ConnectionTreeItem extends vscode.TreeItem
    return element;
  }

-  async listDatabasesUserHasAccessTo(


Cool that we're sharing this with compass now 👍

Yes! I was very excited to see this change in compass and know the fact that we can reuse it in vscode!

alenakhineika added 3 commits December 15, 2021 22:09

refactor: switch to new data service and connection model VSCODE-312

79e2e83

feat: handle secrets and migrate old connections VSCODE-313, VSCODE-314

066fae1

fix: set default objects for storages

0165fd4

alenakhineika added the WIP label Dec 16, 2021

alenakhineika added 2 commits December 16, 2021 20:49

test: mock connection model

e54503a

refactor: use mongodb-connection-string-url instead of connectionMode…

ef0a91c

…l.from

alenakhineika added WIP and removed WIP labels Dec 16, 2021

fix: keep all connection options from new connection model

3ddf407

alenakhineika removed the WIP label Dec 17, 2021

addaleax reviewed Dec 17, 2021

View reviewed changes

alenakhineika added the WIP label Dec 17, 2021

alenakhineika added 3 commits December 18, 2021 01:05

refactor: give connectionString to users to copy instead of driverUrl…

d687679

…WithSsh

refactor: use getMongoClientConnectionOptions to launch shell

f445cce

refactor: adress pr comments

e2b1f36

alenakhineika removed the WIP label Dec 20, 2021

addaleax approved these changes Dec 20, 2021

View reviewed changes

Anemy reviewed Dec 20, 2021

View reviewed changes

refactor: code style cleaning up

ad3bc19

alenakhineika mentioned this pull request Dec 21, 2021

feat: export secrets methods and parse raw models COMPASS-5378 mongodb-js/compass#2650

Merged

10 tasks

refactor: pull secrets methods from data service

c9570f4

alenakhineika requested a review from Anemy December 21, 2021 16:30

refactor: convertConnectionModelToInfo accepts raw models now

a8e614a

Anemy approved these changes Jan 5, 2022

View reviewed changes

fix: copy connection string without appname

5e5172d

Anemy reviewed Jan 7, 2022

View reviewed changes

alenakhineika added 2 commits January 7, 2022 14:59

fix: do not repeat indexes error for each expand action in the sidebar

7fa2ff5

fix: change kerberosCanonicalizeHostname to boolean

74c69b0

alenakhineika merged commit ff15215 into main Jan 7, 2022

alenakhineika deleted the VSCODE-312-switch-to-new-data-service-and-model-v2 branch January 7, 2022 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VSCODE-311: new data service and connection model #377

VSCODE-311: new data service and connection model #377

alenakhineika commented Dec 16, 2021 •

edited

Loading

addaleax left a comment

addaleax left a comment

addaleax Dec 20, 2021

Anemy Dec 20, 2021

Anemy Dec 20, 2021

alenakhineika Dec 21, 2021 •

edited

Loading

addaleax Dec 21, 2021

alenakhineika commented Dec 20, 2021 •

edited

Loading

Anemy left a comment

Anemy Jan 7, 2022

alenakhineika Jan 7, 2022

VSCODE-311: new data service and connection model #377

VSCODE-311: new data service and connection model #377

Conversation

alenakhineika commented Dec 16, 2021 • edited Loading

Description

Checklist

Motivation and Context

Dependents

Types of changes

addaleax left a comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

addaleax Dec 20, 2021

Choose a reason for hiding this comment

Anemy Dec 20, 2021

Choose a reason for hiding this comment

Anemy Dec 20, 2021

Choose a reason for hiding this comment

alenakhineika Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

addaleax Dec 21, 2021

Choose a reason for hiding this comment

alenakhineika commented Dec 20, 2021 • edited Loading

Anemy left a comment

Choose a reason for hiding this comment

Anemy Jan 7, 2022

Choose a reason for hiding this comment

alenakhineika Jan 7, 2022

Choose a reason for hiding this comment

alenakhineika commented Dec 16, 2021 •

edited

Loading

alenakhineika Dec 21, 2021 •

edited

Loading

alenakhineika commented Dec 20, 2021 •

edited

Loading