search

grpc-ecosystem / grpc-gateway

gRPC to JSON proxy generator following the gRPC HTTP spec
https://grpc-ecosystem.github.io/grpc-gateway/
BSD 3-Clause "New" or "Revised" License
18.16k stars 2.23k forks source link

v2 release planning #1223

Closed johanbrandhorst closed 4 years ago

johanbrandhorst commented 4 years ago

I figured we should have an issue to track the outstanding work for tagging a first stable v2 version of the gateway and switching the default branches. I've looked through the issue tracker and I think these are some of the issues we want to tackle:

I will update this issue as we find more things we want to have in a v2 release. I've also updated the v2 milestone: https://github.com/grpc-ecosystem/grpc-gateway/milestone/3.

johanbrandhorst commented 4 years ago

Another one: rename swagger to openapi everywhere (why not).

johanbrandhorst commented 4 years ago

Minimal public API is in https://github.com/grpc-ecosystem/grpc-gateway/pull/1249

johanbrandhorst commented 4 years ago

Consolidating error handling configuration is here: https://github.com/grpc-ecosystem/grpc-gateway/pull/1265

johanbrandhorst commented 4 years ago

@jhump Note that WithStreamErrorHandler was recently removed in an attempt to simplify the error configuration API. I think this was a contribution of yours, so I'm interested in hearing your use case, so that we can still support that. If we have to, we can add back the StreamErrorHandler, but I'd ideally like it to be as free-form as the unary error handler. What do you think?

jhump commented 4 years ago

Is the unary error handler signature changing, too?

With a server stream, if the failure is mid-stream (e.g. response status and headers already sent), the unary signature does not suffice -- you can't really set an error status or headers at that point. So the error must be translated into some sort of final error message on the stream.

Also, the swagger definition refers to an error structure in its representation of an element in the stream, since it can either have data or an error. And it seems like the stream error handler must produce something with that shape in order for swagger-generated stuff to be able to process the message.

johanbrandhorst commented 4 years ago

Is the unary error handler signature changing, too?

The unary error handler has the same signature, but it crucially gives complete power to the user to specify how to write errors to the response body. It is currently under review: https://github.com/grpc-ecosystem/grpc-gateway/pull/1265.

With a server stream, if the failure is mid-stream (e.g. response status and headers already sent), the unary signature does not suffice -- you can't really set an error status or headers at that point. So the error must be translated into some sort of final error message on the stream.

Also, the swagger definition refers to an error structure in its representation of an element in the stream, since it can either have data or an error. And it seems like the stream error handler must produce something with that shape in order for swagger-generated stuff to be able to process the message.

The issue I had with the existing stream handler was that it did not give enough power to the user to justify having the API (I'd gladly change my mind on this, which is why I'm asking for feedback). It was only letting you change a few variables. Ideally we'd understand the use cases here so we can support that by default, or give the user full control over how to write the error, like we do for the unary error handler (perhaps unify them somehow).

The point that the swagger signature wouldn't match is already true for the unary case, and it's possible to override the return type with swagger annotations and turn off the generation of the default error return in the swagger generator so that they match up in the case of a custom error handler. Ideally we'd be able to support that in the streaming case, too, within the defined stream envelope.

What do you think?

jhump commented 4 years ago

The issue I had with the existing stream handler was that it did not give enough power to the user to justify having the API (I'd gladly change my mind on this, which is why I'm asking for feedback). It was only letting you change a few variables. Ideally we'd understand the use cases here so we can support that by default, or give the user full control over how to write the error, like we do for the unary error handler (perhaps unify them somehow).

This sounds great to me. What I contributed had the shape it did so that it was (a) compatible with the existing APIs in the package and (b) compatible with ancillary things, like the swagger representation.

With brand new APIs, my hope would be that there is a way to customize errors that we can easily write them to the body in the case of a stream error, where a status code and headers have already been sent. Also, it would be nice to be able to emit a swagger spec that accurately describes the error format. So as long as a solution has those properties, I do not have a strong opinion. I am certainly not wed to the way this worked before. I'll admit it was a bit of a hack, but was adequate at the time (and still is today -- we mainly wanted to omit certain properties from the error response and to be able to do other kinds of filtering, and didn't need to make any significant changes to the response format).

Anyhow, I left a couple of comments on the PR you linked above. Hopefully they make sense. And thanks for pinging me to get my input! I do appreciate it.

johanbrandhorst commented 4 years ago

This is probably going to be blocked on https://github.com/googleapis/google-cloud-go/issues/1815 as we will rely on all message types implementing the protoreflect.ProtoMessage interface, and old generated files in genproto do not.

jhump commented 4 years ago

You can convert via protoimpl.X.MessageOf(msgV1). This returns the same things as the ProtoReflect() method would on a newer generated message.

johanbrandhorst commented 4 years ago

I considered it, and even started implementing it, but I think unless it becomes a requirement that we support both new and old it's not something worth adding (unless it becomes clear that it will take a long time for this to happen upstream).

Sidenote; that package explicitly exists outside their compatibility promise: https://github.com/protocolbuffers/protobuf-go/blob/master/runtime/protoimpl/impl.go#L8-L12, did you mean to link https://pkg.go.dev/github.com/golang/protobuf/proto?tab=doc#MessageV2?

jhump commented 4 years ago

did you mean to link https://pkg.go.dev/github.com/golang/protobuf/proto?tab=doc#MessageV2?

