v2 release planning

[johanbrandhorst]

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:

• Issues mentioned in Massive breaking changes in preparation for 2.0 #546, including:
  • Emitting defaults (Support emitting default values in JSON #233)
  • Using jsonpb marshaller by default
  • Changing the output of errors to match the JSON representation of a gRPC status (Remove "error" field from errors. #1098).
  • Adopt correct behaviour for custom verbs (see 404s using colons in the middle of the last path segment #224 (comment)).
  • Change default JSON marshalling to use camelCase (Change swagger generator to produce lowerCamelCase field names, change default marshaler to do the same, and also emit default values #540) (and update swagger: Swagger: JSON definitions aren't CamelCased #375)
• Switch to using the new Go protobuf stack (depends on cmd/protoc-gen-go-grpc: add code generator grpc/grpc-go#3453) (Upgrade protobuf to 1.4.0 #1165 is the first part of this). Feature request: support of go protocol buffer v2 #1147 Pin protoc-gen-go-grpc to v1.0.0 #1719
• Use rules_go version which support APIv2 well known types (Inconsistent APIv2 generated protobuf well known types behaviour bazel-contrib/rules_go#2514)
• Minimize public API per Minimize API surface in anticipation of gorelease #1146
• Rename swagger to openapi (cleanup: rename 'swagger' references to 'openapi' #675)
• Consolidate error handler configuration.
• Make HTTPBodyMarshaler behaviour the default
• Remove runtime.DisallowUnknownFields

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]

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

[johanbrandhorst]

Minimal public API is in #1249

[johanbrandhorst]

Consolidating error handling configuration is here: #1265

[johanbrandhorst]

@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]

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]

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: #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]

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]

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

[jhump]

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

[johanbrandhorst]

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]

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]

❓ 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]

As discussed in #1335, it would be nice to make configuring an unmarshaler easier, but it's not entirely clear to me how to do that.

[johanbrandhorst]

#1358 removes all uses of the github.com/golang/protobuf/proto and github.com/golang/protobuf/jsonpb packages.

[johanbrandhorst]

Change to UseProtoNames: false in default marshaler is #1376

[johanbrandhorst]

Default values in default marshaler is #1377

[johanbrandhorst]

Last match wins behaviour is in #1384

[johanbrandhorst]

Removing DisallowUnknownFields is in #1386

[johanbrandhorst]

Swagger renaming is in #1390

[johanbrandhorst]

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 grpc/grpc-go#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]

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]

@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]

#1419 switches us to protoc-gen-go-grpc

[johanbrandhorst]

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

[slimsag]

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]

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

[mvrhov]

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]

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]

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 grpc/grpc-go#3669, which now has a candidate implementation.

[mvrhov]

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]

@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]

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]

I think you've got it right. Thanks Johan

[ivucica]

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]

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]

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