Skip to content

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

Merged
merged 16 commits into from
Jul 9, 2025

Conversation

lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Jul 8, 2025

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.

@Copilot Copilot AI review requested due to automatic review settings July 8, 2025 11:22
@lerouxb lerouxb requested a review from a team as a code owner July 8, 2025 11:22
Copy link

@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 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 in CliUserConfig 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({

@lerouxb lerouxb changed the title feat: allow opting out of schema sampling with config disableSchemaSampling=false MONGOSH-2370 feat: allow opting out of schema sampling with config disableSchemaSampling=true MONGOSH-2370 Jul 8, 2025

// TODO: we need MQL support in mongodb-ts-autocomplete in order for it
// to autocomplete collection field names
wantQueryOperators = false;
Copy link
Contributor Author

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');
Copy link
Contributor Author

@lerouxb lerouxb Jul 8, 2025

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([
Copy link
Contributor Author

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
Copy link
Contributor Author

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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor Author

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.

@lerouxb lerouxb merged commit 0acc023 into main Jul 9, 2025
124 of 125 checks passed
@lerouxb lerouxb deleted the sample-config branch July 9, 2025 10:45
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