HLRC: Use Optional in validation logic #33104
Conversation
The Validatable class comes from an old class in server code, that assumed null was returned in the event of validation having no errors. This commit changes that to use Optional, which is cleaner than passing around null objects.
| if (validationException != null && validationException.validationErrors().isEmpty() == false) { | ||
| throw validationException; | ||
| Optional<ValidationException> validationException = request.validate(); | ||
| if (validationException != null && validationException.isPresent()) { |
There was a problem hiding this comment.
The philosophy I've usually gone with is that you shouldn't null-check Optionals - just let it throw an NPE because you use an empty Optional to represent "no value" so a null value means something is misbehaving.
The counterargument is that leaving the null-check here is probably safer, especially if we're converting code that used to use null to represent "no value" - but if we're going to go with that logic, we should probably still check for the isEmpty() case too.
There was a problem hiding this comment.
We used to the null check to mean "no validation errors", therefore I am using isPresent(). My only fear about removing the null check could trip people because they might still return null, instead of Optional.EMPTY or Optional.ofNullable.
I think the isEmpty() is less of an issue than before, given that we have to redo the code here anyway. Its just a matter of accounting for the null check itself. I think its worthwhile to remove the null check once we have moved all the actions over, but to remove the isEmpty() check now to reduce a potential bug (any non null validation exception will be assumed to be a valid exception). Thoughts?
(edit: remove bullet point at the begin of first sentence)
There was a problem hiding this comment.
Both used to mean "no validation errors", but looking through the code it looks like returning a request where isEmpty() is true is pretty rare. So I'm okay with removing that check now and removing the null check later.
| if (validationException != null && validationException.validationErrors().isEmpty() == false) { | ||
| listener.onFailure(validationException); | ||
| Optional<ValidationException> validationException = request.validate(); | ||
| if (validationException != null && validationException.isPresent()) { |
| * | ||
| * @return potentially null, in the event of older actions, an empty {@link ValidationException} in newer actions, or finally a | ||
| * {@link ValidationException} that contains a list of all failed validation. | ||
| * @return An {@link Optional} {@link ValidationException} that may or may not contain a list of validation errors. |
There was a problem hiding this comment.
If we aren't checking for isEmpty(), we should make sure to say in the documentation that if the Optional isn't empty, the ValidationException should contain at least one error, rather than "may or may not" - otherwise we might end up throwing an empty ValidationException.
There was a problem hiding this comment.
yes true. Anything non null should be thrown here.
* master: [Rollup] Better error message when trying to set non-rollup index (#32965) HLRC: Use Optional in validation logic (#33104) Remove unused User class from protocol (#33137) ingest: Introduce the dissect processor (#32884) [Docs] Add link to es-kotlin-wrapper-client (#32618) [Docs] Remove repeating words (#33087) Minor spelling and grammar fix (#32931) Remove support for deprecated params._agg/_aggs for scripted metric aggregations (#32979) Watcher: Simplify finding next date in cron schedule (#33015) Run Third party audit with forbidden APIs CLI (part3/3) (#33052) Fix plugin build test on Windows (#33078) HLRC+MINOR: Remove Unused Private Method (#33165) Remove old unused test script files (#32970) Build analysis-icu client JAR (#33184) Ensure to generate identical NoOp for the same failure (#33141) ShardSearchFailure#readFrom to set index and shardId (#33161)
The Validatable class comes from an old class in server code, that assumed null was returned in the event of validation having no errors. This commit changes that to use Optional, which is cleaner than passing around null objects.
* 6.x: Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test TEST: Disable soft-deletes in ParentChildTestCase TEST: Disable randomized soft-deletes settings Integrates soft-deletes into Elasticsearch (#33222) drop `index.shard.check_on_startup: fix` (#32279) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi [DOCS] Moves ml folder from x-pack/docs to docs (#33248) TEST: mute more SmokeTestWatcherWithSecurityIT tests [DOCS] Move rollup APIs to docs (#31450) [DOCS] Rename X-Pack Commands section (#33005) Fixes SecurityIntegTestCase so it always adds at least one alias (#33296) TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307) MINOR: Remove Dead Code from PathTrie (#33280) (#33306) Fix pom for build-tools (#33300) Lazy evaluate java9home (#33301) SQL: test coverage for JdbcResultSet (#32813) Work around to be able to generate eclipse projects (#33295) Different handling for security specific errors in the CLI. Fix for #33230 (#33255) [ML] Refactor delimited file structure detection (#33233) SQL: Support multi-index format as table identifier (#33278) Enable forbiddenapis server java9 (#33245) [MUTE] SmokeTestWatcherWithSecurityIT flaky tests Add region ISO code to GeoIP Ingest plugin (#31669) (#33276) Don't be strict for 6.x Update serialization versions for custom IndexMetaData backport Replace IndexMetaData.Custom with Map-based custom metadata (#32749) Painless: Fix Bindings Bug (#33274) SQL: prevent duplicate generation for repeated aggs (#33252) TEST: Mute testMonitorClusterHealth Fix serialization of empty field capabilities response (#33263) Fix nested _source retrieval with includes/excludes (#33180) [DOCS] TLS file resources are reloadable (#33258) Watcher: Ensure TriggerEngine start replaces existing watches (#33157) Ignore module-info in jar hell checks (#33011) Fix docs build after #33241 [DOC] Repository GCS ADC not supported (#33238) Upgrade to latest Gradle 4.10 (#32801) Fix/30904 cluster formation part2 (#32877) Move file-based discovery to core (#33241) HLRC: add client side RefreshPolicy (#33209) [Kerberos] Add unsupported languages for tests (#33253) Watcher: Reload properly on remote shard change (#33167) Fix classpath security checks for external tests. (#33066) [Rollup] Only allow aggregating on multiples of configured interval (#32052) Added deprecation warning for rescore in scroll queries (#33070) Apply settings filter to get cluster settings API (#33247) [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability (#32743) HLRC: create base timed request class (#33216) HLRC: Use Optional in validation logic (#33104) Painless: Add Bindings (#33042)
The Validatable class comes from an old class in server code, that
assumed null was returned in the event of validation having no
errors. This commit changes that to use Optional, which is cleaner than
passing around null objects.