From 2d1b9e8d6dbd577ba7f265b73efa8d18ffe8bf97 Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Tue, 21 Apr 2026 20:27:13 -0500 Subject: [PATCH 1/3] fix(connector): test verb fetches config via GetConnector before probing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server's TestConnector RPC validates a full `{config: {...}}` body — it's a pre-create probe, not a test-existing op. The previous CLI sent a bare `{connectorId}` which the server rejected with `config is required` (typed-error 400) and, before that, with a string-vs-int field-type mismatch on `connectorId`. Behavior change: - `ana connector test ` now does GetConnector → builds a config from the returned `Metadata` block → posts to TestConnector. The CLI surface (id-only positional) is unchanged. - `Metadata` blocks omit secrets; we fill `password: "redacted"` so the dialect-specific config validates. The driver returns its auth/dial error via the standard `200 {error: ...}` shape, which the CLI surfaces as `FAIL: ` — the same contract as before. Coverage gate stays at 100% on `./internal/...`. The previous CATALOG DEVIATION docstring is replaced with a CATALOG REALITY block describing why the verb is forced through Get + Test. --- internal/connector/test.go | 71 +++++++++++++++++++------ internal/connector/test_test.go | 91 +++++++++++++++++++++++++++++++-- 2 files changed, 142 insertions(+), 20 deletions(-) diff --git a/internal/connector/test.go b/internal/connector/test.go index cd87e32..eecbcc4 100644 --- a/internal/connector/test.go +++ b/internal/connector/test.go @@ -4,26 +4,21 @@ import ( "context" "fmt" "io" + "strings" "github.com/highperformance-tech/ana-cli/internal/cli" ) // testCmd implements `ana connector test ` — TestConnector. // -// CATALOG DEVIATION: the task brief specifies -// -// POST TestConnector {"connectorId": } -// -// but the captured API requires a full config body: -// -// POST TestConnector {"config": {connectorType, name, postgres: {...}}} -// -// (see api-catalog/POST_...TestConnector.json). Since the brief says "if -// catalog differs from this brief, prefer catalog," we follow the catalog -// shape and send `{connectorId}` anyway — this matches the brief's CLI UX -// (test-by-id) and will either be accepted by a future server change or -// return the current driver error, which we surface verbatim. Server response -// shape `{error: }` is empty/absent on success. +// CATALOG REALITY: the server's TestConnector endpoint validates a full config +// body (`{config: {connectorType, name, : {...}}}`), not a bare id — +// it's a pre-create probe, not a test-existing op. To preserve the CLI's +// id-based UX we GET the connector first, rebuild a config from the returned +// `Metadata` block, and POST that to TestConnector. Passwords/secrets +// are redacted in GetConnector responses, so the dial typically fails with an +// auth or connection error — which the server still returns as a 200 with the +// driver message in `error`. Both `OK` and `FAIL: ` are valid CLI outputs. type testCmd struct{ deps Deps } func (c *testCmd) Help() string { @@ -32,7 +27,7 @@ func (c *testCmd) Help() string { } type testReq struct { - ConnectorID int `json:"connectorId"` + Config map[string]any `json:"config"` } type testResp struct { @@ -51,8 +46,16 @@ func (c *testCmd) Run(ctx context.Context, args []string, stdio cli.IO) error { if err != nil { return err } + var getRaw map[string]any + if err := c.deps.Unary(ctx, servicePath+"/GetConnector", getReq{ConnectorID: id}, &getRaw); err != nil { + return fmt.Errorf("connector test: %w", err) + } + cfg, err := configFromGetConnector(getRaw) + if err != nil { + return fmt.Errorf("connector test: %w", err) + } var raw map[string]any - if err := c.deps.Unary(ctx, servicePath+"/TestConnector", testReq{ConnectorID: id}, &raw); err != nil { + if err := c.deps.Unary(ctx, servicePath+"/TestConnector", testReq{Config: cfg}, &raw); err != nil { return fmt.Errorf("connector test: %w", err) } var typed testResp @@ -68,3 +71,39 @@ func (c *testCmd) Run(ctx context.Context, args []string, stdio cli.IO) error { } return nil } + +// configFromGetConnector rebuilds a TestConnector `config` body from a +// GetConnector response. It copies the top-level `name` + `connectorType`, and +// moves the `Metadata` block into ``. Secrets are absent +// from the metadata block; the server accepts the probe and returns a driver +// error for the missing credential. +func configFromGetConnector(raw map[string]any) (map[string]any, error) { + conn, _ := raw["connector"].(map[string]any) + if conn == nil { + return nil, fmt.Errorf("GetConnector: missing connector object") + } + connectorType, _ := conn["connectorType"].(string) + name, _ := conn["name"].(string) + if connectorType == "" { + return nil, fmt.Errorf("GetConnector: missing connectorType") + } + cfg := map[string]any{"connectorType": connectorType, "name": name} + for k, v := range conn { + if !strings.HasSuffix(k, "Metadata") { + continue + } + if block, ok := v.(map[string]any); ok { + dialectKey := strings.TrimSuffix(k, "Metadata") + // GetConnector redacts secrets, so fill a placeholder for any + // required secret field. The server returns the driver's auth + // failure as a 200 `{error: ...}` which the CLI surfaces as FAIL:. + for _, secret := range []string{"password"} { + if _, present := block[secret]; !present { + block[secret] = "redacted" + } + } + cfg[dialectKey] = block + } + } + return cfg, nil +} diff --git a/internal/connector/test_test.go b/internal/connector/test_test.go index b5d20b8..8e4b6d5 100644 --- a/internal/connector/test_test.go +++ b/internal/connector/test_test.go @@ -10,10 +10,28 @@ import ( "github.com/highperformance-tech/ana-cli/internal/testcli" ) +// stubGetConnector returns a minimal GetConnector response so `connector test` +// can build a config body for the follow-up TestConnector call. +func stubGetConnector(resp any) { + out := resp.(*map[string]any) + *out = map[string]any{ + "connector": map[string]any{ + "id": 1.0, + "name": "probe", + "connectorType": "POSTGRES", + "postgresMetadata": map[string]any{"host": "h", "port": 5432.0, "user": "u", "database": "d", "dialect": "postgres", "sslMode": true}, + }, + } +} + func TestTestOK(t *testing.T) { t.Parallel() f := &fakeDeps{ - unaryFn: func(_ context.Context, _ string, _, resp any) error { + unaryFn: func(_ context.Context, path string, _, resp any) error { + if strings.HasSuffix(path, "/GetConnector") { + stubGetConnector(resp) + return nil + } out := resp.(*map[string]any) *out = map[string]any{"error": ""} return nil @@ -35,7 +53,11 @@ func TestTestOK(t *testing.T) { func TestTestFail(t *testing.T) { t.Parallel() f := &fakeDeps{ - unaryFn: func(_ context.Context, _ string, _, resp any) error { + unaryFn: func(_ context.Context, path string, _, resp any) error { + if strings.HasSuffix(path, "/GetConnector") { + stubGetConnector(resp) + return nil + } out := resp.(*map[string]any) *out = map[string]any{"error": "connection refused"} return nil @@ -54,7 +76,11 @@ func TestTestFail(t *testing.T) { func TestTestJSON(t *testing.T) { t.Parallel() f := &fakeDeps{ - unaryFn: func(_ context.Context, _ string, _, resp any) error { + unaryFn: func(_ context.Context, path string, _, resp any) error { + if strings.HasSuffix(path, "/GetConnector") { + stubGetConnector(resp) + return nil + } out := resp.(*map[string]any) *out = map[string]any{"error": ""} return nil @@ -115,7 +141,11 @@ func TestTestBadFlag(t *testing.T) { func TestTestRemarshalErr(t *testing.T) { t.Parallel() f := &fakeDeps{ - unaryFn: func(_ context.Context, _ string, _, resp any) error { + unaryFn: func(_ context.Context, path string, _, resp any) error { + if strings.HasSuffix(path, "/GetConnector") { + stubGetConnector(resp) + return nil + } out := resp.(*map[string]any) *out = map[string]any{"error": 123.0} return nil @@ -128,3 +158,56 @@ func TestTestRemarshalErr(t *testing.T) { t.Errorf("err=%v", err) } } + +func TestTestTestConnectorUnaryErr(t *testing.T) { + t.Parallel() + f := &fakeDeps{ + unaryFn: func(_ context.Context, path string, _, resp any) error { + if strings.HasSuffix(path, "/GetConnector") { + stubGetConnector(resp) + return nil + } + return errors.New("boom") + }, + } + cmd := &testCmd{deps: f.deps()} + stdio, _, _ := testcli.NewIO(strings.NewReader("")) + err := cmd.Run(context.Background(), []string{"1"}, stdio) + if err == nil || !strings.Contains(err.Error(), "boom") { + t.Errorf("err=%v", err) + } +} + +func TestTestMissingConnectorType(t *testing.T) { + t.Parallel() + f := &fakeDeps{ + unaryFn: func(_ context.Context, _ string, _, resp any) error { + out := resp.(*map[string]any) + *out = map[string]any{"connector": map[string]any{"id": 1.0}} + return nil + }, + } + cmd := &testCmd{deps: f.deps()} + stdio, _, _ := testcli.NewIO(strings.NewReader("")) + err := cmd.Run(context.Background(), []string{"1"}, stdio) + if err == nil || !strings.Contains(err.Error(), "missing connectorType") { + t.Errorf("err=%v", err) + } +} + +func TestTestMissingConnector(t *testing.T) { + t.Parallel() + f := &fakeDeps{ + unaryFn: func(_ context.Context, _ string, _, resp any) error { + out := resp.(*map[string]any) + *out = map[string]any{} + return nil + }, + } + cmd := &testCmd{deps: f.deps()} + stdio, _, _ := testcli.NewIO(strings.NewReader("")) + err := cmd.Run(context.Background(), []string{"1"}, stdio) + if err == nil || !strings.Contains(err.Error(), "missing connector object") { + t.Errorf("err=%v", err) + } +} From 33d8c5eff3889dd170c7db3e7589697b00c4bb24 Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Tue, 21 Apr 2026 20:46:09 -0500 Subject: [PATCH 2/3] refactor(connector): shallow-copy metadata block in test config Avoid in-place mutation of the caller's map when inserting the 'redacted' password placeholder. The function is currently called once with a fresh map, but defending against future reuse keeps the helper a pure function. Addresses CodeRabbit nit on PR #19. --- internal/connector/test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/connector/test.go b/internal/connector/test.go index eecbcc4..421a1e8 100644 --- a/internal/connector/test.go +++ b/internal/connector/test.go @@ -94,15 +94,20 @@ func configFromGetConnector(raw map[string]any) (map[string]any, error) { } if block, ok := v.(map[string]any); ok { dialectKey := strings.TrimSuffix(k, "Metadata") + // Shallow-copy so we never mutate the caller's map. + out := make(map[string]any, len(block)+1) + for bk, bv := range block { + out[bk] = bv + } // GetConnector redacts secrets, so fill a placeholder for any // required secret field. The server returns the driver's auth // failure as a 200 `{error: ...}` which the CLI surfaces as FAIL:. for _, secret := range []string{"password"} { - if _, present := block[secret]; !present { - block[secret] = "redacted" + if _, present := out[secret]; !present { + out[secret] = "redacted" } } - cfg[dialectKey] = block + cfg[dialectKey] = out } } return cfg, nil From 1d8c181aa4c0b30676a18b3cae5444bc06c8f386 Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Wed, 22 Apr 2026 10:31:02 -0500 Subject: [PATCH 3/3] docs(connector): note dialect limitation of test placeholder Only "password" is placeholdered today. Postgres uses it; Snowflake's non-password auth modes (keypair, oauth-sso, oauth-individual) use privateKey / oauthClientSecret. Document that so a reader extending the dialect matrix knows to update the slice. Addresses PR #19 review nit. --- internal/connector/test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/connector/test.go b/internal/connector/test.go index 421a1e8..0e47cc4 100644 --- a/internal/connector/test.go +++ b/internal/connector/test.go @@ -102,6 +102,12 @@ func configFromGetConnector(raw map[string]any) (map[string]any, error) { // GetConnector redacts secrets, so fill a placeholder for any // required secret field. The server returns the driver's auth // failure as a 200 `{error: ...}` which the CLI surfaces as FAIL:. + // NOTE: only "password" is placeholdered today. Postgres uses it; + // Snowflake's non-password auth modes (keypair, oauth-sso, + // oauth-individual) use privateKey / oauthClientSecret and will + // still surface a driver-side auth error via the same FAIL: path + // — extend this slice when adding a dialect whose TestConnector + // body requires a different secret field name. for _, secret := range []string{"password"} { if _, present := out[secret]; !present { out[secret] = "redacted"