feat: support getCollection for names and shell methods

[forivall]

Currently this implementation is unpolished, but it works fine in my local install.

Description

Parsed out calls to getCollection so that they will also be given completion. Also, detect when the collection is being typed out as a string so that completion works properly.

Checklist

I'll do these if the feature is desired.

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

Motivation and Context

The default "Search For Documents" option on a collection creates a playground document with db.getCollection('collection_name'), but the language service only provides completion when accessing the collection like db.collection_name.

• [x] Bugfix
• [x] New feature
• [ ] Dependency update
• [ ] Misc

Open Questions

I'm not sure about the names of the props in the CompletionState, so some feedback there would be appreciated.

The tests to filter on the correct AST node generate high complexity in the eslint complexity check, so I did a bit of refactoring. Methods could be cleaned up in general. Not sure how far i should go, or the style i should use in appeasing eslint there.

The commits could also be split up a bit more. Let me know if you want that.

In the longer term, I'd suggest moving towards using, or at least supporting a typescript plugin, but that's out of scope for this simpler change. I'd also like editor suggestions for query parameters, but that might be more maintainable in typescript definitions. At least, I'll create a feature request for it. I saw that there's definitions for those completions in mongodb-ace-autocompleter, but the operators dont have the same links to documentation that the aggregation pipeline stages do.

Dependents

Types of changes

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

[alenakhineika]

Hey @forivall, thank you for opening the PR! This solution can work, but as you mentioned, chosen names and the visiting flow are a bit confusing. For example, you introduce the isProperty argument that you check later inside the if statement:

if (!isProperty || this._isValidPropertyName(item.name)) {}

So people could read as "if this is not a property but a valid property name". Hard to guess what to expect here, right?

In the visitor itself, you introduce an extra complexity of identifying your node. You want to check if you are inside a string literal (by the way you should also check a string template here), and this is a call expressing with the getCollection property name. But to get this information, you change other not related visitors, like shell methods and db call (what actually breaks a shell completion case and is caught by one of the tests).

You also add the isCollectionString state, again the name is misleading and we don't really need to export this info from the visitor.

You could consider taking the _checkIsUseCall as an example and build the _checkIsGetCollectionCall visitor. To find your node you can check if it has the getCollection property name and the string argument includes a trigger character (see the _checkIsGetCollectionCall function as an example).

The completion tests live in the ./src/test/suite/language/mongoDBService.test.ts file. The broken test I have mentioned above is one of those. When you finish your implementation, please add additional tests to this file to check a new completion use case.

Feel free to reach out with questions! Sorry for such a later response this time, we will be faster next time :)

[mmarcon]

Hi @forivall! First of all, thank you again for your contribution!

I would love to get this merged: were you planning to rework this PR based on @alenakhineika's recommendations above?

Also, if you do intend to get this PR merged, could you please sign our contributor's agreement if you have not done so already?

Thank you!

[forivall]

Sure, I do still want to rework the PR, I think i just forgot about this one as my day-to-day shifted off of working directly with the mongo shell.

[alenakhineika]

Hey @forivall, thank you for putting in the effort to bring this feature to life. However, due to timing constraints, we had to take over and implement it ourselves as part of the other visitor module improvements. We will be happy to see more PRs from you in the future!

[forivall]

no worries; yeah, while it was on my mind, i just didnt have the time to get to it on any of my weekends, as this wasn't pressing on my day-to-day work.