Add support for SDK native types in dyn conversion #4385
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.
libs/dyn/convert/sdk_native_types.go
Outdated
| switch src.Kind() { | ||
| case dyn.KindString: | ||
| // Use JSON unmarshaling since SDK native types implement json.Unmarshaler. | ||
| jsonStr := fmt.Sprintf("%q", src.MustString()) |
Contributor
There was a problem hiding this comment.
This uses go string representation, not JSON, right?
Contributor
Author
There was a problem hiding this comment.
You're right, fixed to use json.Marshal of the string to produce the unmarshallable representation.
| assert.Equal(t, dyn.V("300s"), nv) | ||
| } | ||
|
|
||
| func TestFromTypedDurationPointer(t *testing.T) { |
Contributor
There was a problem hiding this comment.
This says Pointer in the name, but I don't see a pointer, where is it?
Contributor
Author
There was a problem hiding this comment.
Fixed with the table test.
|
|
||
| err := ToTyped(&out, v) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, 300*time.Second, out.AsDuration()) |
Contributor
There was a problem hiding this comment.
Is there any way to organize these tests as
a) table tests
b) automatically checking roundtrip
so you can specify
{
typed: sdkduration.Duration(300*time.Second),
string: "300s"
}
and it checks both fromType and toTyped as well as pointer to typed.
Contributor
Author
There was a problem hiding this comment.
Adding a table test.
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?
| } | ||
|
|
||
| // Handle SDK native types using JSON unmarshaling. | ||
| if isSDKNativeType(dstv.Type()) { |
Contributor
There was a problem hiding this comment.
Is there any way we can incorporate this into switch below? It's nice to see overview for each type and this hides it.
If it must be handled outside, can you add an explanation comment, why it's special-cased here?
Contributor
Author
There was a problem hiding this comment.
Good point, looking into it.
Contributor
Author
There was a problem hiding this comment.
It's not possible to inline it into the switch because it switches on the Kind() which is always reflect.Struct.
Inlining the isSDKNativeType is not worth it because it duplicates the logic between FromTyped and ToTyped.
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.
| }) | ||
| } | ||
|
|
||
| func TestNilValuesNormalize(t *testing.T) { |
Contributor
There was a problem hiding this comment.
This and previous tests look like they can be written as a single table test.
denik
reviewed
Jan 30, 2026
| // All SDK native types marshal to JSON strings. Unmarshal to get the raw string value. | ||
| // For example: duration.Duration(300s) -> JSON "300s" -> string "300s" | ||
| var str string | ||
| if err := json.Unmarshal(jsonBytes, &str); err != nil { |
Contributor
There was a problem hiding this comment.
can this be a variable reference?
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.