You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@servicecomb.apache.org by GitBox <gi...@apache.org> on 2018/08/23 11:36:10 UTC
[GitHub] liubao68 closed pull request #881: [SCB-861]490,
590 response not properly used
liubao68 closed pull request #881: [SCB-861]490,590 response not properly used
URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/881
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/CodeFirstRestTemplateSpringmvc.java b/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/CodeFirstRestTemplateSpringmvc.java
index af27ccdf3..e34b340c7 100644
--- a/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/CodeFirstRestTemplateSpringmvc.java
+++ b/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/CodeFirstRestTemplateSpringmvc.java
@@ -173,6 +173,7 @@ private String testRestTemplateUpload(RestTemplate template, String cseUrlPrefix
}
private void testFallback(RestTemplate template, String cseUrlPrefix) {
+ long start = System.currentTimeMillis();
String result = template.getForObject(cseUrlPrefix + "/fallback/returnnull/hello", String.class);
TestMgr.check(result, "hello");
result = template.getForObject(cseUrlPrefix + "/fallback/returnnull/throwexception", String.class);
@@ -199,6 +200,10 @@ private void testFallback(RestTemplate template, String cseUrlPrefix) {
result = template.getForObject(cseUrlPrefix + "/fallback/force/hello", String.class);
TestMgr.check(result, "mockedreslut");
+
+ // This test case is fallback testing and will return null if failed.
+ // In order to check if failed due to some unexpected timeout exception, check the time.
+ TestMgr.check(System.currentTimeMillis() - start < 10000, true);
}
private void testResponseEntity(String microserviceName, RestTemplate template, String cseUrlPrefix) {
diff --git a/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/SpringmvcClient.java b/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/SpringmvcClient.java
index 37aa87054..3e5aa1c15 100644
--- a/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/SpringmvcClient.java
+++ b/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/SpringmvcClient.java
@@ -54,12 +54,19 @@
private static Controller controller;
public static void main(String[] args) throws Exception {
- Log4jUtils.init();
- BeanUtils.init();
+ try {
+ Log4jUtils.init();
+ BeanUtils.init();
- run();
+ run();
- TestMgr.summary();
+ TestMgr.summary();
+ } catch (Throwable e) {
+ TestMgr.check("success", "failed");
+ System.err.println("-------------- test failed -------------");
+ e.printStackTrace();
+ System.err.println("-------------- test failed -------------");
+ }
}
public static void run() {
diff --git a/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/TestResponse.java b/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/TestResponse.java
index 0cd60284f..95db734ed 100644
--- a/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/TestResponse.java
+++ b/demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/TestResponse.java
@@ -146,15 +146,7 @@ private void testDecodeResponseError() {
}
Objects.requireNonNull(exception);
// 2. CseException: bizKeeper exception
- Throwable wrappedException = exception.getCause();
- TestMgr.check(CseException.class, wrappedException.getClass());
- // 3. InvocationException: decoder wrapping exception
- wrappedException = wrappedException.getCause();
- TestMgr.check(InvocationException.class, wrappedException.getClass());
- TestMgr.check("InvocationException: code=490;msg=CommonExceptionData [message=Cse Internal Bad Request]",
- wrappedException.getMessage());
- // 4. InvalidFormatException: decode exception
- Object cause = wrappedException.getCause();
+ Throwable cause = exception.getCause();
TestMgr.check(InvalidFormatException.class, cause.getClass());
TestMgr.check(
"Cannot deserialize value of type `java.util.Date` from String \"returnOK\": not a valid representation "
diff --git a/demo/demo-springmvc/springmvc-server/src/main/java/org/apache/servicecomb/demo/springmvc/server/CodeFirstSpringmvc.java b/demo/demo-springmvc/springmvc-server/src/main/java/org/apache/servicecomb/demo/springmvc/server/CodeFirstSpringmvc.java
index 6ec2bc59a..0ab7487d3 100644
--- a/demo/demo-springmvc/springmvc-server/src/main/java/org/apache/servicecomb/demo/springmvc/server/CodeFirstSpringmvc.java
+++ b/demo/demo-springmvc/springmvc-server/src/main/java/org/apache/servicecomb/demo/springmvc/server/CodeFirstSpringmvc.java
@@ -22,7 +22,6 @@
import java.util.Date;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import javax.servlet.http.HttpServletRequest;
@@ -51,6 +50,7 @@
import org.apache.servicecomb.swagger.invocation.Response;
import org.apache.servicecomb.swagger.invocation.context.ContextUtils;
import org.apache.servicecomb.swagger.invocation.context.InvocationContext;
+import org.apache.servicecomb.swagger.invocation.exception.CommonExceptionData;
import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
import org.apache.servicecomb.swagger.invocation.response.Headers;
import org.slf4j.Logger;
@@ -270,42 +270,44 @@ public String addString(@RequestParam(name = "s") List<String> s) {
return result;
}
+ // Using 490, 590 error code, the response type should be CommonExceptionData. Or we need
+ // complex ExceptionConverters to deal with exceptions thrown by Hanlders, etc.
@RequestMapping(path = "/fallback/returnnull/{name}", method = RequestMethod.GET)
@ApiResponses(value = {@ApiResponse(code = 200, response = String.class, message = "xxx"),
- @ApiResponse(code = 490, response = String.class, message = "xxx")})
+ @ApiResponse(code = 490, response = CommonExceptionData.class, message = "xxx")})
public String fallbackReturnNull(@PathVariable(name = "name") String name) {
if ("throwexception".equals(name)) {
- throw new InvocationException(490, "490", "xxx");
+ throw new InvocationException(490, "490", new CommonExceptionData("xxx"));
}
return name;
}
@RequestMapping(path = "/fallback/throwexception/{name}", method = RequestMethod.GET)
@ApiResponses(value = {@ApiResponse(code = 200, response = String.class, message = "xxx"),
- @ApiResponse(code = 490, response = String.class, message = "xxx")})
+ @ApiResponse(code = 490, response = CommonExceptionData.class, message = "xxx")})
public String fallbackThrowException(@PathVariable(name = "name") String name) {
if ("throwexception".equals(name)) {
- throw new InvocationException(490, "490", "xxx");
+ throw new InvocationException(490, "490", new CommonExceptionData("xxx"));
}
return name;
}
@RequestMapping(path = "/fallback/fromcache/{name}", method = RequestMethod.GET)
@ApiResponses(value = {@ApiResponse(code = 200, response = String.class, message = "xxx"),
- @ApiResponse(code = 490, response = String.class, message = "xxx")})
+ @ApiResponse(code = 490, response = CommonExceptionData.class, message = "xxx")})
public String fallbackFromCache(@PathVariable(name = "name") String name) {
if ("throwexception".equals(name)) {
- throw new InvocationException(490, "490", "xxx");
+ throw new InvocationException(490, "490", new CommonExceptionData("xxx"));
}
return name;
}
@RequestMapping(path = "/fallback/force/{name}", method = RequestMethod.GET)
@ApiResponses(value = {@ApiResponse(code = 200, response = String.class, message = "xxx"),
- @ApiResponse(code = 490, response = String.class, message = "xxx")})
+ @ApiResponse(code = 490, response = CommonExceptionData.class, message = "xxx")})
public String fallbackForce(@PathVariable(name = "name") String name) {
if ("throwexception".equals(name)) {
- throw new InvocationException(490, "490", "xxx");
+ throw new InvocationException(490, "490", new CommonExceptionData("xxx"));
}
return name;
}
@@ -317,7 +319,7 @@ public String fallbackForce(@PathVariable(name = "name") String name) {
@RequestMapping(path = "/testenum/{name}", method = RequestMethod.GET)
@ApiResponses(value = {@ApiResponse(code = 200, response = String.class, message = "200 normal"),
- @ApiResponse(code = 490, response = String.class, message = "490 exception")})
+ @ApiResponse(code = 490, response = CommonExceptionData.class, message = "490 exception")})
public String testEnum(@RequestParam(name = "username") String username,
@PathVariable(value = "name") NameType nameType) {
return nameType.toString();
diff --git a/demo/demo-springmvc/springmvc-server/src/main/resources/microservice.yaml b/demo/demo-springmvc/springmvc-server/src/main/resources/microservice.yaml
index 9c047c817..fba7554db 100644
--- a/demo/demo-springmvc/springmvc-server/src/main/resources/microservice.yaml
+++ b/demo/demo-springmvc/springmvc-server/src/main/resources/microservice.yaml
@@ -54,11 +54,11 @@ servicecomb:
uploads:
directory: target
rest:
- address: 0.0.0.0:8080?sslEnabled=true
+ address: 0.0.0.0:8082?sslEnabled=true
server:
compression: true
highway:
- address: 0.0.0.0:7070?sslEnabled=true
+ address: 0.0.0.0:7072?sslEnabled=true
handler:
chain:
Provider:
diff --git a/swagger/swagger-invocation/invocation-core/src/main/java/org/apache/servicecomb/swagger/invocation/response/ResponsesMeta.java b/swagger/swagger-invocation/invocation-core/src/main/java/org/apache/servicecomb/swagger/invocation/response/ResponsesMeta.java
index 856e3f00d..881cb812f 100644
--- a/swagger/swagger-invocation/invocation-core/src/main/java/org/apache/servicecomb/swagger/invocation/response/ResponsesMeta.java
+++ b/swagger/swagger-invocation/invocation-core/src/main/java/org/apache/servicecomb/swagger/invocation/response/ResponsesMeta.java
@@ -53,7 +53,6 @@
// 如果不传return类型进来，完全以swagger为标准，会导致生成的class不等于return
public void init(SwaggerToClassGenerator swaggerToClassGenerator, Operation operation, Type returnType) {
initSuccessResponse(returnType);
- initInternalErrorResponse();
initGlobalDefaultMapper();
for (Entry<String, Response> entry : operation.getResponses().entrySet()) {
@@ -68,6 +67,8 @@ public void init(SwaggerToClassGenerator swaggerToClassGenerator, Operation oper
responseMeta.init(swaggerToClassGenerator, entry.getValue());
}
+ initInternalErrorResponse();
+
if (defaultResponse == null) {
// swagger中没有定义default，加上default专用于处理exception
ResponseMeta responseMeta = new ResponseMeta();
@@ -86,8 +87,8 @@ protected void initSuccessResponse(Type returnType) {
protected void initInternalErrorResponse() {
ResponseMeta internalErrorResponse = new ResponseMeta();
internalErrorResponse.setJavaType(COMMON_EXCEPTION_JAVA_TYPE);
- responseMap.put(ExceptionFactory.CONSUMER_INNER_STATUS_CODE, internalErrorResponse);
- responseMap.put(ExceptionFactory.PRODUCER_INNER_STATUS_CODE, internalErrorResponse);
+ responseMap.putIfAbsent(ExceptionFactory.CONSUMER_INNER_STATUS_CODE, internalErrorResponse);
+ responseMap.putIfAbsent(ExceptionFactory.PRODUCER_INNER_STATUS_CODE, internalErrorResponse);
}
protected void initGlobalDefaultMapper() {
diff --git a/transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/http/DefaultHttpClientFilter.java b/transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/http/DefaultHttpClientFilter.java
index 60921c506..806a9c916 100644
--- a/transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/http/DefaultHttpClientFilter.java
+++ b/transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/http/DefaultHttpClientFilter.java
@@ -31,6 +31,9 @@
import org.apache.servicecomb.foundation.vertx.http.HttpServletRequestEx;
import org.apache.servicecomb.foundation.vertx.http.HttpServletResponseEx;
import org.apache.servicecomb.swagger.invocation.Response;
+import org.apache.servicecomb.swagger.invocation.context.HttpStatus;
+import org.apache.servicecomb.swagger.invocation.exception.CommonExceptionData;
+import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
import org.apache.servicecomb.swagger.invocation.response.ResponseMeta;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -91,8 +94,22 @@ protected Response extractResponse(Invocation invocation, HttpServletResponseEx
result = produceProcessor.decodeResponse(responseEx.getBodyBuffer(), responseMeta.getJavaType());
return Response.create(responseEx.getStatusType(), result);
} catch (Exception e) {
- LOGGER.error("failed to decode response body, exception type is [{}]", e.getClass());
- return Response.createConsumerFail(e);
+ LOGGER.error("failed to decode response body, exception is [{}]", e.getMessage());
+ String msg =
+ String.format("method %s, path %s, statusCode %d, reasonPhrase %s, response content-type %s is not supported",
+ swaggerRestOperation.getHttpMethod(),
+ swaggerRestOperation.getAbsolutePath(),
+ responseEx.getStatus(),
+ responseEx.getStatusType().getReasonPhrase(),
+ responseEx.getHeader(HttpHeaders.CONTENT_TYPE));
+ if (HttpStatus.isSuccess(responseEx.getStatus())) {
+ return Response.createConsumerFail(
+ new InvocationException(400, responseEx.getStatusType().getReasonPhrase(),
+ new CommonExceptionData(msg), e));
+ }
+ return Response.createConsumerFail(
+ new InvocationException(responseEx.getStatus(), responseEx.getStatusType().getReasonPhrase(),
+ new CommonExceptionData(msg), e));
}
}
diff --git a/transports/transport-rest/transport-rest-client/src/test/java/org/apache/servicecomb/transport/rest/client/http/TestDefaultHttpClientFilter.java b/transports/transport-rest/transport-rest-client/src/test/java/org/apache/servicecomb/transport/rest/client/http/TestDefaultHttpClientFilter.java
index d14bc1da4..9c0028b20 100644
--- a/transports/transport-rest/transport-rest-client/src/test/java/org/apache/servicecomb/transport/rest/client/http/TestDefaultHttpClientFilter.java
+++ b/transports/transport-rest/transport-rest-client/src/test/java/org/apache/servicecomb/transport/rest/client/http/TestDefaultHttpClientFilter.java
@@ -36,7 +36,6 @@
import org.apache.servicecomb.foundation.vertx.http.ReadStreamPart;
import org.apache.servicecomb.swagger.invocation.Response;
import org.apache.servicecomb.swagger.invocation.exception.CommonExceptionData;
-import org.apache.servicecomb.swagger.invocation.exception.ExceptionFactory;
import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
import org.apache.servicecomb.swagger.invocation.response.ResponseMeta;
import org.junit.Assert;
@@ -130,6 +129,54 @@ public void extractResult_decodeError(@Mocked Invocation invocation, @Mocked Rea
@Mocked RestOperationMeta swaggerRestOperation,
@Mocked HttpServletResponseEx responseEx) {
Map<String, Object> handlerContext = new HashMap<>();
+ new Expectations() {
+ {
+ invocation.getHandlerContext();
+ result = handlerContext;
+ invocation.getOperationMeta();
+ result = operationMeta;
+ operationMeta.findResponseMeta(400);
+ result = responseMeta;
+ operationMeta.getExtData(RestConst.SWAGGER_REST_OPERATION);
+ result = swaggerRestOperation;
+ responseMeta.getJavaType();
+ result = SimpleType.constructUnsafe(Date.class);
+ responseEx.getStatus();
+ result = 400;
+ responseEx.getBodyBuffer();
+ result = new BufferImpl().appendString("abc");
+ }
+ };
+ new MockUp<DefaultHttpClientFilter>() {
+ @Mock
+ ProduceProcessor findProduceProcessor(RestOperationMeta restOperation, HttpServletResponseEx responseEx) {
+ return new ProduceJsonProcessor();
+ }
+ };
+
+ Response response = filter.extractResponse(invocation, responseEx);
+ Assert.assertEquals(400, response.getStatusCode());
+ Assert.assertEquals(InvocationException.class, response.<InvocationException>getResult().getClass());
+ InvocationException invocationException = response.getResult();
+ Assert.assertEquals(
+ "InvocationException: code=400;msg=CommonExceptionData [message=method null, path null, statusCode 400, reasonPhrase null, response content-type null is not supported]",
+ invocationException.getMessage());
+ Assert.assertEquals("Unrecognized token 'abc': was expecting ('true', 'false' or 'null')\n"
+ + " at [Source: (org.apache.servicecomb.foundation.vertx.stream.BufferInputStream); line: 1, column: 7]",
+ invocationException.getCause().getMessage());
+ Assert.assertEquals(CommonExceptionData.class, invocationException.getErrorData().getClass());
+ CommonExceptionData commonExceptionData = (CommonExceptionData) invocationException.getErrorData();
+ Assert.assertEquals(
+ "method null, path null, statusCode 400, reasonPhrase null, response content-type null is not supported",
+ commonExceptionData.getMessage());
+ }
+
+ @Test
+ public void extractResult_decodeError200(@Mocked Invocation invocation, @Mocked ReadStreamPart part,
+ @Mocked OperationMeta operationMeta, @Mocked ResponseMeta responseMeta,
+ @Mocked RestOperationMeta swaggerRestOperation,
+ @Mocked HttpServletResponseEx responseEx) {
+ Map<String, Object> handlerContext = new HashMap<>();
new Expectations() {
{
invocation.getHandlerContext();
@@ -156,18 +203,20 @@ ProduceProcessor findProduceProcessor(RestOperationMeta restOperation, HttpServl
};
Response response = filter.extractResponse(invocation, responseEx);
- Assert.assertEquals(490, response.getStatusCode());
- Assert.assertEquals(ExceptionFactory.CONSUMER_INNER_REASON_PHRASE, response.getReasonPhrase());
+ Assert.assertEquals(400, response.getStatusCode());
Assert.assertEquals(InvocationException.class, response.<InvocationException>getResult().getClass());
InvocationException invocationException = response.getResult();
- Assert.assertEquals("InvocationException: code=490;msg=CommonExceptionData [message=Cse Internal Bad Request]",
+ Assert.assertEquals(
+ "InvocationException: code=400;msg=CommonExceptionData [message=method null, path null, statusCode 200, reasonPhrase null, response content-type null is not supported]",
invocationException.getMessage());
Assert.assertEquals("Unrecognized token 'abc': was expecting ('true', 'false' or 'null')\n"
+ " at [Source: (org.apache.servicecomb.foundation.vertx.stream.BufferInputStream); line: 1, column: 7]",
invocationException.getCause().getMessage());
Assert.assertEquals(CommonExceptionData.class, invocationException.getErrorData().getClass());
CommonExceptionData commonExceptionData = (CommonExceptionData) invocationException.getErrorData();
- Assert.assertEquals("Cse Internal Bad Request", commonExceptionData.getMessage());
+ Assert.assertEquals(
+ "method null, path null, statusCode 200, reasonPhrase null, response content-type null is not supported",
+ commonExceptionData.getMessage());
}
@Test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services