HTTP API helpers #112

cretz · 2023-06-26T16:20:00Z

What changed?

Implements helpers for HTTP API as proposed at temporalio/proposals#79, primarily around payload formatting. Specifically:

Added go.temporal.io/api/internal/temporaljsonpb package
- Added JSONPBMaybeMarshaler and JSONPBMaybeUnmarshaler interfaces which protos can implement for custom, optional JSON support
- A copy of https://github.com/gogo/protobuf/blob/v1.3.2/jsonpb/jsonpb.go but with support for those interfaces
Added go.temporal.io/api/internal/temporalgateway package
- Implements gogo-supported equivalent of https://pkg.go.dev/github.com/grpc-ecosystem/grpc-gateway/runtime#JSONPb akin to https://github.com/gogo/gateway but with JSONPBMaybeMarshaler/JSONPBMaybeUnmarshaler support
Added go.temporal.io/api/internal/temporalcommonv1 package
- Implements JSONPBMaybeMarshaler and JSONPBMaybeUnmarshaler for Payloads and Payload to support shorthand JSON representation
- Makefile updated to copy code written here to go.temporal.io/api/common/v1 where it lives alongside generated code (instead of trying to write code directly alongside generated code)
Updated go.temporal.io/api/proxy
- Added JSONPBMarshaler/NewJSONPBMarshaler and JSONPBUnmarshaler/NewJSONPBUnmarshaler to create marshalers to support maybe-JSON interfaces (so for Payload shorthand)
- Added NewGRPCGatewayJSONPBMarshaler to create a https://pkg.go.dev/github.com/grpc-ecosystem/grpc-gateway/runtime#Marshaler from the above marshaler/unmarshaler
Added tests for shorthand payloads and gRPC gateway
Updated Makefile to support building with gRPC gateway + Google HTTP annotations

This is ready for review. This will not have the generated code updates until the API PR is merged. Related PRs:

Depends on HTTP API annotations api#301
Depended on by HTTP API support temporal#4543

cretz · 2023-06-26T16:42:18Z

