Skip to content

Commit

Permalink
[New Platform] Validate config upfront (elastic#35453) (elastic#36439)
Browse files Browse the repository at this point in the history
* Introduce new convention for config definition.

We need to define a way to acquire configuration schema as a part of
plugin definition. Having schema we can split steps of
config validation and plugin instantiation.

* Discover plugins, read their schema and validate the config.

Config validation finished before core services and plugins read from it.
That allows us to fail fast and have predictable validation results.

* Instantiate plugins using DiscoveredPluginsDefinitions.

* Update tests for new API.

* test server is not created if config validation fails

* move plugin discovery to plugin service pre-setup stage.

Set validation schemes in ConfigService.preSetup stage.

* fix eslint problem

* generate docs

* address Rudolfs comments

* separate core services and plugins validation

* rename files for consistency

* address comments for root.js

* address comments #1

* useSchema everywhere for consistency. get rid of validateAll

* plugin system runs plugin config validation

* rename configDefinition

* move plugin schema registration in plugins plugins service

plugins system is not setup when kibana is run in optimizer mode,
so config keys aren't marked as applied.

* cleanup

* update docs

* address comments
  • Loading branch information
mshustov authored May 10, 2019
1 parent 92e70ca commit 581d499
Show file tree
Hide file tree
Showing 33 changed files with 524 additions and 233 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [ConfigService](./kibana-plugin-server.configservice.md) &gt; [setSchema](./kibana-plugin-server.configservice.setschema.md)

## ConfigService.setSchema() method

Set config schema for a path and performs its validation

<b>Signature:</b>

```typescript
setSchema(path: ConfigPath, schema: Type<unknown>): Promise<void>;
```

## Parameters

| Parameter | Type | Description |
| --- | --- | --- |
| path | <code>ConfigPath</code> | |
| schema | <code>Type&lt;unknown&gt;</code> | |

<b>Returns:</b>

`Promise<void>`

17 changes: 0 additions & 17 deletions src/core/server/__snapshots__/server.test.ts.snap

This file was deleted.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions src/core/server/config/config_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,16 @@ import { ObjectToConfigAdapter } from './object_to_config_adapter';

import { ConfigService } from './config_service';

type ConfigSericeContract = PublicMethodsOf<ConfigService>;
type ConfigServiceContract = PublicMethodsOf<ConfigService>;
const createConfigServiceMock = () => {
const mocked: jest.Mocked<ConfigSericeContract> = {
const mocked: jest.Mocked<ConfigServiceContract> = {
atPath: jest.fn(),
getConfig$: jest.fn(),
optionalAtPath: jest.fn(),
getUsedPaths: jest.fn(),
getUnusedPaths: jest.fn(),
isEnabledAtPath: jest.fn(),
setSchema: jest.fn(),
};
mocked.atPath.mockReturnValue(new BehaviorSubject({}));
mocked.getConfig$.mockReturnValue(new BehaviorSubject(new ObjectToConfigAdapter({})));
Expand Down
118 changes: 74 additions & 44 deletions src/core/server/config/config_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,16 @@ const emptyArgv = getEnvOptions();
const defaultEnv = new Env('/kibana', emptyArgv);
const logger = loggingServiceMock.create();

class ExampleClassWithStringSchema {
public static schema = schema.string();

constructor(readonly value: string) {}
}

test('returns config at path as observable', async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'foo' }));
const configService = new ConfigService(config$, defaultEnv, logger);
await configService.setSchema('key', schema.string());

const configs = configService.atPath('key', ExampleClassWithStringSchema);
const exampleConfig = await configs.pipe(first()).toPromise();
Expand All @@ -45,22 +52,45 @@ test('returns config at path as observable', async () => {
});

test('throws if config at path does not match schema', async () => {
expect.assertions(1);

const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 123 }));

const configService = new ConfigService(config$, defaultEnv, logger);
const configs = configService.atPath('key', ExampleClassWithStringSchema);

try {
await configs.pipe(first()).toPromise();
} catch (e) {
expect(e.message).toMatchSnapshot();
}
await expect(
configService.setSchema('key', schema.string())
).rejects.toThrowErrorMatchingInlineSnapshot(
`"[key]: expected value of type [string] but got [number]"`
);
});

test('re-validate config when updated', async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));

const configService = new ConfigService(config$, defaultEnv, logger);
configService.setSchema('key', schema.string());

