From ce32f7cf5cf11b404023a9d82cd5148740109c03 Mon Sep 17 00:00:00 2001 From: Andrea Grandi Date: Thu, 21 May 2026 21:46:15 +0200 Subject: [PATCH] Classify JSON errors from typed errors instead of message substrings Add an internal/mberr package with typed errors for config, API, auth, timeout, cancellation, resource requests, name resolution, and parameterized queries. classifyError now inspects error types via errors.As rather than matching message substrings, so structured --error-format json output stays reliable when wording changes. Closes #12 --- CHANGELOG.md | 1 + internal/cli/card.go | 17 +++--- internal/cli/query.go | 26 ++++++++-- internal/cli/root.go | 91 ++++++++++++++++++++++---------- internal/client/cards.go | 8 ++- internal/client/client.go | 7 +-- internal/client/dashboards.go | 14 ++++- internal/config/config.go | 9 ++-- internal/mberr/errors.go | 93 +++++++++++++++++++++++++++++++++ tests/error_format_test.go | 98 ++++++++++++++++++++++++++++------- 10 files changed, 298 insertions(+), 66 deletions(-) create mode 100644 internal/mberr/errors.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 617cd91..c6d377f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## [Unreleased] +- Classify `--error-format json` errors from typed errors instead of message substrings, making structured error output reliable when wording changes (#12) - Add `--timeout` flag and propagate request contexts so commands abort cleanly on Ctrl+C, with clear timeout and cancellation errors (#11) - Add `MB_SESSION_TOKEN` as an alternative authentication method for users without admin access to mint an API key; mutually exclusive with `MB_API_KEY` (#9) - Document changelog update workflow in `AGENTS.md` and add a pull request template prompting contributors to update `CHANGELOG.md` for user-visible changes (#20) diff --git a/internal/cli/card.go b/internal/cli/card.go index c162a81..b52f763 100644 --- a/internal/cli/card.go +++ b/internal/cli/card.go @@ -1,13 +1,16 @@ package cli import ( + "errors" "fmt" + "net/http" "os" "strconv" "strings" "github.com/andreagrandi/mb-cli/internal/client" "github.com/andreagrandi/mb-cli/internal/formatter" + "github.com/andreagrandi/mb-cli/internal/mberr" "github.com/spf13/cobra" ) @@ -179,12 +182,14 @@ func formatQueryResultOutput(cmd *cobra.Command, result *client.QueryResult) err } func wrapParameterizedRunError(err error) error { - message := err.Error() - if strings.Contains(message, "API request failed with status 400") { - return fmt.Errorf("parameterized query failed: check parameter keys and values (%w)", err) - } - if strings.Contains(message, "API request failed with status 404") { - return fmt.Errorf("query target was not found (%w)", err) + var apiErr *mberr.APIError + if errors.As(err, &apiErr) { + switch apiErr.StatusCode { + case http.StatusBadRequest: + return &mberr.ParameterizedQueryError{Err: err} + case http.StatusNotFound: + return fmt.Errorf("query target was not found (%w)", err) + } } return err } diff --git a/internal/cli/query.go b/internal/cli/query.go index 1958d02..4e6f06c 100644 --- a/internal/cli/query.go +++ b/internal/cli/query.go @@ -9,6 +9,7 @@ import ( "github.com/andreagrandi/mb-cli/internal/client" "github.com/andreagrandi/mb-cli/internal/formatter" + "github.com/andreagrandi/mb-cli/internal/mberr" "github.com/andreagrandi/mb-cli/internal/validation" "github.com/spf13/cobra" ) @@ -237,7 +238,10 @@ func matchTableByName(tables []client.TableMetadata, name string) (int, error) { switch len(matches) { case 0: - return 0, fmt.Errorf("no table matching '%s' found", name) + return 0, &mberr.ResolutionError{ + Kind: mberr.ResourceTable, + Message: fmt.Sprintf("no table matching '%s' found", name), + } case 1: return matches[0].ID, nil default: @@ -245,7 +249,10 @@ func matchTableByName(tables []client.TableMetadata, name string) (int, error) { for i, t := range matches { names[i] = fmt.Sprintf("%s (id=%d)", t.Name, t.ID) } - return 0, fmt.Errorf("ambiguous table name '%s', matches: %s. Use table ID instead", name, strings.Join(names, ", ")) + return 0, &mberr.ResolutionError{ + Kind: mberr.ResourceTable, + Message: fmt.Sprintf("ambiguous table name '%s', matches: %s. Use table ID instead", name, strings.Join(names, ", ")), + } } } @@ -270,7 +277,10 @@ func resolveFieldID(fields []client.Field, name string) (int, error) { return f.ID, nil } } - return 0, fmt.Errorf("no field matching '%s' found in table", name) + return 0, &mberr.ResolutionError{ + Kind: mberr.ResourceField, + Message: fmt.Sprintf("no field matching '%s' found in table", name), + } } func matchDatabaseByName(databases []client.Database, name string) (int, error) { @@ -284,7 +294,10 @@ func matchDatabaseByName(databases []client.Database, name string) (int, error) switch len(matches) { case 0: - return 0, fmt.Errorf("no database matching '%s' found", name) + return 0, &mberr.ResolutionError{ + Kind: mberr.ResourceDatabase, + Message: fmt.Sprintf("no database matching '%s' found", name), + } case 1: return matches[0].ID, nil default: @@ -292,6 +305,9 @@ func matchDatabaseByName(databases []client.Database, name string) (int, error) for i, db := range matches { names[i] = fmt.Sprintf("%s (id=%d)", db.Name, db.ID) } - return 0, fmt.Errorf("ambiguous database name '%s', matches: %s. Use database ID instead", name, strings.Join(names, ", ")) + return 0, &mberr.ResolutionError{ + Kind: mberr.ResourceDatabase, + Message: fmt.Sprintf("ambiguous database name '%s', matches: %s. Use database ID instead", name, strings.Join(names, ", ")), + } } } diff --git a/internal/cli/root.go b/internal/cli/root.go index b5e9519..ea93b80 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -3,13 +3,15 @@ package cli import ( "context" "encoding/json" + "errors" "fmt" + "net/http" "os" "os/signal" - "strings" "syscall" "time" + "github.com/andreagrandi/mb-cli/internal/mberr" "github.com/andreagrandi/mb-cli/internal/version" "github.com/spf13/cobra" ) @@ -90,39 +92,74 @@ func ClassifyError(err error) (errorType, suggestion string) { return classifyError(err) } +// classifyError inspects the error's type (not its message text) to determine +// the structured error type and an actionable suggestion. func classifyError(err error) (errorType, suggestion string) { - msg := err.Error() - - switch { - case strings.Contains(msg, "MB_HOST") || strings.Contains(msg, "MB_API_KEY"): + var configErr *mberr.ConfigError + if errors.As(err, &configErr) { return "CONFIG_ERROR", "Set MB_HOST and MB_API_KEY environment variables" - case strings.Contains(msg, "request timed out"): + } + + var timeoutErr *mberr.TimeoutError + if errors.As(err, &timeoutErr) { return "TIMEOUT_ERROR", "Increase --timeout or check connectivity to MB_HOST" - case strings.Contains(msg, "request canceled"): + } + + var canceledErr *mberr.CanceledError + if errors.As(err, &canceledErr) { return "CANCELED_ERROR", "" - case strings.Contains(msg, "parameterized query failed"): + } + + var paramErr *mberr.ParameterizedQueryError + if errors.As(err, ¶mErr) { return "API_ERROR", "Check parameter IDs with 'mb-cli dashboard get ' or 'mb-cli card get --full'" - case strings.Contains(msg, "API request failed with status 401"), - strings.Contains(msg, "API request failed with status 403"): - return "AUTH_ERROR", "Check that MB_API_KEY is valid and can access the requested resource" - case strings.Contains(msg, "failed to get dashboard") && strings.Contains(msg, "status 404"): - return "API_ERROR", "Check that the dashboard ID exists and is visible to this API key" - case strings.Contains(msg, "failed to get card") && strings.Contains(msg, "status 404"): - return "API_ERROR", "Check that the card ID exists and is visible to this API key" - case strings.Contains(msg, "failed to get values for dashboard") && strings.Contains(msg, "status 404"): - return "API_ERROR", "Check that the dashboard parameter ID exists for this dashboard" - case strings.Contains(msg, "API request failed with status"): + } + + var resolutionErr *mberr.ResolutionError + if errors.As(err, &resolutionErr) { + return "RESOLUTION_ERROR", resolutionSuggestion(resolutionErr.Kind) + } + + var apiErr *mberr.APIError + if errors.As(err, &apiErr) { + if apiErr.IsAuth() { + return "AUTH_ERROR", "Check that MB_API_KEY is valid and can access the requested resource" + } + var reqErr *mberr.RequestError + if apiErr.StatusCode == http.StatusNotFound && errors.As(err, &reqErr) { + return "API_ERROR", notFoundSuggestion(reqErr.Resource) + } return "API_ERROR", "" - case strings.Contains(msg, "no database matching"), - strings.Contains(msg, "ambiguous database name"): - return "RESOLUTION_ERROR", "Use a database ID instead of a name" - case strings.Contains(msg, "no table matching"), - strings.Contains(msg, "ambiguous table name"): - return "RESOLUTION_ERROR", "Use a table ID instead of a name" - case strings.Contains(msg, "no field matching"): - return "RESOLUTION_ERROR", "Check field names with 'mb-cli table metadata '" + } + + return "GENERAL_ERROR", "" +} + +// resolutionSuggestion returns advice for a name-to-ID resolution failure. +func resolutionSuggestion(kind mberr.ResourceKind) string { + switch kind { + case mberr.ResourceDatabase: + return "Use a database ID instead of a name" + case mberr.ResourceTable: + return "Use a table ID instead of a name" + case mberr.ResourceField: + return "Check field names with 'mb-cli table metadata '" + default: + return "" + } +} + +// notFoundSuggestion returns advice for a 404 on a specific resource request. +func notFoundSuggestion(kind mberr.ResourceKind) string { + switch kind { + case mberr.ResourceDashboard: + return "Check that the dashboard ID exists and is visible to this API key" + case mberr.ResourceCard: + return "Check that the card ID exists and is visible to this API key" + case mberr.ResourceDashboardParameter: + return "Check that the dashboard parameter ID exists for this dashboard" default: - return "GENERAL_ERROR", "" + return "" } } diff --git a/internal/client/cards.go b/internal/client/cards.go index 7b994aa..ed95cc1 100644 --- a/internal/client/cards.go +++ b/internal/client/cards.go @@ -5,6 +5,8 @@ import ( "fmt" "net/http" "net/url" + + "github.com/andreagrandi/mb-cli/internal/mberr" ) // ListCards retrieves all saved questions (cards). @@ -29,7 +31,11 @@ func (c *Client) GetCard(ctx context.Context, id int) (*Card, error) { resp, err := c.Get(ctx, fmt.Sprintf("/api/card/%d", id), params) if err != nil { - return nil, fmt.Errorf("failed to get card %d: %w", id, err) + return nil, &mberr.RequestError{ + Resource: mberr.ResourceCard, + Op: fmt.Sprintf("failed to get card %d", id), + Err: err, + } } var card Card diff --git a/internal/client/client.go b/internal/client/client.go index c9ac6ec..c265241 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -12,6 +12,7 @@ import ( "os" "github.com/andreagrandi/mb-cli/internal/config" + "github.com/andreagrandi/mb-cli/internal/mberr" "github.com/andreagrandi/mb-cli/internal/version" ) @@ -69,7 +70,7 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { if resp.StatusCode >= 400 { defer resp.Body.Close() body, _ := io.ReadAll(resp.Body) - return nil, fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, string(body)) + return nil, &mberr.APIError{StatusCode: resp.StatusCode, Body: string(body)} } return resp, nil @@ -80,9 +81,9 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { func requestError(ctx context.Context, err error) error { switch { case errors.Is(err, context.DeadlineExceeded), ctx.Err() == context.DeadlineExceeded: - return fmt.Errorf("request timed out: %w", err) + return &mberr.TimeoutError{Err: err} case errors.Is(err, context.Canceled), ctx.Err() == context.Canceled: - return fmt.Errorf("request canceled: %w", err) + return &mberr.CanceledError{Err: err} default: return fmt.Errorf("http request failed: %w", err) } diff --git a/internal/client/dashboards.go b/internal/client/dashboards.go index 0cda110..0f68c8d 100644 --- a/internal/client/dashboards.go +++ b/internal/client/dashboards.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "net/url" + + "github.com/andreagrandi/mb-cli/internal/mberr" ) // ListDashboards retrieves all dashboards. @@ -25,7 +27,11 @@ func (c *Client) ListDashboards(ctx context.Context) ([]Dashboard, error) { func (c *Client) GetDashboard(ctx context.Context, id int) (*Dashboard, error) { resp, err := c.Get(ctx, fmt.Sprintf("/api/dashboard/%d", id), nil) if err != nil { - return nil, fmt.Errorf("failed to get dashboard %d: %w", id, err) + return nil, &mberr.RequestError{ + Resource: mberr.ResourceDashboard, + Op: fmt.Sprintf("failed to get dashboard %d", id), + Err: err, + } } var dashboard Dashboard @@ -40,7 +46,11 @@ func (c *Client) GetDashboard(ctx context.Context, id int) (*Dashboard, error) { func (c *Client) GetDashboardParamValues(ctx context.Context, dashboardID int, paramKey string) (*ParameterValues, error) { resp, err := c.Get(ctx, fmt.Sprintf("/api/dashboard/%d/params/%s/values", dashboardID, url.PathEscape(paramKey)), nil) if err != nil { - return nil, fmt.Errorf("failed to get values for dashboard %d parameter %s: %w", dashboardID, paramKey, err) + return nil, &mberr.RequestError{ + Resource: mberr.ResourceDashboardParameter, + Op: fmt.Sprintf("failed to get values for dashboard %d parameter %s", dashboardID, paramKey), + Err: err, + } } var values ParameterValues diff --git a/internal/config/config.go b/internal/config/config.go index 99027b2..2f6390a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,8 +1,9 @@ package config import ( - "fmt" "os" + + "github.com/andreagrandi/mb-cli/internal/mberr" ) type Config struct { @@ -14,18 +15,18 @@ type Config struct { func LoadConfig() (*Config, error) { host := os.Getenv("MB_HOST") if host == "" { - return nil, fmt.Errorf("MB_HOST environment variable is required") + return nil, &mberr.ConfigError{Message: "MB_HOST environment variable is required"} } apiKey := os.Getenv("MB_API_KEY") sessionToken := os.Getenv("MB_SESSION_TOKEN") if apiKey == "" && sessionToken == "" { - return nil, fmt.Errorf("either MB_API_KEY or MB_SESSION_TOKEN environment variable is required") + return nil, &mberr.ConfigError{Message: "either MB_API_KEY or MB_SESSION_TOKEN environment variable is required"} } if apiKey != "" && sessionToken != "" { - return nil, fmt.Errorf("MB_API_KEY and MB_SESSION_TOKEN are mutually exclusive, set only one") + return nil, &mberr.ConfigError{Message: "MB_API_KEY and MB_SESSION_TOKEN are mutually exclusive, set only one"} } return &Config{ diff --git a/internal/mberr/errors.go b/internal/mberr/errors.go new file mode 100644 index 0000000..9ce0bdf --- /dev/null +++ b/internal/mberr/errors.go @@ -0,0 +1,93 @@ +// Package mberr defines typed errors shared across mb-cli so that failures can +// be classified from their type rather than by matching message substrings. +package mberr + +import ( + "fmt" + "net/http" +) + +// ResourceKind identifies the kind of Metabase resource a failure relates to. +type ResourceKind string + +const ( + ResourceDatabase ResourceKind = "database" + ResourceTable ResourceKind = "table" + ResourceField ResourceKind = "field" + ResourceDashboard ResourceKind = "dashboard" + ResourceCard ResourceKind = "card" + ResourceDashboardParameter ResourceKind = "dashboard parameter" +) + +// ConfigError indicates missing or invalid configuration, such as required +// environment variables not being set. +type ConfigError struct { + Message string +} + +func (e *ConfigError) Error() string { return e.Message } + +// APIError represents a non-2xx response from the Metabase API. +type APIError struct { + StatusCode int + Body string +} + +func (e *APIError) Error() string { + return fmt.Sprintf("API request failed with status %d: %s", e.StatusCode, e.Body) +} + +// IsAuth reports whether the response indicates an authentication or +// authorization failure. +func (e *APIError) IsAuth() bool { + return e.StatusCode == http.StatusUnauthorized || e.StatusCode == http.StatusForbidden +} + +// TimeoutError indicates an API request that exceeded its deadline. +type TimeoutError struct { + Err error +} + +func (e *TimeoutError) Error() string { return fmt.Sprintf("request timed out: %v", e.Err) } +func (e *TimeoutError) Unwrap() error { return e.Err } + +// CanceledError indicates an API request canceled before it completed. +type CanceledError struct { + Err error +} + +func (e *CanceledError) Error() string { return fmt.Sprintf("request canceled: %v", e.Err) } +func (e *CanceledError) Unwrap() error { return e.Err } + +// RequestError wraps a failed request for a specific Metabase resource, +// preserving which resource was targeted so the failure can be given +// resource-specific guidance. +type RequestError struct { + Resource ResourceKind + Op string + Err error +} + +func (e *RequestError) Error() string { return e.Op + ": " + e.Err.Error() } +func (e *RequestError) Unwrap() error { return e.Err } + +// ResolutionError indicates a failure to resolve a name to a resource ID, +// because no resource matched or the name was ambiguous. +type ResolutionError struct { + Kind ResourceKind + Message string +} + +func (e *ResolutionError) Error() string { return e.Message } + +// ParameterizedQueryError indicates a parameterized query was rejected by the +// API, typically because parameter keys or values were invalid. +type ParameterizedQueryError struct { + Err error +} + +func (e *ParameterizedQueryError) Error() string { + return fmt.Sprintf("parameterized query failed: check parameter keys and values (%v)", e.Err) +} + +func (e *ParameterizedQueryError) Unwrap() error { return e.Err } diff --git a/tests/error_format_test.go b/tests/error_format_test.go index 75ebf82..e4b058c 100644 --- a/tests/error_format_test.go +++ b/tests/error_format_test.go @@ -1,98 +1,160 @@ package tests import ( + "context" + "errors" "fmt" "testing" "github.com/andreagrandi/mb-cli/internal/cli" + "github.com/andreagrandi/mb-cli/internal/mberr" ) func TestClassifyConfigError(t *testing.T) { tests := []struct { name string - errMsg string + err error expectedType string hasSuggestion bool }{ { name: "missing MB_HOST", - errMsg: "MB_HOST is required", + err: &mberr.ConfigError{Message: "MB_HOST environment variable is required"}, expectedType: "CONFIG_ERROR", hasSuggestion: true, }, { name: "missing MB_API_KEY", - errMsg: "MB_API_KEY is required", + err: &mberr.ConfigError{Message: "either MB_API_KEY or MB_SESSION_TOKEN environment variable is required"}, expectedType: "CONFIG_ERROR", hasSuggestion: true, }, { name: "auth 401", - errMsg: "API request failed with status 401: Unauthorized", + err: &mberr.APIError{StatusCode: 401, Body: "Unauthorized"}, expectedType: "AUTH_ERROR", hasSuggestion: true, }, { name: "auth 403", - errMsg: "API request failed with status 403: Forbidden", + err: &mberr.APIError{StatusCode: 403, Body: "Forbidden"}, expectedType: "AUTH_ERROR", hasSuggestion: true, }, { - name: "dashboard not found", - errMsg: "failed to get dashboard 298: API request failed with status 404: Not found", + name: "dashboard not found", + err: &mberr.RequestError{ + Resource: mberr.ResourceDashboard, + Op: "failed to get dashboard 298", + Err: &mberr.APIError{StatusCode: 404, Body: "Not found"}, + }, + expectedType: "API_ERROR", + hasSuggestion: true, + }, + { + name: "card not found", + err: &mberr.RequestError{ + Resource: mberr.ResourceCard, + Op: "failed to get card 5", + Err: &mberr.APIError{StatusCode: 404, Body: "Not found"}, + }, + expectedType: "API_ERROR", + hasSuggestion: true, + }, + { + name: "dashboard parameter not found", + err: &mberr.RequestError{ + Resource: mberr.ResourceDashboardParameter, + Op: "failed to get values for dashboard 1 parameter region", + Err: &mberr.APIError{StatusCode: 404, Body: "Not found"}, + }, expectedType: "API_ERROR", hasSuggestion: true, }, { name: "parameterized query failure", - errMsg: "parameterized query failed: check parameter keys and values (API request failed with status 400: bad request)", + err: &mberr.ParameterizedQueryError{Err: &mberr.APIError{StatusCode: 400, Body: "bad request"}}, expectedType: "API_ERROR", hasSuggestion: true, }, { name: "request timeout", - errMsg: "failed to list databases: request timed out: context deadline exceeded", + err: &mberr.TimeoutError{Err: context.DeadlineExceeded}, expectedType: "TIMEOUT_ERROR", hasSuggestion: true, }, { name: "request canceled", - errMsg: "request canceled: context canceled", + err: &mberr.CanceledError{Err: context.Canceled}, expectedType: "CANCELED_ERROR", }, { name: "api 404", - errMsg: "API request failed with status 404: Not Found", + err: &mberr.APIError{StatusCode: 404, Body: "Not Found"}, expectedType: "API_ERROR", }, { name: "api 500", - errMsg: "API request failed with status 500: Internal Server Error", + err: &mberr.APIError{StatusCode: 500, Body: "Internal Server Error"}, expectedType: "API_ERROR", }, { - name: "no database match", - errMsg: "no database matching 'foo' found", + name: "no database match", + err: &mberr.ResolutionError{ + Kind: mberr.ResourceDatabase, + Message: "no database matching 'foo' found", + }, + expectedType: "RESOLUTION_ERROR", + hasSuggestion: true, + }, + { + name: "ambiguous database", + err: &mberr.ResolutionError{ + Kind: mberr.ResourceDatabase, + Message: "ambiguous database name 'prod', matches: Production (id=1), Prod-staging (id=2). Use database ID instead", + }, expectedType: "RESOLUTION_ERROR", hasSuggestion: true, }, { - name: "ambiguous database", - errMsg: "ambiguous database name 'prod', matches: Production (id=1), Prod-staging (id=2). Use database ID instead", + name: "no table match", + err: &mberr.ResolutionError{ + Kind: mberr.ResourceTable, + Message: "no table matching 'bar' found", + }, expectedType: "RESOLUTION_ERROR", hasSuggestion: true, }, + { + name: "no field match", + err: &mberr.ResolutionError{ + Kind: mberr.ResourceField, + Message: "no field matching 'baz' found in table", + }, + expectedType: "RESOLUTION_ERROR", + hasSuggestion: true, + }, + { + name: "wrapped api error", + err: fmt.Errorf("failed to list databases: %w", &mberr.APIError{StatusCode: 500, Body: "boom"}), + expectedType: "API_ERROR", + }, + { + name: "wrapped timeout error", + err: fmt.Errorf("failed to list databases: %w", &mberr.TimeoutError{Err: context.DeadlineExceeded}), + expectedType: "TIMEOUT_ERROR", + hasSuggestion: true, + }, { name: "generic error", - errMsg: "something went wrong", + err: errors.New("something went wrong"), expectedType: "GENERAL_ERROR", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - errType, suggestion := cli.ClassifyError(fmt.Errorf("%s", tt.errMsg)) + errType, suggestion := cli.ClassifyError(tt.err) if errType != tt.expectedType { t.Errorf("expected type %s, got %s", tt.expectedType, errType)