Add support for SDK native types in dyn conversion #4385
Merged
+529
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implements conversion support for SDK native types across the dyn package conversion layer (FromTyped, ToTyped, Normalize). Supports three SDK types that use custom JSON marshaling: - duration.Duration: Protobuf duration format (e.g., "300s") - time.Time: RFC3339 timestamp format (e.g., "2023-12-25T10:30:00Z") - fieldmask.FieldMask: Comma-separated paths (e.g., "name,age,email") The implementation uses a generic approach with all SDK-specific code in sdk_native_types.go and sdk_native_types_test.go. All SDK type imports are prefixed with "sdk" for clarity. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Adds comprehensive round-trip tests using actual SDK types from the postgres service (BranchSpec and UpdateBranchRequest) to verify that serialization and deserialization work correctly in real-world usage. Tests cover: - BranchSpec with time.Time and duration.Duration fields - UpdateRequest with fieldmask.FieldMask field - Full round-trip FromTyped -> dyn.Value -> ToTyped - Normalization with SDK types Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
pietern
requested review from
andrewnester,
anton-107,
denik and
shreyas-goenka
as code owners
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
reviewed
Jan 29, 2026
libs/dyn/convert/sdk_native_types.go
Outdated
| return dyn.InvalidValue, err | ||
| } | ||
|
|
||
| // The JSON marshaling produces a quoted string, unmarshal to get the raw string. |
Contributor
There was a problem hiding this comment.
This only handles types that are encoded via string; Are all SDK Native types encoded via string?
Contributor
Author
There was a problem hiding this comment.
At the moment, yes.
Contributor
There was a problem hiding this comment.
From a Proto point of view, there is Value, which represents an arbitrary JSON.
https://protobuf.dev/reference/protobuf/google.protobuf/#value
But we do represent it using a json.RawMessage in the SDK, so it may not need special handling here
Contributor
Author
There was a problem hiding this comment.
@hectorcast-db Is that used in any of our structs today?
If there is an example we can figure out how to make it work here.
Contributor
There was a problem hiding this comment.
Currently, no team is using it. Someone just tried to introduce it, but it is not supported in the Rust backend, so it was postponed. (Java/Scala backends are supported).
We do have one instance not as a normal resource, but as part of the Operation object in LRO.
https://github.com/databricks/databricks-sdk-go/blob/98181c316a238887661235d656818dfbdcafc92f/service/postgres/model.go#L258
It is not something that the CLI usually sees, but it may help to understand how data is transferred and stored.
Collaborator
|
Commit: df65828
20 interesting tests: 9 KNOWN, 5 SKIP, 4 RECOVERED, 2 flaky
Top 50 slowest tests (at least 2 minutes):
|
Update test code to follow linter recommendations including struct field alignment and using assert.True for boolean assertions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1. Clarified that all SDK native types marshal to JSON strings with
improved comments explaining the conversion process.
2. Fixed JSON encoding in toTypedSDKNative to use json.Marshal instead
of fmt.Sprintf("%q") for proper JSON string literal creation.
3. Fixed TestFromTypedDurationPointer to actually test with a pointer
instead of a value.
4. Added comprehensive table-driven roundtrip tests with reusable
equality check functions for all three SDK types (Duration, Time,
FieldMask) testing both value and pointer variants.
5. Removed redundant basic tests that are now covered by the roundtrip
tests, while keeping edge case tests for nil, zero, error cases,
normalization, and in-struct scenarios.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removed 18 tests that are now covered by the comprehensive roundtrip tests. The remaining tests focus on unique edge cases: - Nil value handling (one per type) - Zero values - Error cases (invalid format, wrong type) - Empty values (FieldMask) - Normalize with nil (important edge case) - End-to-end tests with real postgres types This keeps test coverage while improving maintainability. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Converted 13 individual edge case tests into 3 table-driven tests: - TestSDKNativeTypesNilValues: Tests nil handling for FromTyped and Normalize across all 3 SDK types (6 subtests) - TestSDKNativeTypesErrors: Tests error handling for ToTyped with invalid inputs across all 3 types (4 subtests) - TestSDKNativeTypesSpecialCases: Tests zero values and empty FieldMask (3 subtests) Final test suite for SDK native types: - 1 comprehensive roundtrip test (18 subtests) - 3 edge case table tests (13 subtests) - 3 end-to-end tests with real postgres types This improves maintainability while preserving full test coverage. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Split TestSDKNativeTypesRoundtrip into three type-specific tests: - TestDurationRoundtrip (3 cases: 5min, 7days, 1hour) - TestTimeRoundtrip (3 cases: no_nanos, with_nanos, epoch) - TestFieldMaskRoundtrip (3 cases: single, multiple, nested) Benefits: - No more type switches or 'any' types in test tables - Each test is strongly typed with specific field names - Clearer test structure and better type safety - Still maintains same coverage (18 subtests total) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removed the three helper functions (assertDurationEqual, assertTimeEqual, assertFieldMaskEqual) and inlined their logic directly where used: - Duration: assert.Equal(t, src.AsDuration(), out.AsDuration()) - Time: assert.Equal(t, src.AsTime(), out.AsTime()) - FieldMask: assert.Equal(t, src.Paths, out.Paths) Benefits: - Simpler code with fewer abstractions - Easier to understand what's being compared - No need for 'any' types or type assertions in helpers - Still maintains same test coverage Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replaced testFunc callback pattern with simple subtests: Before (fake table tests): - TestSDKNativeTypesNilValues with testFunc callbacks - TestSDKNativeTypesErrors with testFunc callbacks - TestSDKNativeTypesSpecialCases with testFunc callbacks After (simple subtests): - TestNilValuesFromTyped - 3 subtests, one per type - TestNilValuesNormalize - 3 subtests, one per type - TestToTypedErrors - 4 subtests for error cases - TestSpecialCases - 3 subtests for edge cases Benefits: - No testFunc callbacks - just straightforward t.Run subtests - Each test is self-contained and easy to read - Reduced repetition (e.g., wrongTypeInput shared in TestToTypedErrors) - Simplified diagnostic assertions (just check key fields) - Same test coverage with clearer structure Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove duplicate and stale comments. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
reviewed
Jan 30, 2026
| func TestTimeRoundtrip(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| time time.Time |
Contributor
There was a problem hiding this comment.
Why not replace time time.Time with obj any and handle all type conversion in a single test?
The ToTyped/FromTyped functions are already any-based, so that aligns well.
Contributor
Author
There was a problem hiding this comment.
Two reasons: the target type in ToTyped is defined inline, changing it means using more reflection, and the equality test needs a different function call for every type. Moving that into a single test means adding a switch/case on type, defying the purpose.
Contributor
There was a problem hiding this comment.
Two reasons: the target type in ToTyped is defined inline, changing it means using more reflection,
I don't insist, but reflection can be avoided, out can be another entry in table test:
e.g. out: &sdktime.Time{}, (did not test).
the equality test needs a different function call for every type.
For my understanding, are you referring to this?
assert.Equal(t, src.AsTime(), out.AsTime()) assert.Equal(t, src.Paths, out.Paths)
why not do assert.Equal(t, src, out) in both cases?
Contributor
Author
There was a problem hiding this comment.
I tried this but it's not possible. The values contain internal pointers making direct comparison impossible:
Error Trace: /Users/pieter.noordhuis/dev/wt/cli-sdk-duration-type/libs/dyn/convert/sdk_native_types_test.go:112
Error: Not equal:
expected: duration.Duration{internal:(*durationpb.Duration)(0x1400018b000)}
actual : duration.Duration{internal:(*durationpb.Duration)(0x1400018b440)}
Adding a type switch to the table test defies its purpose, so leaving as is.
denik
approved these changes
Jan 30, 2026
Contributor
denik
left a comment
denik
left a comment
There was a problem hiding this comment.
Stamping, but I think there are a few more opportunities to reduce LOC in tests by switching to table tests. Claude should be able to convert if prompted explicitly.
denik
reviewed
Jan 30, 2026
hectorcast-db
approved these changes
Feb 2, 2026
Convert TestNilValuesFromTyped, TestNilValuesNormalize, and TestToTypedErrors to use table-driven test patterns for better maintainability and reduced code duplication. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove isSDKNativeType helper and use slices.Contains directly in the struct case of each conversion function. This is more efficient since pointer dereferencing already happens before the switch statement, making the helper's pointer handling redundant. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Merge TestNilValuesFromTyped and TestNilValuesNormalize into TestNilValues with from_typed and normalize subtests, sharing the same test data for better maintainability. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add handling for pure variable references in both FromTyped and ToTyped for SDK native types (Duration, Time, FieldMask): - FromTyped: Return ref unchanged when it's a variable reference - ToTyped: Set destination to zero value when source is a variable reference This matches the behavior of other types in the conversion system. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
Author
|
@denik The last revision includes a few more table tests where appropriate. The total LOC didn't change much, though it's clearer where future new types should be added, so still net positive. Please take a final look. |
andrewnester
approved these changes
Feb 2, 2026
denik
approved these changes
Feb 2, 2026
Contributor
denik
left a comment
denik
left a comment
There was a problem hiding this comment.
Thanks, moving check inside Struct case looks more natural.
| switch src.Kind() { | ||
| case dyn.KindString: | ||
| // Ignore pure variable references (e.g. ${var.foo}). | ||
| if dynvar.IsPureVariableReference(src.MustString()) { |
Contributor
There was a problem hiding this comment.
This may be out of scope for this PR, but is there a way to centralize variable check? Currently every type-specific function does it, but it's the same everywhere.
Same is probably true for KindNil handling.
Contributor
Author
There was a problem hiding this comment.
Might be possible, good point. Would have to be done before the type switch.
| } | ||
| return json.Unmarshal(jsonBytes, dst.Addr().Interface()) | ||
| case dyn.KindNil: | ||
| dst.SetZero() |
Contributor
There was a problem hiding this comment.
There seems to be no coverage for this case. Up to you if it's needed. Same q re various error cases, seem untested.
Contributor
Author
There was a problem hiding this comment.
Noted a follow up to check coverage.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Implements conversion support for SDK native types across the dyn package conversion layer (FromTyped, ToTyped, Normalize). Supports three SDK types that use custom JSON marshaling:
The implementation uses a generic approach with all SDK-specific code in sdk_native_types.go and sdk_native_types_test.go.
Why
The new Postgres APIs use these types and need the to/from conversion to work.
Also see databricks/databricks-sdk-go#1271.