From ddf107f8b1c62cf2dec57f0cc393a6203e4f6665 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 9 Apr 2021 13:05:10 +0100 Subject: [PATCH] WebInput minor refactoring Use getters so the base RequestInput is usable for JSON serialization, e.g. for the testing support. Remove WebSocketMessageInput which only had an extra requestId and add that to WebInput instead. See gh-42 --- .../springframework/graphql/RequestInput.java | 77 +++++++++++-------- .../org/springframework/graphql/WebInput.java | 43 ++++++++++- .../graphql/WebInterceptor.java | 2 +- .../graphql/WebSocketMessageInput.java | 53 ------------- .../graphql/webflux/GraphQLHttpHandler.java | 7 +- .../webflux/GraphQLWebSocketHandler.java | 6 +- .../graphql/webmvc/GraphQLHttpHandler.java | 6 +- .../webmvc/GraphQLWebSocketHandler.java | 29 ++++--- .../DefaultWebGraphQLServiceTests.java | 2 +- 9 files changed, 111 insertions(+), 114 deletions(-) delete mode 100644 spring-graphql-web/src/main/java/org/springframework/graphql/WebSocketMessageInput.java diff --git a/spring-graphql-web/src/main/java/org/springframework/graphql/RequestInput.java b/spring-graphql-web/src/main/java/org/springframework/graphql/RequestInput.java index 875032ae..938f5e01 100644 --- a/spring-graphql-web/src/main/java/org/springframework/graphql/RequestInput.java +++ b/spring-graphql-web/src/main/java/org/springframework/graphql/RequestInput.java @@ -16,6 +16,7 @@ package org.springframework.graphql; import java.util.Collections; +import java.util.LinkedHashMap; import java.util.Map; import graphql.ExecutionInput; @@ -23,45 +24,42 @@ import graphql.ExecutionInput; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; -import org.springframework.util.StringUtils; -import org.springframework.web.server.ServerWebInputException; /** * Container for a GraphQL request. */ public class RequestInput { - protected final String query; + private final String query; @Nullable - protected final String operationName; + private final String operationName; - protected final Map variables; + private final Map variables; + public RequestInput(String query, @Nullable String operationName, @Nullable Map vars) { + Assert.notNull(query, "'query' is required"); + this.query = query; + this.operationName = operationName; + this.variables = (vars != null ? vars : Collections.emptyMap()); + } + + public RequestInput(Map body) { + this(getKey("query", body), getKey("operationName", body), getKey("variables", body)); + } + @SuppressWarnings("unchecked") - public RequestInput(Map body) { - Assert.notNull(body, "'body' is required'"); - this.query = getAndValidateQuery(body); - this.operationName = (String) body.get("operationName"); - this.variables = (body.get("variables") != null ? - (Map) body.get("variables") : Collections.emptyMap()); + private static T getKey(String key, Map body) { + return (T) body.get(key); } - private static String getAndValidateQuery(Map body) { - String query = (String) body.get("query"); - if (!StringUtils.hasText(query)) { - throw new ServerWebInputException("Query is required"); - } - return query; - } /** * Return the query name extracted from the request body. This is guaranteed - * to be a non-empty string, or otherwise the request is rejected via - * {@link ServerWebInputException} as a 400 error. + * to be a non-empty string. */ - public String query() { + public String getQuery() { return this.query; } @@ -70,7 +68,7 @@ public class RequestInput { * {@code null} if not provided. */ @Nullable - public String operationName() { + public String getOperationName() { return this.operationName; } @@ -78,27 +76,44 @@ public class RequestInput { * Return the query variables that can be referenced via $syntax extracted * from the request body or a {@code null} if not provided. */ - public Map variables() { + public Map getVariables() { return this.variables; } + /** - * Create an {@link ExecutionInput} initialized with the {@link #query()}, - * {@link #operationName()}, and {@link #variables()}. + * Create an {@link ExecutionInput} initialized with the {@link #getQuery()}, + * {@link #getOperationName()}, and {@link #getVariables()}. */ public ExecutionInput toExecutionInput() { return ExecutionInput.newExecutionInput() - .query(query()) - .operationName(operationName()) - .variables(variables()) + .query(getQuery()) + .operationName(getOperationName()) + .variables(getVariables()) .build(); } + /** + * Return a Map representation of the request input. + */ + public Map toMap() { + Map map = new LinkedHashMap<>(); + map.put("query", getQuery()); + if (getOperationName() != null) { + map.put("operationName", getOperationName()); + } + if (CollectionUtils.isEmpty(getVariables())) { + map.put("variables", new LinkedHashMap<>(getVariables())); + } + return map; + } + + @Override public String toString() { - return "Query='" + query() + "'" + - (operationName() != null ? ", Operation='" + operationName() + "'" : "") + - (!CollectionUtils.isEmpty(variables()) ? ", Variables=" + variables() : ""); + return "Query='" + getQuery() + "'" + + (getOperationName() != null ? ", Operation='" + getOperationName() + "'" : "") + + (!CollectionUtils.isEmpty(getVariables()) ? ", Variables=" + getVariables() : ""); } } diff --git a/spring-graphql-web/src/main/java/org/springframework/graphql/WebInput.java b/spring-graphql-web/src/main/java/org/springframework/graphql/WebInput.java index 4d0f8459..a19e140d 100644 --- a/spring-graphql-web/src/main/java/org/springframework/graphql/WebInput.java +++ b/spring-graphql-web/src/main/java/org/springframework/graphql/WebInput.java @@ -19,7 +19,11 @@ import java.net.URI; import java.util.Map; import org.springframework.http.HttpHeaders; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.ObjectUtils; +import org.springframework.util.StringUtils; +import org.springframework.web.server.ServerWebInputException; import org.springframework.web.util.UriComponents; import org.springframework.web.util.UriComponentsBuilder; @@ -34,13 +38,33 @@ public class WebInput extends RequestInput { private final HttpHeaders headers; + private final String id; - public WebInput(URI uri, HttpHeaders headers, Map body) { - super(body); + + /** + * Create an instance. + * @param uri the url for the HTTP request, or WebSocket handshake + * @param headers the HTTP request headers + * @param body the content of the request deserialized from JSON + * @param id an identifier for the GraphQL request, e.g. a subscription id + * for correlating request and response messages, or it could be an id + * associated with the underlying request/connection id, if available + */ + public WebInput(URI uri, HttpHeaders headers, Map body, @Nullable String id) { + super(validateQuery(body)); Assert.notNull(uri, "URI is required'"); Assert.notNull(headers, "HttpHeaders is required'"); this.uri = UriComponentsBuilder.fromUri(uri).build(true); this.headers = headers; + this.id = (id != null ? id : ObjectUtils.identityToString(this)); + } + + private static Map validateQuery(Map body) { + String query = (String) body.get("query"); + if (!StringUtils.hasText(query)) { + throw new ServerWebInputException("Query is required"); + } + return body; } @@ -48,15 +72,26 @@ public class WebInput extends RequestInput { * Return the URI of the HTTP request including * {@link UriComponents#getQueryParams() query parameters}. */ - public UriComponents uri() { + public UriComponents getUri() { return this.uri; } /** * Return the headers of the request. */ - public HttpHeaders headers() { + public HttpHeaders getHeaders() { return this.headers; } + /** + * Return the identifier for the request, which may be a subscription id for + * correlating request and response messages, or the underlying request or + * connection id, when available, or otherwise it's an + * {@link ObjectUtils#identityToString(Object) identity} hash based this + * {@code WebInput} instance. + */ + public String getId() { + return this.id; + } + } \ No newline at end of file diff --git a/spring-graphql-web/src/main/java/org/springframework/graphql/WebInterceptor.java b/spring-graphql-web/src/main/java/org/springframework/graphql/WebInterceptor.java index ea3993b6..fb6232a2 100644 --- a/spring-graphql-web/src/main/java/org/springframework/graphql/WebInterceptor.java +++ b/spring-graphql-web/src/main/java/org/springframework/graphql/WebInterceptor.java @@ -41,7 +41,7 @@ public interface WebInterceptor { * *

{@code ExecutionInput} is initially populated with the input from the * request body via {@link WebInput#toExecutionInput()} where the - * {@link WebInput#query() query} is guaranteed to be a non-empty String. + * {@link WebInput#getQuery() query} is guaranteed to be a non-empty String. * Interceptors are then executed in order to further customize the input * and or perform other actions or checks. * diff --git a/spring-graphql-web/src/main/java/org/springframework/graphql/WebSocketMessageInput.java b/spring-graphql-web/src/main/java/org/springframework/graphql/WebSocketMessageInput.java deleted file mode 100644 index f19c4d59..00000000 --- a/spring-graphql-web/src/main/java/org/springframework/graphql/WebSocketMessageInput.java +++ /dev/null @@ -1,53 +0,0 @@ -/* - * Copyright 2002-2021 the original author or authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.springframework.graphql; - -import java.net.URI; -import java.util.Map; - -import org.springframework.http.HttpHeaders; - -/** - * Extension of {@link WebInput} that contains a GraphQL subscription received - * as a message over a WebSocket connection. - */ -public class WebSocketMessageInput extends WebInput { - - private final String requestId; - - - public WebSocketMessageInput( - URI uri, HttpHeaders headers, String subscribeId, Map payload) { - - super(uri, headers, payload); - this.requestId = subscribeId; - } - - - /** - * Return the id that will correlate server responses to client requests - * within a multiplexed WebSocket connection. - */ - public String requestId() { - return this.requestId; - } - - @Override - public String toString() { - return "requestId='" + requestId() + "', " + super.toString(); - } - -} diff --git a/spring-graphql-web/src/main/java/org/springframework/graphql/webflux/GraphQLHttpHandler.java b/spring-graphql-web/src/main/java/org/springframework/graphql/webflux/GraphQLHttpHandler.java index 54dc52e9..4992f165 100644 --- a/spring-graphql-web/src/main/java/org/springframework/graphql/webflux/GraphQLHttpHandler.java +++ b/spring-graphql-web/src/main/java/org/springframework/graphql/webflux/GraphQLHttpHandler.java @@ -58,11 +58,12 @@ public class GraphQLHttpHandler { public Mono handleQuery(ServerRequest request) { return request.bodyToMono(MAP_PARAMETERIZED_TYPE_REF) .flatMap(body -> { - WebInput webInput = new WebInput(request.uri(), request.headers().asHttpHeaders(), body); + String id = request.exchange().getRequest().getId(); + WebInput input = new WebInput(request.uri(), request.headers().asHttpHeaders(), body, id); if (logger.isDebugEnabled()) { - logger.debug("Executing: " + webInput); + logger.debug("Executing: " + input); } - return this.graphQLService.execute(webInput); + return this.graphQLService.execute(input); }) .flatMap(output -> { Map spec = output.toSpecification(); diff --git a/spring-graphql-web/src/main/java/org/springframework/graphql/webflux/GraphQLWebSocketHandler.java b/spring-graphql-web/src/main/java/org/springframework/graphql/webflux/GraphQLWebSocketHandler.java index f7b3047e..ae96f2c3 100644 --- a/spring-graphql-web/src/main/java/org/springframework/graphql/webflux/GraphQLWebSocketHandler.java +++ b/spring-graphql-web/src/main/java/org/springframework/graphql/webflux/GraphQLWebSocketHandler.java @@ -40,8 +40,8 @@ import org.springframework.core.codec.Encoder; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.graphql.WebGraphQLService; +import org.springframework.graphql.WebInput; import org.springframework.graphql.WebOutput; -import org.springframework.graphql.WebSocketMessageInput; import org.springframework.http.MediaType; import org.springframework.http.codec.DecoderHttpMessageReader; import org.springframework.http.codec.EncoderHttpMessageWriter; @@ -159,8 +159,8 @@ public class GraphQLWebSocketHandler implements WebSocketHandler { if (id == null) { return GraphQLStatus.close(session, GraphQLStatus.INVALID_MESSAGE_STATUS); } - WebSocketMessageInput input = new WebSocketMessageInput( - handshakeInfo.getUri(), handshakeInfo.getHeaders(), id, getPayload(map)); + WebInput input = new WebInput( + handshakeInfo.getUri(), handshakeInfo.getHeaders(), getPayload(map), id); if (logger.isDebugEnabled()) { logger.debug("Executing: " + input); } diff --git a/spring-graphql-web/src/main/java/org/springframework/graphql/webmvc/GraphQLHttpHandler.java b/spring-graphql-web/src/main/java/org/springframework/graphql/webmvc/GraphQLHttpHandler.java index b361cb56..36c659fd 100644 --- a/spring-graphql-web/src/main/java/org/springframework/graphql/webmvc/GraphQLHttpHandler.java +++ b/spring-graphql-web/src/main/java/org/springframework/graphql/webmvc/GraphQLHttpHandler.java @@ -65,11 +65,11 @@ public class GraphQLHttpHandler { * e.g. {@link HttpMediaTypeNotSupportedException}. */ public ServerResponse handle(ServerRequest request) throws ServletException { - WebInput webInput = new WebInput(request.uri(), request.headers().asHttpHeaders(), readBody(request)); + WebInput input = new WebInput(request.uri(), request.headers().asHttpHeaders(), readBody(request), null); if (logger.isDebugEnabled()) { - logger.debug("Executing: " + webInput); + logger.debug("Executing: " + input); } - Mono responseMono = this.graphQLService.execute(webInput) + Mono responseMono = this.graphQLService.execute(input) .map(output -> { if (logger.isDebugEnabled()) { logger.debug("Execution complete"); diff --git a/spring-graphql-web/src/main/java/org/springframework/graphql/webmvc/GraphQLWebSocketHandler.java b/spring-graphql-web/src/main/java/org/springframework/graphql/webmvc/GraphQLWebSocketHandler.java index aa192579..fa724b4a 100644 --- a/spring-graphql-web/src/main/java/org/springframework/graphql/webmvc/GraphQLWebSocketHandler.java +++ b/spring-graphql-web/src/main/java/org/springframework/graphql/webmvc/GraphQLWebSocketHandler.java @@ -42,8 +42,8 @@ import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; import org.springframework.graphql.WebGraphQLService; +import org.springframework.graphql.WebInput; import org.springframework.graphql.WebOutput; -import org.springframework.graphql.WebSocketMessageInput; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpInputMessage; import org.springframework.http.HttpOutputMessage; @@ -120,7 +120,7 @@ public class GraphQLWebSocketHandler extends TextWebSocketHandler implements Sub Mono.delay(this.initTimeoutDuration) .then(Mono.fromRunnable(() -> { - if (!sessionState.isConnectionInitProcessed()) { + if (sessionState.isConnectionInitNotProcessed()) { GraphQLStatus.closeSession(session, GraphQLStatus.INIT_TIMEOUT_STATUS); } })) @@ -140,7 +140,7 @@ public class GraphQLWebSocketHandler extends TextWebSocketHandler implements Sub SessionState sessionState = getSessionInfo(session); switch (messageType) { case SUBSCRIBE: - if (!sessionState.isConnectionInitProcessed()) { + if (sessionState.isConnectionInitNotProcessed()) { GraphQLStatus.closeSession(session, GraphQLStatus.UNAUTHORIZED_STATUS); return; } @@ -149,13 +149,14 @@ public class GraphQLWebSocketHandler extends TextWebSocketHandler implements Sub return; } URI uri = session.getUri(); + Assert.notNull(uri, "Expected handshake url"); HttpHeaders headers = session.getHandshakeHeaders(); - WebSocketMessageInput input = new WebSocketMessageInput(uri, headers, id, getPayload(map)); + WebInput input = new WebInput(uri, headers, getPayload(map), id); if (logger.isDebugEnabled()) { logger.debug("Executing: " + input); } this.service.execute(input) - .flatMapMany(output -> handleWebOutput(session, input.requestId(), output)) + .flatMapMany(output -> handleWebOutput(session, input.getId(), output)) .publishOn(sessionState.getScheduler()) // Serial blocking send via single thread .subscribe(new SendMessageSubscriber(id, session, sessionState)); return; @@ -172,7 +173,7 @@ public class GraphQLWebSocketHandler extends TextWebSocketHandler implements Sub GraphQLStatus.closeSession(session, GraphQLStatus.TOO_MANY_INIT_REQUESTS_STATUS); return; } - TextMessage outputMessage = encode(session, null, MessageType.CONNECTION_ACK, null); + TextMessage outputMessage = encode(null, MessageType.CONNECTION_ACK, null); session.sendMessage(outputMessage); return; default: @@ -227,9 +228,9 @@ public class GraphQLWebSocketHandler extends TextWebSocketHandler implements Sub return outputFlux .map(result -> { Map dataMap = result.toSpecification(); - return encode(session, id, MessageType.NEXT, dataMap); + return encode(id, MessageType.NEXT, dataMap); }) - .concatWith(Mono.fromCallable(() -> encode(session, id, MessageType.COMPLETE, null))) + .concatWith(Mono.fromCallable(() -> encode(id, MessageType.COMPLETE, null))) .onErrorResume(ex -> { if (ex instanceof SubscriptionExistsException) { CloseStatus status = new CloseStatus(4409, "Subscriber for " + id + " already exists"); @@ -243,14 +244,12 @@ public class GraphQLWebSocketHandler extends TextWebSocketHandler implements Sub .message(message) .build() .toSpecification(); - return Mono.just(encode(session, id, MessageType.ERROR, errorMap)); + return Mono.just(encode(id, MessageType.ERROR, errorMap)); }); } @SuppressWarnings("unchecked") - private TextMessage encode( - WebSocketSession session, @Nullable String id, MessageType messageType, @Nullable Object payload) { - + private TextMessage encode(@Nullable String id, MessageType messageType, @Nullable Object payload) { Map payloadMap = new HashMap<>(3); payloadMap.put("type", messageType.getType()); if (id != null) { @@ -397,8 +396,8 @@ public class GraphQLWebSocketHandler extends TextWebSocketHandler implements Sub this.scheduler = Schedulers.newSingle("GraphQL-WsSession-" + sessionId); } - public boolean isConnectionInitProcessed() { - return this.connectionInitProcessed; + public boolean isConnectionInitNotProcessed() { + return !this.connectionInitProcessed; } public synchronized boolean setConnectionInitProcessed() { @@ -430,7 +429,7 @@ public class GraphQLWebSocketHandler extends TextWebSocketHandler implements Sub } - private class SendMessageSubscriber extends BaseSubscriber { + private static class SendMessageSubscriber extends BaseSubscriber { private final String subscriptionId; diff --git a/spring-graphql-web/src/test/java/org/springframework/graphql/DefaultWebGraphQLServiceTests.java b/spring-graphql-web/src/test/java/org/springframework/graphql/DefaultWebGraphQLServiceTests.java index 44638713..3303edc2 100644 --- a/spring-graphql-web/src/test/java/org/springframework/graphql/DefaultWebGraphQLServiceTests.java +++ b/spring-graphql-web/src/test/java/org/springframework/graphql/DefaultWebGraphQLServiceTests.java @@ -62,7 +62,7 @@ public class DefaultWebGraphQLServiceTests { ObjectMapper mapper = new ObjectMapper(); Map body = mapper.reader().readValue("{\"query\": \"" + query + "\"}", Map.class); - WebInput webInput = new WebInput(URI.create("/graphql"), new HttpHeaders(), body); + WebInput webInput = new WebInput(URI.create("/graphql"), new HttpHeaders(), body, "1"); DefaultWebGraphQLService requestHandler = new DefaultWebGraphQLService(createGraphQL()); requestHandler.setInterceptors(interceptors);