feat(schedule): add clean-type overloads for createSchedule and updateSchedule

[abhishekj720]

What changed

Adds convenience overloads to ScheduleClient and ScheduleClientImpl so callers can use clean client types instead of raw Thrift gen types.

Before:

ScheduleClient sc = workflowClient.scheduleClient();
CreateScheduleRequest request = new CreateScheduleRequest()
    .setSpec(new com.uber.cadence.ScheduleSpec().setCronExpression("* * * * *"))
    .setAction(new com.uber.cadence.ScheduleAction().setStartWorkflow(...))
    .setPolicies(new com.uber.cadence.SchedulePolicies()...);
sc.createSchedule("my-schedule", request).join();

After:

sc.createSchedule(
    "my-schedule",
    ScheduleSpec.newBuilder().setCronExpression("* * * * *").build(),
    ScheduleAction.newBuilder().setStartWorkflow(
        StartWorkflowAction.newBuilder()
            .setWorkflowType("MyWorkflow")
            .setTaskList("my-tl")
            .build()).build(),
    SchedulePolicies.newBuilder().setOverlapPolicy(ScheduleOverlapPolicy.SKIP_NEW).build()
).join();

The raw-request overloads remain for callers that need to set memo or search attributes (which require Thrift gen types).

New private helpers in ScheduleClientImpl: toThriftSpec, toThriftAction, toThriftStartWorkflow, toThriftPolicies, toThriftCatchUpPolicy, toThriftRetryPolicy.

Why

Makes the API surface usable without knowledge of generated Thrift types. Enables clean samples in cadence-java-samples.

How it was tested

• ./gradlew compileJava -x generateProto — clean build

Dependencies

