Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ test('throws if reading "enabled" when it is not present in the schema', async (
await expect(
async () => await configService.isEnabledAtPath('foo')
).rejects.toThrowErrorMatchingInlineSnapshot(
`"[config validation of [foo].enabled]: definition for this key is missing"`
`"[foo]: enabled status cannot be changed. Please, remove [foo.enabled] from the configuration file."`
);
});

Expand Down
20 changes: 16 additions & 4 deletions src/platform/packages/shared/kbn-config/src/config_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,22 @@ export class ConfigService {
typeof config.get(path)?.enabled !== 'undefined' &&
typeof validatedConfig?.enabled === 'undefined'
) {
validatedConfig = await firstValueFrom(
this.getValidatedConfigAtPath$(path) as Observable<{ enabled?: boolean }>,
{ defaultValue: undefined }
);
try {
validatedConfig = await firstValueFrom(
this.getValidatedConfigAtPath$(path) as Observable<{ enabled?: boolean }>,
{ defaultValue: undefined }
);
} catch (error) {
if (error instanceof ValidationError) {
throw new ValidationError(
new SchemaTypeError(
Copy link
Member

Choose a reason for hiding this comment

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

so these are ultimately schema checks and we do have a better way to handle throwing custom validation errors within the schema validate function itself. https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-config-schema/README.md#custom-validation

any reason why you did not do that here instead of catching and rethrowing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello! I reviewed the custom validation you shared, and from what I understand, it would mean adding custom validation to each plugin’s schema for plugins that don’t use the enabled field. To avoid updating all those plugins and because the isEnabledAtPath function is called for every discovered plugin when kibana is booting up, I figured I would do a try catch around the specific scenario in the code instead.

I also based my changes on the comments in this PR which I think suggest catching and throwing a specific error message.

Let me know if I missed anything or if you have other suggestions!

`enabled status cannot be changed. Please, remove [${namespace}.enabled] from the configuration file.`,
[namespace]
)
);
}
throw error;
}
}

const isDisabled = validatedConfig?.enabled === false;
Expand Down