From c592ee09626cbd781c82204fcad3d99c87c5b1de Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 9 Sep 2022 17:32:19 +0100 Subject: [PATCH 1/4] GraphiQlHandler supports path variables Closes gh-478 --- .../server/webflux/GraphiQlHandler.java | 4 +- .../server/webmvc/GraphiQlHandler.java | 4 +- .../server/webflux/GraphiQlHandlerTests.java | 29 ++++++++++++++- .../server/webmvc/GraphiQlHandlerTests.java | 37 +++++++++++++++++-- 4 files changed, 64 insertions(+), 10 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/server/webflux/GraphiQlHandler.java b/spring-graphql/src/main/java/org/springframework/graphql/server/webflux/GraphiQlHandler.java index ceec57ea..913fe5f2 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/server/webflux/GraphiQlHandler.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/server/webflux/GraphiQlHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2020-2021 the original author or authors. + * Copyright 2020-2022 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. @@ -86,7 +86,7 @@ public class GraphiQlHandler { String wsPathQueryParam = applyContextPath(request, this.graphQlWsPath); builder.queryParam("wsPath", wsPathQueryParam); } - return builder.build(); + return builder.build(request.pathVariables()); } private String applyContextPath(ServerRequest request, String path) { diff --git a/spring-graphql/src/main/java/org/springframework/graphql/server/webmvc/GraphiQlHandler.java b/spring-graphql/src/main/java/org/springframework/graphql/server/webmvc/GraphiQlHandler.java index c07cea0e..fd03317c 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/server/webmvc/GraphiQlHandler.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/server/webmvc/GraphiQlHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2020-2021 the original author or authors. + * Copyright 2020-2022 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. @@ -85,7 +85,7 @@ public class GraphiQlHandler { String wsPathQueryParam = applyPathPrefix(request, this.graphQlWsPath); builder.queryParam("wsPath", wsPathQueryParam); } - return builder.build(); + return builder.build(request.pathVariables()); } private String applyPathPrefix(ServerRequest request, String path) { diff --git a/spring-graphql/src/test/java/org/springframework/graphql/server/webflux/GraphiQlHandlerTests.java b/spring-graphql/src/test/java/org/springframework/graphql/server/webflux/GraphiQlHandlerTests.java index 6e34b6ff..63b533dd 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/server/webflux/GraphiQlHandlerTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/server/webflux/GraphiQlHandlerTests.java @@ -19,6 +19,7 @@ package org.springframework.graphql.server.webflux; import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.List; +import java.util.Map; import org.junit.jupiter.api.Test; @@ -31,9 +32,11 @@ import org.springframework.http.codec.HttpMessageReader; import org.springframework.http.codec.HttpMessageWriter; import org.springframework.mock.http.server.reactive.MockServerHttpRequest; import org.springframework.mock.web.server.MockServerWebExchange; +import org.springframework.web.reactive.function.server.RouterFunctions; import org.springframework.web.reactive.function.server.ServerRequest; import org.springframework.web.reactive.function.server.ServerResponse; import org.springframework.web.reactive.result.view.ViewResolver; +import org.springframework.web.util.UriComponentsBuilder; import static org.assertj.core.api.Assertions.assertThat; @@ -45,10 +48,13 @@ class GraphiQlHandlerTests { private static final List> MESSAGE_READERS = Collections.emptyList(); - private final GraphiQlHandler handler = new GraphiQlHandler("/graphql", null, - new ByteArrayResource("GRAPHIQL".getBytes(StandardCharsets.UTF_8))); + private final GraphiQlHandler handler = initHandler("/graphql"); + private static GraphiQlHandler initHandler(String path) { + return new GraphiQlHandler(path, null, new ByteArrayResource("GRAPHIQL".getBytes(StandardCharsets.UTF_8))); + } + @Test void shouldRedirectWithPathQueryParameter() { MockServerHttpRequest httpRequest = MockServerHttpRequest.get("/graphiql").build(); @@ -73,6 +79,25 @@ class GraphiQlHandlerTests { assertThat(response.headers().getLocation().toASCIIString()).isEqualTo("/graphiql?path=/graphql&wsPath=/graphql"); } + @Test // gh-478 + void shouldRedirectWithPathVariables() { + Map pathVariables = Collections.singletonMap("envId", "123"); + UriComponentsBuilder uriBuilder = UriComponentsBuilder.fromUriString("/env/{envId}/graphiql"); + String path = uriBuilder.build(pathVariables).toString(); + + MockServerHttpRequest httpRequest = MockServerHttpRequest.get(path).build(); + MockServerWebExchange exchange = MockServerWebExchange.from(httpRequest); + exchange.getAttributes().put(RouterFunctions.URI_TEMPLATE_VARIABLES_ATTRIBUTE, pathVariables); + ServerRequest request = ServerRequest.create(exchange, MESSAGE_READERS); + + GraphiQlHandler graphiQlHandler = initHandler(uriBuilder.build().toString()); + ServerResponse response = graphiQlHandler.handleRequest(request).block(); + + assertThat(response.statusCode()).isEqualTo(HttpStatus.TEMPORARY_REDIRECT); + assertThat(response.headers().getLocation()).isNotNull(); + assertThat(response.headers().getLocation().toASCIIString()).isEqualTo(path + "?path=" + path); + } + @Test void shouldServeGraphiQlHtmlResource() { MockServerHttpRequest httpRequest = MockServerHttpRequest.get("/graphiql").queryParam("path", "/graphql").build(); diff --git a/spring-graphql/src/test/java/org/springframework/graphql/server/webmvc/GraphiQlHandlerTests.java b/spring-graphql/src/test/java/org/springframework/graphql/server/webmvc/GraphiQlHandlerTests.java index d1673a9b..63d810ac 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/server/webmvc/GraphiQlHandlerTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/server/webmvc/GraphiQlHandlerTests.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.List; +import java.util.Map; import javax.servlet.ServletException; @@ -33,8 +34,10 @@ import org.springframework.http.converter.HttpMessageConverter; import org.springframework.http.converter.ResourceHttpMessageConverter; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.web.servlet.function.RouterFunctions; import org.springframework.web.servlet.function.ServerRequest; import org.springframework.web.servlet.function.ServerResponse; +import org.springframework.web.util.UriComponentsBuilder; import static org.assertj.core.api.Assertions.assertThat; @@ -47,8 +50,13 @@ class GraphiQlHandlerTests { private static final List> MESSAGE_READERS = Collections.emptyList(); - private GraphiQlHandler handler = new GraphiQlHandler("/graphql", null, - new ByteArrayResource("GRAPHIQL".getBytes(StandardCharsets.UTF_8))); + private final GraphiQlHandler handler = initHandler("/graphql"); + + + private static GraphiQlHandler initHandler(String path) { + return new GraphiQlHandler(path, null, new ByteArrayResource("GRAPHIQL".getBytes(StandardCharsets.UTF_8))); + } + @Test void shouldRedirectWithPathQueryParameter() { @@ -57,7 +65,8 @@ class GraphiQlHandlerTests { ServerResponse response = this.handler.handleRequest(request); assertThat(response.statusCode()).isEqualTo(HttpStatus.TEMPORARY_REDIRECT); assertThat(response.headers().getLocation()).isNotNull(); - assertThat(response.headers().getLocation().toASCIIString()).isEqualTo("http://localhost/graphiql?path=/graphql"); + assertThat(response.headers().getLocation().toASCIIString()) + .isEqualTo("http://localhost/graphiql?path=/graphql"); } @Test @@ -69,7 +78,27 @@ class GraphiQlHandlerTests { ServerResponse response = wsHandler.handleRequest(request); assertThat(response.statusCode()).isEqualTo(HttpStatus.TEMPORARY_REDIRECT); assertThat(response.headers().getLocation()).isNotNull(); - assertThat(response.headers().getLocation().toASCIIString()).isEqualTo("http://localhost/graphiql?path=/graphql&wsPath=/graphql"); + assertThat(response.headers().getLocation().toASCIIString()) + .isEqualTo("http://localhost/graphiql?path=/graphql&wsPath=/graphql"); + } + + @Test // gh-478 + void shouldRedirectWithPathVariables() { + Map pathVariables = Collections.singletonMap("envId", "123"); + UriComponentsBuilder uriBuilder = UriComponentsBuilder.fromUriString("/env/{envId}/graphiql"); + String path = uriBuilder.build(pathVariables).toString(); + + MockHttpServletRequest servletRequest = new MockHttpServletRequest("GET", path); + ServerRequest request = ServerRequest.create(servletRequest, MESSAGE_READERS); + servletRequest.setAttribute(RouterFunctions.URI_TEMPLATE_VARIABLES_ATTRIBUTE, pathVariables); + + GraphiQlHandler graphiQlHandler = initHandler(uriBuilder.build().toString()); + ServerResponse response = graphiQlHandler.handleRequest(request); + + assertThat(response.statusCode()).isEqualTo(HttpStatus.TEMPORARY_REDIRECT); + assertThat(response.headers().getLocation()).isNotNull(); + assertThat(response.headers().getLocation().toASCIIString()) + .isEqualTo("http://localhost" + path + "?path=" + path); } @Test From 17f6a7c1031c23389fb8dee87070def1cee07d21 Mon Sep 17 00:00:00 2001 From: Vijayasarathy <71578090+vijaya-sarathy@users.noreply.github.com> Date: Sat, 27 Aug 2022 17:37:33 +0530 Subject: [PATCH 2/4] Fix sample in reference docs Closes gh-475 --- spring-graphql-docs/src/docs/asciidoc/client.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spring-graphql-docs/src/docs/asciidoc/client.adoc b/spring-graphql-docs/src/docs/asciidoc/client.adoc index 124e0dba..ea5b8480 100644 --- a/spring-graphql-docs/src/docs/asciidoc/client.adoc +++ b/spring-graphql-docs/src/docs/asciidoc/client.adoc @@ -320,7 +320,7 @@ For example, given a file called `projectReleases.graphql` in `src/main/resources/graphql-documents`, with content: [source,graphql,indent=0,subs="verbatim,quotes"] -.src/main/resources/graphql/project.graphql +.src/main/resources/graphql-documents/projectReleases.graphql ---- query projectReleases($slug: ID!) { project(slug: $slug) { @@ -341,7 +341,7 @@ You can then: .retrieve() .toEntity(Project.class); ---- -<1> Load the document from "project.graphql" +<1> Load the document from "projectReleases.graphql" <2> Provide variable values. The "JS GraphQL" plugin for IntelliJ supports GraphQL query files with code completion. From 65facf41f3ebb2255c2b5ed1c5434095f0fb10b0 Mon Sep 17 00:00:00 2001 From: Alexander Zhuravlev Date: Fri, 9 Sep 2022 00:47:49 +0300 Subject: [PATCH 3/4] Fix NPE when GraphQL argument is list with null See gh-486 --- .../graphql/data/GraphQlArgumentBinder.java | 2 +- .../data/GraphQlArgumentBinderTests.java | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentBinder.java b/spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentBinder.java index 5adb3512..ce63da99 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentBinder.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentBinder.java @@ -213,7 +213,7 @@ public class GraphQlArgumentBinder { int i = 0; for (Object rawValue : rawCollection) { segments.push("[" + i++ + "]"); - if (elementClass.isAssignableFrom(rawValue.getClass())) { + if (rawValue == null || elementClass.isAssignableFrom(rawValue.getClass())) { collection.add((T) rawValue); } else if (rawValue instanceof Map) { diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java index b9862ff8..fdaa9479 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java @@ -32,6 +32,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import graphql.schema.DataFetchingEnvironment; import graphql.schema.DataFetchingEnvironmentImpl; +import org.assertj.core.api.CollectionAssert; import org.junit.jupiter.api.Test; import org.springframework.core.ResolvableType; @@ -281,6 +282,44 @@ class GraphQlArgumentBinderTests { assertThat(((ItemSetHolder) result).getItems()).hasSize(5); } + @Test + @SuppressWarnings("unchecked") + void list() throws Exception { + Object result = this.binder.bind( + environment("{\"key\": [\"1\", \"2\", \"3\"]}"), + "key", + ResolvableType.forClassWithGenerics(List.class, String.class) + ); + + assertThat(result).isNotNull().isInstanceOf(List.class); + new CollectionAssert<>((List) result).containsExactly("1", "2", "3"); + } + + @Test + @SuppressWarnings("unchecked") + void listWithNullItem() throws Exception { + Object result = this.binder.bind( + environment("{\"key\": [\"1\", null, \"3\"]}"), + "key", + ResolvableType.forClassWithGenerics(List.class, String.class) + ); + + assertThat(result).isNotNull().isInstanceOf(List.class); + new CollectionAssert<>((List) result).containsExactly("1", null, "3"); + } + + @Test + @SuppressWarnings("unchecked") + void emptyList() throws Exception { + Object result = this.binder.bind( + environment("{\"key\": []}"), + "key", + ResolvableType.forClassWithGenerics(List.class, String.class) + ); + + assertThat(result).isNotNull().isInstanceOf(List.class); + new CollectionAssert<>((List) result).isEmpty(); + } @SuppressWarnings("unchecked") private DataFetchingEnvironment environment(String jsonPayload) throws JsonProcessingException { From 257f30981849dd5b0d9f45106aac6ffd844e4521 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 9 Sep 2022 17:50:17 +0100 Subject: [PATCH 4/4] Polishing contribution Closes gh-486 --- .../data/GraphQlArgumentBinderTests.java | 86 ++++++++----------- 1 file changed, 34 insertions(+), 52 deletions(-) diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java index fdaa9479..3e1390c2 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java @@ -16,30 +16,23 @@ package org.springframework.graphql.data; -import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.IntStream; -import java.util.stream.Stream; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import graphql.schema.DataFetchingEnvironment; import graphql.schema.DataFetchingEnvironmentImpl; -import org.assertj.core.api.CollectionAssert; import org.junit.jupiter.api.Test; import org.springframework.core.ResolvableType; import org.springframework.graphql.Book; -import org.springframework.graphql.data.GraphQlArgumentBinder; -import org.springframework.util.ReflectionUtils; -import org.springframework.util.StringUtils; import org.springframework.validation.BindException; import org.springframework.validation.FieldError; @@ -61,7 +54,7 @@ class GraphQlArgumentBinderTests { @Test - void defaultConstructor() throws Exception { + void dataBinding() throws Exception { Object result = this.binder.bind( environment("{\"key\":{\"name\":\"test\"}}"), "key", @@ -72,7 +65,7 @@ class GraphQlArgumentBinderTests { } @Test - void defaultConstructorWithNestedBeanProperty() throws Exception { + void dataBindingWithNestedBeanProperty() throws Exception { Object result = this.binder.bind( environment( @@ -93,7 +86,7 @@ class GraphQlArgumentBinderTests { } @Test - void defaultConstructorWithNestedBeanListProperty() throws Exception { + void dataBindingWithNestedBeanListProperty() throws Exception { Object result = this.binder.bind( environment("{\"key\":{\"items\":[{\"name\":\"first\"},{\"name\":\"second\"}]}}"), "key", @@ -105,7 +98,7 @@ class GraphQlArgumentBinderTests { } @Test // gh-301 - void defaultConstructorWithNestedBeanListEmpty() throws Exception { + void dataBindingWithNestedBeanListEmpty() throws Exception { Object result = this.binder.bind( environment("{\"key\":{\"items\": []}}"), "key", @@ -116,7 +109,7 @@ class GraphQlArgumentBinderTests { } @Test // gh-280 - void defaultConstructorBindingError() { + void dataBindingBindingError() { assertThatThrownBy( () -> this.binder.bind( @@ -131,6 +124,35 @@ class GraphQlArgumentBinderTests { }); } + @Test + @SuppressWarnings("unchecked") + void dataBindingToList() throws Exception { + + Object result = this.binder.bind( + environment("{\"key\": [\"1\", \"2\", \"3\"]}"), "key", + ResolvableType.forClassWithGenerics(List.class, String.class)); + + assertThat(result).isNotNull().isInstanceOf(List.class); + assertThat((List) result).containsExactly("1", "2", "3"); + + // gh-486: List with null element + result = this.binder.bind( + environment("{\"key\": [\"1\", null, \"3\"]}"), "key", + ResolvableType.forClassWithGenerics(List.class, String.class)); + + assertThat(result).isNotNull().isInstanceOf(List.class); + assertThat((List) result).containsExactly("1", null, "3"); + + // Empty list + + result = this.binder.bind( + environment("{\"key\": []}"), "key", + ResolvableType.forClassWithGenerics(List.class, String.class)); + + assertThat(result).isNotNull().isInstanceOf(List.class); + assertThat((List) result).isEmpty(); + } + @Test void primaryConstructor() throws Exception { @@ -262,7 +284,6 @@ class GraphQlArgumentBinderTests { } @Test // gh-392 - @SuppressWarnings("unchecked") void shouldHaveHigherDefaultAutoGrowLimit() throws Exception { String items = IntStream.range(0, 260).mapToObj(value -> "{\"name\":\"test\"}").collect(Collectors.joining(",")); Object result = this.binder.bind( @@ -282,45 +303,6 @@ class GraphQlArgumentBinderTests { assertThat(((ItemSetHolder) result).getItems()).hasSize(5); } - @Test - @SuppressWarnings("unchecked") - void list() throws Exception { - Object result = this.binder.bind( - environment("{\"key\": [\"1\", \"2\", \"3\"]}"), - "key", - ResolvableType.forClassWithGenerics(List.class, String.class) - ); - - assertThat(result).isNotNull().isInstanceOf(List.class); - new CollectionAssert<>((List) result).containsExactly("1", "2", "3"); - } - - @Test - @SuppressWarnings("unchecked") - void listWithNullItem() throws Exception { - Object result = this.binder.bind( - environment("{\"key\": [\"1\", null, \"3\"]}"), - "key", - ResolvableType.forClassWithGenerics(List.class, String.class) - ); - - assertThat(result).isNotNull().isInstanceOf(List.class); - new CollectionAssert<>((List) result).containsExactly("1", null, "3"); - } - - @Test - @SuppressWarnings("unchecked") - void emptyList() throws Exception { - Object result = this.binder.bind( - environment("{\"key\": []}"), - "key", - ResolvableType.forClassWithGenerics(List.class, String.class) - ); - - assertThat(result).isNotNull().isInstanceOf(List.class); - new CollectionAssert<>((List) result).isEmpty(); - } - @SuppressWarnings("unchecked") private DataFetchingEnvironment environment(String jsonPayload) throws JsonProcessingException { Map arguments = this.mapper.readValue(jsonPayload, Map.class);