Support for RequestIdReference in Nexus links #1876
Conversation
| @@ -47,6 +47,7 @@ const ( | |||
| linkWorkflowEventReferenceTypeKey = "referenceType" | |||
| linkEventReferenceEventIDKey = "eventID" | |||
| linkEventReferenceEventTypeKey = "eventType" | |||
| linkEventReferenceRequestIDKey = "requestID" | |||
There was a problem hiding this comment.
Ug, I wish we hadn't capitalized ID for query parameters, but too late now
| @@ -137,6 +141,11 @@ func ConvertNexusLinkToLinkWorkflowEvent(link nexus.Link) (*commonpb.Link_Workfl | |||
| we.Reference = &commonpb.Link_WorkflowEvent_EventRef{ | |||
| EventRef: eventRef, | |||
| } | |||
| case requestIDReferenceType: | |||
| requestIDRef := convertURLQueryToLinkWorkflowEventRequestIdReference(link.URL.Query()) | |||
| we.Reference = &commonpb.Link_WorkflowEvent_RequestIdRef{ | |||
There was a problem hiding this comment.
What happens on an older/current server if this link reference is set on StartWorkflowExecutionRequest.links? Meaning before any server change, what would happen today if this was present? Would it fail? Just curious how strict we made link validation.
EDIT: Looking at tests, we have a problem here:
time=2025-03-19T05:51:58.787 level=ERROR msg="failed to parse link to "temporal.api.common.v1.Link.WorkflowEvent": temporal:///namespaces/integration-test-namespace/workflows/4236f93f-390a-4afe-b9a6-592ec5883b09/0195acf5-673f-7b1f-94a8-1f386af0c47d/history?referenceType=RequestIdReference&requestID=4236f93f-390a-4afe-b9a6-592ec5883b09" error="failed to parse link to Link_WorkflowEvent: unknown reference type: "RequestIdReference""
We can't break people using older servers. So somehow this concept of a request ID reference is going to need to be opt-in or otherwise make sure that it is supported before attempting to use.
There was a problem hiding this comment.
So, the error is logged, but the operation does not fail. The server fails to parse the link, so it won't be added to the history event.
There was a problem hiding this comment.
Alternatively, I can add a flag to server capabilities.
There was a problem hiding this comment.
hm so that means if a user is using a new SDK that sends this event, on Server v1.27.1 the link won't work? If something is GA, which I think this is, we don't break compatibility with old servers.
There was a problem hiding this comment.
Good to know this is a log statement and not an error sent back to clients. I do think it's a gray area on whether this is "compatibility" or not. Silently dropping a link we never had before when using older servers is probably fine (well, it's not "silent" since it logs server side, but it has no SDK impact). Am I right in assuming we never had this link before?
There was a problem hiding this comment.
Not this type of link. But I'm replacing the Nexus link from EventReference to this new RequestIdReference.
There was a problem hiding this comment.
The impact would be the links from the caller -> handler would not be written on Server v1.27.x correct? And that is visible to users through the UI linking feature?
There was a problem hiding this comment.
If upgrading the SDK but not the server causes a negative effect for users, we need some kind of compatibility field. We do not want to alter the get system info one anymore, but maybe the nexus task response can let the implementation know that the server supports this type of link.
temporalnexus/link_converter_test.go
Outdated
There was a problem hiding this comment.
I think we need an integration test confirming this works end to end
There was a problem hiding this comment.
There is already an end to end test, I just fixed it.
| @@ -39,7 +39,7 @@ require ( | |||
| github.com/secure-systems-lab/go-securesystemslib v0.7.0 // indirect | |||
| github.com/stretchr/objx v0.5.2 // indirect | |||
| github.com/tinylib/msgp v1.1.8 // indirect | |||
| go.temporal.io/api v1.44.1 // indirect | |||
| go.temporal.io/api v1.45.1-0.20250319003343-cdea10847991 // indirect | |||
There was a problem hiding this comment.
Can't merge until this is a stable tag and there is an integration test confirming this behavior even works
rodrigozhou
force-pushed
the
rodrigozhou/nexus-links-request-id-ref
branch
from
d4d34d0 to
822699a
Compare
rodrigozhou
changed the base branch from
master
to
rodrigozhou/link-request-id-ref
|
@cretz I split the PR in two
|
rodrigozhou
force-pushed
the
rodrigozhou/link-request-id-ref
branch
from
9e0ee49 to
e42403c
Compare
What was changed
Support for RequestIdReference in Nexus links.
Change
WorkflowRunOperationto always return a link with request ID reference instead of event reference.Why?
Checklist
Closes
How was this tested: