From 5905f02bd6f29f742e25f6b54bada629c7765abc Mon Sep 17 00:00:00 2001
From: rstoyanchev <rstoyanchev@vmware.com>
Date: Fri, 4 Nov 2022 11:01:58 +0000
Subject: [PATCH] Deprecate ResponseField getError and hasValue

See gh-499
---
 .../graphql/GraphQlResponse.java              |  4 +-
 .../graphql/ResponseField.java                | 59 +++++++++++++++----
 .../graphql/client/ClientResponseField.java   | 12 ++--
 .../client/DefaultClientResponseField.java    |  6 +-
 .../graphql/client/DefaultGraphQlClient.java  |  2 +-
 .../graphql/client/FieldAccessException.java  |  7 +--
 .../graphql/client/GraphQlClient.java         | 41 +++++--------
 .../support/AbstractGraphQlResponse.java      |  4 +-
 .../graphql/client/GraphQlClientTests.java    | 23 ++++----
 9 files changed, 94 insertions(+), 64 deletions(-)

diff --git a/spring-graphql/src/main/java/org/springframework/graphql/GraphQlResponse.java b/spring-graphql/src/main/java/org/springframework/graphql/GraphQlResponse.java
index fc809930..661b8556 100644
--- a/spring-graphql/src/main/java/org/springframework/graphql/GraphQlResponse.java
+++ b/spring-graphql/src/main/java/org/springframework/graphql/GraphQlResponse.java
@@ -78,8 +78,8 @@ public interface GraphQlResponse {
 	 * </pre>
 	 * @param path relative to the "data" key
 	 * @return representation for the field with further options to inspect or
-	 * decode its value; use {@link ResponseField#hasValue()} to check if
-	 * the field actually exists and has a value.
+	 * decode its value; use {@link ResponseField#getValue()} to check if
+	 * the field actually has a value.
 	 */
 	ResponseField field(String path);
 
diff --git a/spring-graphql/src/main/java/org/springframework/graphql/ResponseField.java b/spring-graphql/src/main/java/org/springframework/graphql/ResponseField.java
index c7ec8dce..5f0b3e6f 100644
--- a/spring-graphql/src/main/java/org/springframework/graphql/ResponseField.java
+++ b/spring-graphql/src/main/java/org/springframework/graphql/ResponseField.java
@@ -37,9 +37,13 @@ public interface ResponseField {
 	 * <li>{@code "true"} means the field is not {@code null}, and therefore valid,
 	 * although it may be partial with nested field {@link #getErrors() errors}.
 	 * <li>{@code "false"} means the field is {@code null} or doesn't exist; use
-	 * {@link #getError()} to check if the field is {@code null} due to an error.
+	 * {@link #getErrors()} to check for field errors, and
+	 * {@link GraphQlResponse#isValid()} to check if the entire response is
+	 * valid or not.
 	 * </ul>
+	 * @deprecated as of 1.0.3 in favor of checking via {@link #getValue()}
 	 */
+	@Deprecated
 	boolean hasValue();
 
 	/**
@@ -65,10 +69,10 @@ public interface ResponseField {
 
 	/**
 	 * Return the error that provides the reason for a failed field.
-	 * <p>When the field <strong>does not</strong> {@link #hasValue() have} a
-	 * value, this method looks for the first field error. According to the
-	 * GraphQL spec, section 6.4.4, "Handling Field Errors", there should be
-	 * only one error per field. The returned field error may be:
+	 * <p>When the field is {@code null}, this method looks for the first field
+	 * error. According to the GraphQL spec, section 6.4.4, "Handling Field
+	 * Errors", there should be only one error per field. The returned field
+	 * error may be:
 	 * <ul>
 	 * <li>on the field
 	 * <li>on a parent field, when the field is not present
@@ -83,16 +87,51 @@ public interface ResponseField {
 	 * partial and contain {@link #getErrors() errors} on nested fields.
 	 * @return return the error for this field, or {@code null} if there is no
 	 * error with the same path as the field path
+	 * @deprecated since 1.0.3 in favor of {@link #getErrors()}
 	 */
 	@Nullable
+	@Deprecated
 	ResponseError getError();
 
 	/**
-	 * Return all field errors including errors above, at, and below this field.
-	 * <p>In practice, when the field <strong>does have</strong> a value, it is
-	 * considered valid but possibly partial with nested field errors. When the
-	 * field <strong>does not have</strong> a value, there should be only one
-	 * field error, and in that case it is better to use {@link #getError()}.
+	 * Return all errors that have a path, and it is at above, or below the field path.
+	 * <p>According to the GraphQL spec, section 6.4.4, "Handling Field Errors"
+	 * if a field has an error it is set to {@code null}. That means a field
+	 * has either a value or an error, and there is only one error per field.
+	 * <p>Errors may also occur at paths above or below the field path. Consider
+	 * the following cases:
+	 * <table>
+	 * <tr>
+	 * <th>Value</th>
+	 * <th>Errors</th>
+	 * <th>Case</th>
+	 * </tr>
+	 * <tr>
+	 * <td>Non-{@code null}</td>
+	 * <td>Empty</td>
+	 * <td>Success</td>
+	 * </tr>
+	 * <tr>
+	 * <td>Non-{@code null}</td>
+	 * <td>Errors below</td>
+	 * <td>Partial with errors on nested fields</td>
+	 * </tr>
+	 * <tr>
+	 * <td>{@code null}</td>
+	 * <td>Error at field</td>
+	 * <td>Field failure</td>
+	 * </tr>
+	 * <tr>
+	 * <td>{@code null}</td>
+	 * <td>Error above field</td>
+	 * <td>Parent field failure</td>
+	 * </tr>
+	 * <tr>
+	 * <td>{@code null}</td>
+	 * <td>Error below field</td>
+	 * <td>Nested field failure bubbles up because field is required</td>
+	 * </tr>
+	 * </table>
 	 */
 	List<ResponseError> getErrors();
 
diff --git a/spring-graphql/src/main/java/org/springframework/graphql/client/ClientResponseField.java b/spring-graphql/src/main/java/org/springframework/graphql/client/ClientResponseField.java
index 99921591..0ddbaf25 100644
--- a/spring-graphql/src/main/java/org/springframework/graphql/client/ClientResponseField.java
+++ b/spring-graphql/src/main/java/org/springframework/graphql/client/ClientResponseField.java
@@ -34,8 +34,8 @@ public interface ClientResponseField extends ResponseField {
 	 * Decode the field to an entity of the given type.
 	 * @param entityType the type to convert to
 	 * @return the decoded entity, never {@code null}
-	 * @throws FieldAccessException if the target field is not present or
-	 * has no value, checked via {@link #hasValue()}.
+	 * @throws FieldAccessException if the target field is {@code null} and has
+	 * {@link ResponseField#getErrors() errors}.
 	 */
 	<D> D toEntity(Class<D> entityType);
 
@@ -45,16 +45,14 @@ public interface ClientResponseField extends ResponseField {
 	<D> D toEntity(ParameterizedTypeReference<D> entityType);
 
 	/**
-	 * Decode the field to a list of entities with the given type.
+	 * Variant of {@link #toEntity(Class)} to decode to a list of entities.
 	 * @param elementType the type of elements in the list
-	 * @return the decoded list of entities, possibly empty
-	 * @throws FieldAccessException if the target field is not present or
-	 * has no value, checked via {@link #hasValue()}.
 	 */
 	<D> List<D> toEntityList(Class<D> elementType);
 
 	/**
-	 * Variant of {@link #toEntityList(Class)} with {@link ParameterizedTypeReference}.
+	 * Variant of {@link #toEntity(Class)} to decode to a list of entities.
+	 * @param elementType the type of elements in the list
 	 */
 	<D> List<D> toEntityList(ParameterizedTypeReference<D> elementType);
 
diff --git a/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultClientResponseField.java b/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultClientResponseField.java
index 20ed2a06..41e4c004 100644
--- a/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultClientResponseField.java
+++ b/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultClientResponseField.java
@@ -54,9 +54,10 @@ final class DefaultClientResponseField implements ClientResponseField {
 	}
 
 
+	@SuppressWarnings("deprecation")
 	@Override
 	public boolean hasValue() {
-		return this.field.hasValue();
+		return (this.field.getValue() != null);
 	}
 
 	@Override
@@ -74,6 +75,7 @@ final class DefaultClientResponseField implements ClientResponseField {
 		return this.field.getValue();
 	}
 
+	@SuppressWarnings("deprecation")
 	@Override
 	public ResponseError getError() {
 		return this.field.getError();
@@ -106,7 +108,7 @@ final class DefaultClientResponseField implements ClientResponseField {
 
 	@SuppressWarnings({"unchecked", "ConstantConditions"})
 	private <T> T toEntity(ResolvableType targetType) {
-		if (!hasValue()) {
+		if (getValue() == null) {
 			throw new FieldAccessException(this.response.getRequest(), this.response, this);
 		}
 
diff --git a/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultGraphQlClient.java b/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultGraphQlClient.java
index e63adf9b..a2f5975f 100644
--- a/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultGraphQlClient.java
+++ b/spring-graphql/src/main/java/org/springframework/graphql/client/DefaultGraphQlClient.java
@@ -199,7 +199,7 @@ final class DefaultGraphQlClient implements GraphQlClient {
 				throw new FieldAccessException(
 						((DefaultClientGraphQlResponse) response).getRequest(), response, field);
 			}
-			return (field.hasValue() ? field : null);
+			return (field.getValue() != null ? field : null);
 		}
 
 	}
diff --git a/spring-graphql/src/main/java/org/springframework/graphql/client/FieldAccessException.java b/spring-graphql/src/main/java/org/springframework/graphql/client/FieldAccessException.java
index 502f8565..6bddc787 100644
--- a/spring-graphql/src/main/java/org/springframework/graphql/client/FieldAccessException.java
+++ b/spring-graphql/src/main/java/org/springframework/graphql/client/FieldAccessException.java
@@ -21,10 +21,9 @@ import org.springframework.graphql.GraphQlResponse;
 import org.springframework.graphql.ResponseField;
 
 /**
- * An exception raised on an attempt to decode data from a
- * {@link GraphQlResponse#isValid() failed response} or a field is not present,
- * or has no value, checked via
- * {@link ResponseField#hasValue()}.
+ * An exception raised when an attempt is made to decode data from a response
+ * that is not {@link GraphQlResponse#isValid() valid} or where field value
+ * is {@code null}, or there field {@link ResponseField#getErrors() errors}.
  *
  * @author Rossen Stoyanchev
  * @since 1.0.0
diff --git a/spring-graphql/src/main/java/org/springframework/graphql/client/GraphQlClient.java b/spring-graphql/src/main/java/org/springframework/graphql/client/GraphQlClient.java
index fb3c6a8d..985f834d 100644
--- a/spring-graphql/src/main/java/org/springframework/graphql/client/GraphQlClient.java
+++ b/spring-graphql/src/main/java/org/springframework/graphql/client/GraphQlClient.java
@@ -243,11 +243,11 @@ public interface GraphQlClient {
 		/**
 		 * Decode the field to an entity of the given type.
 		 * @param entityType the type to convert to
-		 * @return {@code Mono} with the decoded entity. Completes empty when
-		 * the field is {@code null} without errors, or ends with
-		 * {@link FieldAccessException} for an invalid response or a failed field
-		 * @see GraphQlResponse#isValid()
-		 * @see ResponseField#getError()
+		 * @return {@code Mono} with the decoded entity; completes with
+		 * {@link FieldAccessException} in case of {@link ResponseField field
+		 * errors} or an {@link GraphQlResponse#isValid() invalid} response;
+		 * completes empty if the field is {@code null} but has no errors.
+		 * @see ResponseField#getErrors()
 		 */
 		<D> Mono<D> toEntity(Class<D> entityType);
 
@@ -257,18 +257,14 @@ public interface GraphQlClient {
 		<D> Mono<D> toEntity(ParameterizedTypeReference<D> entityType);
 
 		/**
-		 * Decode the field to a list of entities with the given type.
+		 * Variant of {@link #toEntity(Class)} to decode to a List of entities.
 		 * @param elementType the type of elements in the list
-		 * @return {@code Mono} with a list of decoded entities, possibly an
-		 * empty list, or ends with {@link FieldAccessException} if the target
-		 * field is not present or has no value.
-		 * @see GraphQlResponse#isValid()
-		 * @see ResponseField#getError()
 		 */
 		<D> Mono<List<D>> toEntityList(Class<D> elementType);
 
 		/**
-		 * Variant of {@link #toEntityList(Class)} with {@link ParameterizedTypeReference}.
+		 * Variant of {@link #toEntity(Class)} to decode to a List of entities.
+		 * @param elementType the type of elements in the list
 		 */
 		<D> Mono<List<D>> toEntityList(ParameterizedTypeReference<D> elementType);
 
@@ -283,12 +279,11 @@ public interface GraphQlClient {
 		/**
 		 * Decode the field to an entity of the given type.
 		 * @param entityType the type to convert to
-		 * @return a stream of decoded entities, one for each response, excluding
-		 * responses in which the field is {@code null} without errors. Ends with
-		 * {@link FieldAccessException} for an invalid response or a failed field.
-		 * May also end with a {@link GraphQlTransportException}.
-		 * @see GraphQlResponse#isValid()
-		 * @see ResponseField#getError()
+		 * @return {@code Mono} with the decoded entity; completes with
+		 * {@link FieldAccessException} in case of {@link ResponseField field
+		 * errors} or an {@link GraphQlResponse#isValid() invalid} response;
+		 * completes empty if the field is {@code null} but has no errors.
+		 * @see ResponseField#getErrors()
 		 */
 		<D> Flux<D> toEntity(Class<D> entityType);
 
@@ -298,19 +293,13 @@ public interface GraphQlClient {
 		<D> Flux<D> toEntity(ParameterizedTypeReference<D> entityType);
 
 		/**
-		 * Decode the field to a list of entities with the given type.
+		 * Variant of {@link #toEntity(Class)} to decode each response to a List of entities.
 		 * @param elementType the type of elements in the list
-		 * @return lists of decoded entities, one for each response, excluding
-		 * responses in which the field is {@code null} without errors. Ends with
-		 * {@link FieldAccessException} for an invalid response or a failed field.
-		 * May also end with a {@link GraphQlTransportException}.
-		 * @see GraphQlResponse#isValid()
-		 * @see ResponseField#getError()
 		 */
 		<D> Flux<List<D>> toEntityList(Class<D> elementType);
 
 		/**
-		 * Variant of {@link #toEntityList(Class)} with {@link ParameterizedTypeReference}.
+		 * Variant of {@link #toEntity(Class)} to decode each response to a List of entities.
 		 */
 		<D> Flux<List<D>> toEntityList(ParameterizedTypeReference<D> elementType);
 
diff --git a/spring-graphql/src/main/java/org/springframework/graphql/support/AbstractGraphQlResponse.java b/spring-graphql/src/main/java/org/springframework/graphql/support/AbstractGraphQlResponse.java
index 85e1a9b2..92a4de0f 100644
--- a/spring-graphql/src/main/java/org/springframework/graphql/support/AbstractGraphQlResponse.java
+++ b/spring-graphql/src/main/java/org/springframework/graphql/support/AbstractGraphQlResponse.java
@@ -160,6 +160,7 @@ public abstract class AbstractGraphQlResponse implements GraphQlResponse {
 			return this.parsedPath;
 		}
 
+		@SuppressWarnings("deprecation")
 		@Override
 		public boolean hasValue() {
 			return (this.value != null);
@@ -171,9 +172,10 @@ public abstract class AbstractGraphQlResponse implements GraphQlResponse {
 			return (T) this.value;
 		}
 
+		@SuppressWarnings("deprecation")
 		@Override
 		public ResponseError getError() {
-			if (!hasValue()) {
+			if (getValue() != null) {
 				if (!this.fieldErrors.isEmpty()) {
 					return this.fieldErrors.get(0);
 				}
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 b4e4cb2b..2ffe80ad 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
@@ -179,7 +179,7 @@ public class GraphQlClientTests extends GraphQlClientTestSupport {
 
 		assertThat(response).isNotNull();
 		assertThat(response.isValid()).isFalse();
-		assertThat(response.field("me").hasValue()).isFalse();
+		assertThat(response.field("me")).isNotNull();
 
 		assertThatThrownBy(() -> response.field("me").toEntity(MovieCharacter.class))
 				.isInstanceOf(FieldAccessException.class);
@@ -201,26 +201,27 @@ public class GraphQlClientTests extends GraphQlClientTestSupport {
 				.isTrue();
 
 		ClientResponseField field = response.field("me");
-		assertThat(field.hasValue()).isTrue();
+		assertThat((Object) field.getValue()).isNotNull();
 		assertThat(field.getErrors()).hasSize(1);
 		assertThat(field.getErrors().get(0).getParsedPath()).containsExactly("me", "name");
 		assertThat(field.toEntity(MovieCharacter.class))
 				.as("Decoding with nested field error should not be precluded")
 				.isNotNull();
 
-		ClientResponseField nameField = response.field("me.name");
-		assertThat(nameField.hasValue()).isFalse();
-		assertThat(nameField.getError()).isNotNull();
-		assertThat(nameField.getError().getParsedPath()).containsExactly("me", "name");
-		assertThatThrownBy(() -> nameField.toEntity(String.class))
+		field = response.field("me.name");
+		assertThat((Object) field.getValue()).isNull();
+		assertThat(field.getErrors()).isNotEmpty();
+		assertThat(field.getErrors().get(0).getParsedPath()).containsExactly("me", "name");
+		ClientResponseField theField = field;
+		assertThatThrownBy(() -> theField.toEntity(String.class))
 				.as("Decoding field null with direct field error should be rejected")
 				.isInstanceOf(FieldAccessException.class)
 				.hasMessageContaining("Test error");
 
-		ClientResponseField nonExistingField = response.field("me.name.other");
-		assertThat(nonExistingField.hasValue()).isFalse();
-		assertThat(nameField.getError()).isNotNull();
-		assertThat(nameField.getError().getParsedPath()).containsExactly("me", "name");
+		field = response.field("me.name.other");
+		assertThat((Object) field.getValue()).isNull();
+		assertThat(field.getErrors()).isNotEmpty();
+		assertThat(field.getErrors().get(0).getParsedPath()).containsExactly("me", "name");
 	}
 
 	private GraphQLError errorForPath(String errorPath) {