-
Notifications
You must be signed in to change notification settings - Fork 79
feat: allow opting out of schema sampling with config disableSchemaSampling=true MONGOSH-2370 #2503
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.
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
Conversation
There was a problem hiding this 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 adds a new disableSchemaSampling
user configuration flag to opt out of sampling collection schemas when using the new autocomplete feature, and wires it through the shell state, REPL initialization, autocomplete entry point, and tests.
- Introduce
disableSchemaSampling
inCliUserConfig
and its validator - Update
ShellInstanceState.getAutocompletionContext
and the autocomplete initializer to respect the new flag - Pass the flag from the REPL, and add tests to cover enabling/disabling schema sampling
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/types/src/index.ts | Add disableSchemaSampling property and validation |
packages/shell-api/src/shell-instance-state.ts | Wire flag into getAutocompletionContext logic |
packages/cli-repl/src/mongosh-repl.ts | Forward config to initNewAutocompleter |
packages/cli-repl/src/mongosh-repl.spec.ts | Test behavior when sampling is disabled/enabled |
packages/autocomplete/src/index.ts | Accept and pass through the new option |
Comments suppressed due to low confidence (2)
packages/types/src/index.ts:523
- [nitpick] Add a JSDoc comment describing what
disableSchemaSampling
does (e.g. "If true, skips querying server for schema samples in autocomplete").
disableSchemaSampling = false;
packages/shell-api/src/shell-instance-state.ts:418
- [nitpick] Consider updating the method’s docstring or interface comment to explain the new
disableSchemaSampling
parameter and its default behavior.
public getAutocompletionContext({
|
||
// TODO: we need MQL support in mongodb-ts-autocomplete in order for it | ||
// to autocomplete collection field names | ||
wantQueryOperators = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by but I realised this test should actually be passing by now (with the env var set, obviously
@@ -2509,9 +2510,25 @@ describe('CliRepl', function () { | |||
if (!(testServer as any)?._opts.args?.includes('--auth')) { | |||
// make sure there are some collections we can autocomplete on below | |||
input.write('db.coll.insertOne({})\n'); | |||
input.write('db.movies.insertOne({})\n'); | |||
input.write('db.movies.insertOne({ year: 1 })\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I haven't really given much thought before, but the mql library won't autocomplete under fields that it doesn't know about:
const query: Query<{/* empty schema */}> = {
year: {
$g // you can't auto-complete here
}
}
@@ -71,10 +71,7 @@ async function waitEval(bus: MongoshBus) { | |||
} | |||
|
|||
async function waitCompletion(bus: MongoshBus) { | |||
await Promise.race([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This timeout was just always going to be flaky and pressing tab twice doesn't necessarily mean it completed a potentially slow operation anyway. tabtab() always relied on 2 * 250ms being enough.
@@ -510,6 +505,8 @@ class MongoshNodeRepl implements EvaluationListener { | |||
results = results.filter( | |||
(result) => !CONTROL_CHAR_REGEXP.test(result) | |||
); | |||
// emit here so that on nextTick the results should be output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG. autocomplete became async and there's another await.
@@ -293,7 +293,9 @@ export async function completer( | |||
// from https://github.com/mongodb-js/devtools-shared/commit/e4a5b00a83b19a76bdf380799a421511230168db | |||
function satisfiesVersion(v1: string, v2: string): boolean { | |||
const isGTECheck = /^\d+?\.\d+?\.\d+?$/.test(v2); | |||
return semver.satisfies(v1, isGTECheck ? `>=${v2}` : v2); | |||
return semver.satisfies(v1, isGTECheck ? `>=${v2}` : v2, { | |||
includePrerelease: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> semver.satisfies('8.2.0-alpha-2686-g3770008', '>=8.0', {
... includePrerelease: true,
... });
true
>
> semver.satisfies('8.2.0-alpha-2686-g3770008', '>=8.0', {
... includePrerelease: false, // the default
... });
false
Didn't realise this was only failing on latest and that latest means latest alpha, not latest production release.
This is only relevant for the new autocompleter which is currently feature-flagged and only accessible if you run mongosh with the environment variable USE_NEW_AUTOCOMPLETE=true
Without it set we'll still read database and collection names like we've always done. With this set we'll just assume an empty collection/schema, so you won't see field names autocomplete.