const valuesReceived: any[] = [];
await configService.atPath('key', ExampleClassWithStringSchema).subscribe(
config => {
valuesReceived.push(config.value);
},
error => {
valuesReceived.push(error);
}
);

config$.next(new ObjectToConfigAdapter({ key: 123 }));

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

test("returns undefined if fetching optional config at a path that doesn't exist", async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ foo: 'bar' }));
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({}));
const configService = new ConfigService(config$, defaultEnv, logger);

const configs = configService.optionalAtPath('unique-name', ExampleClassWithStringSchema);
Expand All @@ -72,6 +102,7 @@ test("returns undefined if fetching optional config at a path that doesn't exist
test('returns observable config at optional path if it exists', async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ value: 'bar' }));
const configService = new ConfigService(config$, defaultEnv, logger);
await configService.setSchema('value', schema.string());

const configs = configService.optionalAtPath('value', ExampleClassWithStringSchema);
const exampleConfig: any = await configs.pipe(first()).toPromise();
Expand All @@ -83,6 +114,7 @@ test('returns observable config at optional path if it exists', async () => {
test("does not push new configs when reloading if config at path hasn't changed", async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));
const configService = new ConfigService(config$, defaultEnv, logger);
await configService.setSchema('key', schema.string());

const valuesReceived: any[] = [];
configService.atPath('key', ExampleClassWithStringSchema).subscribe(config => {
Expand All @@ -97,6 +129,7 @@ test("does not push new configs when reloading if config at path hasn't changed"
test('pushes new config when reloading and config at path has changed', async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));
const configService = new ConfigService(config$, defaultEnv, logger);
await configService.setSchema('key', schema.string());

const valuesReceived: any[] = [];
configService.atPath('key', ExampleClassWithStringSchema).subscribe(config => {
Expand All @@ -108,21 +141,27 @@ test('pushes new config when reloading and config at path has changed', async ()
expect(valuesReceived).toEqual(['value', 'new value']);
});

test("throws error if config class does not implement 'schema'", async () => {
expect.assertions(1);

test("throws error if 'schema' is not defined for a key", async () => {
class ExampleClass {}

const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));
const configService = new ConfigService(config$, defaultEnv, logger);

const configs = configService.atPath('key', ExampleClass as any);

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

test("throws error if 'setSchema' called several times for the same key", async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));
const configService = new ConfigService(config$, defaultEnv, logger);
const addSchema = async () => await configService.setSchema('key', schema.string());
await addSchema();
await expect(addSchema()).rejects.toMatchInlineSnapshot(
`[Error: Validation schema for key was already registered.]`
);
});

test('tracks unhandled paths', async () => {
Expand Down Expand Up @@ -178,28 +217,25 @@ test('correctly passes context', async () => {
const env = new Env('/kibana', getEnvOptions());

const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ foo: {} }));
const schemaDefinition = schema.object({
branchRef: schema.string({
defaultValue: schema.contextRef('branch'),
}),
buildNumRef: schema.number({
defaultValue: schema.contextRef('buildNum'),
}),
buildShaRef: schema.string({
defaultValue: schema.contextRef('buildSha'),
}),
devRef: schema.boolean({ defaultValue: schema.contextRef('dev') }),
prodRef: schema.boolean({ defaultValue: schema.contextRef('prod') }),
versionRef: schema.string({
defaultValue: schema.contextRef('version'),
}),
});
const configService = new ConfigService(config$, env, logger);
const configs = configService.atPath(
'foo',
createClassWithSchema(
schema.object({
branchRef: schema.string({
defaultValue: schema.contextRef('branch'),
}),
buildNumRef: schema.number({
defaultValue: schema.contextRef('buildNum'),
}),
buildShaRef: schema.string({
defaultValue: schema.contextRef('buildSha'),
}),
devRef: schema.boolean({ defaultValue: schema.contextRef('dev') }),
prodRef: schema.boolean({ defaultValue: schema.contextRef('prod') }),
versionRef: schema.string({
defaultValue: schema.contextRef('version'),
}),
})
)
);
await configService.setSchema('foo', schemaDefinition);
const configs = configService.atPath('foo', createClassWithSchema(schemaDefinition));

expect(await configs.pipe(first()).toPromise()).toMatchSnapshot();
});
Expand Down Expand Up @@ -278,9 +314,3 @@ function createClassWithSchema(s: Type<any>) {
constructor(readonly value: TypeOf<typeof s>) {}
};
}

class ExampleClassWithStringSchema {
public static schema = schema.string();

constructor(readonly value: string) {}
}
64 changes: 40 additions & 24 deletions src/core/server/config/config_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export class ConfigService {
* then list all unhandled config paths when the startup process is completed.
*/
private readonly handledPaths: ConfigPath[] = [];
private readonly schemas = new Map<string, Type<unknown>>();

constructor(
private readonly config$: Observable<Config>,
Expand All @@ -43,6 +44,22 @@ export class ConfigService {
this.log = logger.get('config');
}

/**
* Set config schema for a path and performs its validation
*/
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.`);
}

this.schemas.set(namespace, schema);

await this.validateConfig(path)
.pipe(first())
.toPromise();
}

/**
* Returns the full config object observable. This is not intended for
* "normal use", but for features that _need_ access to the full object.
Expand All @@ -59,13 +76,11 @@ export class ConfigService {
* @param ConfigClass - A class (not an instance of a class) that contains a
* static `schema` that we validate the config at the given `path` against.
*/
public atPath<TSchema extends Type<any>, TConfig>(
public atPath<TSchema extends Type<unknown>, TConfig>(
path: ConfigPath,
ConfigClass: ConfigWithSchema<TSchema, TConfig>
) {
return this.getDistinctConfig(path).pipe(
map(config => this.createConfig(path, config, ConfigClass))
);
return this.validateConfig(path).pipe(map(config => this.createConfig(config, ConfigClass)));
}

/**
Expand All @@ -79,9 +94,11 @@ export class ConfigService {
ConfigClass: ConfigWithSchema<TSchema, TConfig>
) {
return this.getDistinctConfig(path).pipe(
map(config =>
config === undefined ? undefined : this.createConfig(path, config, ConfigClass)
)
map(config => {
if (config === undefined) return undefined;
const validatedConfig = this.validate(path, config);
return this.createConfig(validatedConfig, ConfigClass);
})
);
}

Expand Down Expand Up @@ -122,24 +139,13 @@ export class ConfigService {
return config.getFlattenedPaths().filter(path => isPathHandled(path, handledPaths));
}

private createConfig<TSchema extends Type<any>, TConfig>(
path: ConfigPath,
config: Record<string, any>,
ConfigClass: ConfigWithSchema<TSchema, TConfig>
) {
const namespace = Array.isArray(path) ? path.join('.') : path;

const configSchema = ConfigClass.schema;

if (configSchema === undefined || typeof configSchema.validate !== 'function') {
throw new Error(
`The config class [${
ConfigClass.name
}] did not contain a static 'schema' field, which is required when creating a config instance`
);
private validate(path: ConfigPath, config: Record<string, unknown>) {
const namespace = pathToString(path);
const schema = this.schemas.get(namespace);
if (!schema) {
throw new Error(`No validation schema has been defined for ${namespace}`);
}

const validatedConfig = ConfigClass.schema.validate(
return schema.validate(
config,
{
dev: this.env.mode.dev,
Expand All @@ -148,9 +154,19 @@ export class ConfigService {
},
namespace
);
}

private createConfig<TSchema extends Type<unknown>, TConfig>(
validatedConfig: unknown,
ConfigClass: ConfigWithSchema<TSchema, TConfig>
) {
return new ConfigClass(validatedConfig, this.env);
}

private validateConfig(path: ConfigPath) {
return this.getDistinctConfig(path).pipe(map(config => this.validate(path, config)));
}

private getDistinctConfig(path: ConfigPath) {
this.markAsHandled(path);

Expand Down
10 changes: 7 additions & 3 deletions src/core/server/dev/dev_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@ const createDevSchema = schema.object({
}),
});

type DevConfigType = TypeOf<typeof createDevSchema>;
export const config = {
path: 'dev',
schema: createDevSchema,
};

export type DevConfigType = TypeOf<typeof createDevSchema>;
export class DevConfig {
/**
* @internal
Expand All @@ -38,7 +42,7 @@ export class DevConfig {
/**
* @internal
*/
constructor(config: DevConfigType) {
this.basePathProxyTargetPort = config.basePathProxyTarget;
constructor(rawConfig: DevConfigType) {
this.basePathProxyTargetPort = rawConfig.basePathProxyTarget;
}
}
Loading

0 comments on commit 581d499

Please sign in to comment.