Makefile


 fix-path:
 	mv -f $(PROTO_OUT)/temporal/api/* $(PROTO_OUT) && rm -rf $(PROTO_OUT)/temporal
+	# Also copy the payload JSON helper
+	cp $(PROTO_OUT)/internal/temporalcommonv1/payload_json.go $(PROTO_OUT)/common/v1/


So I have to add methods to the generated Payload/Payloads structs. I could have written it in that directory and did a bunch of changes to the delete-all-regen logic in the Makefile, but instead I just wrote the implementation outside the directory and I just copy it on code gen.

cretz · 2023-06-26T16:48:16Z

proxy/gateway_test.go

+		t.grpcServer.Stop()
+		return nil, err
+	}
+	if err = workflowservice.RegisterWorkflowServiceHandlerServer(ctx, serveMux, t); err != nil {


I will push the generated gateway code when temporalio/api#301 is reviewed

proxy/marshal_test.go

MichaelSnowden

If this depends on open PRs, please make a stacked PR to make review easier (e.g. push all the dependencies to some arbitrary branch and then set that as the base here).

cretz · 2023-06-30T18:11:02Z

If this depends on open PRs, please make a stacked PR to make review easier (e.g. push all the dependencies to some arbitrary branch and then set that as the base here).

It's more than that. It depends on a submodule from another repo being updated and a Makefile rerun (or rather a GitHub workflow triggered manually probably). You cannot just merge the other and then this magically is ok to merge. The PRs are in different repos and are inherently different things to review. Not sure how these different-repo PRs can be made any easier to review.

# Conflicts: # go.mod # go.sum

Quinn-With-Two-Ns · 2023-08-02T21:34:15Z

internal/temporalcommonv1/message.go

+
+// These are dummy messages, never used
+
+type Payloads struct {


Dumb question, why do we need these?

Oh I guess because the "real" types on exist in the destination folder your copying into

Exactly. I struggled with "write code over there and make the special file excluded from the wipe-and-regen process", or "write over here and copy". I chose the latter. There are pros/cons to both approaches.

Quinn-With-Two-Ns

I'm good with the approach taken to write over and copy. It's hard to verify all the marshalling code is 100% correct, but I don't see anything obviously wrong

alexshtin · 2023-08-03T19:57:20Z

internal/temporalgateway/marshal_jsonpb.go

+
+// JSONPb is a Marshaler which marshals/unmarshals into/from JSON
+// with the "github.com/golang/protobuf/jsonpb".
+// It supports fully functionality of protobuf unlike JSONBuiltin.


Did you mean?

Suggested change

// It supports fully functionality of protobuf unlike JSONBuiltin.

// It supports fully functionality of protobuf unlike built in JSON.

From the top of this file:

// This file taken from // https://github.com/grpc-ecosystem/grpc-gateway/blob/v1.16.0/runtime/marshal_jsonpb.go // and altered to support Temporal JSONPB implementation. Specifically, the // JSONPb struct was altered to accept an unmarshaler also and to change the // marshaler to Temporal's.

This comment is in the source this came from. I would write code way better :-)

alexshtin · 2023-08-03T21:08:52Z

common/v1/payload_json.go

+	// the outer unmarshaler allows them.
+	if gogojsonpb.Unmarshal(bytes.NewReader(b), p) == nil && len(p.Metadata) > 0 {
+		// A raw payload must either have data or a binary/null encoding
+		if len(p.Data) > 0 || string(p.Metadata["encoding"]) == "binary/null" {


I am not an apologet of "every hardcoded string must be a const" but if you reference same string more than once, I would like to see them on top of this file in the const block (I am talking about "encoding" and "binary/null"). Similar to DisablePayloadShorthandMetadataKey but these will be really public (exported). Similar consts in Go SDK could just alias them.

On other hand, this is useful when one wants to keep ability to change them later (and change in one place only), and these consts are very well hardcoded (and immutable) not only in this repo but across many repos in entire org.

if you reference same string more than once, I would like to see them on top of this file in the const block

In this case, these are basically company-wide constants have have no real home. I can move them to private constants in this file, but it makes little sense to me as they are used by two private methods colocated together.

Similar to DisablePayloadShorthandMetadataKey but these will be really public (exported).

But that is exported for a reason (it is needed by the server repo), there is no reason to export the other ones (user already has them from a better place). I only use these literals in two adjacent private methods and never would a user use them (they're in the Go SDK). I don't think we need to export more consts out of this mostly-generated package.

Similar consts in Go SDK could just alias them.

Aliasing was maybe not the best decision. But assuming you mean just have the Go SDK reference a new set of consts here, what benefit is there? If it's just to satisfy the idea that they're shared string literals (even though there would not be two public API places for them), I don't think that has enough benefits practically.

these consts are very well hardcoded (and immutable) not only in this repo but across many repos in entire org.

Exactly. They might as well be part of our proto definitions, but since they aren't we have string literals in the projects that reference them. It is unfortunate that api-go even exists (the SDK and server should be able to own their own proto generation) and luckily other languages aren't burdened with that. If it was reasonable I would not put this logic here but instead on the server where I need this, but since this is our "shared Go place for payloads", we need the specialized JSON conversion here I think.

alexshtin · 2023-08-03T21:13:41Z

internal/temporalgateway/marshal_jsonpb.go

+//
+// The NewDecoder method returns a DecoderWrapper, so the underlying
+// *json.Decoder methods can be used.
+type JSONPb struct {


I think if it is JSON (all caps) then it should be PB not Pb.

This is not my code. See comment near top of file.

# Conflicts: # cmd/proxygenerator/go.mod # cmd/proxygenerator/go.sum # go.mod # go.sum # sdk/v1/task_complete_metadata.pb.go

HTTP API support

a163d73

cretz changed the title ~~HTTP API helpers~~ HTTP API helpers (early preview) Jun 26, 2023

This was referenced Jun 26, 2023

HTTP API support temporalio/temporal#4543

Merged

HTTP API annotations temporalio/api#301

Merged

cretz commented Jun 26, 2023

View reviewed changes

proxy/marshal_test.go Show resolved Hide resolved

cretz changed the title ~~HTTP API helpers (early preview)~~ HTTP API helpers Jun 30, 2023

cretz marked this pull request as ready for review June 30, 2023 15:47

cretz requested review from a team as code owners June 30, 2023 15:47

MichaelSnowden reviewed Jun 30, 2023

View reviewed changes

Merge remote-tracking branch 'remotes/origin/master' into http-api

bac3aeb

# Conflicts: # go.mod # go.sum

Quinn-With-Two-Ns reviewed Aug 2, 2023

View reviewed changes

Quinn-With-Two-Ns approved these changes Aug 2, 2023

View reviewed changes

feedmeapples approved these changes Aug 2, 2023

View reviewed changes

alexshtin approved these changes Aug 3, 2023

View reviewed changes

cretz added 2 commits August 18, 2023 11:03

Update with new API protos

f246aeb

Merge remote-tracking branch 'remotes/origin/master' into http-api

f17d49d

# Conflicts: # cmd/proxygenerator/go.mod # cmd/proxygenerator/go.sum # go.mod # go.sum # sdk/v1/task_complete_metadata.pb.go

cretz merged commit 75bc8f1 into temporalio:master Aug 18, 2023
3 checks passed

cretz deleted the http-api branch August 18, 2023 16:26

sync-by-unito bot mentioned this pull request Aug 24, 2023

Bump go.temporal.io/api from 1.23.0 to 1.24.0 temporalio/cli#313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP API helpers #112

HTTP API helpers #112

cretz commented Jun 26, 2023 •

edited by alexshtin

Loading

cretz Jun 26, 2023 •

edited

Loading

cretz Jun 26, 2023

MichaelSnowden left a comment

cretz commented Jun 30, 2023

Quinn-With-Two-Ns Aug 2, 2023

Quinn-With-Two-Ns Aug 2, 2023

cretz Aug 2, 2023

Quinn-With-Two-Ns left a comment

alexshtin Aug 3, 2023

cretz Aug 3, 2023 •

edited

Loading

alexshtin Aug 3, 2023

cretz Aug 3, 2023 •

edited

Loading

alexshtin Aug 3, 2023

cretz Aug 3, 2023


		// These are dummy messages, never used

		type Payloads struct {

	// It supports fully functionality of protobuf unlike JSONBuiltin.
	// It supports fully functionality of protobuf unlike built in JSON.

HTTP API helpers #112

HTTP API helpers #112

Conversation

cretz commented Jun 26, 2023 • edited by alexshtin Loading

cretz Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSnowden left a comment

Choose a reason for hiding this comment

cretz commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz commented Jun 26, 2023 •

edited by alexshtin

Loading

cretz Jun 26, 2023 •

edited

Loading

cretz Aug 3, 2023 •

edited

Loading

cretz Aug 3, 2023 •

edited

Loading