Skip to content

Adding SDK Ergonomics Query link - #2988

Open
Evanthx wants to merge 2 commits into
mainfrom
nexus-query
Open

Adding SDK Ergonomics Query link#2988
Evanthx wants to merge 2 commits into
mainfrom
nexus-query

Conversation

@Evanthx

@Evanthx Evanthx commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

The one thing to note is that there was some duplication around link converters and still is. I would like to fix that separately, and have created an epic to check over the link converters for each SDK which I am planning on doing in the next sprint.
https://temporalio.atlassian.net/browse/NEXUS-578

The proto bump brings in the link field.

@Evanthx
Evanthx requested a review from a team as a code owner August 3, 2026 19:23
@Quinn-With-Two-Ns

Copy link
Copy Markdown
Contributor

duplication around link converters and still is

What duplication? I am not aware of any except for what we had in the Go SDK/Server

.setNexusServiceImplementation(new QueryingNexusServiceImpl())
// The workflow being queried parks on a signal, so time skipping would fast-forward it
// into its execution timeout and it would be gone before the Query lands.
.setUseTimeskipping(false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You'll need an assume in this class too, to stop these test from running against the test server in CI

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

* a query handler that throws, and a Query rejected by the client's reject condition. All of these
* must fail the caller's Nexus operation rather than hanging or returning a default.
*
* <p>The response link the server attaches to {@code QueryWorkflowResponse} is verified in {@link

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test should assert on the link , we will need to publish a real server before we merge this anyway

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True. Test added

@Quinn-With-Two-Ns

Copy link
Copy Markdown
Contributor

Other then the tests the actual SDK changes LGTM

@Quinn-With-Two-Ns

Copy link
Copy Markdown
Contributor

FWIW I also ran a Codex review and it only had one finding

  - Medium — nexusLinkToWorkflowLink (https://github.com/temporalio/sdk-java/blob/accc671abce2496605da46ec28c1966613e8b220/temporal-sdk/src/main/java/io/temporal/internal/common/LinkConverter.java#L232-L238) uses form-style URLDecoder for path
    segments, turning literal + into a space. A Go-produced workflow ID a+b would therefore become a b, potentially pointing to the wrong execution. The new test currently codifies this bug
    (https://github.com/temporalio/sdk-java/blob/accc671abce2496605da46ec28c1966613e8b220/temporal-sdk/src/test/java/io/temporal/internal/common/LinkConverterTest.java#L855-L883). Use a path-segment percent decoder that preserves +, then invert
    that assertion. The Go reference converter uses PathEscape/PathUnescape (https://github.com/temporalio/api-go/blob/85042fb4769bb46c3ed1addac9de526af51f4d83/temporalnexus/link_converter.go#L336-L405).


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.

2 participants