Skip to content

feat: add inferNamespacesFromPrivileges, default to true COMPASS-9572 #7153

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 8 commits into from
Jul 31, 2025

Conversation

lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Jul 30, 2025

Toggling the setting off will disable our fallback code that reads databases and collections from privileges then merges those in with the results of listDatabases and listCollections.

Also renaming is_non_existent to inferred_from_privileges (and all other permutations I could find) throughout the codebase so that it is less confusing. This is because we don't know if these exist or not.

I also fixed the text to say that it might not exist. Long story: COMPASS-9641.

Screenshot 2025-07-30 at 15 35 59

@Copilot Copilot AI review requested due to automatic review settings July 30, 2025 15:17
@lerouxb lerouxb requested a review from a team as a code owner July 30, 2025 15:17
@github-actions github-actions bot added the feat label Jul 30, 2025
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 adds a new user preference inferNamespacesFromPrivileges (defaulting to true) that controls whether Compass infers additional namespaces from user privileges. When disabled, it prevents the fallback code that reads databases and collections from privileges and merges them with listDatabases and listCollections results. Additionally, the PR renames is_non_existent to is_ghost_namespace throughout the codebase for clarity.

  • Adds new preference inferNamespacesFromPrivileges with default value true
  • Renames is_non_existent to is_ghost_namespace across all files and interfaces
  • Updates data service methods to respect the new preference setting

Reviewed Changes

Copilot reviewed 30 out of 33 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/instance-model/lib/model.js Adds shouldFetchNamespacesFromPrivileges property and preference listener
packages/databases-collections/src/collections-plugin-title.tsx Renames prop from isNonExistent to isGhostNamespace
packages/databases-collections-list/src/namespace-card.tsx Updates component props and styling references for ghost namespace
packages/databases-collections-list/src/index.spec.tsx Updates test data to use new property name
packages/databases-collections-list/src/databases.tsx Updates prop reference in database list component
packages/databases-collections-list/src/collections.tsx Updates prop reference in collection list component
packages/database-model/lib/model.js Updates model property and data service call parameters
packages/data-service/src/instance-detail-helper.ts Updates type definitions for ghost namespace property
packages/data-service/src/data-service.ts Implements preference-based namespace fetching logic
packages/data-service/src/data-service.spec.ts Updates test expectations for renamed property
packages/compass-preferences-model/src/preferences-schema.tsx Defines new preference schema with validation
packages/compass-settings/src/components/settings/general.tsx Adds preference to general settings UI
packages/compass-app-stores/src/stores/instance-store.ts Adds preference change listener for database refresh

@lerouxb lerouxb marked this pull request as draft July 30, 2025 19:40
@lerouxb
Copy link
Contributor Author

lerouxb commented Jul 30, 2025

I turned this back into a draft so I can push some more code tomorrow - the events are firing in the wrong order and I'm going to remove them and solve it in a different way.

const NON_EXISTANT_NAMESPACE_TEXT =
'Your privileges grant you access to this namespace, but it does not currently exist';
const INFERRED_FROM_PRIVILEGES_TEXT =
'Your privileges grant you access to this namespace, but it might not currently exist';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't know if these collections exist if the user didn't have access to listDatabases or listCollections. We don't record whether they had access or not and therefore we can't know at this point. (If they DID have access to those commands then we can probably safely say that these don't exist. But right now we don't know that.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Arguably we could also change the code in dataService to only infer namespaces from privileges if the corresponding listDatabases() or listCollections() call failed due to not having access. That would simplify matters a bit.)

Copy link
Contributor Author

@lerouxb lerouxb Jul 31, 2025

Choose a reason for hiding this comment

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

I guess inside dataService's listCollections() or listDatabases() calls we can immediately tell if the corresponding call worked or not and then have different booleans for dbs that definitely don't exist and that may or may not exist 🤔

Copy link
Contributor Author

@lerouxb lerouxb Jul 31, 2025

Choose a reason for hiding this comment

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

I added a follow-up ticket COMPASS-9641.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to update this bit in packages/databases-collections-list/src/namespace-card.tsx:318

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Updated.

@@ -409,11 +396,15 @@ const InstanceModel = AmpersandModel.extend(
return coll;
},

shouldFetchDbAndCollStats() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By making these methods we don't have to be concerned about the event handlers in the store catching the event first and then calling refresh before they got updated here.

@lerouxb lerouxb force-pushed the disable-ghost-collections branch from dd30950 to 3038293 Compare July 31, 2025 10:22
@lerouxb lerouxb marked this pull request as ready for review July 31, 2025 10:28
@lerouxb lerouxb merged commit 1ff2033 into main Jul 31, 2025
56 of 58 checks passed
@lerouxb lerouxb deleted the disable-ghost-collections branch July 31, 2025 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants