Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Result of gateway's Stream response is wrapped with "result" #579

Closed
giiita opened this issue Mar 20, 2018 · 7 comments
Closed

Result of gateway's Stream response is wrapped with "result" #579

giiita opened this issue Mar 20, 2018 · 7 comments

Comments

@giiita
Copy link

giiita commented Mar 20, 2018

The output result of swagger is not wrapped with result even if it is stream.
However, because the gateway 's response definition is wrapped in result, it can not be automatically corresponded.
Is there a way to associate these with each other?

@cy-zheng
Copy link
Contributor

Same question. Is there any way remove this wrapper?

@achew22
Copy link
Collaborator

achew22 commented May 11, 2018

Can you provide some sample inputs and outputs (both expected and what you did receive) as well as the proto you used to define?

@cy-zheng
Copy link
Contributor

Well, It's simple. The actual return value of server side stream is like:
{"result":{ ...myobject... }}
{"result":{ ...myobject... }}
{"result":{ ...myobject... }}
But I need:
{ ...myobject... }
{ ...myobject... }
{ ...myobject... }

@giiita
Copy link
Author

giiita commented Jun 13, 2018

Are you waiting for a response?
For example.

message Term {
    int64 startAt = 1;
    int64 endAt = 2;
}

message Response {
  repeated Term terms = 1;
}

message Request {
  string token = 1;
}

service Setting {
    rpc run(Request) returns (stream Response) {
        option (google.api.http) = {
            get: "/request"
        };
    };
}

At that time,

  "responses": {
          "200": {
            "description": "",
            "schema": {
              "$ref": "#/definitions/apiResponse"
            }
          }
        }
  ...
  "apiResponse": {
      "type": "object",
      "properties": {
        "terms": {
          "type": "array",
          "items": {
            "$ref": "#/definitions/term"
          }
        }
      }
    },

However, the response between the server and the client using the generated gateway is as follows.

{result: {terms: []}}

This "result" is attached only to grpc-gateway, it is unnecessary.

@birdayz
Copy link
Contributor

birdayz commented Dec 23, 2018

Yeah i'm confused, what's the reason for this?
It would be a breaking change, so i'm not going to get my hopes up before a v2..
however it's highly inconsistent with the swagger output, the results field is missing there.

@johanbrandhorst any guidance how to tackle this?

so what could we do? i see, without thinking over it too long, these possibilities:

  1. Remove the "result" field wrapper, to be 1) consistent and 2) it's unexpected, so i don't think it belongs here unless there's some technical limitation..
    That's a breaking change though.
  2. Fix the swagger-ui generation and include the "results" object. I'm not sure how easy this is, as we have to detect if it's a streaming response. but should be possible without too much effort
  3. Add support for SSE/Websocket, enabled by maybe a specific option. Any1 who's not satisfied with the current situation could switch over to that.

I guess 1) would be best, as the "results" field just looks awkward and should not be there. But realistically, i guess we won't introduce such a breaking change.
3) Would be nice, as it would be more "standard". However it's more work and i'm not sure if anyone would fine the time to work on it (i won't, at least for the next weeks)
So there's 2), i dislike it but would at least make it consistent..

thoughts?

edit: also: any plans for a targeted v2 release, where such things could be implemented/fixed? maybe a v2 / "next" branch where early adopters could get these changes asap?

@johanbrandhorst
Copy link
Collaborator

This puzzled me a lot as well initially, but I think now that the wrapper is so that we can provide "result" with successful calls and "error" when a call fails and we return a marshalled error. Because we can't use trailers with success or failure codes like gRPC does, we have to have some way to differentiate between success and failure, hence the wrapper.

With that in mind, option 2 is the preferred solution here. In addition, I'm not sure swagger really supports streaming responses anyway?

Yes, we do plan on releasing a v2 at some point, though all the current maintainers are busy so the project is really in maintenance mode. There's a v2 PR open and an issue label you can take a look at if you are interested in picking that up.

As to option 3, you'll be pleased to know that such a solution kind of exists in https://github.com/tmc/grpc-websocket-proxy.

@giiita giiita closed this as completed Dec 27, 2018
@johanbrandhorst
Copy link
Collaborator

Reopening since @birdayz might still want to submit a fix for this.

johanbrandhorst pushed a commit that referenced this issue Jan 23, 2019
…#850)

* #579 fixing protoc-gen-swagger to wrap server stream messages with "result" object to match gateway behavior

* #579 fixing test example swagger json

* change streamdefinitions to x-stream-definitions and add test

* move runtime internal to root of repo for use in protoc-gen-swagger, adding error to stream wrapper

* adding comment explaining AddStreamError

* fix bazel
adasari pushed a commit to adasari/grpc-gateway that referenced this issue Apr 9, 2020
…grpc-ecosystem#850)

* grpc-ecosystem#579 fixing protoc-gen-swagger to wrap server stream messages with "result" object to match gateway behavior

* grpc-ecosystem#579 fixing test example swagger json

* change streamdefinitions to x-stream-definitions and add test

* move runtime internal to root of repo for use in protoc-gen-swagger, adding error to stream wrapper

* adding comment explaining AddStreamError

* fix bazel
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

No branches or pull requests

5 participants