From 60b70c9afbb24e32413231a564971c51a246372f Mon Sep 17 00:00:00 2001 From: Olga MaciaszekSharma Date: Fri, 16 Feb 2024 17:35:35 +0100 Subject: [PATCH] Check for null response body in SpringDecoder. Fixes gh-972. --- .../openfeign/support/SpringDecoder.java | 4 +- .../SpringDecoderIntegrationTests.java | 275 ++++++++++++++++++ .../cloud/openfeign/SpringDecoderTests.java | 264 ++--------------- 3 files changed, 301 insertions(+), 242 deletions(-) create mode 100644 spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderIntegrationTests.java diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringDecoder.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringDecoder.java index d7229522..269e1c74 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringDecoder.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringDecoder.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2023 the original author or authors. + * Copyright 2013-2024 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. @@ -108,7 +108,7 @@ public class SpringDecoder implements Decoder { @Override public InputStream getBody() throws IOException { - return response.body().asInputStream(); + return response.body() != null ? response.body().asInputStream() : null; } @Override diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderIntegrationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderIntegrationTests.java new file mode 100644 index 00000000..c70da1b9 --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderIntegrationTests.java @@ -0,0 +1,275 @@ +/* + * Copyright 2013-2024 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.cloud.openfeign; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.context.SpringBootTest.WebEnvironment; +import org.springframework.boot.test.web.server.LocalServerPort; +import org.springframework.cloud.openfeign.support.SpringDecoder; +import org.springframework.cloud.openfeign.test.NoSecurityConfiguration; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Import; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; +import org.springframework.http.ResponseEntity; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RestController; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Integration tests for {@link SpringDecoder}. + * + * @author Spencer Gibb + * @author Olga Maciaszek-Sharma + * @author Szymon Linowski + */ +@SpringBootTest(classes = SpringDecoderIntegrationTests.Application.class, webEnvironment = WebEnvironment.RANDOM_PORT, + value = { "spring.application.name=springdecodertest", "spring.jmx.enabled=false" }) +@DirtiesContext +class SpringDecoderIntegrationTests extends FeignClientFactoryBean { + + @Autowired + FeignClientFactory context; + + @LocalServerPort + private int port = 0; + + SpringDecoderIntegrationTests() { + setName("test"); + setContextId("test"); + } + + public TestClient testClient() { + return testClient(false); + } + + public TestClient testClient(boolean dismiss404) { + setType(this.getClass()); + setDismiss404(dismiss404); + return feign(this.context).target(TestClient.class, "http://localhost:" + this.port); + } + + @Test + void testResponseEntity() { + ResponseEntity response = testClient().getHelloResponse(); + assertThat(response).as("response was null").isNotNull(); + assertThat(response.getStatusCode()).as("wrong status code").isEqualTo(HttpStatus.OK); + Hello hello = response.getBody(); + assertThat(hello).as("hello was null").isNotNull(); + assertThat(hello).as("first hello didn't match").isEqualTo(new Hello("hello world via response")); + } + + @Test + void testSimpleType() { + Hello hello = testClient().getHello(); + assertThat(hello).as("hello was null").isNotNull(); + assertThat(hello).as("first hello didn't match").isEqualTo(new Hello("hello world 1")); + } + + @Test + void testUserParameterizedTypeDecode() { + List hellos = testClient().getHellos(); + assertThat(hellos).as("hellos was null").isNotNull(); + assertThat(hellos.size()).as("hellos was not the right size").isEqualTo(2); + assertThat(hellos.get(0)).as("first hello didn't match").isEqualTo(new Hello("hello world 1")); + } + + @Test + void testSimpleParameterizedTypeDecode() { + List hellos = testClient().getHelloStrings(); + assertThat(hellos).as("hellos was null").isNotNull(); + assertThat(hellos.size()).as("hellos was not the right size").isEqualTo(2); + assertThat(hellos.get(0)).as("first hello didn't match").isEqualTo("hello world 1"); + } + + @Test + @SuppressWarnings("unchecked") + void testWildcardTypeDecode() { + ResponseEntity wildcard = testClient().getWildcard(); + assertThat(wildcard).as("wildcard was null").isNotNull(); + assertThat(wildcard.getStatusCode()).as("wrong status code").isEqualTo(HttpStatus.OK); + Object wildcardBody = wildcard.getBody(); + assertThat(wildcardBody).as("wildcardBody was null").isNotNull(); + assertThat(wildcardBody instanceof Map).as("wildcard not an instance of Map").isTrue(); + Map hello = (Map) wildcardBody; + assertThat(hello.get("message")).as("first hello didn't match").isEqualTo("wildcard"); + } + + @Test + void testResponseEntityVoid() { + ResponseEntity response = testClient().getHelloVoid(); + assertThat(response).as("response was null").isNotNull(); + List headerVals = response.getHeaders().get("x-test-header"); + assertThat(headerVals).as("headerVals was null").isNotNull(); + assertThat(headerVals.size()).as("headerVals size was wrong").isEqualTo(1); + String header = headerVals.get(0); + assertThat(header).as("header was wrong").isEqualTo("myval"); + } + + @Test + void test404() { + Assertions.assertThatExceptionOfType(RuntimeException.class).isThrownBy(() -> testClient().getNotFound()); + } + + @Test + void testDecodes404() { + final ResponseEntity response = testClient(true).getNotFound(); + assertThat(response).as("response was null").isNotNull(); + assertThat(response.getBody()).as("response body was not null").isNull(); + } + + @Test + // Issue: https://github.com/spring-cloud/spring-cloud-openfeign/issues/456 + void testResponseEntityHeaders() { + ResponseEntity response = testClient().getContentType(); + assertThat(response.getHeaders().getContentType()).isEqualTo(MediaType.APPLICATION_JSON); + } + + protected interface TestClient { + + @GetMapping("/helloresponse") + ResponseEntity getHelloResponse(); + + @GetMapping("/hellovoid") + ResponseEntity getHelloVoid(); + + @GetMapping("/hello") + Hello getHello(); + + @GetMapping("/hellos") + List getHellos(); + + @GetMapping("/hellostrings") + List getHelloStrings(); + + @GetMapping("/hellonotfound") + ResponseEntity getNotFound(); + + @GetMapping("/helloWildcard") + ResponseEntity getWildcard(); + + @GetMapping(path = "/contentType", produces = MediaType.APPLICATION_JSON_VALUE) + ResponseEntity getContentType(); + + } + + public static class Hello { + + private String message; + + Hello() { + } + + Hello(String message) { + this.message = message; + } + + public String getMessage() { + return this.message; + } + + public void setMessage(String message) { + this.message = message; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + Hello that = (Hello) o; + return Objects.equals(this.message, that.message); + } + + @Override + public int hashCode() { + return Objects.hash(this.message); + } + + } + + @Configuration(proxyBeanMethods = false) + @EnableAutoConfiguration + @RestController + @Import(NoSecurityConfiguration.class) + protected static class Application implements TestClient { + + @Override + public ResponseEntity getHelloResponse() { + return ResponseEntity.ok(new Hello("hello world via response")); + } + + @Override + public ResponseEntity getHelloVoid() { + return ResponseEntity.noContent().header("X-test-header", "myval").build(); + } + + @Override + public Hello getHello() { + return new Hello("hello world 1"); + } + + @Override + public List getHellos() { + ArrayList hellos = new ArrayList<>(); + hellos.add(new Hello("hello world 1")); + hellos.add(new Hello("oi terra 2")); + return hellos; + } + + @Override + public List getHelloStrings() { + ArrayList hellos = new ArrayList<>(); + hellos.add("hello world 1"); + hellos.add("oi terra 2"); + return hellos; + } + + @Override + public ResponseEntity getNotFound() { + return ResponseEntity.status(HttpStatus.NOT_FOUND).body(null); + } + + @Override + public ResponseEntity getWildcard() { + return ResponseEntity.ok(new Hello("wildcard")); + } + + @Override + public ResponseEntity getContentType() { + return ResponseEntity.ok().header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE).body("test"); + } + + } + +} diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderTests.java index bc975bab..70a6eba0 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2022 the original author or authors. + * Copyright 2013-2024 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. @@ -16,257 +16,41 @@ package org.springframework.cloud.openfeign; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.Objects; - -import org.assertj.core.api.Assertions; +import feign.Request; +import feign.Response; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.autoconfigure.EnableAutoConfiguration; -import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.boot.test.context.SpringBootTest.WebEnvironment; -import org.springframework.boot.test.web.server.LocalServerPort; -import org.springframework.cloud.openfeign.test.NoSecurityConfiguration; -import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.Import; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpStatus; -import org.springframework.http.MediaType; -import org.springframework.http.ResponseEntity; -import org.springframework.test.annotation.DirtiesContext; -import org.springframework.web.bind.annotation.GetMapping; -import org.springframework.web.bind.annotation.RestController; +import org.springframework.beans.factory.ObjectFactory; +import org.springframework.boot.autoconfigure.http.HttpMessageConverters; +import org.springframework.cloud.openfeign.support.SpringDecoder; -import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; /** - * @author Spencer Gibb + * Tests for {@link SpringDecoder}. + * * @author Olga Maciaszek-Sharma - * @author Szymon Linowski */ -@SpringBootTest(classes = SpringDecoderTests.Application.class, webEnvironment = WebEnvironment.RANDOM_PORT, - value = { "spring.application.name=springdecodertest", "spring.jmx.enabled=false" }) -@DirtiesContext -class SpringDecoderTests extends FeignClientFactoryBean { +class SpringDecoderTests { - @Autowired - FeignClientFactory context; + SpringDecoder decoder; - @LocalServerPort - private int port = 0; - - SpringDecoderTests() { - setName("test"); - setContextId("test"); - } - - public TestClient testClient() { - return testClient(false); - } - - public TestClient testClient(boolean dismiss404) { - setType(this.getClass()); - setDismiss404(dismiss404); - return feign(this.context).target(TestClient.class, "http://localhost:" + this.port); + @BeforeEach + void setUp() { + ObjectFactory factory = mock(); + when(factory.getObject()).thenReturn(new HttpMessageConverters()); + decoder = new SpringDecoder(factory); } + // Issue: https://github.com/spring-cloud/spring-cloud-openfeign/issues/972 @Test - void testResponseEntity() { - ResponseEntity response = testClient().getHelloResponse(); - assertThat(response).as("response was null").isNotNull(); - assertThat(response.getStatusCode()).as("wrong status code").isEqualTo(HttpStatus.OK); - Hello hello = response.getBody(); - assertThat(hello).as("hello was null").isNotNull(); - assertThat(hello).as("first hello didn't match").isEqualTo(new Hello("hello world via response")); - } - - @Test - void testSimpleType() { - Hello hello = testClient().getHello(); - assertThat(hello).as("hello was null").isNotNull(); - assertThat(hello).as("first hello didn't match").isEqualTo(new Hello("hello world 1")); - } - - @Test - void testUserParameterizedTypeDecode() { - List hellos = testClient().getHellos(); - assertThat(hellos).as("hellos was null").isNotNull(); - assertThat(hellos.size()).as("hellos was not the right size").isEqualTo(2); - assertThat(hellos.get(0)).as("first hello didn't match").isEqualTo(new Hello("hello world 1")); - } - - @Test - void testSimpleParameterizedTypeDecode() { - List hellos = testClient().getHelloStrings(); - assertThat(hellos).as("hellos was null").isNotNull(); - assertThat(hellos.size()).as("hellos was not the right size").isEqualTo(2); - assertThat(hellos.get(0)).as("first hello didn't match").isEqualTo("hello world 1"); - } - - @Test - @SuppressWarnings("unchecked") - void testWildcardTypeDecode() { - ResponseEntity wildcard = testClient().getWildcard(); - assertThat(wildcard).as("wildcard was null").isNotNull(); - assertThat(wildcard.getStatusCode()).as("wrong status code").isEqualTo(HttpStatus.OK); - Object wildcardBody = wildcard.getBody(); - assertThat(wildcardBody).as("wildcardBody was null").isNotNull(); - assertThat(wildcardBody instanceof Map).as("wildcard not an instance of Map").isTrue(); - Map hello = (Map) wildcardBody; - assertThat(hello.get("message")).as("first hello didn't match").isEqualTo("wildcard"); - } - - @Test - void testResponseEntityVoid() { - ResponseEntity response = testClient().getHelloVoid(); - assertThat(response).as("response was null").isNotNull(); - List headerVals = response.getHeaders().get("x-test-header"); - assertThat(headerVals).as("headerVals was null").isNotNull(); - assertThat(headerVals.size()).as("headerVals size was wrong").isEqualTo(1); - String header = headerVals.get(0); - assertThat(header).as("header was wrong").isEqualTo("myval"); - } - - @Test - void test404() { - Assertions.assertThatExceptionOfType(RuntimeException.class).isThrownBy(() -> testClient().getNotFound()); - } - - @Test - void testDecodes404() { - final ResponseEntity response = testClient(true).getNotFound(); - assertThat(response).as("response was null").isNotNull(); - assertThat(response.getBody()).as("response body was not null").isNull(); - } - - @Test - // Issue: https://github.com/spring-cloud/spring-cloud-openfeign/issues/456 - void testResponseEntityHeaders() { - ResponseEntity response = testClient().getContentType(); - assertThat(response.getHeaders().getContentType()).isEqualTo(MediaType.APPLICATION_JSON); - } - - protected interface TestClient { - - @GetMapping("/helloresponse") - ResponseEntity getHelloResponse(); - - @GetMapping("/hellovoid") - ResponseEntity getHelloVoid(); - - @GetMapping("/hello") - Hello getHello(); - - @GetMapping("/hellos") - List getHellos(); - - @GetMapping("/hellostrings") - List getHelloStrings(); - - @GetMapping("/hellonotfound") - ResponseEntity getNotFound(); - - @GetMapping("/helloWildcard") - ResponseEntity getWildcard(); - - @GetMapping(path = "/contentType", produces = MediaType.APPLICATION_JSON_VALUE) - ResponseEntity getContentType(); - - } - - public static class Hello { - - private String message; - - Hello() { - } - - Hello(String message) { - this.message = message; - } - - public String getMessage() { - return this.message; - } - - public void setMessage(String message) { - this.message = message; - } - - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - Hello that = (Hello) o; - return Objects.equals(this.message, that.message); - } - - @Override - public int hashCode() { - return Objects.hash(this.message); - } - - } - - @Configuration(proxyBeanMethods = false) - @EnableAutoConfiguration - @RestController - @Import(NoSecurityConfiguration.class) - protected static class Application implements TestClient { - - @Override - public ResponseEntity getHelloResponse() { - return ResponseEntity.ok(new Hello("hello world via response")); - } - - @Override - public ResponseEntity getHelloVoid() { - return ResponseEntity.noContent().header("X-test-header", "myval").build(); - } - - @Override - public Hello getHello() { - return new Hello("hello world 1"); - } - - @Override - public List getHellos() { - ArrayList hellos = new ArrayList<>(); - hellos.add(new Hello("hello world 1")); - hellos.add(new Hello("oi terra 2")); - return hellos; - } - - @Override - public List getHelloStrings() { - ArrayList hellos = new ArrayList<>(); - hellos.add("hello world 1"); - hellos.add("oi terra 2"); - return hellos; - } - - @Override - public ResponseEntity getNotFound() { - return ResponseEntity.status(HttpStatus.NOT_FOUND).body(null); - } - - @Override - public ResponseEntity getWildcard() { - return ResponseEntity.ok(new Hello("wildcard")); - } - - @Override - public ResponseEntity getContentType() { - return ResponseEntity.ok().header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE).body("test"); - } - + void shouldNotThrownNPEWhenNoContent() { + assertThatCode( + () -> decoder.decode(Response.builder().request(mock(Request.class)).status(200).build(), String.class)) + .doesNotThrowAnyException(); } }