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

Override HTTP response code for particular service method? #517

Closed
anweiss opened this issue Jan 8, 2018 · 29 comments
Closed

Override HTTP response code for particular service method? #517

anweiss opened this issue Jan 8, 2018 · 29 comments

Comments

@anweiss
Copy link

anweiss commented Jan 8, 2018

Is there any way to choose specific HTTP status codes in a response rather than relying on the pre-mapped values defined here? Similar to #240

@anweiss
Copy link
Author

anweiss commented Jan 8, 2018

@achew22
Copy link
Collaborator

achew22 commented Feb 16, 2018

Maybe the correct thing to do is to fix the central list of these mappings. I think somewhere in the bowels of the beast we have a mapping like that. What is the mapping you would prefer?

@OscarHanzely
Copy link

Well I ended on same problem. My Create method must return HTTP status 201 Created. gRPC has no code for that, in fact gRPC is all 200 OK or 4xx+ if error occurs.
So the grpc-gateway just has complete missing implementation of any 2xx codes.

I am ending up with implementing own forwarder for each of the Create end point, however it is poorly designed, because I have to implement entire forwarder, since the status code comes as last method of Writer and after that headers cannot be modified. So I cannot even call the generic runtime.ForwardResponseMessage because the headers would be already sent. This is a big gap in implementation of grpc-gateway

@stale
Copy link

stale bot commented Sep 17, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Sep 17, 2019
@stale stale bot closed this as completed Sep 24, 2019
@robinbryce
Copy link

What is the problem with providing a sensible means to set the response codes directly ? 206 partial content is a pretty reasonable ask isn't it ?

@johanbrandhorst
Copy link
Collaborator

There is only one successful response code in gRPC, and there is an argument to be made that if you need something that is much more than just mapping gRPC onto HTTP/REST, maybe the grpc-gateway isn't what you should be using.

Having said that, I think we'd all be open to exploring any ideas you or anyone else might have for how to implement this sort of ask, be it custom proto options and what not. What we will not do, however, is put our own valuable time into it, unless we have a personal need for it.

@deefstes
Copy link

There is only one successful response code in gRPC, and there is an argument to be made that if you need something that is much more than just mapping gRPC onto HTTP/REST, maybe the grpc-gateway isn't what you should be using.

I don't think a requirement to provide HTTP/REST codes when responding on HTTP/REST and gRPC codes when responding on gRPC could really be described as "much more than just mapping gRPC onto HTTP/REST".

I've had this requirement more than just once or twice but even so, I can still not think of a better option that the grpc-gateway. I would be disappointed if a simple requirement like this is really a sign that the grpc-gateway is not what I should be using.

@johanbrandhorst
Copy link
Collaborator

Is it possible to do this with a custom ResponseForwarder today? Could be flagged on some metadata value perhaps.

@Xuyuanp
Copy link

Xuyuanp commented Mar 19, 2020

I just find a way to do this.

const HttpCodeHeader = "X-Http-Code"

mux := gwruntime.NewServerMux(
    // ...
    gwruntime.WithForwardResponseOption(forwardReponseFunc),
)

func forwardReponseFunc(ctx context.Context, w http.ResponseWriter, _ proto.Message) error {
	smd, ok := gwruntime.ServerMetadataFromContext(ctx)
	if !ok {
		return nil
	}
	if vals := smd.HeaderMD.Get(HttpCodeHeader); len(vals) > 0 {
		code, err := strconv.Atoi(vals[0])
		if err != nil {
			return err
		}
		w.WriteHeader(code)
	}
	return nil
}

// set header in your handler
md := metadata.Pairs(HttpCodeHeader, "302")
grpc.SetHeader(ctx, md)

@johanbrandhorst
Copy link
Collaborator

Nice, that's exactly what I was hoping. I'd recommend removing the header from the metadata as well before returning.

@zhughes3
Copy link
Contributor

zhughes3 commented May 10, 2020