Stacked on feat/schedule-impl (#1069). Merge #1067 and #1069 first.

[codecov]

Codecov Report

❌ Patch coverage is 69.38776% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.14%. Comparing base (073c574) to head (3509da1).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...uber/cadence/internal/sync/ScheduleClientImpl.java	69.38%	19 Missing and 11 partials ⚠️

❌ Your patch check has failed because the patch coverage (69.38%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project check has failed because the head coverage (68.14%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage.

Files with missing lines	Coverage Δ	Complexity Δ
...uber/cadence/internal/sync/ScheduleClientImpl.java	32.00% <69.38%> (ø)	27.00 <22.00> (?)

... and 6 files with indirect coverage changes

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f53be6...3509da1. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gitar-bot]

Code Review ✅ Approved 3 resolved / 3 findings

Introduces clean-type overloads for schedule creation to improve API ergonomics. Nanosecond precision, type validation, and unit test coverage findings have been addressed.

✅ 3 resolved

✅ Edge Case: Unsafe (byte[]) cast on memo/searchAttributes can throw

📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:289-298
toThriftStartWorkflow casts every memo and search-attribute value to byte[]: sw.getMemo().forEach((k, v) -> fields.put(k, (byte[]) v)). But StartWorkflowAction.getMemo()/getSearchAttributes() are typed Map<String, Object> (see ScheduleAction.java:179,184) and the builder's setMemo/setSearchAttributes accept any Object. If a caller supplies a value that is not a byte[] (e.g. a String or already-serialized object), this throws a raw ClassCastException with no context about which key/type failed. Note also that a describe→toBuilder→update round-trip is safe only because Memo.fields happens to be Map<String, byte[]>; any other value type breaks. Consider validating the value type and throwing a descriptive IllegalArgumentException (naming the offending key), or documenting on the setters that values must be byte[].

✅ Bug: startTime/endTime conversion truncates to millisecond precision

📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:249-254
toThriftSpec converts the Instant start/end times with getStartTime().toEpochMilli() * 1_000_000L, which discards sub-millisecond precision. The reverse conversion nanosToInstant (line 512-515) preserves full nanosecond precision, so a describe→modify→update round-trip silently truncates any nanosecond component of the timestamps. Instant.toEpochMilli() can also overflow with ArithmeticException for extreme values. Prefer computing full nanos, e.g. i.getEpochSecond() * 1_000_000_000L + i.getNano().

✅ Quality: No unit tests for new clean-type conversion helpers

📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:245-259
This PR adds substantial new conversion logic (toThriftSpec, toThriftAction, toThriftStartWorkflow, toThriftPolicies, toThriftCatchUpPolicy, toThriftRetryPolicy) but no tests were added. The change was only validated by compileJava. These conversions contain unit-testable edge cases (null handling, seconds/nanos conversion, enum mapping, memo/search-attribute casting) that would benefit from tests to guard against regressions and confirm round-trip fidelity with the existing toScheduleSpec/toScheduleAction/toSchedulePolicies inverse mappings. Consider adding tests covering null inputs, precision, and a describe→create round-trip.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[gitar-bot]

Code Review ✅ Approved 4 resolved / 4 findings

Introduces clean-type overloads for createSchedule and updateSchedule to improve API ergonomics. Resolves missing unit tests, nanosecond precision truncation, and unsafe type casting in conversion helpers.

✅ 4 resolved

✅ Edge Case: Unsafe (byte[]) cast on memo/searchAttributes can throw

📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:289-298
toThriftStartWorkflow casts every memo and search-attribute value to byte[]: sw.getMemo().forEach((k, v) -> fields.put(k, (byte[]) v)). But StartWorkflowAction.getMemo()/getSearchAttributes() are typed Map<String, Object> (see ScheduleAction.java:179,184) and the builder's setMemo/setSearchAttributes accept any Object. If a caller supplies a value that is not a byte[] (e.g. a String or already-serialized object), this throws a raw ClassCastException with no context about which key/type failed. Note also that a describe→toBuilder→update round-trip is safe only because Memo.fields happens to be Map<String, byte[]>; any other value type breaks. Consider validating the value type and throwing a descriptive IllegalArgumentException (naming the offending key), or documenting on the setters that values must be byte[].

✅ Bug: startTime/endTime conversion truncates to millisecond precision

📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:249-254
toThriftSpec converts the Instant start/end times with getStartTime().toEpochMilli() * 1_000_000L, which discards sub-millisecond precision. The reverse conversion nanosToInstant (line 512-515) preserves full nanosecond precision, so a describe→modify→update round-trip silently truncates any nanosecond component of the timestamps. Instant.toEpochMilli() can also overflow with ArithmeticException for extreme values. Prefer computing full nanos, e.g. i.getEpochSecond() * 1_000_000_000L + i.getNano().

✅ Quality: No unit tests for new clean-type conversion helpers

📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:245-259
This PR adds substantial new conversion logic (toThriftSpec, toThriftAction, toThriftStartWorkflow, toThriftPolicies, toThriftCatchUpPolicy, toThriftRetryPolicy) but no tests were added. The change was only validated by compileJava. These conversions contain unit-testable edge cases (null handling, seconds/nanos conversion, enum mapping, memo/search-attribute casting) that would benefit from tests to guard against regressions and confirm round-trip fidelity with the existing toScheduleSpec/toScheduleAction/toSchedulePolicies inverse mappings. Consider adding tests covering null inputs, precision, and a describe→create round-trip.

✅ Bug: backfillSchedule now fires requests concurrently instead of sequentially

📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:148-162
The previous implementation iterated over backfills and called service.BackfillSchedule(request) one at a time inside a single blocking task, so requests were submitted and completed strictly in order. The new implementation submits all backfill requests up front (futures.add(service.BackfillSchedule(request)) in the loop) and then waits on CompletableFuture.allOf(...). Because the service methods are now non-blocking, this fires every backfill request concurrently rather than sequentially.

This is a semantic change: backfills for overlapping time ranges may now be processed by the server out of order (or interleaved), which can matter for overlap-policy handling. If sequential ordering was relied upon, this could produce different backfill results. If concurrency is intentional and acceptable, no action is needed; otherwise chain the futures so each request starts only after the prior one completes (e.g. future = future.thenCompose(prev -> service.BackfillSchedule(nextRequest))).

Note also the results are still collected in submission order via f.join() after allOf, so the returned list ordering is preserved even though execution is concurrent.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}