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

[FEATURE REQUEST] MVC serving gRPC-compatible controller #1449

Closed
syklevin opened this issue Feb 12, 2020 · 18 comments
Closed

[FEATURE REQUEST] MVC serving gRPC-compatible controller #1449

syklevin opened this issue Feb 12, 2020 · 18 comments

Comments

@syklevin
Copy link

Is your feature request related to a problem? Please describe.

currently iris mvc would accept a controller func like

func (s *acctService) PostLogin(ctx iris.Context, input *LoginParam) (*LoginResult, error) {}

It just like a grpc service func, but when i change iris.Context to context.Context, it cause panic.
Instead writing a wrapper func, could iris.MVC support context.Context as input parameter?
If ture, then all the grpc services could serve by iris.MVC without adding a line.

Describe the solution you'd like
could accept like this

func (s *acctService) PostLogin(ctx context.Context, input *LoginParam) (*LoginResult, error) {}

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

@kataras
Copy link
Owner

kataras commented Feb 12, 2020

Hello @syklevin,

currently iris mvc would accept a controller func like .... func (s *acctService) PostLogin(ctx iris.Context, input *LoginParam)...

Yes, that's because you can either declare the iris.Context at a Controller's field (which will be binded automatically) or through first input argument of a Controller's methods and the framework passes the iris.Context of the request, just like a normal handler. The context.Context, how can framework determinate what context.Context you want? (with cancel- and where is the cancel function, with timeout- and what's the timeout value?)