Indeed! I was looking for this entry point in the wrong branch, so found the caveat-unsupported one instead of the right one.

srenatus commented 4 years ago

~❓ Can we add something like: you can configure unmarshaling per-method, or per-content-type?~

My needs has been resolved:

Ayush Gupta: @srenatus WithMarshaler is a misnomer, you can define a Unmashalller by embedding the default marshaler in it

johanbrandhorst commented 4 years ago

As discussed in https://github.com/grpc-ecosystem/grpc-gateway/pull/1335, it would be nice to make configuring an unmarshaler easier, but it's not entirely clear to me how to do that.

johanbrandhorst commented 4 years ago

https://github.com/grpc-ecosystem/grpc-gateway/pull/1358 removes all uses of the github.com/golang/protobuf/proto and github.com/golang/protobuf/jsonpb packages.

johanbrandhorst commented 4 years ago

Change to UseProtoNames: false in default marshaler is https://github.com/grpc-ecosystem/grpc-gateway/pull/1376

johanbrandhorst commented 4 years ago

Default values in default marshaler is https://github.com/grpc-ecosystem/grpc-gateway/pull/1377

johanbrandhorst commented 4 years ago

Last match wins behaviour is in https://github.com/grpc-ecosystem/grpc-gateway/pull/1384

johanbrandhorst commented 4 years ago

Removing DisallowUnknownFields is in https://github.com/grpc-ecosystem/grpc-gateway/pull/1386

johanbrandhorst commented 4 years ago

Swagger renaming is in https://github.com/grpc-ecosystem/grpc-gateway/pull/1390

johanbrandhorst commented 4 years ago

This concludes all of the changes we wanted to make for v2, as far as I know.

Because I would like to move the gateway entirely off the old stack, we are now waiting on https://github.com/grpc/grpc-go/pull/3453 and, by extension, an update to rules_go to remove dependencies on old well known type packages. After those features are available, we should be able to consider tagging a stable v2 release.

Goobs commented 4 years ago

Hi, are you going to include #1278 in v2 release? There is a label "V2" on that issue, but it doesn't present in check-list above. Thanks.

johanbrandhorst commented 4 years ago

@Goobs it's something we can add support for after making the first stable release of the gateway, so it's not part of this planning.

johanbrandhorst commented 4 years ago

https://github.com/grpc-ecosystem/grpc-gateway/pull/1419 switches us to protoc-gen-go-grpc

johanbrandhorst commented 4 years ago

We'll have to wait for a stable release of protoc-gen-go-grpc, but we're getting much closer.

slimsag commented 4 years ago

Have you considered marking the v2 branch as the primary branch on GitHub? I almost started work on a large docs PR before I saw this issue :)

Or, if opposed to that, maybe calling it out in the v1 README perhaps?

johanbrandhorst commented 4 years ago

We're going to merge v2 into master once we have a stable release of protoc-gen-go-grpc. Sorry for the confusion!

mvrhov commented 4 years ago

The protoc-gen-go-grpc are saying they still don't have timeline for v1 of their tool does this mean that this has stalled indefinitely? P.S. I find optional annotation very important.

achew22 commented 4 years ago

It simply means we are on the same release schedule as them. You can always depend upon this branch of the code before it is 2.0. We are not going to release our 2.0 unless the API we are upgrading to internally is stable.

johanbrandhorst commented 4 years ago

The protoc-gen-go-grpc are saying they still don't have timeline for v1 of their tool does this mean that this has stalled indefinitely? P.S. I find optional annotation very important.

FYI I think we can expect a 1.0 tag for protoc-gen-go-grpc soonish, AFAIK it was mostly blocked on https://github.com/grpc/grpc-go/issues/3669, which now has a candidate implementation.

mvrhov commented 4 years ago

Can you then implement the support for new experimental optional parameter, if there is anything to implement... The generators mostly just had to say that they support it... Thanks

achew22 commented 4 years ago

@mvrhov, the pull request button is at the top of the page. I'm really looking forward to engaging more in the code you submit to enhance the project.

Thanks!

johanbrandhorst commented 4 years ago

https://github.com/grpc/grpc-go/releases/tag/cmd/protoc-gen-go-grpc/v1.0.0 has just been tagged so potentially that's the last hurdle for us tagging v2. My plan for the release is as follows:

  1. Pin to protoc-gen-go-grpc 1.0.0 in v2 and make sure everything still works.
  2. Create a final v1 release before creating the v1 release branch
  3. Create v1 release branch
  4. Merge v2 into master. This PR will also have to update all the v2 specific branch links.
  5. Create first stable v2 release from master.

Any thoughts on this plan @achew22 @tmc?

achew22 commented 4 years ago

I think you've got it right. Thanks Johan

ivucica commented 4 years ago

Merge v2 into master

@johanbrandhorst This may break GOPATH users like me. (Obligatory xkcd)

Would just making v2 branch the new 'default branch' work better?

johanbrandhorst commented 4 years ago

Merge v2 into master

@johanbrandhorst This may break GOPATH users like me. (Obligatory xkcd)

Would just making v2 branch the new 'default branch' work better?

To answer this question in public; this isn't a realistic option for us, unfortunately. So yes, the v2 release will break GOPATH users. Please migrate to modules or stay on v1.

johanbrandhorst commented 4 years ago

I have tagged v1.15.1 on master and v1.15.2 on the new v1 branch.