Implement Nexus Serializer for Temporal Payloads #5126

bergundy · 2023-11-16T06:57:33Z

What changed?

See description

Why?

The server should do best effort conversion between Nexus Content and Temporal Payloads so pure Temporal applications don't have to deal with the separate Nexus Content concept.

How did you test it?

Added tests.

Potential risks

Non-standard content is converter into a Payload with metadata encoding of unknown/nexus-content. Users will need to prepare for that or their Nexus requests may fail.
I believe this is the best option we have to avoid lossy conversion and expose as much information and empower the application.
I considered using binary/plain for non-standard content but I'd rather not have these payloads accidentally slip into user's applications.

Is hotfix candidate?

Far from it.

common/nexus/payload_serializer.go

tdeebswihart · 2023-11-16T18:00:28Z

common/nexus/payload_serializer.go

+	if !isStandardPayload(payload) {
+		data, err := payload.Marshal()
+		if err != nil {
+			return nil, fmt.Errorf("%w: payload marshal error: %w", errSerializer, err)


You may as well just have errSerializer be a string prefix; it doesn't look like we gain anything by wrapping it into the error tree

Mostly did this to satisfy our linter.

Would avoid a double %w, there's only one true cause. In fact, would avoid errSerializer altogether, it has no value from what I see (but I understand server has a bit of a history with hard to read code by precreating errors often unnecessarily regardless of some opinions about performance or attempting to predict future uses)

Again, I could make a linter exception here. I was just "following the rules".

common/nexus/payload_serializer.go

common/nexus/payload_serializer_test.go

bergundy · 2023-11-16T22:04:27Z

common/nexus/payload_serializer.go

+	commonpb "go.temporal.io/api/common/v1"
+)
+
+type nexusPayloadSerializer struct{}


I'm going to rename this to payloadSerializer and avoid the stuttering.

common/nexus/payload_serializer.go

cretz · 2023-11-17T14:39:14Z

common/nexus/payload_serializer.go

+	commonpb "go.temporal.io/api/common/v1"
+)
+
+type payloadSerializer struct{}


What is the purpose of this type? Not that it's wrong, but you check that it implements an interface and everything but nothing uses it and it's not exported

I still don't know if I want to export it but I'm going to use it in a bit when I implement the Nexus Handler in this code base.

Would be willing to wait until then to review since how it is used affects how it is written (or at least set a reminder to go review this already-merged piece if it does get merged before the usage of it is done)

cretz · 2023-11-17T14:44:06Z

common/nexus/payload_serializer.go

