Validate index name time format setting at parse time #47911
Conversation
|
Pinging @elastic/es-core-features (:Core/Features/Monitoring) |
|
Test fixed. |
| public void validate(String value) { | ||
| try { | ||
| DateFormatter.forPattern(value).withZone(ZoneOffset.UTC); | ||
| } catch (IllegalArgumentException e) { |
There was a problem hiding this comment.
I would suggest to open this up to catch RuntimeException so that validation (not cluster state application) catches the error.
|
@elasticmachine run elasticsearch-ci/2 |
|
@jakelandis, I think this one is ready for another look. |
| public void validate(String value) { | ||
| try { | ||
| if (value != null) { | ||
| DateFormatter.forPattern(value).withZone(ZoneOffset.UTC); |
There was a problem hiding this comment.
Is the generic type for this AffixSetting wrong? It seems like if we are going to parse into a DateFormatter, we should be doing that once, not duplicating here and in Exporter.dateTimeFormatter (where it also looks like we have default value logic?).
There was a problem hiding this comment.
@rjernst, it is true that a DateFormatter instance is ultimately what is produced for this setting. I could change INDEX_NAME_TIME_FORMAT_SETTING from its original AffixSetting<String> to a AffixSetting<DateFormatter> if that would be preferable.
There was a problem hiding this comment.
I think that makes the most sense. In general, a major goal of the settings infrastructure is to do type parsing and validation for the intended type. In this case it seems we are adding checking, but the parsing isn't actually happening within the Setting instance.
|
@elasticmachine update branch |
|
@rjernst, I've updated this to be an |
Provides parse-time validation for
INDEX_NAME_TIME_FORMAT_SETTINGas described in #47711.