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)