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.

Sign up for GitHub

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

Jump to bottom

HLRC: Use Optional in validation logic #33104

Merged
merged 4 commits into from
Aug 28, 2018
Merged

HLRC: Use Optional in validation logic #33104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6504612
HLRC: Use Optional in validation logic
hub-cap Aug 23, 2018
1c6b618
Merge remote-tracking branch 'upstream/master' into hlrc_optional_val…
hub-cap Aug 27, 2018
55d522c
Fix validation wording for clarification
hub-cap Aug 27, 2018
2df9d9c
Merge remote-tracking branch 'upstream/master' into hlrc_optional_val…
hub-cap Aug 27, 2018
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
13 changes: 7 additions & 6 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.function.Function;
Expand Down Expand Up @@ -1011,9 +1012,9 @@ protected final <Req extends Validatable, Resp> Resp performRequest(Req request,
RequestOptions options,
CheckedFunction<Response, Resp, IOException> responseConverter,
Set<Integer> ignores) throws IOException {
ValidationException validationException = request.validate();
if (validationException != null && validationException.validationErrors().isEmpty() == false) {
throw validationException;
Optional<ValidationException> validationException = request.validate();
if (validationException != null && validationException.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@hub-cap hub-cap Aug 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

throw validationException.get();
}
return internalPerformRequest(request, requestConverter, options, responseConverter, ignores);
}
Expand Down Expand Up @@ -1106,9 +1107,9 @@ protected final <Req extends Validatable, Resp> void performRequestAsync(Req req
RequestOptions options,
CheckedFunction<Response, Resp, IOException> responseConverter,
ActionListener<Resp> listener, Set<Integer> ignores) {
ValidationException validationException = request.validate();
if (validationException != null && validationException.validationErrors().isEmpty() == false) {
listener.onFailure(validationException);
Optional<ValidationException> validationException = request.validate();
if (validationException != null && validationException.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

listener.onFailure(validationException.get());
return;
}
internalPerformRequestAsync(request, requestConverter, options, responseConverter, listener, ignores);
Expand Down
20 changes: 8 additions & 12 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/Validatable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,20 @@
*/
package org.elasticsearch.client;

import java.util.Optional;

/**
* Defines a validation layer for Requests.
*/
public interface Validatable {
ValidationException EMPTY_VALIDATION = new ValidationException() {
@Override
public void addValidationError(String error) {
throw new UnsupportedOperationException("Validation messages should not be added to the empty validation");
}
};

/**
* Perform validation. This method does not have to be overridden in the event that no validation needs to be done.
* Perform validation. This method does not have to be overridden in the event that no validation needs to be done,
* or the validation was done during object construction time. A {@link ValidationException} that is not null is
* assumed to contain validation errors and will be thrown.
*
* @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 contains a list of validation errors.
*/
default ValidationException validate() {
return EMPTY_VALIDATION;
default Optional<ValidationException> validate() {
return Optional.empty();
}
}