Skip to content

Commit ee0ba65

Browse files
jonathanKingstonjonathanKingston
committed
Add in array matching of conditions and simplify logic to prevent errors in future
1 parent aed4fb3 commit ee0ba65

File tree

2 files changed

+257
-53
lines changed

2 files changed

+257
-53
lines changed

‎injected/src/config-feature.js

Lines changed: 152 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -49,61 +49,120 @@ export default class ConfigFeature {
4949
* @protected
5050
*/
5151
matchConditionalFeatureSetting(featureKeyName) {
52-
const domains = this._getFeatureSettings()?.[featureKeyName] || [];
53-
return domains.filter((rule) => {
52+
const conditionalChanges = this._getFeatureSettings()?.[featureKeyName] || [];
53+
return conditionalChanges.filter((rule) => {
54+
let condition = rule.condition;
5455
// Support shorthand for domain matching for backwards compatibility
55-
return this.matchConditionalChanges(rule.condition || { domain: rule.domain });
56+
if (condition === undefined && 'domain' in rule) {
57+
condition = this._domainToConditonBlocks(rule.domain);
58+
}
59+
return this._matchConditionalBlockOrArray(condition);
5660
});
5761
}
5862

63+
/**
64+
* Takes a list of domains and returns a list of condition blocks
65+
* @param {string|string[]} domain
66+
* @returns {ConditionBlock[]}
67+
*/
68+
_domainToConditonBlocks(domain) {
69+
if (Array.isArray(domain)) {
70+
return domain.map((domain) => ({ domain }));
71+
} else {
72+
return [{ domain }];
73+
}
74+
}
75+
5976
/**
6077
* Used to match conditional changes for a settings feature.
6178
* @typedef {object} ConditionBlock
62-
* @property {string[] | string} domain?
63-
* @property {object} urlPattern?
79+
* @property {string[] | string} [domain]
80+
* @property {object} [urlPattern]
81+
*/
82+
83+
/**
84+
* Takes multiple conditional blocks and returns true if any apply.
85+
* @param {ConditionBlock|ConditionBlock[]} conditionBlock
86+
* @returns {boolean}
6487
*/
88+
_matchConditionalBlockOrArray(conditionBlock) {
89+
if (Array.isArray(conditionBlock)) {
90+
return conditionBlock.some((block) => this._matchConditionalBlock(block));
91+
}
92+
return this._matchConditionalBlock(conditionBlock);
93+
}
6594

6695
/**
6796
* Takes a conditional block and returns true if it applies.
6897
* All conditions must be met to return true.
6998
* @param {ConditionBlock} conditionBlock
7099
* @returns {boolean}
71100
*/
72-
matchConditionalChanges(conditionBlock) {
101+
_matchConditionalBlock(conditionBlock) {
102+
// List of conditions that we support currently, these return truthy if the condition is met
103+
/** @type {Record<string, (conditionBlock: ConditionBlock) => boolean>} */
104+
const conditionChecks = {
105+
domain: this._matchDomainConditional,
106+
urlPattern: this._matchUrlPatternConditional,
107+
};
108+
73109
for (const key in conditionBlock) {
74-
switch (key) {
75-
case 'domain': {
76-
if (!this._matchDomainConditional(conditionBlock)) {
77-
return false;
110+
/*
111+
Unsupported condition so fail for backwards compatibility
112+
If you wish to support older clients you should create an old condition block
113+
without the unsupported key also.
114+
Such as:
115+
[
116+
{
117+
condition: {
118+
domain: 'example.com'
119+
}
120+
},
121+
{
122+
condition: {
123+
domain: 'example.com',
124+
newKey: 'value'
125+
}
78126
}
79-
break;
80-
}
81-
case 'urlPattern': {
82-
const pattern = new URLPattern(conditionBlock.urlPattern);
83-
if (!this.args?.site.url || !pattern.test(this.args?.site.url)) {
84-
return false;
85-
}
86-
break;
87-
}
88-
// For unknown keys we should assume the condition is not met
89-
default: {
90-
return false;
91-
}
127+
]
128+
*/
129+
if (!conditionChecks[key]) {
130+
return false;
131+
} else if (!conditionChecks[key].call(this, conditionBlock)) {
132+
return false;
92133
}
93134
}
94-
95-
// All conditions are met
96135
return true;
97136
}
98137

138+
/**
139+
* Takes a condtion block and returns true if the current url matches the urlPattern.
140+
* @param {ConditionBlock} conditionBlock
141+
* @returns {boolean}
142+
*/
143+
_matchUrlPatternConditional(conditionBlock) {
144+
const url = this.args?.site.url;
145+
if (!url) return false;
146+
if (typeof conditionBlock.urlPattern === 'string') {
147+
// Use the current URL as the base for matching
148+
return new URLPattern(conditionBlock.urlPattern, url).test(url);
149+
}
150+
const pattern = new URLPattern(conditionBlock.urlPattern);
151+
return pattern.test(url);
152+
}
153+
154+
/**
155+
* Takes a condition block and returns true if the current domain matches the domain.
156+
* @param {ConditionBlock} conditionBlock
157+
* @returns {boolean}
158+
*/
99159
_matchDomainConditional(conditionBlock) {
100160
if (!conditionBlock.domain) return false;
101161
const domain = this.args?.site.domain;
102162
if (!domain) return false;
103163
if (Array.isArray(conditionBlock.domain)) {
104-
return conditionBlock.domain.some((domainRule) => {
105-
return matchHostname(domain, domainRule);
106-
});
164+
// Explicitly check for an empty array as matchHostname will return true a single item array that matches
165+
return false;
107166
}
108167
return matchHostname(domain, conditionBlock.domain);
109168
}
@@ -151,31 +210,71 @@ export default class ConfigFeature {
151210
}
152211

153212
/**
154-
* Return a specific setting from the feature settings
155-
* If the "settings" key within the config has a "domains" key, it will be used to override the settings.
156-
* This uses JSONPatch to apply the patches to settings before getting the setting value.
157-
* For example.com getFeatureSettings('val') will return 1:
158-
* ```json
159-
* {
160-
* "settings": {
161-
* "domains": [
162-
* {
163-
* "domain": "example.com",
164-
* "patchSettings": [
165-
* { "op": "replace", "path": "/val", "value": 1 }
166-
* ]
167-
* }
168-
* ]
169-
* }
170-
* }
171-
* ```
172-
* "domain" can either be a string or an array of strings.
173-
174-
* For boolean states you should consider using getFeatureSettingEnabled.
175-
* @param {string} featureKeyName
176-
* @param {string} [featureName]
177-
* @returns {any}
178-
*/
213+
* Return a specific setting from the feature settings
214+
* If the "settings" key within the config has a "conditionalChanges" key, it will be used to override the settings.
215+
* This uses JSONPatch to apply the patches to settings before getting the setting value.
216+
* For example.com getFeatureSettings('val') will return 1:
217+
* ```json
218+
* {
219+
* "settings": {
220+
* "conditionalChanges": [
221+
* {
222+
* "domain": "example.com",
223+
* "patchSettings": [
224+
* { "op": "replace", "path": "/val", "value": 1 }
225+
* ]
226+
* }
227+
* ]
228+
* }
229+
* }
230+
* ```
231+
* "domain" can either be a string or an array of strings.
232+
* Additionally we support urlPattern for more complex matching.
233+
* For example.com getFeatureSettings('val') will return 1:
234+
* ```json
235+
* {
236+
* "settings": {
237+
* "conditionalChanges": [
238+
* {
239+
* "condition": {
240+
* "urlPattern": "https://example.com/*",
241+
* },
242+
* "patchSettings": [
243+
* { "op": "replace", "path": "/val", "value": 1 }
244+
* ]
245+
* }
246+
* ]
247+
* }
248+
* }
249+
* ```
250+
* We also support multiple conditions:
251+
* ```json
252+
* {
253+
* "settings": {
254+
* "conditionalChanges": [
255+
* {
256+
* "condition": [
257+
* {
258+
* "urlPattern": "https://example.com/*",
259+
* },
260+
* {
261+
* "urlPattern": "https://other.com/path/something",
262+
* },
263+
* ],
264+
* "patchSettings": [
265+
* { "op": "replace", "path": "/val", "value": 1 }
266+
* ]
267+
* }
268+
* ]
269+
* }
270+
* }
271+
* ```
272+
*
273+
* For boolean states you should consider using getFeatureSettingEnabled.
274+
* @param {string} featureKeyName
275+
* @param {string} [featureName]
276+
* @returns {any}
277+
*/
179278
getFeatureSetting(featureKeyName, featureName) {
180279
let result = this._getFeatureSettings(featureName);
181280
if (featureKeyName in ['domains', 'conditionalChanges']) {

‎injected/unit-test/content-feature.js

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,111 @@ describe('ContentFeature class', () => {
167167
expect(didRun).withContext('Should run').toBeTrue();
168168
});
169169

170+
it('Should trigger getFeatureSetting for the correct conditions', () => {
171+
let didRun = false;
172+
class MyTestFeature3 extends ContentFeature {
173+
init() {
174+
expect(this.getFeatureSetting('test')).toBe('enabled');
175+
expect(this.getFeatureSetting('otherTest')).toBe('disabled');
176+
expect(this.getFeatureSetting('test2')).toBe('noop');
177+
expect(this.getFeatureSetting('otherTest2')).toBe('me');
178+
expect(this.getFeatureSetting('test3')).toBe('yep');
179+
expect(this.getFeatureSetting('otherTest3')).toBe('expected');
180+
expect(this.getFeatureSetting('test4')).toBe('yep');
181+
expect(this.getFeatureSetting('otherTest4')).toBe('expected');
182+
expect(this.getFeatureSetting('test5')).toBe('yep');
183+
expect(this.getFeatureSetting('otherTest5')).toBe('expected');
184+
didRun = true;
185+
}
186+
}
187+
188+
const args = {
189+
site: {
190+
domain: 'beep.example.com',
191+
url: 'http://beep.example.com/path/path/me',
192+
},
193+
featureSettings: {
194+
test: {
195+
test: 'enabled',
196+
otherTest: 'disabled',
197+
test4: 'yep',
198+
otherTest4: 'expected',
199+
conditionalChanges: [
200+
{
201+
condition: {
202+
// This array case is unsupported currently.
203+
domain: ['example.com'],
204+
},
205+
patchSettings: [
206+
{ op: 'replace', path: '/test', value: 'enabled2' },
207+
{ op: 'replace', path: '/otherTest', value: 'bloop' },
208+
],
209+
},
210+
{
211+
condition: [
212+
{
213+
domain: 'example.com',
214+
},
215+
{
216+
domain: 'other.com',
217+
},
218+
],
219+
patchSettings: [
220+
{ op: 'replace', path: '/test2', value: 'noop' },
221+
{ op: 'replace', path: '/otherTest2', value: 'me' },
222+
],
223+
},
224+
{
225+
condition: [
226+
{
227+
urlPattern: '*://*.example.com',
228+
},
229+
{
230+
urlPattern: '*://other.com',
231+
},
232+
],
233+
patchSettings: [
234+
{ op: 'replace', path: '/test3', value: 'yep' },
235+
{ op: 'replace', path: '/otherTest3', value: 'expected' },
236+
],
237+
},
238+
{
239+
condition: [
240+
{
241+
// This is at the apex so should not match
242+
urlPattern: '*://example.com',
243+
},
244+
{
245+
urlPattern: '*://other.com',
246+
},
247+
],
248+
patchSettings: [
249+
{ op: 'replace', path: '/test4', value: 'nope' },
250+
{ op: 'replace', path: '/otherTest4', value: 'notexpected' },
251+
],
252+
},
253+
{
254+
condition: [
255+
{
256+
urlPattern: {
257+
hostname: '*.example.com',
258+
},
259+
},
260+
],
261+
patchSettings: [
262+
{ op: 'replace', path: '/test5', value: 'yep' },
263+
{ op: 'replace', path: '/otherTest5', value: 'expected' },
264+
],
265+
},
266+
],
267+
},
268+
},
269+
};
270+
const me = new MyTestFeature3('test', {}, args);
271+
me.callInit(args);
272+
expect(didRun).withContext('Should run').toBeTrue();
273+
});
274+
170275
describe('addDebugFlag', () => {
171276
class MyTestFeature extends ContentFeature {
172277
// eslint-disable-next-line

0 commit comments

Comments
 (0)