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.

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

allows plugins to define validation schema for "enabled" flag #50286

Merged
merged 4 commits into from
Nov 18, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
85 changes: 77 additions & 8 deletions src/core/server/config/config_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ test('throws if config at path does not match schema', async () => {
await expect(
configService.setSchema('key', schema.string())
).rejects.toThrowErrorMatchingInlineSnapshot(
`"[key]: expected value of type [string] but got [number]"`
`"[config validation of [key]]: expected value of type [string] but got [number]"`
);
});

Expand All @@ -78,11 +78,11 @@ test('re-validate config when updated', async () => {
config$.next(new ObjectToConfigAdapter({ key: 123 }));

await expect(valuesReceived).toMatchInlineSnapshot(`
Array [
"value",
[Error: [key]: expected value of type [string] but got [number]],
]
`);
Array [
"value",
[Error: [config validation of [key]]: expected value of type [string] but got [number]],
]
`);
});

test("returns undefined if fetching optional config at a path that doesn't exist", async () => {
Expand Down Expand Up @@ -143,7 +143,7 @@ test("throws error if 'schema' is not defined for a key", async () => {
const configs = configService.atPath('key');

await expect(configs.pipe(first()).toPromise()).rejects.toMatchInlineSnapshot(
`[Error: No validation schema has been defined for key]`
`[Error: No validation schema has been defined for [key]]`
);
});

Expand All @@ -153,7 +153,7 @@ test("throws error if 'setSchema' called several times for the same key", async
const addSchema = async () => await configService.setSchema('key', schema.string());
await addSchema();
await expect(addSchema()).rejects.toMatchInlineSnapshot(
`[Error: Validation schema for key was already registered.]`
`[Error: Validation schema for [key] was already registered.]`
);
});

Expand Down Expand Up @@ -280,6 +280,33 @@ test('handles disabled path and marks config as used', async () => {
expect(unusedPaths).toEqual([]);
});

test('does not throw if schema does not define "enabled" schema', async () => {
const initialConfig = {
pid: {
file: '/some/file.pid',
},
};

const config$ = new BehaviorSubject(new ObjectToConfigAdapter(initialConfig));
const configService = new ConfigService(config$, defaultEnv, logger);
expect(
configService.setSchema(
'pid',
schema.object({
file: schema.string(),
})
)
).resolves.toBeUndefined();

const value$ = configService.atPath('pid');
const value: any = await value$.pipe(first()).toPromise();
expect(value.enabled).toBe(undefined);

const valueOptional$ = configService.optionalAtPath('pid');
const valueOptional: any = await valueOptional$.pipe(first()).toPromise();
expect(valueOptional.enabled).toBe(undefined);
});

test('treats config as enabled if config path is not present in config', async () => {
const initialConfig = {};

Expand All @@ -292,3 +319,45 @@ test('treats config as enabled if config path is not present in config', async (
const unusedPaths = await configService.getUnusedPaths();
expect(unusedPaths).toEqual([]);
});

test('read "enabled" even if its schema is not present', async () => {
const initialConfig = {
foo: {
enabled: true,
},
};

const config$ = new BehaviorSubject(new ObjectToConfigAdapter(initialConfig));
const configService = new ConfigService(config$, defaultEnv, logger);

const isEnabled = await configService.isEnabledAtPath('foo');
expect(isEnabled).toBe(true);
});

test('allows plugins to specify "enabled" flag via validation schema', async () => {
const initialConfig = {};

const config$ = new BehaviorSubject(new ObjectToConfigAdapter(initialConfig));
const configService = new ConfigService(config$, defaultEnv, logger);

await configService.setSchema(
'foo',
schema.object({ enabled: schema.boolean({ defaultValue: false }) })
);

expect(await configService.isEnabledAtPath('foo')).toBe(false);

await configService.setSchema(
'bar',
schema.object({ enabled: schema.boolean({ defaultValue: true }) })
);

expect(await configService.isEnabledAtPath('bar')).toBe(true);

await configService.setSchema(
'baz',
schema.object({ different: schema.boolean({ defaultValue: true }) })
);

expect(await configService.isEnabledAtPath('baz')).toBe(true);
});
26 changes: 20 additions & 6 deletions src/core/server/config/config_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export class ConfigService {
public async setSchema(path: ConfigPath, schema: Type<unknown>) {
const namespace = pathToString(path);
if (this.schemas.has(namespace)) {
throw new Error(`Validation schema for ${path} was already registered.`);
throw new Error(`Validation schema for [${path}] was already registered.`);
}

this.schemas.set(namespace, schema);
Expand Down Expand Up @@ -98,14 +98,28 @@ export class ConfigService {
}

public async isEnabledAtPath(path: ConfigPath) {
const enabledPath = createPluginEnabledPath(path);
const namespace = pathToString(path);

const validatedConfig = this.schemas.has(namespace)
? await this.atPath<{ enabled?: boolean }>(path)
.pipe(first())
.toPromise()
: undefined;

const enabledPath = createPluginEnabledPath(path);
const config = await this.config$.pipe(first()).toPromise();
if (!config.has(enabledPath)) {

// if plugin hasn't got a config schema, we try to read "enabled" directly
const isEnabled =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative solution, we can always extend the plugin config with a schema for the enabled flag. Although, I'd prefer to do not touch the user schema.

validatedConfig && validatedConfig.enabled !== undefined
? validatedConfig.enabled
: config.get(enabledPath);

// not declared. consider that plugin is enabled by default
if (isEnabled === undefined) {
return true;
}

const isEnabled = config.get(enabledPath);
if (isEnabled === false) {
// If the plugin is _not_ enabled, we mark the entire plugin path as
// handled, as it's expected that it won't be used.
Expand Down Expand Up @@ -138,7 +152,7 @@ export class ConfigService {
const namespace = pathToString(path);
const schema = this.schemas.get(namespace);
if (!schema) {
throw new Error(`No validation schema has been defined for ${namespace}`);
throw new Error(`No validation schema has been defined for [${namespace}]`);
}
return schema.validate(
config,
Expand All @@ -147,7 +161,7 @@ export class ConfigService {
prod: this.env.mode.prod,
...this.env.packageInfo,
},
namespace
`config validation of [${namespace}]`
);
}

Expand Down