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

HLRC: Use Optional in validation logic #33104

Merged
merged 4 commits into from Aug 28, 2018

Conversation

@hub-cap
Copy link
Contributor

commented Aug 23, 2018

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.

HLRC: Use Optional in validation logic
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.
@gwbrown
Copy link
Contributor

left a comment

We should either go all-in on Optional, or still do all the checks - either way works, but we should pick one.

if (validationException != null && validationException.validationErrors().isEmpty() == false) {
throw validationException;
Optional<ValidationException> validationException = request.validate();
if (validationException != null && validationException.isPresent()) {

This comment has been minimized.

Copy link
@gwbrown

gwbrown Aug 23, 2018

Contributor

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.

This comment has been minimized.

Copy link
@hub-cap

hub-cap Aug 27, 2018

Author Contributor

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)

This comment has been minimized.

Copy link
@gwbrown

gwbrown Aug 27, 2018

Contributor

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()) {

This comment has been minimized.

Copy link
@gwbrown

gwbrown Aug 23, 2018

Contributor

Same as above.

*
* @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.

This comment has been minimized.

Copy link
@gwbrown

gwbrown Aug 23, 2018

Contributor

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.

This comment has been minimized.

Copy link
@hub-cap

hub-cap Aug 27, 2018

Author Contributor

yes true. Anything non null should be thrown here.

@hub-cap hub-cap merged commit 5f0f990 into elastic:master Aug 28, 2018

4 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
dnhatn added a commit that referenced this pull request Aug 28, 2018
Merge branch 'master' into ccr
* 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)
hub-cap added a commit that referenced this pull request Aug 29, 2018
HLRC: Use Optional in validation logic (#33104)
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.
dnhatn added a commit that referenced this pull request Sep 1, 2018
Merge branch '6.x' into ccr-6.x
* 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)

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.