The iris.Context is not a context.Context and they have no share any commons, in gRPC you use that to cancel the request&response or the operation, why you want to pass an iris.Context and not a context.Background() (if you don't really care about cancellation)? You can already pass a context.Context as the first argument, if the controller's method declares it. Please give me a code snippet to understand the reasons behind the usecase so I can provide a more complete solution to your feature request.

Thanks,
Gerasimos Maropoulos

@syklevin
Copy link
Author

Thanks for reply, here a more concrete example:

first of all, we got a grpc services.proto

syntax = "proto3";

package api;

message AcctLoginRequest {
    string username = 1;
    string password = 2;
}

message AcctLoginResponse {
    string sessionId = 1;
}

service AcctService {
    rpc PostLogin(AcctLoginRequest) returns (AcctLoginResponse) {}
}

and here is the main.go, for convenient, I put all the code in one file

package main

import (
	"context"

	"github.com/go-pg/pg/v9"
	uuid "github.com/iris-contrib/go.uuid"
	"github.com/kataras/iris/v12"
	"github.com/kataras/iris/v12/mvc"
	"golang.org/x/crypto/bcrypt"

	"github.com/syklevin/mvctest/api"
)

var (
	//AcctService is default db conn
	// DB *pg.DB = connectDB("postgres://dbuser:dbpass@localhost:5432/dbtest?sslmode=disable", 5)
	DB *pg.DB = connectDB("postgres://dbuser:dbpass@localhost:5432/dbtest?sslmode=disable", 5)

	//AcctService is default service implemented api.AcctServiceServer
	AcctService api.AcctServiceServer = &acctService{}
)

func connectDB(dsn string, pool int) *pg.DB {
	opts, err := pg.ParseURL(dsn)
	if err != nil {
		panic(err)
	}
	opts.PoolSize = pool
	db := pg.Connect(opts)
	return db
}

type acctModel struct {
	ID     int64
	Login  string
	Secret string
}

//acctService implemented grpc generated api.AcctServiceServer
type acctService struct{}

func (s *acctService) PostLogin(ctx context.Context, input *api.AcctLoginRequest) (*api.AcctLoginResponse, error) {

	acct := &acctModel{}
	err := DB.WithContext(ctx).Model(acct).Where("login = ?", input.Username).Select()
	if err != nil {
		return nil, err
	}

	err = bcrypt.CompareHashAndPassword([]byte(acct.Secret), []byte(input.Password))
	if err != nil {
		return nil, err
	}

	sid, err := uuid.NewV4()
	if err != nil {
		return nil, err
	}

	res := &api.AcctLoginResponse{
		SessionId: sid.String(),
	}

	return res, nil
}

type acctServiceWrapper struct{}

func (c *acctServiceWrapper) PostLogin(ctx iris.Context) (*api.AcctLoginResponse, error) {
	input := &api.AcctLoginRequest{}
	err := ctx.ReadJSON(input)
	if err != nil {
		return nil, err
	}
	return AcctService.PostLogin(ctx.Request().Context(), input)
}

func main() {

	app := iris.New()

	//currently works like this
	mvc.New(app.Party("/acct")).Handle(&acctServiceWrapper{})

	//could we serve AcctService directly?
	// mvc.New(app.Party("/acct2")).Handle(AcctService)

	app.Run(iris.Addr(":8080"))
}

It is just works, but required write a controller(aka: wrapper) wrapping each the grpc service funcs.
So my question is could we serve AcctService directly by iris.MVC?

@kataras
Copy link
Owner

kataras commented Feb 12, 2020

OK I see, so the ctx.Request().Context() ( I wasn't sure that's why I asked) is what you want to be binded. Iris is very powerful and you may don't know all of its features but you can already do that in organised and well-written code. Adding a request-scope dependency, you do it one through github.com/kataras/iris/v12/hero.Register or to a single MVC application instance through MVC.Register. No need to touch the framework or wrap your controllers methods:

mvc.New(app).Register(func(ctx iris.Context) context.Context {
		return ctx.Request().Context()
	}).Handle(...yourController)

Full example code:

package main

import (
	"context"

	"github.com/kataras/iris/v12"
	"github.com/kataras/iris/v12/mvc"
)

func main() {
	app := iris.New()
	app.Logger().SetLevel("debug")

	mvc.New(app).
		// Request-scope binding for context.Context-type controller's method or field.
		// (or import github.com/kataras/iris/v12/hero and hero.Register(...))
		Register(func(ctx iris.Context) context.Context {
			return ctx.Request().Context()
		}).
		// Bind loginRequest.
		Register(func(ctx iris.Context) loginRequest {
			var req loginRequest
			ctx.ReadJSON(&req)
			return req
		}).
		Handle(&myController{})

	app.Listen(":8080")
}

type myController struct{}

type loginRequest struct {
	Username string `json:"username"`
}

type loginResponse struct {
	Message string `json:"message"`
}

func (c *myController) PostLogin(ctx context.Context, input loginRequest) (loginResponse, error) {
	if ctx == nil { // ctx == iris.Context.Request().Context()
		panic("expected ctx")
	}

	return loginResponse{
		Message: input.Username + " logged",
	}, nil
}
POST: http://localhost:8080/login

With body:

{
	"username":"syklevin"
}

Expected output:

{
    "message": "syklevin logged"
}

If that suits your needs @syklevin please close this issue, otherwise tell me to add a builtin binding for the context.Context (however, users should be able to bind any context.Context manually, e.g. maybe we want to add a value inside request.Context() before bind it and e.t.c).

Thanks,
Gerasimos Maropoulos

kataras added a commit that referenced this issue Feb 12, 2020
@syklevin
Copy link
Author

OK I see, so the ctx.Request().Context() ( I wasn't sure that's why I asked) is what you want to be binded.

Yes, that's what i want to achieve and thanks for the example.

Beside context.Context, could we have a way to auto bind (without register) the second parameter struct (loginRequest in example),? since in real projects, that would have hundreds' or even thousands struct need to be registered.

@kataras
Copy link
Owner

kataras commented Feb 13, 2020

You are welcome any time! About your second question, yes I suppose we can, with an option like mvc.New(...).RegisterAll(iris.JSONRequests) (and XMLRequests and e.t.c for other types depending on the client's request header Content-Type) ?

@syklevin
Copy link
Author

Sounds good!, that would be very helpful to serve existing grpc service directly.

@syklevin syklevin changed the title [FEATURE REQUEST] MVC support context.Context as parameter [FEATURE REQUEST] MVC serving gRPC-compatible controller Feb 13, 2020
@kataras kataras added this to the v12.1.8 milestone Feb 14, 2020
@kataras
Copy link
Owner

kataras commented Feb 16, 2020

Hello @syklevin I pushed a new release of v12.1.8. Update with go get -u github.com/kataras/iris/v12@latest inside your project's go.mod directory.

This new, minor, release brings support for .Register(mvc.AutoBinding) to map XML, YAML, Query, Form and JSON(default) to the last input argument of a controller's method, pointer of a structure or structure when no other dependency is registered to that type. Example can be found at _examples/mvc/grpc-compatible/main.go.

Example Code (NOTE: OLD CODE, Read below comments for a more up-to-dated and better solution):

 mvc.New(app).
	Register(func(ctx iris.Context) context.Context {
		return ctx.Request().Context()
	}).
	Register(mvc.AutoBinding).
	Handle(&myController{})
import "context"

func (s *acctService) PostLogin(ctx context.Context, input *api.AcctLoginRequest) (*api.AcctLoginResponse, error) {

	acct := &acctModel{}
	err := DB.WithContext(ctx).Model(acct).Where("login = ?", input.Username).Select()
	if err != nil {
		return nil, err
	}

	err = bcrypt.CompareHashAndPassword([]byte(acct.Secret), []byte(input.Password))
	if err != nil {
		return nil, err
	}

	sid, err := uuid.NewV4()
	if err != nil {
		return nil, err
	}

	res := &api.AcctLoginResponse{
		SessionId: sid.String(),
	}

	return res, nil
}

Please test it and post your thoughts.

Thanks,
Gerasimos Maropoulos

@syklevin
Copy link
Author

I have tried it out and it works like a charm! everything work as expected.
@kataras Thank you for this crazy fast support!

@kataras
Copy link
Owner

kataras commented Feb 16, 2020

You are welcome @syklevin as always! Thanks for the feature request, Iris community is grateful!

@WLun001
Copy link

WLun001 commented May 4, 2020

@kataras Does this means write in gRPC and serve it as gRPC and HTTP ? Like gRPC-gateway

@kataras
Copy link
Owner

kataras commented May 4, 2020

@WLun001 At the upcoming Iris release 12.2 that exactly it does, take a look on the fresh example, it contains both http and gRPC clients for testing too: https://github.com/kataras/iris/tree/master/_examples/mvc/grpc-compatible (fetch iris from master and run it)

@WLun001
Copy link

WLun001 commented May 4, 2020

@kataras I should have found out Iris earlier

@kataras
Copy link
Owner

kataras commented May 4, 2020

That means a lot, thanks! I am here to help you migrate from X framework to Iris.

@WLun001
Copy link

WLun001 commented May 4, 2020

@kataras , I am currently using gRPC gateway for my project, does it hard to migration if I wanted to? By the way, does it has to use with MVC package?

mvc.New(app).Handle(ctrl, mvc.GRPC{

How to add multiple services?

Server: grpcServer, // Required.
ServiceName: "helloworld.Greeter", // Required.
Strict: false,

@kataras
Copy link
Owner

kataras commented May 4, 2020

does it has to use with MVC package?

The gRPC in Go requires a struct for its service's methods right?

pb.RegisterGreeterServer(grpcServer, ctrl)

So, It has to live inside mvc because that package already contains the necessary functionality to bind struct's methods for your HTTP service, when gRPC is requested by user, it's delivered by the given grpcServer so you don't lose any performance benefit from using go-grpc.

And yes, you may not use the mvc package for gRPC if your gRPC implementation is a customized one and does not uses struct values, there is a simple gRPC wrapper, but as I said, this won't be useful if your services are structs:

grpcServer := grpc.NewServer()
app := iris.New()
app.WrapRouter(grpcWrapper.New(grpcServer))
// [...]
app.Listen(...)

How to add multiple services?

The ServiceName there is mostly used to resolve the request path for the service. To add multiple services you simply bind a new MVC application, yes you can do it over the same Iris Party. Example:

pb.RegisterGreeterServer(grpcServer, service1)
pb.RegisterOtherServiceServer(grpcServer, service2)

mvc.New(app).Handle(service1, mvc.GRPC{
	Server:      grpcServer,        
	ServiceName: "helloworld.Service1", 
	Strict:      false,
})

mvc.New(app).Handle(service2, mvc.GRPC{
	Server:      grpcServer,        
	ServiceName: "helloworld.Service2", 
	Strict:      false,
})

@WLun001
Copy link

WLun001 commented May 7, 2020

@kataras, does swagger middleware works with gRPC? we just write swagger docs comments on gRPC top of functions

@kataras
Copy link
Owner

kataras commented May 8, 2020

Hello @WLun001, yes of course it does work, example:

// SayHello implements helloworld.GreeterServer.
//
// @Description greet service
// @Accept  json
// @Produce  json
// @Success 200 {string} string	"Hello {name}"
// @Router /helloworld.Greeter/SayHello [post]
func (c *myController) SayHello(ctx context.Context, in *pb.HelloRequest) (*pb.HelloReply, error) {
	return &pb.HelloReply{Message: "Hello " + in.GetName()}, nil
}

image

@kataras kataras modified the milestones: v12.1.8, v12.2.0 May 8, 2020
@WLun001
Copy link

WLun001 commented May 8, 2020

@kataras can't wait for v12.2 release to try out

kataras added a commit that referenced this issue Jul 26, 2020
Former-commit-id: 0cb5121e7d44644f7f0eb34597ff34274157fe95
github-actions bot pushed a commit to goproxies/github.aaakk.us.kg-kataras-iris that referenced this issue Jul 27, 2020
as requested at: kataras#1449


Former-commit-id: a0af1a78bcfef85f297c5087c8cbb00124226036
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

3 participants