From 8da201aed8b11b078c09acb29f14659f6e6b70d3 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 19 Nov 2024 14:53:01 +0100 Subject: [PATCH] feat: add potentiallyStale filter (#8784) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR adds support for the `potentiallyStale` value in the feature search API. The value is added as a third option for `state` (in addition to `stale` and `active`). Potentially stale is a subset of active flags, so stale flags are never considered potentially stale, even if they have the flag set in the db. Because potentially stale is a separate column in the db, this complicates the query a bit. As such, I've created a specialized handling function in the feature search store: if the query doesn't include `potentiallyStale`, handle it as we did before (the mapping has just been moved). If the query *does* contain potentially stale, though, the handling is quite a bit more involved because we need to check multiple different columns against each other. In essence, it's based on this logic: when you’re searching for potentially stale flags, you should only get flags that are active and marked as potentially stale. You should not get stale flags. This can cause some confusion, because in the db, we don’t clear the potentially stale status when we mark a flag as stale, so we can get flags that are both stale and potentially stale. However, as a user, if you’re looking for potentially stale flags, I’d be surprised to also get (only some) stale flags, because if a flag is stale, it’s definitely stale, not potentially stale. This leads us to these six different outcomes we need to handle when your search includes potentially stale and stale or active: 1. You filter for “potentially stale” flags only. The API will give you only flags that are active and marked as potentially stale. You will not get stale flags. 2. You filter only for flags that are not potentially stale. You will get all flags that are active and not potentially stale and all stale flags. 3. You search for “is any of stale, potentially stale”. This is our “unhealthy flags” metric. You get all stale flags and all flags that are active and potentially stale 4. You search for “is none of stale, potentially stale”: This gives you all flags that are active and not potentially stale. Healthy flags, if you will. 5. “is any of active, potentially stale”: you get all active flags. Because we treat potentially stale as a subset of active, this is the same as “is active” 6. “is none of active, potentially stale”: you get all stale flags. As in the previous point, this is the same as “is not active” --- .../feature-search/feature-search-service.ts | 3 - .../feature-search/feature-search-store.ts | 102 +++++++++++++++++- .../feature-search/feature.search.e2e.test.ts | 73 ++++++++++--- .../features/feature-search/search-utils.ts | 2 +- 4 files changed, 162 insertions(+), 18 deletions(-) diff --git a/src/lib/features/feature-search/feature-search-service.ts b/src/lib/features/feature-search/feature-search-service.ts index a07d860b0d..10e45b28f0 100644 --- a/src/lib/features/feature-search/feature-search-service.ts +++ b/src/lib/features/feature-search/feature-search-service.ts @@ -45,9 +45,6 @@ export class FeatureSearchService { if (params.state) { const parsedState = parseSearchOperatorValue('stale', params.state); if (parsedState) { - parsedState.values = parsedState.values.map((value) => - value === 'active' ? 'false' : 'true', - ); queryParams.push(parsedState); } } diff --git a/src/lib/features/feature-search/feature-search-store.ts b/src/lib/features/feature-search/feature-search-store.ts index f08eb624f8..b70c33599a 100644 --- a/src/lib/features/feature-search/feature-search-store.ts +++ b/src/lib/features/feature-search/feature-search-store.ts @@ -569,19 +569,119 @@ class FeatureSearchStore implements IFeatureSearchStore { } } +const applyStaleConditions = ( + query: Knex.QueryBuilder, + staleConditions?: IQueryParam, +): void => { + if (!staleConditions) return; + + const { values, operator } = staleConditions; + + if (!values.includes('potentiallyStale')) { + applyGenericQueryParams(query, [ + { + ...staleConditions, + values: values.map((value) => + value === 'active' ? 'false' : 'true', + ), + }, + ]); + return; + } + + const valueSet = new Set( + values.filter((value) => + ['stale', 'active', 'potentiallyStale'].includes(value || ''), + ), + ); + const allSelected = valueSet.size === 3; + const onlyPotentiallyStale = valueSet.size === 1; + const staleAndPotentiallyStale = + valueSet.has('stale') && valueSet.size === 2; + + if (allSelected) { + switch (operator) { + case 'IS': + case 'IS_ANY_OF': + // All flags included; no action needed + break; + case 'IS_NOT': + case 'IS_NONE_OF': + // All flags excluded + query.whereNotIn('features.stale', [false, true]); + break; + } + return; + } + + if (onlyPotentiallyStale) { + switch (operator) { + case 'IS': + case 'IS_ANY_OF': + query + .where('features.stale', false) + .where('features.potentially_stale', true); + break; + case 'IS_NOT': + case 'IS_NONE_OF': + query.where((qb) => + qb + .where('features.stale', true) + .orWhere('features.potentially_stale', false), + ); + break; + } + return; + } + + if (staleAndPotentiallyStale) { + switch (operator) { + case 'IS': + case 'IS_ANY_OF': + query.where((qb) => + qb + .where('features.stale', true) + .orWhere('features.potentially_stale', true), + ); + break; + case 'IS_NOT': + case 'IS_NONE_OF': + query + .where('features.stale', false) + .where('features.potentially_stale', false); + break; + } + } else { + switch (operator) { + case 'IS': + case 'IS_ANY_OF': + query.where('features.stale', false); + break; + case 'IS_NOT': + case 'IS_NONE_OF': + query.where('features.stale', true); + break; + } + } +}; const applyQueryParams = ( query: Knex.QueryBuilder, queryParams: IQueryParam[], ): void => { const tagConditions = queryParams.filter((param) => param.field === 'tag'); + const staleConditions = queryParams.find( + (param) => param.field === 'stale', + ); const segmentConditions = queryParams.filter( (param) => param.field === 'segment', ); const genericConditions = queryParams.filter( - (param) => param.field !== 'tag', + (param) => !['tag', 'stale'].includes(param.field), ); applyGenericQueryParams(query, genericConditions); + applyStaleConditions(query, staleConditions); + applyMultiQueryParams( query, tagConditions, diff --git a/src/lib/features/feature-search/feature.search.e2e.test.ts b/src/lib/features/feature-search/feature.search.e2e.test.ts index 41d52ad589..ec46c6c3bd 100644 --- a/src/lib/features/feature-search/feature.search.e2e.test.ts +++ b/src/lib/features/feature-search/feature.search.e2e.test.ts @@ -968,27 +968,74 @@ test('should search features by state with operators', async () => { }); }); -test('should search features by created date with operators', async () => { +test('should search features by potentially stale', async () => { await app.createFeature({ name: 'my_feature_a', - createdAt: '2023-01-27T15:21:39.975Z', + stale: false, }); await app.createFeature({ name: 'my_feature_b', - createdAt: '2023-01-29T15:21:39.975Z', + stale: true, + }); + await app.createFeature({ + name: 'my_feature_c', + stale: false, + }); + await app.createFeature({ + name: 'my_feature_d', + stale: true, }); - const { body } = await filterFeaturesByCreated('IS_BEFORE:2023-01-28'); - expect(body).toMatchObject({ - features: [{ name: 'my_feature_a' }], - }); + // this is all done on a schedule, so there's no imperative way to mark something as potentially stale today. + await db + .rawDatabase('features') + .update('potentially_stale', true) + .whereIn('name', ['my_feature_c', 'my_feature_d']); - const { body: afterBody } = await filterFeaturesByCreated( - 'IS_ON_OR_AFTER:2023-01-28', - ); - expect(afterBody).toMatchObject({ - features: [{ name: 'my_feature_b' }], - }); + const check = async (filter: string, expectedFlags: string[]) => { + const { body } = await filterFeaturesByState(filter); + expect(body).toMatchObject({ + features: expectedFlags.map((flag) => ({ name: flag })), + }); + }; + + // single filters work + await check('IS:potentiallyStale', ['my_feature_c']); + // (stale or !potentiallyStale) + await check('IS_NOT:potentiallyStale', [ + 'my_feature_a', + 'my_feature_b', + 'my_feature_d', + ]); + + // combo filters work + await check('IS_ANY_OF:active,potentiallyStale', [ + 'my_feature_a', + 'my_feature_c', + ]); + + // (potentiallyStale OR stale) + await check('IS_ANY_OF:potentiallyStale,stale', [ + 'my_feature_b', + 'my_feature_c', + 'my_feature_d', + ]); + + await check('IS_ANY_OF:active,potentiallyStale,stale', [ + 'my_feature_a', + 'my_feature_b', + 'my_feature_c', + 'my_feature_d', + ]); + + await check('IS_NONE_OF:active,potentiallyStale,stale', []); + + await check('IS_NONE_OF:active,potentiallyStale', [ + 'my_feature_b', + 'my_feature_d', + ]); + + await check('IS_NONE_OF:potentiallyStale,stale', ['my_feature_a']); }); test('should filter features by combined operators', async () => { diff --git a/src/lib/features/feature-search/search-utils.ts b/src/lib/features/feature-search/search-utils.ts index a9dbe49d0a..98ce277008 100644 --- a/src/lib/features/feature-search/search-utils.ts +++ b/src/lib/features/feature-search/search-utils.ts @@ -116,7 +116,7 @@ export const parseSearchOperatorValue = ( return { field, operator: match[1] as IQueryOperator, - values: match[2].split(','), + values: match[2].split(',').map((value) => value.trim()), }; }