@johanbrandhorst How could I go about removing the header from the metadata?

I have been trying to do it like this grpc.SetHeader(ctx, metadata.Pairs("x-http-code", "")) but am still getting the Grpc-Metadata-X-Http-Code http header.

@johanbrandhorst
Copy link
Collaborator

You overwrite the metadata in the context.

https://play.golang.org/p/ysICtpP9Fe9

@zhughes3
Copy link
Contributor

Thanks. For posterity, I was able to delete all outgoing HTTP response headers added by grpc-gateway with this call:

delete(w.Header(), "Grpc-Metadata-X-Http-Code")

@FunnyDevP
Copy link

Thanks. For posterity, I was able to delete all outgoing HTTP response headers added by grpc-gateway with this call:

delete(w.Header(), "Grpc-Metadata-X-Http-Code")

delete(w.Header(), "Grpc-Metadata-X-Http-Code") Is process before return ?

		smd, ok := runtime.ServerMetadataFromContext(ctx)
		if !ok {
			return nil
		}
		if vals := smd.HeaderMD.Get("X-Http-Code"); len(vals) > 0 {
			code, err := strconv.Atoi(vals[0])
			if err != nil {
				return err
			}

			w.WriteHeader(code)
		}
		delete(w.Header(), "X-Http-Code")
		return nil

@zhughes3
Copy link
Contributor

zhughes3 commented May 24, 2020

@FunnyDevP So I set up an http response modifier in the grpc-gateway that looks like this:

func httpResponseModifier(ctx context.Context, w http.ResponseWriter, p proto.Message) error {
	md, ok := runtime.ServerMetadataFromContext(ctx)
	if !ok {
		return nil
	}

	// set http status code
	if vals := md.HeaderMD.Get("x-http-code"); len(vals) > 0 {
		code, err := strconv.Atoi(vals[0])
		if err != nil {
			return err
		}
		w.WriteHeader(code)
		delete(md.HeaderMD, "x-http-code")
		delete(w.Header(), "Grpc-Metadata-X-Http-Code")
	}

	return nil
}

This response modifier gets set in the grpc-gateway like this:

gwMux := runtime.NewServeMux(
		runtime.WithForwardResponseOption(httpResponseModifier),
	)

I do this so I don't expose any grpc headers in the outgoing http response.

@johanbrandhorst
Copy link
Collaborator

Hey @zhughes3 would you be willing to contribute this to the gateway docs? Maybe somewhere in https://github.com/grpc-ecosystem/grpc-gateway/blob/master/docs/_docs/customizingyourgateway.md?

@zhughes3
Copy link
Contributor

@johanbrandhorst Will do. Should have a PR up shortly.

@zhughes3
Copy link
Contributor

zhughes3 commented May 24, 2020

@johanbrandhorst I cloned the repo locally and committed my changes to a branch. I'm unable to push my new branch to the repo to open up a PR. Any suggestions?

I also signed the Contributor License Agreement as found here: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/CONTRIBUTING.md

@Xuyuanp
Copy link

Xuyuanp commented May 25, 2020

@zhughes3 fork it, push to your own repo, PR

@ghostiam
Copy link
Contributor

ghostiam commented Sep 1, 2020

After:

grpc.SetHeader(ctx, metadata.Pairs(CustomHeaderSetCookieToken, token))

I get an error:

grpc: failed to fetch the stream from the context context.Background.WithValue(type *http.contextKey, val <not Stringer>).WithValue(type *http.contextKey, val 127.0.0.1:3000).WithCancel.WithCancel.WithValue(type runtime.rpcMethodKey, val /partner.Users/UserLogin).WithValue(type metadata.mdIncomingKey, val <not Stringer>)

I use this without GRPC server.

@johanbrandhorst
Copy link
Collaborator

Not seen that kind of error before, but I don't think it's related to the gateway. How do you mean you're using it without a gRPC server? You can't use grpc.SetHeader if you're not using gRPC.

@ghostiam
Copy link
Contributor

ghostiam commented Sep 2, 2020

I am using gateway to generate HTTP API, I don't need gRPC in one of the tasks.
It turns out that in the context there is no "streamKey" which contains "ServerTransportStream", if do not use the gRPC server.

@ghostiam
Copy link
Contributor

ghostiam commented Sep 2, 2020

I use v2 branch, but this has no effect on the error (in the master branch, I get panic)
Minimum code to repeat the error:

main.go

package main

import (
	"context"
	"fmt"
	"log"
	"net/http/httptest"

	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime" // use v2
	"google.golang.org/grpc"
	"google.golang.org/grpc/metadata"
)

type echo struct{}

func (e *echo) Echo(ctx context.Context, msg *EchoMessage) (*EchoMessage, error) {
	err := grpc.SetHeader(ctx, metadata.Pairs("test", "test"))
	if err != nil {
		return nil, err
	}

	return &EchoMessage{Message: msg.Message}, nil
}

func main() {
	mux := runtime.NewServeMux()

	err := RegisterEchoHandlerServer(context.Background(), mux, &echo{})
	if err != nil {
		log.Fatal(err)
	}

	w := httptest.NewRecorder()
	r := httptest.NewRequest("GET", "/echo", nil)
	mux.ServeHTTP(w, r)

	fmt.Println(w.Code)
	fmt.Println(w.Body)
}

proto

syntax = "proto3";

package main;
option go_package = ".;main";

import "google/api/annotations.proto";

message EchoMessage {
    string message = 1;
}

service Echo {
    rpc Echo (EchoMessage) returns (EchoMessage) {
        option (google.api.http) = {
            get: "/echo"
        };
    }
}

install and generate command

go install \
    github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-grpc-gateway \
    github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2 \
    github.com/golang/protobuf/protoc-gen-go

protoc -I/usr/local/include -I. \
	-I${GOPATH}/src \
	-I${GOPATH}/pkg/mod \
	-I${GOPATH}/pkg/mod/github.com/grpc-ecosystem/grpc-gateway/v2@v2.0.0-beta.4/third_party/googleapis \
	-I${GOPATH}/pkg/mod/github.com/grpc-ecosystem/grpc-gateway/v2@v2.0.0-beta.4 \
	--go_out=plugins=grpc:. \
	--grpc-gateway_out=logtostderr=true,allow_delete_body=true,paths=source_relative:. \
	*.proto

Output:

500
{"code":13, "message":"grpc: failed to fetch the stream from the context context.Background.WithCancel.WithValue(type runtime.rpcMethodKey, val /main.Echo/Echo).WithValue(type metadata.mdIncomingKey, val <not Stringer>)", "details":[]}

@ghostiam
Copy link
Contributor

ghostiam commented Sep 2, 2020

I studied the code, at the moment I do not see any way to pass anything outside the handler.
Also, it seems that this error does not depend on the presence of the gRPC server, since their context does not overlap.

The solution that I see is to allow the metadata to be changed from the context, and for that it needs to be passed there.

func local_request_Echo_Echo_0(ctx context.Context, marshaler runtime.Marshaler, server EchoServer, req *http.Request, pathParams map[string]string) (proto.Message, runtime.ServerMetadata, error) {
	var protoReq EchoMessage
	var metadata runtime.ServerMetadata

+	ctx = runtime.NewServerMetadataContext(ctx, runtime.ServerMetadata{
+		HeaderMD: map[string][]string{},
+		TrailerMD: map[string][]string{},
+	})

	if err := req.ParseForm(); err != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err)
	}
	if err := runtime.PopulateQueryParameters(&protoReq, req.Form, filter_Echo_Echo_0); err != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err)
	}

	msg, err := server.Echo(ctx, &protoReq)

