Skip to content

Conversation

wujimin
Copy link
Contributor

@wujimin wujimin commented Sep 25, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 86.02% when pulling c531c63 on wujimin:extract-rest-invocation into 054519a on ServiceComb:master.

for (HttpServerFilter filter : httpServerFilters) {
Response response = filter.afterReceiveRequest(invocation, requestEx);
if (response != null) {
sendResponseQuietly(response);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the filter could hijack the response message which calling the doInvoke method.
I don't think it's good idea, there are too much assumption.

Copy link
Contributor Author

@wujimin wujimin Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if user decided to break the filter chain, and direct send response, user return a Response.
if do not do like this, we need to build another mechanism like handler chain or some api to send response, it's too complex.

what's your suggestion?

Copy link
Member

@WillemJiang WillemJiang Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some comments in the HttpServerFilter.afterReceiveRequst, to make sure user know about this kind of feature.
We still need to make sure the doInvoke() is called in the invoke method, no matter what kind of HttpServerFilter is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already have comments in HttpServerFilter.afterReceiveRequst and HttpClientFilter.afterReceiveResponse in current PR code.

image

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

import io.servicecomb.swagger.invocation.Response;
import io.servicecomb.swagger.invocation.exception.InvocationException;

public class AbstractRestServer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to remove the AbstractRestServer?

Copy link
Contributor Author

@wujimin wujimin Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is our internal class, not a interface to developers

@WillemJiang WillemJiang merged commit 8e8bffb into apache:master Sep 27, 2017
@wujimin wujimin changed the title Extract rest invocation JAV-355 Extract rest invocation Oct 12, 2017
@wujimin wujimin deleted the extract-rest-invocation branch October 25, 2017 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants