fix(compass-preference-model): account for all the preferences for the implementation of ReadOnlyPreferenceService COMPASS-7415 #5356

himanshusinghs · 2024-01-18T13:18:09Z

Description

This fixes the issue in our earlier implementation of SimplePreferenceService where we were using an empty object as AllPreferences which could have caused issues in our code which expect some of the preferences to be there but can't find it.

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

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)

packages/compass-web/src/index.tsx

addaleax · 2024-01-18T14:16:05Z

packages/compass-preferences-model/src/provider.ts

 export { usePreference, withPreferences } from './react';
 export { capMaxTimeMSAtPreferenceLimit } from './maxtimems';

+export class SimplePreferenceService implements PreferencesAccess {


Can we rename this to something like

Suggested change

export class SimplePreferenceService implements PreferencesAccess {

export class MockPreferenceService implements PreferencesAccess {

? It doesn't really implement the interface from a semantic point of view, so we should make clear that this is only for testing

Yea I am good with the suggested name. But just to state the intention - this is not just for testing but also for cases like Data Explorer where we need some preferences overrides but we don't ever modify them.

Then we should:

Rename this in some way that implies that this is a read-only implementation of preferences where attempts to change preferences are being silently discarded

Fix the createSandbox() method to return a new object, not this (it could just be a copy), or alternatively to throw an exception to indicate that this method is not actually supported

Addressed this also in 77631fd

packages/compass-preferences-model/src/provider.ts

gribnoysup · 2024-01-19T08:48:06Z

packages/compass-preferences-model/src/provider.ts

+  }
+
+  getPreferences() {
+    return this.allPreferences;


This worked for this TODOed version of this implemementation, but I don't think we can just do that here. Preferences have dependencies on other preferences, and while curently this logic is part of the preferences-main implementation (see the _computePreferenceValuesAndStates method), it should be shared with this ReadOnlyPreferencesService implementation. This should also affect the getPreferenceStates and getConfigurableUserPreferences implementations

Right, this is what was failing the existing test cases for me. Will push the change in a bit. The change accounts for the existence of deriveValue function on preference descriptors and use that to apply the derived values on top of defaults.

gribnoysup · 2024-01-19T08:55:24Z

packages/compass-preferences-model/src/provider.ts

+  createSandbox() {
+    return Promise.reject('Method not supported');
+  }


I don't think we can guarantee that this will not be called somewhere in Compass as it's part of the interface and not optional, so I'd suggest doing the other option that Anna suggested, return a deep clone / new instance here instead of returning this (returning this was definitely a quick hack we should get rid of), but not throw

Honestly, if we want to use this in production, we should also consider to just keep using the regular Preferences class with a custom storage service. That matches what we do for other services more closely, and probably just makes a bunch of things more consistent.

We can, but there are additional platform specific code and dependencies to remove from the class: implicit logging instance, everything related to global config and cli args handling. What we want is preferences class that truly doesn't have anything platform specifc in the implementation and then compass can implement a version that also handles global config files, cli arguments, ipc, persisting with user-data, none of those things will ever be part of the version used by data explorer in the same manner that Compass applies them

To the original concern that Sergey pointed out - we might not actually need SandboxPreference anymore, after having this new simple preference service implementation. The only place that I noticed we use the sandbox preferences is tests (please correct if I missed something) but now our provider always gets an instance of our service implementation and our preference locator does not anymore return the defaultPreferenceInstance. So the need for creating sandbox is essentially gone now.

Did I miss something or is it making sense?

Sandbox preferences are used in settings so that the changes can be made to the preferences taking all the logic of deriving the values into account and then applied to the parent preferences. They can't be "read only" because this is what is being changed by the user when clicking around in the settings modal

Wow just noticed that there is also PreferencesSandbox which is probably what you mean is used in settings.

I was looking at this SandboxPreferences (used mostly by our test cases) being made redundant by the changes that I did so far, and in process also making the public interface PreferenceAccess.CreateSandbox redundant.

Preferences sandbox in settings is just a simpler interface over preferences so that we don't need to deal with all preferences apis in settings store actions and tests directly and only get the api we actually need. It calls createSandbox (which calls CreateSandbox) so the sandbox preferences are used by settings:

compass/packages/compass-settings/src/stores/preferences-sandbox.ts

Line 28 in 536720c

this._sandbox = await this.preferences.createSandbox();

Yea this makes sense now, sorry I did not look thoroughly how the sandbox was used in here. Ok so this completely disproves my idea of killing the sandbox as I was under wrong understanding that the sandbox interface was used just to aid in testing.

Honestly, if we want to use this in production, we should also consider to just keep using the regular Preferences class with a custom storage service.

@addaleax we chatted a bit with Himanshu and took a close look at the preferences class implementation, and I think we found a way to cover your suggestion while making the interface more generalized and less platform specific without a lot of changes (there is not that much specific stuff there really), so Himanshu will be working on a follow up that incorporates both your feedback here and the requirement for a slightly less platform dependant implementation. Thank you for the feedback!

Addressed this in 77631fd

…id compass specific imports in provider

…e defaults

…erence-service

himanshusinghs · 2024-01-22T17:02:54Z

Just created another PR to split this into a smaller version.

addaleax

This looks great!

addaleax · 2024-01-22T18:06:54Z

packages/compass-preferences-model/src/storage.ts

@@ -51,11 +46,11 @@ export class SandboxPreferences implements BasePreferencesStorage {
  }
 }

-export class StoragePreferences implements BasePreferencesStorage {
+export class PersistentStorage implements BasePreferencesStorage {


packages/compass-web/src/index.tsx

…-service

himanshusinghs · 2024-01-22T21:22:58Z

packages/compass-preferences-model/src/storage.ts

-export class SandboxPreferences implements BasePreferencesStorage {
-  private preferences = getDefaultPreferences();
+export class InMemoryStorage implements BasePreferencesStorage {
+  private preferences = getInitialValuesForStoredPreferences();


Would like to point out that this initialisation earlier happened with the defaults for StoredPreferences. But now we initialise the preferences with the defaults merged with derived values for StoredPreferences.

This change affects the PreferencesAccess.CreateSandbox but I feel that this should still be fine. Please let me know if I overlooked something.

@himanshusinghs Did you want to update this based on our conversation today? Or was this something for the next/follow-up PR?

github-actions bot added the fix label Jan 18, 2024

himanshusinghs added the no release notes Fix or feature not for release notes label Jan 18, 2024

himanshusinghs requested a review from gribnoysup January 18, 2024 13:19

gribnoysup reviewed Jan 18, 2024

View reviewed changes

packages/compass-web/src/index.tsx Outdated Show resolved Hide resolved

addaleax reviewed Jan 18, 2024

View reviewed changes

himanshusinghs marked this pull request as draft January 19, 2024 08:30

gribnoysup reviewed Jan 19, 2024

View reviewed changes

himanshusinghs force-pushed the fix-simple-preference-service branch 2 times, most recently from 55889d1 to 33a1554 Compare January 19, 2024 14:29

himanshusinghs added 11 commits January 22, 2024 09:57

fix: fixed the simple preference service implementation

64e9c59

fix: use ref for simple service

b38259a

chore: moved preference schema definitions to a different file to avo…

2ced852

…id compass specific imports in provider

chore: review fixup

d60390a

chore: further fixes to imports

9951a1f

chore: fix lost type assertion

6e3b614

chore: fix the import for our service

b52bf71

chore: fix testcases by incorporating the derived values in preferenc…

361451b

…e defaults

chore: further adaptation to read only preference service

982c8a0

chore: provide an implementation for CreateSandbox

e925b99

chore: further refactor, remove getDefaultPreferences duplication

956f90d

himanshusinghs force-pushed the fix-simple-preference-service branch from 4177e36 to 956f90d Compare January 22, 2024 08:57

himanshusinghs marked this pull request as ready for review January 22, 2024 08:58

himanshusinghs marked this pull request as draft January 22, 2024 10:44

himanshusinghs added 2 commits January 22, 2024 15:31

chore: decouple preferences and storage and use them separately

77631fd

chore: import z from zod

510de7f

himanshusinghs marked this pull request as ready for review January 22, 2024 14:45

himanshusinghs added 2 commits January 22, 2024 17:56

chore: split preferences and schema definitions

edf794c

Merge branch 'split-preferences-and-schema-defs' into fix-simple-pref…

9b5c4e2

…erence-service

addaleax reviewed Jan 22, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix-simple-preference…

268a955

…-service

himanshusinghs commented Jan 22, 2024

View reviewed changes

himanshusinghs changed the title ~~fix(compass-preference-model): account for all the preferences for the implementation of SimplePreferenceService~~ fix(compass-preference-model): account for all the preferences for the implementation of ReadOnlyPreferenceService Jan 23, 2024

himanshusinghs requested a review from gribnoysup January 23, 2024 11:10

addaleax approved these changes Jan 23, 2024

View reviewed changes

himanshusinghs merged commit 849955d into main Jan 23, 2024
17 checks passed

himanshusinghs deleted the fix-simple-preference-service branch January 23, 2024 14:53

himanshusinghs changed the title ~~fix(compass-preference-model): account for all the preferences for the implementation of ReadOnlyPreferenceService~~ fix(compass-preference-model): account for all the preferences for the implementation of ReadOnlyPreferenceService COMPASS-7415 Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compass-preference-model): account for all the preferences for the implementation of ReadOnlyPreferenceService COMPASS-7415 #5356

fix(compass-preference-model): account for all the preferences for the implementation of ReadOnlyPreferenceService COMPASS-7415 #5356

himanshusinghs commented Jan 18, 2024

addaleax Jan 18, 2024

himanshusinghs Jan 18, 2024

addaleax Jan 18, 2024

himanshusinghs Jan 22, 2024

gribnoysup Jan 19, 2024

himanshusinghs Jan 19, 2024

gribnoysup Jan 19, 2024 •

edited

Loading

addaleax Jan 19, 2024

gribnoysup Jan 19, 2024 •

edited

Loading

himanshusinghs Jan 19, 2024

gribnoysup Jan 19, 2024

himanshusinghs Jan 19, 2024

gribnoysup Jan 19, 2024 •

edited

Loading

himanshusinghs Jan 19, 2024

gribnoysup Jan 22, 2024 •

edited

Loading

himanshusinghs Jan 22, 2024

himanshusinghs commented Jan 22, 2024

addaleax left a comment

addaleax Jan 22, 2024

himanshusinghs Jan 22, 2024

addaleax Jan 23, 2024

	export class SimplePreferenceService implements PreferencesAccess {
	export class MockPreferenceService implements PreferencesAccess {

fix(compass-preference-model): account for all the preferences for the implementation of ReadOnlyPreferenceService COMPASS-7415 #5356

fix(compass-preference-model): account for all the preferences for the implementation of ReadOnlyPreferenceService COMPASS-7415 #5356

Conversation

himanshusinghs commented Jan 18, 2024

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshusinghs commented Jan 22, 2024

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Jan 19, 2024 •

edited

Loading

gribnoysup Jan 19, 2024 •

edited

Loading

gribnoysup Jan 19, 2024 •

edited

Loading

gribnoysup Jan 22, 2024 •

edited

Loading