From ce2e752885c89bd73c27a4493125584455bcb8f8 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 30 Mar 2022 12:02:51 -0600 Subject: [PATCH 1/2] Ensure that old values aren't cached inappropriately --- .../preferences/preference-proxy.spec.ts | 28 +++++++++++++++++++ .../preferences/validated-preference-proxy.ts | 21 ++++++++++---- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/packages/core/src/browser/preferences/preference-proxy.spec.ts b/packages/core/src/browser/preferences/preference-proxy.spec.ts index 58ffa51973720..5f9c89cfb5499 100644 --- a/packages/core/src/browser/preferences/preference-proxy.spec.ts +++ b/packages/core/src/browser/preferences/preference-proxy.spec.ts @@ -333,6 +333,34 @@ describe('Preference Proxy', () => { expect(events).to.have.length(3, 'One event for base, one for each override'); expect(events.every(event => event.newValue === 'foo'), 'Should have returned the default in case of garbage.'); }); + + it("Validated proxies don't retain old values for overrides after a change (no emitter).", async () => { + const { proxy, validationCallCounter } = await prepareValidationTest(); + prefSchema.registerOverrideIdentifier('swift'); + const initialValue = proxy.get({ preferenceName: 'my.pref', overrideIdentifier: 'swift' }); + expect(initialValue).to.equal('foo', 'Proxy should start with default value.'); + expect(validationCallCounter.calls).to.equal(1, 'Retrieval should validate (1).'); + await prefService.set('my.pref', 'bar', PreferenceScope.User); + expect(proxy.get('my.pref')).to.equal('bar', 'The base should have been updated.'); + expect(validationCallCounter.calls).to.equal(2, 'Retrieval should validate (2).'); + expect(proxy.get({ preferenceName: 'my.pref', overrideIdentifier: 'swift' })).to.equal('bar', 'Proxy should update empty overrides on change.'); + expect(validationCallCounter.calls).to.equal(3, 'Retrieval should validate (3).'); + }); + + // The scenario with a listener differs because the proxy will start caching known valid values when a listener is attached. + it("Validated proxies don't retain old values for overrides after a change (with emitter)", async () => { + const { proxy, validationCallCounter } = await prepareValidationTest(); + prefSchema.registerOverrideIdentifier('swift'); + proxy.onPreferenceChanged; // Initialize the listeners. + const initialValue = proxy.get({ preferenceName: 'my.pref', overrideIdentifier: 'swift' }); + expect(initialValue).to.equal('foo', 'Proxy should start with default value.'); + expect(validationCallCounter.calls).to.equal(1, 'Retrieval should validate.'); + await prefService.set('my.pref', 'bar', PreferenceScope.User); + expect(validationCallCounter.calls).to.equal(2, 'Event should trigger validation.'); + expect(proxy.get('my.pref')).to.equal('bar', 'The base should have been updated.'); + expect(proxy.get({ preferenceName: 'my.pref', overrideIdentifier: 'swift' })).to.equal('bar', 'Proxy should update empty overrides on change.'); + expect(validationCallCounter.calls).to.equal(2, 'Subsequent retrievals should not trigger validation.'); + }); } it('toJSON with deep', async () => { diff --git a/packages/core/src/browser/preferences/validated-preference-proxy.ts b/packages/core/src/browser/preferences/validated-preference-proxy.ts index 1dc62a80704a6..3a44cedc51a77 100644 --- a/packages/core/src/browser/preferences/validated-preference-proxy.ts +++ b/packages/core/src/browser/preferences/validated-preference-proxy.ts @@ -25,7 +25,11 @@ import { PreferenceChanges } from './preference-service'; export class ValidatedPreferenceProxy> extends InjectablePreferenceProxy { @inject(PreferenceValidationService) protected readonly validator: PreferenceValidationService; - protected validPreferences: Map = new Map(); + /** + * This map should be initialized only when the proxy starts listening to events from the PreferenceService in {@link ValidatedPreferenceProxy.subscribeToChangeEvents}. + * Otherwise, it can't guarantee that the cache will remain up-to-date and is better off just retrieving the value. + */ + protected validPreferences?: Map; protected override handlePreferenceChanges(changes: PreferenceChanges): void { if (this.schema) { @@ -57,7 +61,9 @@ export class ValidatedPreferenceProxy> exten for (const override of tracker.overrides) { const name = this.preferences.overridePreferenceName(override); const { domain, oldValue, preferenceName, scope } = changes[name]; - const newValue = changes[name].newValue === configuredValue ? validatedValue : this.ensureValid(name, () => changes[name].newValue, true); + const newValue = changes[name].newValue === configuredValue + ? (this.validPreferences?.set(name, validatedValue), validatedValue) + : this.ensureValid(name, () => changes[name].newValue, true); this.fireChangeEvent(this.buildNewChangeEvent({ domain, oldValue, preferenceName, scope, newValue }, override)); } } @@ -72,19 +78,24 @@ export class ValidatedPreferenceProxy> exten } protected ensureValid(preferenceName: K, getCandidate: () => T[K], isChange?: boolean): T[K] { - if (!isChange && this.validPreferences.has(preferenceName)) { + if (!isChange && this.validPreferences?.has(preferenceName)) { return this.validPreferences.get(preferenceName) as T[K]; } const candidate = getCandidate(); const valid = this.validator.validateByName(preferenceName, candidate) as T[K]; - this.validPreferences.set(preferenceName, valid); + this.validPreferences?.set(preferenceName, valid); return valid; } + protected override subscribeToChangeEvents(): void { + this.validPreferences ??= new Map(); + super.subscribeToChangeEvents(); + } + override dispose(): void { super.dispose(); if (this.options.isDisposable) { - this.validPreferences.clear(); + this.validPreferences?.clear(); } } } From 480705028febdd29ebec688928c44e2f6e60ae6b Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 30 Mar 2022 12:57:49 -0600 Subject: [PATCH 2/2] Use event values rather than tracker changes --- .../preferences/preference-proxy.spec.ts | 22 ++++++++++--- .../preferences/validated-preference-proxy.ts | 32 ++++++++++++------- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/packages/core/src/browser/preferences/preference-proxy.spec.ts b/packages/core/src/browser/preferences/preference-proxy.spec.ts index 5f9c89cfb5499..8d8585a00fe03 100644 --- a/packages/core/src/browser/preferences/preference-proxy.spec.ts +++ b/packages/core/src/browser/preferences/preference-proxy.spec.ts @@ -351,15 +351,29 @@ describe('Preference Proxy', () => { it("Validated proxies don't retain old values for overrides after a change (with emitter)", async () => { const { proxy, validationCallCounter } = await prepareValidationTest(); prefSchema.registerOverrideIdentifier('swift'); + const override = { preferenceName: 'my.pref', overrideIdentifier: 'swift' }; proxy.onPreferenceChanged; // Initialize the listeners. - const initialValue = proxy.get({ preferenceName: 'my.pref', overrideIdentifier: 'swift' }); + const initialValue = proxy.get(override); expect(initialValue).to.equal('foo', 'Proxy should start with default value.'); expect(validationCallCounter.calls).to.equal(1, 'Retrieval should validate.'); + await prefService.set('my.pref', 'bar', PreferenceScope.User); - expect(validationCallCounter.calls).to.equal(2, 'Event should trigger validation.'); + expect(validationCallCounter.calls).to.equal(2, 'Event should trigger validation (1).'); expect(proxy.get('my.pref')).to.equal('bar', 'The base should have been updated.'); - expect(proxy.get({ preferenceName: 'my.pref', overrideIdentifier: 'swift' })).to.equal('bar', 'Proxy should update empty overrides on change.'); - expect(validationCallCounter.calls).to.equal(2, 'Subsequent retrievals should not trigger validation.'); + expect(proxy.get(override)).to.equal('bar', 'Proxy should update empty overrides on change.'); + expect(validationCallCounter.calls).to.equal(2, 'Subsequent retrievals should not trigger validation. (1)'); + + await prefService.set(prefService.overridePreferenceName(override), 'baz', PreferenceScope.User); + expect(validationCallCounter.calls).to.equal(3, 'Event should trigger validation (2).'); + expect(proxy.get('my.pref')).to.equal('bar', 'Base should not have been updated.'); + expect(proxy.get(override)).to.equal('baz', 'Override should have been updated'); + expect(validationCallCounter.calls).to.equal(3, 'Subsequent retrievals should not trigger validation. (2)'); + + await prefService.set('my.pref', 'boom', PreferenceScope.User); + expect(validationCallCounter.calls).to.equal(4, 'Event should trigger validation (3).'); + expect(proxy.get('my.pref')).to.equal('boom', 'Base should have been updated.'); + expect(proxy.get(override)).to.equal('baz', 'Override should not have been updated'); + expect(validationCallCounter.calls).to.equal(4, 'Subsequent retrievals should not trigger validation. (3)'); }); } diff --git a/packages/core/src/browser/preferences/validated-preference-proxy.ts b/packages/core/src/browser/preferences/validated-preference-proxy.ts index 3a44cedc51a77..e6df5dc430372 100644 --- a/packages/core/src/browser/preferences/validated-preference-proxy.ts +++ b/packages/core/src/browser/preferences/validated-preference-proxy.ts @@ -16,10 +16,11 @@ import { JSONValue } from '@phosphor/coreutils'; import { inject, injectable } from 'inversify'; -import { PreferenceValidationService } from '.'; import { InjectablePreferenceProxy } from './injectable-preference-proxy'; import { OverridePreferenceName } from './preference-language-override-service'; +import { PreferenceProvider } from './preference-provider'; import { PreferenceChanges } from './preference-service'; +import { PreferenceValidationService } from './preference-validation-service'; @injectable() export class ValidatedPreferenceProxy> extends InjectablePreferenceProxy { @@ -33,18 +34,16 @@ export class ValidatedPreferenceProxy> exten protected override handlePreferenceChanges(changes: PreferenceChanges): void { if (this.schema) { - interface TrackedOverrides { value?: JSONValue, overrides: OverridePreferenceName[] }; + interface TrackedOverrides { overrides: OverridePreferenceName[] }; const overrideTracker: Map = new Map(); for (const change of Object.values(changes)) { const overridden = this.preferences.overriddenPreferenceName(change.preferenceName); if (this.isRelevantChange(change, overridden)) { let doSet = false; const baseName = overridden?.preferenceName ?? change.preferenceName; - const tracker: TrackedOverrides = overrideTracker.get(baseName) ?? (doSet = true, { value: undefined, overrides: [] }); + const tracker: TrackedOverrides = overrideTracker.get(baseName) ?? (doSet = true, { overrides: [] }); if (overridden) { tracker.overrides.push(overridden); - } else { - tracker.value = change.newValue; } if (doSet) { overrideTracker.set(baseName, tracker); @@ -52,18 +51,29 @@ export class ValidatedPreferenceProxy> exten } } for (const [baseName, tracker] of overrideTracker.entries()) { - const configuredValue = tracker.value as T[typeof baseName]; - const validatedValue = this.ensureValid(baseName, () => configuredValue, true); + const validated: Array<[JSONValue, JSONValue]> = []; + // This could go wrong if someone sets a lot of different, complex values for different language overrides simultaneously. + // In the normal case, we'll just be doing strict equal checks on primitives. + const getValidValue = (name: string): JSONValue => { + const configuredForCurrent = changes[name].newValue; + const existingValue = validated.find(([configuredForValid]) => PreferenceProvider.deepEqual(configuredForValid, configuredForCurrent)); + if (existingValue) { + this.validPreferences?.set(name, existingValue[1]); + return existingValue[1]; + } + const validValue = this.ensureValid(name, () => configuredForCurrent, true); + validated.push([configuredForCurrent, validValue]); + return validValue; + }; if (baseName in changes && this.isRelevantChange(changes[baseName])) { + const newValue = getValidValue(baseName); const { domain, oldValue, preferenceName, scope } = changes[baseName]; - this.fireChangeEvent(this.buildNewChangeEvent({ domain, oldValue, preferenceName, scope, newValue: validatedValue })); + this.fireChangeEvent(this.buildNewChangeEvent({ domain, oldValue, preferenceName, scope, newValue })); } for (const override of tracker.overrides) { const name = this.preferences.overridePreferenceName(override); const { domain, oldValue, preferenceName, scope } = changes[name]; - const newValue = changes[name].newValue === configuredValue - ? (this.validPreferences?.set(name, validatedValue), validatedValue) - : this.ensureValid(name, () => changes[name].newValue, true); + const newValue = getValidValue(name); this.fireChangeEvent(this.buildNewChangeEvent({ domain, oldValue, preferenceName, scope, newValue }, override)); } }