Skip to content

Commit

Permalink
fixes 2-456: Preserve all data from strategy import (#2720)
Browse files Browse the repository at this point in the history
## What
Previously when importing strategies we've used the same data type we've
used for creating strategies (the minimal, a name, an optional
description, optional parameters and an optional editable column). This
meant that exporting strategies and then importing them would reactivate
deprecated strategies. This PR changes to allow the import to preserve
all the data coming in the export file.

## Tests
Added four new tests, two new unit tests using our fake stores and two
new e2e tests. Interestingly the ones in the fake store ran green before
this change as well, probably because we just insert the parsed json
object in the fake store, whereas the real store actually converts the
object from camelCasing to the postgresql snake_casing standard.

## Discussion points:
### Mismatch between fake and real stores
This is inevitable since storing things in javascript arrays vs saving
in a real database will have some differences, but this again shows the
value of our e2e tests.

### Invariants
Should we see if we can add some invariants to our import/export so that
we can write some proptests for it? One candidate is commutativity of
import/export. On a fresh database importing and then exporting should
yield the same file that was imported provided all flags are turned on.
Candidate for Q1 improvement of import/export.
  • Loading branch information
Christopher Kolstad committed Dec 21, 2022
1 parent be045dc commit 5b66346
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 6 deletions.
14 changes: 11 additions & 3 deletions src/lib/db/strategy-store.ts
Expand Up @@ -6,6 +6,7 @@ import {
IEditableStrategy,
IMinimalStrategyRow,
IStrategy,
IStrategyImport,
IStrategyStore,
} from '../types/stores/strategy-store';

Expand Down Expand Up @@ -156,9 +157,16 @@ export default class StrategyStore implements IStrategyStore {
await this.db(TABLE).where({ name }).del();
}

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
async importStrategy(data): Promise<void> {
const rowData = this.eventDataToRow(data);
async importStrategy(data: IStrategyImport): Promise<void> {
const rowData = {
name: data.name,
description: data.description,
deprecated: data.deprecated || false,
parameters: JSON.stringify(data.parameters || []),
built_in: data.builtIn ? 1 : 0,
sort_order: data.sortOrder || 9999,
display_name: data.displayName,
};
await this.db(TABLE).insert(rowData).onConflict(['name']).merge();
}

Expand Down
54 changes: 53 additions & 1 deletion src/lib/services/state-service.test.ts
Expand Up @@ -12,7 +12,7 @@ import {
PROJECT_IMPORT,
} from '../types/events';
import { GLOBAL_ENV } from '../types/environment';

import variantsExportV3 from '../../test/examples/variantsexport_v3.json';
const oldExportExample = require('./state-service-export-v1.json');

function getSetup() {
Expand Down Expand Up @@ -813,3 +813,55 @@ test('Import v1 and exporting v2 should work', async () => {
).toBeTruthy();
expect(exported.featureStrategies).toHaveLength(strategiesCount);
});

test('Importing states with deprecated strategies should keep their deprecated state', async () => {
const { stateService, stores } = getSetup();
const deprecatedStrategyExample = {
version: 4,
features: [],
strategies: [
{
name: 'deprecatedstrat',
description: 'This should be deprecated when imported',
deprecated: true,
parameters: [],
builtIn: false,
sortOrder: 9999,
displayName: 'Deprecated strategy',
},
],
featureStrategies: [],
};
await stateService.import({
data: deprecatedStrategyExample,
userName: 'strategy-importer',
dropBeforeImport: true,
keepExisting: false,
});
const deprecatedStrategy = await stores.strategyStore.get(
'deprecatedstrat',
);
expect(deprecatedStrategy.deprecated).toBe(true);
});

test('Exporting a deprecated strategy and then importing it should keep correct state', async () => {
const { stateService, stores } = getSetup();
await stateService.import({
data: variantsExportV3,
keepExisting: false,
dropBeforeImport: true,
userName: 'strategy importer',
});
const rolloutRandom = await stores.strategyStore.get(
'gradualRolloutRandom',
);
expect(rolloutRandom.deprecated).toBe(true);
const rolloutSessionId = await stores.strategyStore.get(
'gradualRolloutSessionId',
);
expect(rolloutSessionId.deprecated).toBe(true);
const rolloutUserId = await stores.strategyStore.get(
'gradualRolloutUserId',
);
expect(rolloutUserId.deprecated).toBe(true);
});
12 changes: 11 additions & 1 deletion src/lib/types/stores/strategy-store.ts
Expand Up @@ -23,6 +23,16 @@ export interface IMinimalStrategy {
parameters?: any[];
}

export interface IStrategyImport {
name: string;
description?: string;
deprecated?: boolean;
parameters?: object[];
builtIn?: boolean;
sortOrder?: number;
displayName?: string;
}

export interface IMinimalStrategyRow {
name: string;
description?: string;
Expand All @@ -36,7 +46,7 @@ export interface IStrategyStore extends Store<IStrategy, string> {
updateStrategy(update: IMinimalStrategy): Promise<void>;
deprecateStrategy({ name }: Pick<IStrategy, 'name'>): Promise<void>;
reactivateStrategy({ name }: Pick<IStrategy, 'name'>): Promise<void>;
importStrategy(data: IMinimalStrategy): Promise<void>;
importStrategy(data: IStrategyImport): Promise<void>;
dropCustomStrategies(): Promise<void>;
count(): Promise<number>;
}
50 changes: 50 additions & 0 deletions src/test/e2e/services/state-service.e2e.test.ts
Expand Up @@ -162,3 +162,53 @@ test('Should import variants in new format (per environment)', async () => {
let featureEnvironments = await stores.featureEnvironmentStore.getAll();
expect(featureEnvironments).toHaveLength(6); // 3 environments, 2 features === 6 rows
});

test('Importing states with deprecated strategies should keep their deprecated state', async () => {
const deprecatedStrategyExample = {
version: 4,
features: [],
strategies: [
{
name: 'deprecatedstrat',
description: 'This should be deprecated when imported',
deprecated: true,
parameters: [],
builtIn: false,
sortOrder: 9999,
displayName: 'Deprecated strategy',
},
],
featureStrategies: [],
};
await stateService.import({
data: deprecatedStrategyExample,
userName: 'strategy-importer',
dropBeforeImport: true,
keepExisting: false,
});
const deprecatedStrategy = await stores.strategyStore.get(
'deprecatedstrat',
);
expect(deprecatedStrategy.deprecated).toBe(true);
});

test('Exporting a deprecated strategy and then importing it should keep correct state', async () => {
await stateService.import({
data: oldFormat,
keepExisting: false,
dropBeforeImport: true,
userName: 'strategy importer',
});
const rolloutRandom = await stores.strategyStore.get(
'gradualRolloutRandom',
);
expect(rolloutRandom.deprecated).toBe(true);
const rolloutSessionId = await stores.strategyStore.get(
'gradualRolloutSessionId',
);
expect(rolloutSessionId.deprecated).toBe(true);
const rolloutUserId = await stores.strategyStore.get(
'gradualRolloutUserId',
);
expect(rolloutUserId.deprecated).toBe(true);
});
3 changes: 2 additions & 1 deletion src/test/fixtures/fake-strategies-store.ts
Expand Up @@ -2,6 +2,7 @@ import {
IEditableStrategy,
IMinimalStrategy,
IStrategy,
IStrategyImport,
IStrategyStore,
} from '../../lib/types/stores/strategy-store';
import NotFoundError from '../../lib/error/notfound-error';
Expand Down Expand Up @@ -104,7 +105,7 @@ export default class FakeStrategiesStore implements IStrategyStore {
throw new NotFoundError(`Could not find strategy with name: ${name}`);
}

async importStrategy(data: IMinimalStrategy): Promise<void> {
async importStrategy(data: IStrategyImport): Promise<void> {
return this.createStrategy(data);
}

Expand Down

0 comments on commit 5b66346

Please sign in to comment.