Skip to content
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.

Sign up for GitHub

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

Ensure that old values aren't cached inappropriately #10965

Merged
merged 2 commits into from
Mar 31, 2022
Merged

Ensure that old values aren't cached inappropriately #10965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce2e752
Ensure that old values aren't cached inappropriately
colin-grant-work Mar 30, 2022
4807050
Use event values rather than tracker changes
colin-grant-work Mar 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions packages/core/src/browser/preferences/preference-proxy.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,48 @@ 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');
const override = { preferenceName: 'my.pref', overrideIdentifier: 'swift' };
proxy.onPreferenceChanged; // Initialize the listeners.
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 (1).');
expect(proxy.get('my.pref')).to.equal('bar', 'The base should have been updated.');
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)');
});
}

it('toJSON with deep', async () => {
Expand Down
47 changes: 34 additions & 13 deletions packages/core/src/browser/preferences/validated-preference-proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,48 +16,64 @@

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<T extends Record<string, JSONValue>> extends InjectablePreferenceProxy<T> {
@inject(PreferenceValidationService) protected readonly validator: PreferenceValidationService;

protected validPreferences: Map<string, JSONValue> = 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<string, JSONValue>;

protected override handlePreferenceChanges(changes: PreferenceChanges): void {
if (this.schema) {
interface TrackedOverrides { value?: JSONValue, overrides: OverridePreferenceName[] };
interface TrackedOverrides { overrides: OverridePreferenceName[] };
const overrideTracker: Map<string, TrackedOverrides> = 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);
}
}
}
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 ? validatedValue : this.ensureValid(name, () => changes[name].newValue, true);
const newValue = getValidValue(name);
this.fireChangeEvent(this.buildNewChangeEvent({ domain, oldValue, preferenceName, scope, newValue }, override));
}
}
Expand All @@ -72,19 +88,24 @@ export class ValidatedPreferenceProxy<T extends Record<string, JSONValue>> exten
}

protected ensureValid<K extends keyof T & string>(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();
}
}
}