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

Nexus: Better handling of nil and empty payloads #6616

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

bergundy
Copy link
Member

@bergundy bergundy commented Oct 7, 2024

What changed?

Improve conversion of payloads with empty metadata and nil workflow results to Nexus Content.

NOTE: This also upgrades the Go SDK from 1.27.0 to 1.29.1 to test Nexus via the SDK.

Why?

Handle a couple of edge cases that weren't properly handled before.
The fix in mutable_state_impl.go is the more critical one and may cause surprising behavior in the SDKs.

How did you test it?

Added tests.

@bergundy bergundy requested a review from a team as a code owner October 7, 2024 18:58
@bergundy bergundy force-pushed the nexus-nil-and-empty-result-fix branch from 9480f68 to 3f7a4fb Compare October 7, 2024 20:19
common/nexus/payload_serializer.go Show resolved Hide resolved
ev, err := history.Next()
s.NoError(err)
if attr := ev.GetNexusOperationCompletedEventAttributes(); attr != nil {
protorequire.ProtoEqual(s.T(), s.mustToPayload(nil), attr.GetResult())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s.EqualExportedValues() does the same without custom code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this works for protos. protorequire was developed specifically for proto comparison when we transitioned from gogo to google proto.

@bergundy bergundy merged commit abf9dc4 into temporalio:main Oct 7, 2024
45 of 46 checks passed
@bergundy bergundy deleted the nexus-nil-and-empty-result-fix branch October 7, 2024 22:23
xwduan pushed a commit that referenced this pull request Oct 18, 2024
## What changed?

Improve conversion of payloads with empty metadata and nil workflow
results to Nexus Content.

NOTE: This also upgrades the Go SDK from 1.27.0 to 1.29.1 to test Nexus
via the SDK.

## Why?

Handle a couple of edge cases that weren't properly handled before.
The fix in `mutable_state_impl.go` is the more critical one and may
cause surprising behavior in the SDKs.

## How did you test it?

Added tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants