-
Notifications
You must be signed in to change notification settings - Fork 183
Add cmdio.RenderFiltered to strip fields from rendered output #5450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| package cmdio | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "strings" | ||
| ) | ||
|
|
||
| // applyInputOnlyMask returns v with the listed dotted paths removed. If | ||
| // paths is empty, v is returned unchanged. Otherwise v is round-tripped | ||
| // through JSON into a generic representation, the paths are deleted, and | ||
| // the masked value is returned for the caller to marshal in its preferred | ||
| // format. | ||
| // | ||
| // Paths use dotted notation (e.g. "stable_url.initial_workspace_id"). | ||
| // Arrays and dynamically-keyed maps (e.g. proto map<string, V>) are | ||
| // traversed transparently: a single path applies to every element of an | ||
| // array, and to every value of a map when no literal key matches the | ||
| // next path component. List responses and map-valued fields therefore | ||
| // share the same path expression as singletons. | ||
| func applyInputOnlyMask(v any, paths []string) (any, error) { | ||
| if len(paths) == 0 { | ||
| return v, nil | ||
| } | ||
| b, err := json.Marshal(v) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("input-only mask: marshal: %w", err) | ||
| } | ||
| var out any | ||
| if err := json.Unmarshal(b, &out); err != nil { | ||
| return nil, fmt.Errorf("input-only mask: unmarshal: %w", err) | ||
| } | ||
| for _, p := range paths { | ||
| deletePath(out, strings.Split(p, ".")) | ||
| } | ||
| return out, nil | ||
| } | ||
|
|
||
| // deletePath walks v according to keys and removes the leaf key from any | ||
| // object it lands on. | ||
| // | ||
| // Both arrays and dynamically-keyed maps are traversed transparently: | ||
| // | ||
| // - When v is a []any, every element is visited with the same key list. | ||
| // - When v is a map[string]any but the next key is not a literal match, | ||
| // every value is visited with the same key list — this handles proto | ||
| // map<string, V> fields, whose JSON keys are user-provided strings and | ||
| // whose values carry the field name from the path. | ||
| // | ||
| // Both struct fields and proto map<string, V> surface as map[string]any | ||
| // after json.Unmarshal, so a single corner case remains: if a map's | ||
| // user-provided key happens to equal an inner field name, the literal | ||
| // match wins and that entry is removed instead of the field inside each | ||
| // value. Genkit emits paths from the schema, and this matches the | ||
| // expected behavior for any path the schema actually targets. | ||
| func deletePath(v any, keys []string) { | ||
| if len(keys) == 0 { | ||
| return | ||
| } | ||
| switch t := v.(type) { | ||
| case map[string]any: | ||
| if child, ok := t[keys[0]]; ok { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The literal-match-wins rule here is worse than the doc comment describes: for |
||
| if len(keys) == 1 { | ||
| delete(t, keys[0]) | ||
| } else { | ||
| deletePath(child, keys[1:]) | ||
| } | ||
| return | ||
| } | ||
| for _, child := range t { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern with this fallback: after the round-trip, nested structs and proto maps are both This bites exactly in the INPUT_ONLY case: when the server omits the field at its expected location, the descent falls through and strips same-named legitimate output elsewhere in the response. I think the fix for this and the map collision above is the same: make map traversal explicit in the path language. genkit knows where the proto maps are, so it can emit |
||
| deletePath(child, keys) | ||
| } | ||
| case []any: | ||
| for _, el := range t { | ||
| deletePath(el, keys) | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| package cmdio | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| "github.com/databricks/cli/libs/flags" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| type stableURL struct { | ||
| Name string `json:"name"` | ||
| InitialWorkspaceID string `json:"initial_workspace_id"` | ||
| URL string `json:"url,omitempty"` | ||
| } | ||
|
|
||
| type stableURLList struct { | ||
| StableURLs []stableURL `json:"stable_urls"` | ||
| } | ||
|
|
||
| type wrapper struct { | ||
| StableURL stableURL `json:"stable_url"` | ||
| } | ||
|
|
||
| type taggedStableURLs struct { | ||
| Tags map[string]stableURL `json:"tags"` | ||
| } | ||
|
|
||
| type nestedMapWrapper struct { | ||
| Spec struct { | ||
| Tags map[string]stableURL `json:"tags"` | ||
| } `json:"spec"` | ||
| } | ||
|
|
||
| func TestApplyInputOnlyMaskEmptyPathsReturnsValueUnchanged(t *testing.T) { | ||
| in := stableURL{Name: "n", InitialWorkspaceID: "w"} | ||
| out, err := applyInputOnlyMask(in, nil) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, in, out) | ||
| } | ||
|
|
||
| func TestApplyInputOnlyMaskFlatField(t *testing.T) { | ||
| in := stableURL{Name: "n", InitialWorkspaceID: "w", URL: "u"} | ||
| out, err := applyInputOnlyMask(in, []string{"initial_workspace_id"}) | ||
| require.NoError(t, err) | ||
| b, err := json.Marshal(out) | ||
| require.NoError(t, err) | ||
| assert.JSONEq(t, `{"name":"n","url":"u"}`, string(b)) | ||
| } | ||
|
|
||
| func TestApplyInputOnlyMaskFieldAbsentIsNoop(t *testing.T) { | ||
| in := stableURL{Name: "n"} | ||
| out, err := applyInputOnlyMask(in, []string{"missing_field"}) | ||
| require.NoError(t, err) | ||
| b, err := json.Marshal(out) | ||
| require.NoError(t, err) | ||
| // Name retained; missing path silently ignored. InitialWorkspaceID | ||
| // stays present at "" because the struct tag has no omitempty. | ||
| assert.JSONEq(t, `{"name":"n","initial_workspace_id":""}`, string(b)) | ||
| } | ||
|
|
||
| func TestApplyInputOnlyMaskNested(t *testing.T) { | ||
| in := wrapper{StableURL: stableURL{Name: "n", InitialWorkspaceID: "w"}} | ||
| out, err := applyInputOnlyMask(in, []string{"stable_url.initial_workspace_id"}) | ||
| require.NoError(t, err) | ||
| b, err := json.Marshal(out) | ||
| require.NoError(t, err) | ||
| assert.JSONEq(t, `{"stable_url":{"name":"n"}}`, string(b)) | ||
| } | ||
|
|
||
| func TestApplyInputOnlyMaskSliceElements(t *testing.T) { | ||
| in := stableURLList{StableURLs: []stableURL{ | ||
| {Name: "a", InitialWorkspaceID: "1"}, | ||
| {Name: "b", InitialWorkspaceID: "2"}, | ||
| }} | ||
| out, err := applyInputOnlyMask(in, []string{"stable_urls.initial_workspace_id"}) | ||
| require.NoError(t, err) | ||
| b, err := json.Marshal(out) | ||
| require.NoError(t, err) | ||
| assert.JSONEq(t, `{"stable_urls":[{"name":"a"},{"name":"b"}]}`, string(b)) | ||
| } | ||
|
|
||
| func TestApplyInputOnlyMaskMapValues(t *testing.T) { | ||
| in := taggedStableURLs{Tags: map[string]stableURL{ | ||
| "env": {Name: "a", InitialWorkspaceID: "1"}, | ||
| "prod": {Name: "b", InitialWorkspaceID: "2"}, | ||
| }} | ||
| out, err := applyInputOnlyMask(in, []string{"tags.initial_workspace_id"}) | ||
| require.NoError(t, err) | ||
| b, err := json.Marshal(out) | ||
| require.NoError(t, err) | ||
|
|
||
| var got struct { | ||
| Tags map[string]map[string]any `json:"tags"` | ||
| } | ||
| require.NoError(t, json.Unmarshal(b, &got)) | ||
| require.Len(t, got.Tags, 2) | ||
| for k, v := range got.Tags { | ||
| assert.NotContains(t, v, "initial_workspace_id", "tag %q should have initial_workspace_id stripped", k) | ||
| assert.Contains(t, v, "name", "tag %q should retain name", k) | ||
| } | ||
| } | ||
|
|
||
| func TestApplyInputOnlyMaskNestedInMapValue(t *testing.T) { | ||
| // Path lands inside a map at the second segment, then descends two | ||
| // more levels into the map's value. Confirms map transparency | ||
| // composes with regular literal-key descent. | ||
| var in nestedMapWrapper | ||
| in.Spec.Tags = map[string]stableURL{ | ||
| "a": {Name: "x", InitialWorkspaceID: "1"}, | ||
| "b": {Name: "y", InitialWorkspaceID: "2"}, | ||
| } | ||
| out, err := applyInputOnlyMask(in, []string{"spec.tags.initial_workspace_id"}) | ||
| require.NoError(t, err) | ||
| b, err := json.Marshal(out) | ||
| require.NoError(t, err) | ||
|
|
||
| var got struct { | ||
| Spec struct { | ||
| Tags map[string]map[string]any `json:"tags"` | ||
| } `json:"spec"` | ||
| } | ||
| require.NoError(t, json.Unmarshal(b, &got)) | ||
| require.Len(t, got.Spec.Tags, 2) | ||
| for k, v := range got.Spec.Tags { | ||
| assert.NotContains(t, v, "initial_workspace_id", "spec.tags[%q] should have initial_workspace_id stripped", k) | ||
| } | ||
| } | ||
|
|
||
| func TestApplyInputOnlyMaskMultiplePaths(t *testing.T) { | ||
| in := stableURL{Name: "n", InitialWorkspaceID: "w", URL: "u"} | ||
| out, err := applyInputOnlyMask(in, []string{"initial_workspace_id", "url"}) | ||
| require.NoError(t, err) | ||
| b, err := json.Marshal(out) | ||
| require.NoError(t, err) | ||
| assert.JSONEq(t, `{"name":"n"}`, string(b)) | ||
| } | ||
|
|
||
| // TestRenderFilteredStripsInputOnlyField is the integration check: pass | ||
| // the same StableUrl-like value the CLI would receive from the SDK, and | ||
| // confirm the rendered JSON does not contain initial_workspace_id. | ||
| func TestRenderFilteredStripsInputOnlyField(t *testing.T) { | ||
| v := stableURL{Name: "accounts/x/stable-urls/y", InitialWorkspaceID: "ws-1", URL: "https://example.test"} | ||
|
|
||
| out := &bytes.Buffer{} | ||
| c := &cmdIO{ | ||
| capabilities: Capabilities{}, | ||
| outputFormat: flags.OutputJSON, | ||
| out: out, | ||
| err: out, | ||
| } | ||
| ctx := InContext(t.Context(), c) | ||
| require.NoError(t, RenderFiltered(ctx, v, []string{"initial_workspace_id"})) | ||
|
|
||
| assert.JSONEq(t, `{"name":"accounts/x/stable-urls/y","url":"https://example.test"}`, out.String()) | ||
| assert.NotContains(t, out.String(), "initial_workspace_id") | ||
| } | ||
|
|
||
| func TestRenderFilteredNoPathsMatchesRender(t *testing.T) { | ||
| v := stableURL{Name: "n", InitialWorkspaceID: "w"} | ||
|
|
||
| want := &bytes.Buffer{} | ||
| got := &bytes.Buffer{} | ||
| mk := func(buf *bytes.Buffer) *cmdIO { | ||
| return &cmdIO{capabilities: Capabilities{}, outputFormat: flags.OutputJSON, out: buf, err: buf} | ||
| } | ||
| require.NoError(t, Render(InContext(t.Context(), mk(want)), v)) | ||
| require.NoError(t, RenderFiltered(InContext(t.Context(), mk(got)), v, nil)) | ||
| assert.Equal(t, want.String(), got.String()) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: this round-trip corrupts large int64 fields whenever a path is filtered.
json.Unmarshalintoanydecodes every JSON number as float64 (53-bit mantissa), and it hits every numeric field in the response, not just the stripped one. Repro:spark_context_idis a real cluster response field that exceeds 2^53. The fix is small:json.Numberre-marshals verbatim anddeletePathnever touches it. A regression test with a value above 2^53 (i.e.int64(9007199254740993)) would catch this.One more side effect worth a deliberate decision: the
map[string]anytree marshals keys alphabetically while structs marshal in field order, so turning filtering on for a command reorders its whole JSON output.UseNumberdoesn't change that. I think sorted keys are probably acceptable, but it should be a conscious choice, not a surprise.