Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
35 changes: 35 additions & 0 deletions packages/kbn-config-schema/src/types/object_type.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,41 @@ describe('nested unknowns', () => {
},
});
});

describe(`stripUnknownKeys: true in validate`, () => {
test('should strip unknown keys', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

proving that stripUnknownKeys works as intended

const type = schema.object({
myObj: schema.object({
foo: schema.string({ defaultValue: 'test' }),
nested: schema.object({
a: schema.number(),
}),
}),
});

expect(
type.validate(
{
myObj: {
bar: 'baz',
nested: {
a: 1,
b: 2,
},
},
},
void 0,
void 0,
{ stripUnknownKeys: true }
)
).toStrictEqual({
myObj: {
foo: 'test',
nested: { a: 1 },
},
});
});
});
});

test('handles optional properties', () => {
Expand Down
31 changes: 31 additions & 0 deletions packages/kbn-config/src/config_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,37 @@ test('handles disabled path and marks config as used', async () => {
expect(unusedPaths).toEqual([]);
});

test('does not throw if extra options are provided', async () => {
const initialConfig = {
pid: {
enabled: false,
file: '/some/file.pid',
extraUnknownOption: 1,
extraNestedUnknownOptions: {
anOption: true,
anotherOption: 'something',
},
},
};

const rawConfigProvider = createRawConfigServiceMock({ rawConfig: initialConfig });
const configService = new ConfigService(rawConfigProvider, defaultEnv, logger);

configService.setSchema(
'pid',
schema.object({
enabled: schema.boolean({ defaultValue: false }),
file: schema.string(),
})
);

const isEnabled = await configService.isEnabledAtPath('pid');
expect(isEnabled).toBe(false);

const unusedPaths = await configService.getUnusedPaths();
expect(unusedPaths).toEqual([]);
});

test('does not throw if schema does not define "enabled" schema', async () => {
const initialConfig = {
pid: {
Expand Down
44 changes: 37 additions & 7 deletions packages/kbn-config/src/config_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { SchemaTypeError, Type, ValidationError } from '@kbn/config-schema';
import { cloneDeep, isEqual, merge, unset } from 'lodash';
import { set } from '@kbn/safer-lodash-set';
import { BehaviorSubject, combineLatest, firstValueFrom, Observable, identity } from 'rxjs';
import { distinctUntilChanged, first, map, shareReplay, tap } from 'rxjs';
import { distinctUntilChanged, map, shareReplay, tap } from 'rxjs';
import { Logger, LoggerFactory } from '@kbn/logging';
import { getDocLinks, DocLinks } from '@kbn/doc-links';

Expand Down Expand Up @@ -209,10 +209,31 @@ export class ConfigService {
throw new Error(`No validation schema has been defined for [${namespace}]`);
}

const validatedConfig = hasSchema
? await this.atPath<{ enabled?: boolean }>(path).pipe(first()).toPromise()
let validatedConfig = hasSchema
? await firstValueFrom(
Copy link
Member Author

Choose a reason for hiding this comment

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

I took the opportunity to change the deprecated usage of .toPromise()

this.getValidatedConfigAtPath$(
path,
// At this point we don't care about how valid the config is: we just want to read `enabled`
{ stripUnknownKeys: true }
) as Observable<{ enabled?: boolean }>,
{ defaultValue: undefined }
)
: undefined;

// Special use case: when the provided config includes `enabled` and the validated config doesn't,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only at the top level of the object?

Also could we add a test case for this?

Copy link
Member Author

@afharo afharo Dec 2, 2024

Choose a reason for hiding this comment

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

This is the existing test that reminded me that I would introduce a regression with simply using stripUnknownKeys: true above:

test('throws if reading "enabled" when it is not present in the schema', async () => {
const initialConfig = {
foo: {
enabled: false,
},
};
const rawConfigProvider = createRawConfigServiceMock({ rawConfig: initialConfig });
const configService = new ConfigService(rawConfigProvider, defaultEnv, logger);
configService.setSchema(
'foo',
schema.object({
bar: schema.maybe(schema.string()),
})
);
await expect(
async () => await configService.isEnabledAtPath('foo')
).rejects.toThrowErrorMatchingInlineSnapshot(
`"[config validation of [foo].enabled]: definition for this key is missing"`
);
});

// it's quite likely that's not an allowed config and it should fail.
// Applying "normal" validation (not stripping unknowns) in that case.
if (
hasSchema &&
typeof config.get(path)?.enabled !== 'undefined' &&
typeof validatedConfig?.enabled === 'undefined'
) {
validatedConfig = await firstValueFrom(
this.getValidatedConfigAtPath$(path) as Observable<{ enabled?: boolean }>,
{ defaultValue: undefined }
);
}
Comment on lines +223 to +235
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to add this special use case since stripUnknownKeys effectively breaks it 😿

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense, although it's possible that the validation error may contain more than just "enabled"... perhaps we can catch and throw a more specific message here about the special "enabled" setting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point! I created #202490 for us to discuss the new potential message.


const isDisabled = validatedConfig?.enabled === false;
if (isDisabled) {
// If the plugin is explicitly disabled, we mark the entire plugin
Expand Down Expand Up @@ -325,7 +346,13 @@ export class ConfigService {
});
}

private validateAtPath(path: ConfigPath, config: Record<string, unknown>) {
private validateAtPath(
path: ConfigPath,
config: Record<string, unknown>,
validateOptions?: { stripUnknownKeys?: boolean }
) {
const stripUnknownKeys = validateOptions?.stripUnknownKeys || this.stripUnknownKeys;

const namespace = pathToString(path);
const schema = this.schemas.get(namespace);
if (!schema) {
Expand All @@ -340,18 +367,21 @@ export class ConfigService {
...this.env.packageInfo,
},
`config validation of [${namespace}]`,
this.stripUnknownKeys ? { stripUnknownKeys: this.stripUnknownKeys } : {}
stripUnknownKeys ? { stripUnknownKeys } : {}
);
}

private getValidatedConfigAtPath$(
path: ConfigPath,
{ ignoreUnchanged = true }: { ignoreUnchanged?: boolean } = {}
{
ignoreUnchanged = true,
stripUnknownKeys,
}: { ignoreUnchanged?: boolean; stripUnknownKeys?: boolean } = {}
) {
return this.config$.pipe(
map((config) => config.get(path)),
ignoreUnchanged ? distinctUntilChanged(isEqual) : identity,
map((config) => this.validateAtPath(path, config))
map((config) => this.validateAtPath(path, config, { stripUnknownKeys }))
);
}

Expand Down