+		if params["format"] == "protobuf" {
+			payload.Metadata["encoding"] = []byte("json/protobuf")
+			messageType := params["message-type"]
+			if messageType != "" {


I don't think we should allow proto without message type. It's not very useful over normal JSON and message-type-less protos in Temporal are only supported on older SDKs. Arguably you don't need format and message-type as separate params, just a single protobuf-message-type param is good enough to be seen as proto JSON.

I'm fine considering protos without message-type non-standard.
I'll also consider merging into a single param.

Yeah, more and more the single param sounds good.

cretz · 2023-11-17T14:46:01Z

common/nexus/payload_serializer.go

+	case "application/x-protobuf":
+		payload.Metadata["encoding"] = []byte("binary/protobuf")
+		messageType := params["message-type"]
+		if messageType != "" {


Same here, we should not allow message-type-less proto binary

common/nexus/payload_serializer.go

cretz · 2023-11-17T14:53:18Z

common/nexus/payload_serializer.go

+	}
+	contentType := h.Get("type")
+	if contentType == "" {
+		return len(h) == 0 && len(content.Data) == 0


So it was confusing to follow this logic down to here, but I think an absent content type with data should be a binary/plain.

I considered that but it would make this conversion asymmetric. Because we can't interpret the intent I think it may be better that this becomes encoding: unknown/nexus-content so then if it's turned into a content object again it doesn't get an application/octet-stream content type.

An absent content type + data is always asymmetric because it's more loosely defined inbound but we want to explicitly set content type outbound when data is present. So discussion of absent content-type + data only applies in one direction.

cretz · 2023-11-17T14:55:43Z

common/nexus/payload_serializer.go

+	if !isStandardPayload(payload) {
+		data, err := payload.Marshal()
+		if err != nil {
+			return nil, fmt.Errorf("%w: payload marshal error: %w", errSerializer, err)


Would avoid a double %w, there's only one true cause. In fact, would avoid errSerializer altogether, it has no value from what I see (but I understand server has a bit of a history with hard to read code by precreating errors often unnecessarily regardless of some opinions about performance or attempting to predict future uses)

cretz · 2023-11-17T15:00:19Z

common/nexus/payload_serializer.go

+		}
+	case "json/protobuf":
+		content.Header["type"] = fmt.Sprintf("application/json; format=protobuf")
+		if messageType != "" {


I don't think any SDKs leave this off anymore and haven't for a while, I think we can require usage of a somewhat modern SDK version for use with Nexus protos

I'm fine with that.

cretz · 2023-11-17T15:03:11Z

common/nexus/payload_serializer.go

+		return &nexus.Content{Header: nexus.Header{}}, nil
+	}
+
+	if !isStandardPayload(payload) {


This is confusing too, can you just make this stuff part of the switch below? You can set data in each arm of the switch, there's not that much code reuse here. Just handle each type specifically. I can suggest exact rewrite if it helps.

cretz · 2023-11-17T15:05:06Z

(I don't usually do reviews in this repository but I was asked in this case)

bergundy

Thanks for the reviews.

bergundy · 2023-11-17T16:49:42Z

common/nexus/payload_serializer.go

+	commonpb "go.temporal.io/api/common/v1"
+)
+
+type payloadSerializer struct{}


I still don't know if I want to export it but I'm going to use it in a bit when I implement the Nexus Handler in this code base.

common/nexus/payload_serializer.go

bergundy · 2023-11-17T16:54:44Z

common/nexus/payload_serializer.go

+		if params["format"] == "protobuf" {
+			payload.Metadata["encoding"] = []byte("json/protobuf")
+			messageType := params["message-type"]
+			if messageType != "" {


I'm fine considering protos without message-type non-standard.
I'll also consider merging into a single param.

bergundy · 2023-11-17T16:55:01Z

common/nexus/payload_serializer.go

+}
+
+func isStandardNexusContent(content *nexus.Content) bool {
+	h := content.Header


This was supposed to clone the map, I missed that.

bergundy · 2023-11-17T16:58:58Z

common/nexus/payload_serializer.go

+	}
+	contentType := h.Get("type")
+	if contentType == "" {
+		return len(h) == 0 && len(content.Data) == 0


I considered that but it would make this conversion asymmetric. Because we can't interpret the intent I think it may be better that this becomes encoding: unknown/nexus-content so then if it's turned into a content object again it doesn't get an application/octet-stream content type.

bergundy · 2023-11-17T17:00:09Z

common/nexus/payload_serializer.go

+	if !isStandardPayload(payload) {
+		data, err := payload.Marshal()
+		if err != nil {
+			return nil, fmt.Errorf("%w: payload marshal error: %w", errSerializer, err)


Again, I could make a linter exception here. I was just "following the rules".

bergundy · 2023-11-17T17:00:24Z

common/nexus/payload_serializer.go

+		}
+	case "json/protobuf":
+		content.Header["type"] = fmt.Sprintf("application/json; format=protobuf")
+		if messageType != "" {


I'm fine with that.

bergundy · 2023-11-17T18:00:58Z

Merged the detection and serialization logic and address the rest of the comments.
PTAL.

cretz · 2023-11-17T22:19:46Z

common/nexus/payload_serializer.go

+	// We assume that encoding is handled by the transport layer and the content is decoded.
+	delete(h, "encoding")
+	// Length can safely be ignored.
+	delete(h, "length")


Why remove these here when you can just filter and not set them in the one place you actually copy headers, the setUnknownNexusContent call?

This is mostly to simplify the logic below and sanitize any irrelevant fields.

cretz · 2023-11-17T22:31:01Z

common/nexus/payload_serializer.go

+	// Length can safely be ignored.
+	delete(h, "length")
+
+	if len(h) > 1 {


Why do you care what the rest of the headers are so long as the content type is correct? I think you're overthinking here and can just remove this conditional

I don't want to lose information in the conversion

cretz · 2023-11-17T22:35:02Z

common/nexus/payload_serializer.go

+		if len(h) == 0 && len(content.Data) == 0 {
+			payload.Metadata["encoding"] = []byte("binary/null")
+		} else {
+			setUnknownNexusContent(h, payload.Metadata)


I still think this should be binary/plain

This could also be content header that's not type, and I'd rather not lose this information.

cretz · 2023-11-17T22:57:59Z

common/nexus/payload_serializer.go

+var errSerializer = errors.New("serializer error")
+
+// Deserialize implements nexus.Serializer.
+func (payloadSerializer) Deserialize(content *nexus.Content, v any) error {


To me this reads clearer as a future maintainer, but don't have to accept it (just typed real quick, so probably some typos):

// Deserialize implements nexus.Serializer. func (payloadSerializer) Deserialize(content *nexus.Content, v any) error { payloadRef, ok := v.(**commonpb.Payload) if !ok { return fmt.Errorf("cannot deserialize into %v", v) } payload := &commonpb.Payload{Metadata: map[string][]byte{}, Data: content.Data} *payloadRef = payload // Absent content type with no data means null, with data is just binary if contentType := content.Header["type"]; contentType == "" { if len(content.Data) == 0 { payload.Metadata["encoding"] = []byte("binary/null") } else { payload.Metadata["encoding"] = []byte("binary/plain") } return nil } // Different content types make different payloads if mediaType, params, err := mime.ParseMediaType(contentType); err == nil { switch mediaType { case "application/x-temporal-payload": if err := payload.Unmarshal(content.Data); err != nil { return fmt.Errorf("failed to unmarshal payload: %w", err) } return nil case "application/json": if protoMessageType := params["protobuf-message-type"]; protoMessageType != "" { payload.Metadata["encoding"] = []byte("json/protobuf") payload.Metadata["messageType"] = []byte(protoMessageType) } else { payload.Metadata["encoding"] = []byte("json/plain") } return nil case "application/x-protobuf": if protoMessageType := params["protobuf-message-type"]; protoMessageType != "" { payload.Metadata["encoding"] = []byte("binary/protobuf") payload.Metadata["messageType"] = []byte(protoMessageType) return nil } return fmt.Errorf("protobuf message type required for application/x-protobuf content type") case "application/octet-stream": payload.Metadata["encoding"] = []byte("binary/plain") return nil } } // Unknown or bad content type payload.Metadata["encoding"] = "binary/unknown-nexus" for k, v := range content.Header { // Don't want length or encoding if k != "length" && k != "encoding" { payload.Metadata[k] = []byte(v) } } return nil }

I prefer my stricter non-lossy version.

Thanks for the suggestion though.

common/nexus/payload_serializer.go

cretz · 2023-11-17T22:59:35Z

common/nexus/payload_serializer.go

+			}
+		}
+	case "json/protobuf":
+		if len(payload.Metadata) != 2 || messageType == "" {


I think, like with the code in deserialize, you're being overly strict on the metadata. If it's our encoding, we can choose to ignore other metadata

I don't think you're right here.
If a user customizes serialization to add more information, we should treat their payload as opaque and non standard and avoid losing metadata during the conversion.

cretz · 2023-11-17T23:00:06Z

common/nexus/payload_serializer.go

+	case "application/json":
+		if len(params) == 0 {
+			payload.Metadata["encoding"] = []byte("json/plain")
+		} else if len(params) == 2 && params["format"] == "protobuf" && params["message-type"] != "" {


I think you're being overly strict on the mime type params. If it's our content type, we can ignore params we don't care about

again, I'd rather not lose this information. Either you're sticking to the standard or you're a custom nexus payload.

bergundy · 2023-11-20T17:22:04Z

@cretz I thought about it some more and it may be desirable to be a bit less strict with the conversion but I would like to do that in a way that no information is lost.

In any case, I'll get back to this at some point before merging nexus into main but I'd rather make some more progress before that.

Implement Nexus Serializer for Temporal Payloads

0a9b206

bergundy requested a review from a team as a code owner November 16, 2023 06:57

tdeebswihart reviewed Nov 16, 2023

View reviewed changes

bergundy commented Nov 16, 2023

View reviewed changes

Address review comments

3c25687

cretz reviewed Nov 17, 2023

View reviewed changes

bergundy commented Nov 17, 2023

View reviewed changes

bergundy added 2 commits November 17, 2023 09:40

Don't mutate, message type mandatory

426b46f

Merge serialization and standard detection

435bc4a

cretz approved these changes Nov 17, 2023

View reviewed changes

nil payload content header is nil

8923228

bergundy merged commit 4bd580f into temporalio:nexus Nov 17, 2023
8 checks passed

bergundy deleted the nexus-payload-serializer branch November 17, 2023 23:57

Implement Nexus Serializer for Temporal Payloads #5126

Implement Nexus Serializer for Temporal Payloads #5126

Conversation

bergundy commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz commented Nov 17, 2023

bergundy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy commented Nov 20, 2023

cretz Nov 17, 2023 •

edited

Loading