From 5e95fcf60a3f76fe2309642078bbe6805ce08c81 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Thu, 17 Mar 2022 12:27:52 +0000 Subject: [PATCH] Refine getError(s) methods on ResponseField getError now returns either the error at the field or on a parent field, which provides a reliable answer for why a field failed even for a field whose path is below where the error occurred. getErrors likewise now also includes errors above. There is still no need for special logic to get only errors below since where there is a field value, errors are below anyway. See gh-10 --- .../client/DefaultClientGraphQlResponse.java | 2 +- .../graphql/client/ResponseField.java | 17 +++++++++++++---- .../graphql/support/MapGraphQlResponse.java | 4 ++-- .../graphql/client/GraphQlClientTests.java | 9 +++++++++ .../support/MapGraphQlResponseTests.java | 4 ++-- 5 files changed, 27 insertions(+), 9 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultClientGraphQlResponse.java b/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultClientGraphQlResponse.java index d05ed53b..3fce9c6f 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultClientGraphQlResponse.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultClientGraphQlResponse.java @@ -136,7 +136,7 @@ final class DefaultClientGraphQlResponse extends MapGraphQlResponse implements C @Override public GraphQLError getError() { for (GraphQLError error : this.errors) { - if (this.parsedPath.size() == error.getPath().size()) { + if (error.getPath().size() <= this.parsedPath.size()) { return error; } } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/client/ResponseField.java b/spring-graphql/src/main/java/org/springframework/graphql/client/ResponseField.java index ec65d819..6e063429 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/client/ResponseField.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/client/ResponseField.java @@ -60,15 +60,24 @@ public interface ResponseField { T getValue(); /** - * Return the first error whose path is equal to the field path. - *

According to section 6.4.4 "Handling Field Errors" of the GraphQL - * spec, only one error should be added to the errors list per field. + * Return the error for this field, if any. The error may be for this field + * when the field is {@code null}, or it may be for a parent field, when the + * current field does not exist. + *

Note: The field error is identified by searching for + * the first error with a matching path that is shorter or the same as the + * field path. According to the GraphQL spec, section 6.4.4, + * "Handling Field Errors", there should be only one field error per field. + * @return return the error for this field, or {@code null} if there is no + * error with the same path as the field path */ @Nullable GraphQLError getError(); /** - * Return all field errors including those whose path is below the field path. + * Return all field errors including errors above, at, and below this field. + *

In practice, when the field has a value, all errors are for fields + * below. When the field does not have a value, there is only one error, and + * it is the same as {@link #getError()}. */ List getErrors(); diff --git a/spring-graphql/src/main/java/org/springframework/graphql/support/MapGraphQlResponse.java b/spring-graphql/src/main/java/org/springframework/graphql/support/MapGraphQlResponse.java index 1ee11b12..40d4bd91 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/support/MapGraphQlResponse.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/support/MapGraphQlResponse.java @@ -185,11 +185,11 @@ public class MapGraphQlResponse implements GraphQlResponse { List fieldErrors = Collections.emptyList(); for (GraphQLError error : this.errors) { List errorPath = error.getPath(); - if (CollectionUtils.isEmpty(errorPath) || errorPath.size() < fieldPath.size()) { + if (CollectionUtils.isEmpty(errorPath)) { continue; } boolean match = true; - for (int i = 0; match && i < fieldPath.size(); i++) { + for (int i = 0; match && i < fieldPath.size() && i < errorPath.size(); i++) { match = fieldPath.get(i).equals(errorPath.get(i)); } if (!match) { diff --git a/spring-graphql/src/test/java/org/springframework/graphql/client/GraphQlClientTests.java b/spring-graphql/src/test/java/org/springframework/graphql/client/GraphQlClientTests.java index cdd52ecc..2148a1de 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/client/GraphQlClientTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/client/GraphQlClientTests.java @@ -188,15 +188,24 @@ public class GraphQlClientTests extends GraphQlClientTestSupport { ResponseField field = response.field("me"); assertThat(field.isValid()).isTrue(); + assertThat(field.getErrors()).hasSize(1); + assertThat(field.getErrors().get(0).getPath()).containsExactly("me", "name"); assertThat(field.toEntity(MovieCharacter.class)) .as("Decoding with nested field error should not be precluded") .isNotNull(); ResponseField nameField = response.field("me.name"); assertThat(nameField.isValid()).isFalse(); + assertThat(nameField.getError()).isNotNull(); + assertThat(nameField.getError().getPath()).containsExactly("me", "name"); assertThatThrownBy(() -> nameField.toEntity(String.class)) .as("Decoding field null with direct field error should be rejected") .isInstanceOf(FieldAccessException.class); + + ResponseField nonExistingField = response.field("me.name.other"); + assertThat(nonExistingField.isValid()).isFalse(); + assertThat(nameField.getError()).isNotNull(); + assertThat(nameField.getError().getPath()).containsExactly("me", "name"); } private GraphQLError errorForPath(String errorPath) { diff --git a/spring-graphql/src/test/java/org/springframework/graphql/support/MapGraphQlResponseTests.java b/spring-graphql/src/test/java/org/springframework/graphql/support/MapGraphQlResponseTests.java index f130dc2b..49662f31 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/support/MapGraphQlResponseTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/support/MapGraphQlResponseTests.java @@ -142,11 +142,11 @@ public class MapGraphQlResponseTests { MapGraphQlResponse response = MapGraphQlResponse.forErrorsOnly(errorList); List errors = response.getFieldErrors(path); - assertThat(errors).containsExactly(error2, error3); + assertThat(errors).containsExactly(error1, error2, error3); } private GraphQLError createError(@Nullable String errorPath, String message) { - GraphqlErrorBuilder builder = GraphqlErrorBuilder.newError().message(message); + GraphqlErrorBuilder builder = GraphqlErrorBuilder.newError().message(message); if (errorPath != null) { builder = builder.path(ResultPath.parse(errorPath)); }