[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16480223#comment-16480223
]
ASF GitHub Bot commented on SCB-580:
liubao68 closed pull request #702: [SCB-580] fix response of the upload file
too large
URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/702
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/integration-tests/spring-zuul-tracing-tests/src/test/java/org/apache/servicecomb/spring/cloud/zuul/tracing/SpringCloudZuulTracingTest.java
b/integration-tests/spring-zuul-tracing-tests/src/test/java/org/apache/servicecomb/spring/cloud/zuul/tracing/SpringCloudZuulTracingTest.java
index 35a82b1de..1402cc237 100644
---
a/integration-tests/spring-zuul-tracing-tests/src/test/java/org/apache/servicecomb/spring/cloud/zuul/tracing/SpringCloudZuulTracingTest.java
+++
b/integration-tests/spring-zuul-tracing-tests/src/test/java/org/apache/servicecomb/spring/cloud/zuul/tracing/SpringCloudZuulTracingTest.java
@@ -17,7 +17,6 @@
package org.apache.servicecomb.spring.cloud.zuul.tracing;
-import static com.seanyinx.github.unit.scaffolding.AssertUtils.expectFailing;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.core.Is.is;
import static org.junit.Assert.assertEquals;
diff --git
a/transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/RestBodyHandler.java
b/transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/RestBodyHandler.java
index 6c9724e41..a02dffc30 100644
---
a/transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/RestBodyHandler.java
+++
b/transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/RestBodyHandler.java
@@ -30,6 +30,7 @@
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 io.netty.handler.codec.http.HttpHeaderValues;
import
io.netty.handler.codec.http.multipart.HttpPostRequestDecoder.ErrorDataDecoderException;
@@ -195,7 +196,8 @@ public void handle(Buffer buff) {
uploadSize += buff.length();
if (bodyLimit != -1 && uploadSize > bodyLimit) {
failed = true;
-context.fail(Status.REQUEST_ENTITY_TOO_LARGE.getStatusCode());
+context.fail(new InvocationException(Status.REQUEST_ENTITY_TOO_LARGE,
+Status.REQUEST_ENTITY_TOO_LARGE.getReasonPhrase()));
} else {
// multipart requests will not end up in the request body
// url encoded should also not, however jQuery by default
diff --git
a/transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
b/transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
index 364508f23..0d0c0e66c 100644
---
a/transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
+++
b/transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
@@ -17,6 +17,11 @@
package org.apache.servicecomb.transport.rest.vertx;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response.Status;
+import javax.ws.rs.core.Response.Status.Family;
+
import org.apache.servicecomb.common.rest.AbstractRestInvocation;
import org.apache.servicecomb.common.rest.RestConst;
import org.apache.servicecomb.common.rest.RestProducerInvocation;
@@ -32,6 +37,7 @@
import org.slf4j.LoggerFactory;
import
io.netty.handler.codec.http.multipart.HttpPostRequestDecoder.ErrorDataDecoderException;
+import io.vertx.core.json.JsonObject;
import io.vertx.ext.web.Router;
import io.vertx.ext.web.RoutingContext;
import io.vertx.ext.web.handler.CookieHandler;
@@ -64,9 +70,106 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16478414#comment-16478414
]
ASF GitHub Bot commented on SCB-580:
wujimin commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188826253
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +70,106 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(invocationException.getStatusCode()).setStatusMessage(invocationException.getReasonPhrase())
+ .end(wrapResponseBody(invocationException.getReasonPhrase()));
+} else {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode()).end(wrapResponseBody(e.getMessage()));
+}
+context.response().close();
+ }
+
+ /**
+ * Consumer will treat the response body as json by default, so it's
necessary to wrap response body as Json string
+ * to avoid deserialization error.
+ *
+ * @param message response body
+ * @return response body wrapped as Json string
+ */
+ String wrapResponseBody(String message) {
+if (isValidJson(message)) {
+ return message;
+}
+
+JsonObject jsonObject = new JsonObject();
+jsonObject.put("message", message);
+
+return jsonObject.toString();
Review comment:
ok
because your last commit just return a string, now return a
CommonExceptionData structure, so i have this question
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
>
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16477140#comment-16477140
]
ASF GitHub Bot commented on SCB-580:
yhs0092 commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188556614
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +70,106 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(invocationException.getStatusCode()).setStatusMessage(invocationException.getReasonPhrase())
+ .end(wrapResponseBody(invocationException.getReasonPhrase()));
+} else {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode()).end(wrapResponseBody(e.getMessage()));
+}
+context.response().close();
+ }
+
+ /**
+ * Consumer will treat the response body as json by default, so it's
necessary to wrap response body as Json string
+ * to avoid deserialization error.
+ *
+ * @param message response body
+ * @return response body wrapped as Json string
+ */
+ String wrapResponseBody(String message) {
+if (isValidJson(message)) {
+ return message;
+}
+
+JsonObject jsonObject = new JsonObject();
+jsonObject.put("message", message);
+
+return jsonObject.toString();
Review comment:
Actually I have no idea about what kind of Json response should be written.
But I find out that currently we wrap exception and write response like below,
so I copy this pattern.
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
>
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16477044#comment-16477044
]
ASF GitHub Bot commented on SCB-580:
wujimin commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188543532
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +70,106 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(invocationException.getStatusCode()).setStatusMessage(invocationException.getReasonPhrase())
+ .end(wrapResponseBody(invocationException.getReasonPhrase()));
+} else {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode()).end(wrapResponseBody(e.getMessage()));
+}
+context.response().close();
+ }
+
+ /**
+ * Consumer will treat the response body as json by default, so it's
necessary to wrap response body as Json string
+ * to avoid deserialization error.
+ *
+ * @param message response body
+ * @return response body wrapped as Json string
+ */
+ String wrapResponseBody(String message) {
+if (isValidJson(message)) {
+ return message;
+}
+
+JsonObject jsonObject = new JsonObject();
+jsonObject.put("message", message);
+
+return jsonObject.toString();
Review comment:
this will produce:
{
"message": ..
}
is this we expected?
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[ https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16476977#comment-16476977 ] ASF GitHub Bot commented on SCB-580: coveralls commented on issue #702: [SCB-580] fix response of the upload file too large URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#issuecomment-36098 [](https://coveralls.io/builds/17006777) Coverage increased (+0.03%) to 87.372% when pulling **05edc951bdacc4f39501be8fad119f63dd971c29 on yhs0092:fix_response_of_the_upload_file_too_large** into **2f8de349d4b0b82d3dc14fcb9c94d3ae7046f9b5 on apache:master**. 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: us...@infra.apache.org > When upload file size exceeds limitation of provider, consumer will return a > confusing response > --- > > Key: SCB-580 > URL: https://issues.apache.org/jira/browse/SCB-580 > Project: Apache ServiceComb > Issue Type: Bug > Components: Java-Chassis >Reporter: YaoHaishi >Assignee: YaoHaishi >Priority: Major > Fix For: java-chassis-1.0.0-m2 > > Attachments: compile_warning.PNG, consumerResponse.PNG, > providerExceptionLog.PNG > > > Test based on 1.0.0.B009 > Let user upload file to consumer, and then consumer upload file to provider. > If the file size is under the limitation of consumer but exceeds the > limitation of provider, the provider will throw a NullPointerException, but > the consumer will response "method POST, path /upload/multipartFileUpload/, > statusCode 500, reasonPhrase Internal Server Error, response content-type > null is not supported".(In the log of consumer, no error is printed) > provider log: > !providerExceptionLog.PNG! > consumer response: > !consumerResponse.PNG! -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16476955#comment-16476955
]
ASF GitHub Bot commented on SCB-580:
yhs0092 commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188522352
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +69,85 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(invocationException.getStatusCode()).setStatusMessage(invocationException.getReasonPhrase())
+ .end(wrapResponseBody(invocationException.getReasonPhrase()));
+} else {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode()).end(wrapResponseBody(e.getMessage()));
+}
+context.response().close();
+ }
+
+ /**
+ * Consumer will treat the response body as json by default, so it's
necessary to wrap response body with ""
+ * to avoid deserialization error.
+ *
+ * @param message response body
+ * @return response body wrapped with ""
+ */
+ private String wrapResponseBody(String message) {
+return "\"" + message + "\"";
Review comment:
I've done it. Thanks to the powerful special char escape ability of Vertx's
JsonObject, it's not so complex as I expected.
In the UT testWrapResponseBody, you can see that JsonObject can handle
special char well.
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
>
>
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16476946#comment-16476946
]
ASF GitHub Bot commented on SCB-580:
wujimin commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188520339
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +69,85 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(invocationException.getStatusCode()).setStatusMessage(invocationException.getReasonPhrase())
+ .end(wrapResponseBody(invocationException.getReasonPhrase()));
+} else {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode()).end(wrapResponseBody(e.getMessage()));
+}
+context.response().close();
+ }
+
+ /**
+ * Consumer will treat the response body as json by default, so it's
necessary to wrap response body with ""
+ * to avoid deserialization error.
+ *
+ * @param message response body
+ * @return response body wrapped with ""
+ */
+ private String wrapResponseBody(String message) {
+return "\"" + message + "\"";
Review comment:
if there is some special char in message(eg: "), will output a invalid json
string
so must use json encode API
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
>
> Attachments: compile_warning.PNG, consumerResponse.PNG,
> providerExceptionLog.PNG
>
>
> Test based on 1.0.0.B009
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16476860#comment-16476860
]
ASF GitHub Bot commented on SCB-580:
yhs0092 commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188506146
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +69,85 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
Review comment:
I choose this content type to avoid body decode problem.
If user set provider's operation produce "text/plain", and we set the
Content-Type header as "application/json", it will cause consumer failed to
decode response body, and the consumer will response like below:
```json
{
"message": "method POST, path /upload/multipartFileUpload/, statusCode
413, reasonPhrase Request Entity Too Large, response content-type
application/json is not supported"
}
```
But "MediaType.WILDCARD" will let the producer use the default
ProduceProcessor to decode the body. No matter it's json processor or text
processor, it will handle the body without exception thrown.
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
>
> Attachments: compile_warning.PNG, consumerResponse.PNG,
> providerExceptionLog.PNG
>
>
> Test based on 1.0.0.B009
> Let user upload file to consumer, and then consumer upload file to provider.
> If the file size is under the limitation of consumer but exceeds the
> limitation of provider, the provider will throw a
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16476858#comment-16476858
]
ASF GitHub Bot commented on SCB-580:
yhs0092 commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188506146
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +69,85 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
Review comment:
I choose this content type to avoid body decode problem.
If user set provider's operation produce "text/plain", and we set the
Content-Type header as "application/json", it will cause consumer failed to
decode response body, and the consumer will response like below:
```json
{
"message": "method POST, path /upload/multipartFileUpload/, statusCode
413, reasonPhrase Request Entity Too Large, response content-type
application/json is not supported"
}
```
But "*/*" will let the producer use the default ProduceProcessor to decode
the body. No matter it's json processor or text processor, it will handle the
body without exception thrown.
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
>
> Attachments: compile_warning.PNG, consumerResponse.PNG,
> providerExceptionLog.PNG
>
>
> Test based on 1.0.0.B009
> Let user upload file to consumer, and then consumer upload file to provider.
> If the file size is under the limitation of consumer but exceeds the
> limitation of provider, the provider will throw a NullPointerException,
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16476856#comment-16476856
]
ASF GitHub Bot commented on SCB-580:
yhs0092 commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188506146
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +69,85 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
Review comment:
If user set provider's operation produce "text/plain", and we set the
Content-Type header as "application/json", it will cause consumer failed to
decode response body, and the consumer will response like below:
```json
{
"message": "method POST, path /upload/multipartFileUpload/, statusCode
413, reasonPhrase Request Entity Too Large, response content-type
application/json is not supported"
}
```
But "*/*" will let the producer use the default ProduceProcessor to decode
the body. No matter it's json processor or text processor, it will handle the
body without exception thrown.
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
>
> Attachments: compile_warning.PNG, consumerResponse.PNG,
> providerExceptionLog.PNG
>
>
> Test based on 1.0.0.B009
> Let user upload file to consumer, and then consumer upload file to provider.
> If the file size is under the limitation of consumer but exceeds the
> limitation of provider, the provider will throw a NullPointerException, but
> the consumer will response "method POST, path
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16476800#comment-16476800
]
ASF GitHub Bot commented on SCB-580:
liubao68 commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188497084
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +69,85 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
Review comment:
wrapResponseBody uses json, is it here APPLICATION/JSON?
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
>
> Attachments: compile_warning.PNG, consumerResponse.PNG,
> providerExceptionLog.PNG
>
>
> Test based on 1.0.0.B009
> Let user upload file to consumer, and then consumer upload file to provider.
> If the file size is under the limitation of consumer but exceeds the
> limitation of provider, the provider will throw a NullPointerException, but
> the consumer will response "method POST, path /upload/multipartFileUpload/,
> statusCode 500, reasonPhrase Internal Server Error, response content-type
> null is not supported".(In the log of consumer, no error is printed)
> provider log:
> !providerExceptionLog.PNG!
> consumer response:
> !consumerResponse.PNG!
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16476750#comment-16476750
]
ASF GitHub Bot commented on SCB-580:
yhs0092 commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188490031
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +69,85 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(invocationException.getStatusCode()).setStatusMessage(invocationException.getReasonPhrase())
+ .end(wrapResponseBody(invocationException.getReasonPhrase()));
+} else {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode()).end(wrapResponseBody(e.getMessage()));
+}
+context.response().close();
+ }
+
+ /**
+ * Consumer will treat the response body as json by default, so it's
necessary to wrap response body with ""
+ * to avoid deserialization error.
+ *
+ * @param message response body
+ * @return response body wrapped with ""
+ */
+ private String wrapResponseBody(String message) {
+return "\"" + message + "\"";
Review comment:
Maybe it's a little bit complex to cover all situations, because I'm not
sure what is contained in the message.
But I'll try to make it and reply later.
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
>
> Attachments: compile_warning.PNG, consumerResponse.PNG,
>
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16476726#comment-16476726
]
ASF GitHub Bot commented on SCB-580:
liubao68 commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188486239
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +69,85 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
+ // if there is restProducerInvocation, let it send exception in
response. The exception is allowed to be null.
+ sendFailResponseByInvocation(context, restProducerInvocation, e);
+ return;
+}
+
+if (null != e) {
+ // if there exists exception, try to send this exception by
RoutingContext
+ sendExceptionByRoutingContext(context, e);
+ return;
+}
+
+// if there is no exception, the response is determined by status code.
+sendFailureRespDeterminedByStatus(context);
+ }
+
+ /**
+ * Try to determine response by status code, and send response.
+ */
+ private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+Family statusFamily = Family.familyOf(context.statusCode());
+if (Family.CLIENT_ERROR.equals(statusFamily) ||
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+.equals(statusFamily)) {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(context.statusCode()).end();
+} else {
+ // it seems the status code is not set properly
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+ .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+ .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+}
+context.response().close();
+ }
+
+ /**
+ * Use routingContext to send failure information in throwable.
+ */
+ private void sendExceptionByRoutingContext(RoutingContext context, Throwable
e) {
+if (InvocationException.class.isInstance(e)) {
+ InvocationException invocationException = (InvocationException) e;
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(invocationException.getStatusCode()).setStatusMessage(invocationException.getReasonPhrase())
+ .end(wrapResponseBody(invocationException.getReasonPhrase()));
+} else {
+ context.response().putHeader(HttpHeaders.CONTENT_TYPE,
MediaType.WILDCARD)
+
.setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode()).end(wrapResponseBody(e.getMessage()));
+}
+context.response().close();
+ }
+
+ /**
+ * Consumer will treat the response body as json by default, so it's
necessary to wrap response body with ""
+ * to avoid deserialization error.
+ *
+ * @param message response body
+ * @return response body wrapped with ""
+ */
+ private String wrapResponseBody(String message) {
+return "\"" + message + "\"";
Review comment:
Is it possible to use Json encode API?
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
>
> Attachments: compile_warning.PNG, consumerResponse.PNG,
> providerExceptionLog.PNG
>
>
> Test based on 1.0.0.B009
> Let user upload file to consumer, and then consumer upload file to
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[ https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16475306#comment-16475306 ] ASF GitHub Bot commented on SCB-580: coveralls commented on issue #702: [SCB-580] fix response of the upload file too large URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#issuecomment-36098 [](https://coveralls.io/builds/16984002) Coverage increased (+0.02%) to 87.36% when pulling **35890e198d1a24f43c3f0172930846fd37e37e08 on yhs0092:fix_response_of_the_upload_file_too_large** into **2f8de349d4b0b82d3dc14fcb9c94d3ae7046f9b5 on apache:master**. 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: us...@infra.apache.org > When upload file size exceeds limitation of provider, consumer will return a > confusing response > --- > > Key: SCB-580 > URL: https://issues.apache.org/jira/browse/SCB-580 > Project: Apache ServiceComb > Issue Type: Bug > Components: Java-Chassis >Reporter: YaoHaishi >Assignee: YaoHaishi >Priority: Major > Fix For: java-chassis-1.0.0-m2 > > Attachments: compile_warning.PNG, consumerResponse.PNG, > providerExceptionLog.PNG > > > Test based on 1.0.0.B009 > Let user upload file to consumer, and then consumer upload file to provider. > If the file size is under the limitation of consumer but exceeds the > limitation of provider, the provider will throw a NullPointerException, but > the consumer will response "method POST, path /upload/multipartFileUpload/, > statusCode 500, reasonPhrase Internal Server Error, response content-type > null is not supported".(In the log of consumer, no error is printed) > provider log: > !providerExceptionLog.PNG! > consumer response: > !consumerResponse.PNG! -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16475293#comment-16475293
]
ASF GitHub Bot commented on SCB-580:
yhs0092 commented on a change in pull request #702: [SCB-580] fix response of
the upload file too large
URL:
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188167573
##
File path:
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
##
@@ -64,9 +69,71 @@ private void failureHandler(RoutingContext context) {
e = cause;
}
}
-restProducerInvocation.sendFailResponse(e);
-// 走到这里，应该都是不可控制的异常，直接关闭连接
+// only when unexpected exception happens, it will run into here.
+// the connection should be closed.
+handleFailureAndClose(context, restProducerInvocation, e);
+ }
+
+ /**
+ * Try to find out the failure information and send it in response.
+ */
+ private void handleFailureAndClose(RoutingContext context,
AbstractRestInvocation restProducerInvocation,
+ Throwable e) {
+if (null != restProducerInvocation) {
Review comment:
These two problems have been fixed. Please review again.
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:
us...@infra.apache.org
> When upload file size exceeds limitation of provider, consumer will return a
> confusing response
> ---
>
> Key: SCB-580
> URL: https://issues.apache.org/jira/browse/SCB-580
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
>Reporter: YaoHaishi
>Assignee: YaoHaishi
>Priority: Major
> Fix For: java-chassis-1.0.0-m2
>
> Attachments: compile_warning.PNG, consumerResponse.PNG,
> providerExceptionLog.PNG
>
>
> Test based on 1.0.0.B009
> Let user upload file to consumer, and then consumer upload file to provider.
> If the file size is under the limitation of consumer but exceeds the
> limitation of provider, the provider will throw a NullPointerException, but
> the consumer will response "method POST, path /upload/multipartFileUpload/,
> statusCode 500, reasonPhrase Internal Server Error, response content-type
> null is not supported".(In the log of consumer, no error is printed)
> provider log:
> !providerExceptionLog.PNG!
> consumer response:
> !consumerResponse.PNG!
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SCB-580) When upload file size exceeds limitation of provider, consumer will return a confusing response
[ https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16474404#comment-16474404 ] ASF GitHub Bot commented on SCB-580: yhs0092 opened a new pull request #702: [SCB-580] fix response of the upload file too large URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/702 Follow this checklist to help us incorporate your contribution quickly and easily: - [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SCB) filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes. - [ ] Each commit in the pull request should have a meaningful subject line and body. - [ ] Format the pull request title like `[SCB-XXX] Fixes bug in ApproximateQuantiles`, where you replace `SCB-XXX` with the appropriate JIRA issue. - [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why. - [ ] Run `mvn clean install` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically. - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf). --- see details in [SCB-580](https://issues.apache.org/jira/browse/SCB-580)  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: us...@infra.apache.org > When upload file size exceeds limitation of provider, consumer will return a > confusing response > --- > > Key: SCB-580 > URL: https://issues.apache.org/jira/browse/SCB-580 > Project: Apache ServiceComb > Issue Type: Bug > Components: Java-Chassis >Reporter: YaoHaishi >Assignee: YaoHaishi >Priority: Major > Attachments: compile_warning.PNG, consumerResponse.PNG, > providerExceptionLog.PNG > > > Test based on 1.0.0.B009 > Let user upload file to consumer, and then consumer upload file to provider. > If the file size is under the limitation of consumer but exceeds the > limitation of provider, the provider will throw a NullPointerException, but > the consumer will response "method POST, path /upload/multipartFileUpload/, > statusCode 500, reasonPhrase Internal Server Error, response content-type > null is not supported".(In the log of consumer, no error is printed) > provider log: > !providerExceptionLog.PNG! > consumer response: > !consumerResponse.PNG! -- This message was sent by Atlassian JIRA (v7.6.3#76005)
