#51935 fix configuration model to consider an override identifier can…

… exists in more than one overrides
microsoft · Nov 15, 2021 · bdc489b · bdc489b
1 parent 466d041
commit bdc489b
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 6 deletions.
diff --git a/src/vs/platform/configuration/common/configurationModels.ts b/src/vs/platform/configuration/common/configurationModels.ts
@@ -4,6 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as arrays from 'vs/base/common/arrays';
+import { IStringDictionary } from 'vs/base/common/collections';
 import { Emitter, Event } from 'vs/base/common/event';
 import * as json from 'vs/base/common/json';
 import { Disposable } from 'vs/base/common/lifecycle';
@@ -58,12 +59,13 @@ export class ConfigurationModel implements IConfigurationModel {
  }
 
  getKeysForOverrideIdentifier(identifier: string): string[] {
+ const keys: string[] = [];
  for (const override of this.overrides) {
- if (override.identifiers.indexOf(identifier) !== -1) {
- return override.keys;
+ if (override.identifiers.includes(identifier)) {
+ keys.push(...override.keys);
  }
  }
- return [];
+ return arrays.distinct(keys);
  }
 
  override(identifier: string): ConfigurationModel {
@@ -156,12 +158,27 @@ export class ConfigurationModel implements IConfigurationModel {
  }
 
  private getContentsForOverrideIdentifer(identifier: string): any {
+ let contentsForIdentifierOnly: IStringDictionary<any> | null = null;
+ let contents: IStringDictionary<any> | null = null;
+ const mergeContents = (contentsToMerge: any) => {
+ if (contentsToMerge) {
+ if (contents) {
+ this.mergeContents(contents, contentsToMerge);
+ } else {
+ contents = contentsToMerge;
+ }
+ }
+ };
  for (const override of this.overrides) {
- if (override.identifiers.indexOf(identifier) !== -1) {
- return override.contents;
+ if (arrays.equals(override.identifiers, [identifier])) {
+ contentsForIdentifierOnly = override.contents;
+ } else if (override.identifiers.includes(identifier)) {
+ mergeContents(override.contents);
  }
  }
- return null;
+ // Merge contents of the identifier only at the end to take precedence.
+ mergeContents(contentsForIdentifierOnly);
+ return contents;
  }
 
  toJSON(): IConfigurationModel {

diff --git a/src/vs/platform/configuration/test/common/configurationModels.test.ts b/src/vs/platform/configuration/test/common/configurationModels.test.ts
@@ -236,6 +236,30 @@ suite('ConfigurationModel', () => {
 
  assert.deepStrictEqual(testObject.override('b').contents, { 'a': 2, 'c': 1 });
  });
+
+ test('Test override when an override has multiple identifiers', () => {
+ const testObject = new ConfigurationModel({ 'a': 1, 'c': 1 }, ['a', 'c'], [{ identifiers: ['x', 'y'], contents: { 'a': 2 }, keys: ['a'] }]);
+
+ let actual = testObject.override('x');
+ assert.deepStrictEqual(actual.contents, { 'a': 2, 'c': 1 });
+ assert.deepStrictEqual(actual.keys, ['a', 'c']);
+ assert.deepStrictEqual(testObject.getKeysForOverrideIdentifier('x'), ['a']);
+
+ actual = testObject.override('y');
+ assert.deepStrictEqual(actual.contents, { 'a': 2, 'c': 1 });
+ assert.deepStrictEqual(actual.keys, ['a', 'c']);
+ assert.deepStrictEqual(testObject.getKeysForOverrideIdentifier('y'), ['a']);
+ });
+
+ test('Test override when an identifier is defined in multiple overrides', () => {
+ const testObject = new ConfigurationModel({ 'a': 1, 'c': 1 }, ['a', 'c'], [{ identifiers: ['x'], contents: { 'a': 3, 'b': 1 }, keys: ['a', 'b'] }, { identifiers: ['x', 'y'], contents: { 'a': 2 }, keys: ['a'] }]);
+
+ const actual = testObject.override('x');
+ assert.deepStrictEqual(actual.contents, { 'a': 3, 'c': 1, 'b': 1 });
+ assert.deepStrictEqual(actual.keys, ['a', 'c']);
+
+ assert.deepStrictEqual(testObject.getKeysForOverrideIdentifier('x'), ['a', 'b']);
+ });
 });
 
 suite('CustomConfigurationModel', () => {