+	md, ok := runtime.ServerMetadataFromContext(ctx)
+	if ok {
+		metadata = md
+	}

	return msg, metadata, err

}
...
func (e *echo) Echo(ctx context.Context, msg *EchoMessage) (*EchoMessage, error) {
	md, ok := runtime.ServerMetadataFromContext(ctx)
	if ok {
		md.HeaderMD.Set("test", "val")
	}

	return &EchoMessage{Message: msg.Message}, nil
}
...

I also do not understand the meaning of

var metadata runtime.ServerMetadata

which is always empty

@ghostiam
Copy link
Contributor

ghostiam commented Sep 2, 2020

patch for v2 branch

Index: protoc-gen-grpc-gateway/internal/gengateway/template.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- protoc-gen-grpc-gateway/internal/gengateway/template.go	(revision 6c641e210b8b666579c9871253a975b9d831a4b6)
+++ protoc-gen-grpc-gateway/internal/gengateway/template.go	(date 1599019266697)
@@ -481,7 +481,8 @@
 {{$AllowPatchFeature := .AllowPatchFeature}}
 {{template "local-request-func-signature" .}} {
 	var protoReq {{.Method.RequestType.GoType .Method.Service.File.GoPkg.Path}}
-	var metadata runtime.ServerMetadata
+	metadata := runtime.ServerMetadata{HeaderMD: make(map[string][]string), TrailerMD: make(map[string][]string)}
+	ctx = runtime.NewServerMetadataContext(ctx, metadata)
 {{if .Body}}
 	newReader, berr := utilities.IOReaderFactory(req.Body)
 	if berr != nil {
@@ -566,6 +567,9 @@
 	// TODO
 {{else}}
 	msg, err := server.{{.Method.GetName}}(ctx, &protoReq)
+	if md, ok := runtime.ServerMetadataFromContext(ctx); ok {
+		metadata = md
+	}
 	return msg, metadata, err
 {{end}}
 }`))



A quick crutch if the changes are not accepted

# init server metadata in context
sed -i '/func local_request_.*/{n;n;s/var metadata runtime.ServerMetadata/metadata := runtime.ServerMetadata{HeaderMD: make(map[string][]string), TrailerMD: make(map[string][]string)}\n\tctx = runtime.NewServerMetadataContext(ctx, metadata)/}' *.pb.gw.go
# set server metadata from context after method execute
sed -i '/\tmsg, err := server./!b;a\\tif md, ok := runtime.ServerMetadataFromContext(ctx); ok {\n\t\tmetadata = md\n\t}' *.pb.gw.go

@johanbrandhorst
Copy link
Collaborator

Thanks, could you submit this as a PR?

@ghostiam
Copy link
Contributor

ghostiam commented Sep 2, 2020

Ok I'll try to do PR

@ghostiam
Copy link
Contributor

ghostiam commented Sep 2, 2020

@johanbrandhorst I started writing tests and noticed that grpc.SendHeader in an already existing test works
https://github.com/grpc-ecosystem/grpc-gateway/blob/v2/examples/internal/integration/integration_test.go#L315
https://github.com/grpc-ecosystem/grpc-gateway/blob/v2/examples/internal/server/echo.go#L29

As you can see in the debugger, the call chain is different and in the case of the test, gateway is used as a proxy, and not an independent handler.

Снимок экрана 2020-09-03 в 05 17 48

I have a question, if I don’t find another solution, should I replace the description of how to add new headers in the documentation, or add as an alternative option with the note that it works without grpc?

UPD:
I understand, in the gateway tests is called the client, not the server, and therefore it is necessary to me feature is not even called.

UPD2:
I was able to get a failed test, just changed the port from 8088 to 8089 (hardcoded in tests)
Снимок экрана 2020-09-03 в 05 32 37

@ghostiam
Copy link
Contributor

ghostiam commented Sep 2, 2020

I will try to implement ServerTransportStream so that the experience with and without gRPC is the same.
I think this will be the best solution, since it is not a good idea to forward ServerMetadata to gateway, because it is not available in gRPC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants