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

Use schema.never() for dev/!dist config parameters #110239

Open
afharo opened this issue Aug 26, 2021 · 6 comments
Open

Use schema.never() for dev/!dist config parameters #110239

afharo opened this issue Aug 26, 2021 · 6 comments
Labels
discuss enhancement New value added to drive a business result Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@afharo
Copy link
Member

afharo commented Aug 26, 2021

Related discussion: #107663 (comment)

Should we set schema.never() instead of schema.boolean({ validate: (rawValue) => if (rawValue === true) return 'validation error message'; }) for those config entries that are not supported in production?

@afharo afharo added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc enhancement New value added to drive a business result labels Aug 26, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet
Copy link
Contributor

Imho having an explicit error message is more important than an technical implementation detail, so id I had to choose, I'd say to keep the current code.

@afharo
Copy link
Member Author

afharo commented Sep 2, 2021

That brings me to the next question: since the only thing stopping us from using schema.never() is that the message is not clear enough, should we improve it to allow custom validation messages?

@pgayvallet
Copy link
Contributor

is that the message is not clear enough, should we improve it to allow custom validation messages?

Not sure I really like allowing custom error messages for joi types, as it can lead to some overcomplicated things. OTOH, for never, there is only one rule (forbidden), so I guess we could add a customMessage option to the type. It would probably be fine

constructor() {
super(internals.any().forbidden());
}
protected handleError(type: string) {
switch (type) {
case 'any.unknown':
return "a value wasn't expected to be present";
}
}

@afharo
Copy link
Member Author

afharo commented Sep 3, 2021

Alternatively, we could improve the default error message to [key]: is not allowed. However, since we normally use it inside a conditional, we may need the conditional to report why ... when 'dist' is 'true'.

That sounds like a lot of work (and users probably will wonder how to set dist to false) 😅

I think the customMessage approach for this type would be good enough.

What do you think?

@pgayvallet
Copy link
Contributor

we may need the conditional to report why ... when 'dist' is 'true'.

Realistically, it can't be automated, as the amount of work to parse the JOI schema tree would be enormous. So the only option for such specific message would be the customMessage option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss enhancement New value added to drive a business result Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

3 participants