From 5addfa4d2b58710d80a121d15361dd01f173ec9b Mon Sep 17 00:00:00 2001 From: Andrey Markelov Date: Sat, 27 Jun 2026 21:12:59 -0700 Subject: [PATCH] Fix error comparison and reflection robustness in share-link commands Replace fragile interface pointer comparisons with reflection-based samePointer helper, migrate error assertions to errors.Is for correct wrapped-error handling, and extract ErrorSummary via direct field reflection to avoid CanInterface pitfalls. --- cmd/json_contract_test.go | 6 +++--- cmd/output.go | 17 +++++++++++++---- cmd/put_test.go | 6 ------ cmd/share_create_link_test.go | 3 ++- cmd/share_link_download.go | 6 +++--- cmd/share_link_download_test.go | 3 ++- cmd/share_link_info_test.go | 3 ++- cmd/share_link_raw.go | 20 ++++++++++++++++++-- cmd/share_link_raw_test.go | 20 +++++++++----------- cmd/share_link_revoke.go | 6 +++--- cmd/share_link_revoke_test.go | 7 +++++-- cmd/share_link_update_test.go | 3 ++- 12 files changed, 62 insertions(+), 38 deletions(-) diff --git a/cmd/json_contract_test.go b/cmd/json_contract_test.go index 18c9bde..cea1f1c 100644 --- a/cmd/json_contract_test.go +++ b/cmd/json_contract_test.go @@ -541,11 +541,11 @@ func assertGoldenJSONEqual(t *testing.T, command string, fixture json.RawMessage func assertGoldenSuccessOutputStatuses(t *testing.T, command string, fixture json.RawMessage) { t.Helper() - var output jsonOperationOutput - if err := json.Unmarshal(fixture, &output); err != nil { + var got jsonOperationOutput + if err := json.Unmarshal(fixture, &got); err != nil { t.Fatalf("decode golden output for %q: %v", command, err) } - for i, result := range output.Results { + for i, result := range got.Results { if result.Status == "" { t.Errorf("golden output for %q result %d has empty status", command, i) } diff --git a/cmd/output.go b/cmd/output.go index 284c629..1c28df4 100644 --- a/cmd/output.go +++ b/cmd/output.go @@ -470,10 +470,8 @@ func dropboxAPIErrorSummaryValue(err error) (string, bool) { } field := value.FieldByName("APIError") - if field.IsValid() && field.CanInterface() { - if apiErr, ok := field.Interface().(dropbox.APIError); ok { - return apiErr.Error(), true - } + if summary, ok := dropboxAPIErrorFieldSummary(field); ok { + return summary, true } if strings.HasSuffix(typ.Name(), "APIError") { return err.Error(), true @@ -481,6 +479,17 @@ func dropboxAPIErrorSummaryValue(err error) (string, bool) { return "", false } +func dropboxAPIErrorFieldSummary(field reflect.Value) (string, bool) { + if !field.IsValid() || field.Type() != reflect.TypeOf(dropbox.APIError{}) { + return "", false + } + summary := field.FieldByName("ErrorSummary") + if !summary.IsValid() || summary.Kind() != reflect.String { + return "", false + } + return summary.String(), true +} + func dropboxAPISummaryFromMessage(message string) (string, bool) { lower := strings.ToLower(message) if strings.Contains(lower, "error in call to api function") { diff --git a/cmd/put_test.go b/cmd/put_test.go index 3049e35..6ee68ec 100644 --- a/cmd/put_test.go +++ b/cmd/put_test.go @@ -1414,12 +1414,6 @@ func TestPutRecursiveIfExistsSkipContinuesPastExistingFiles(t *testing.T) { } } -func TestSingleShotUploadSizeCutoff(t *testing.T) { - if singleShotUploadSizeCutoff != 32*1024*1024 { - t.Errorf("singleShotUploadSizeCutoff = %d, want %d", singleShotUploadSizeCutoff, 32*1024*1024) - } -} - func TestUploadChunked_RetriesSessionStart(t *testing.T) { stubRetrySleep(t) startCalls := 0 diff --git a/cmd/share_create_link_test.go b/cmd/share_create_link_test.go index 8af907d..4b79536 100644 --- a/cmd/share_create_link_test.go +++ b/cmd/share_create_link_test.go @@ -16,6 +16,7 @@ package cmd import ( "bytes" + "errors" "fmt" "io" "path" @@ -753,7 +754,7 @@ func TestSharedLinkCreateReturnsNonAlreadyExistsError(t *testing.T) { stubSharedLinkClient(t, mock) err := shareLinkCreate(&cobra.Command{}, []string{"/docs"}) - if err != wantErr { + if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want original error", err) } } diff --git a/cmd/share_link_download.go b/cmd/share_link_download.go index 6a75cb3..c2a3f68 100644 --- a/cmd/share_link_download.go +++ b/cmd/share_link_download.go @@ -152,14 +152,14 @@ func parseShareLinkDownloadOptions(cmd *cobra.Command) (shareLinkDownloadOptions if pathArg == "" { return opts, invalidArgumentsErrorWithDetails("`--path` requires a non-empty path", flagErrorDetails("path")) } - path, err := validatePath(pathArg) + dropboxPath, err := validatePath(pathArg) if err != nil { return opts, err } - if path == "" { + if dropboxPath == "" { return opts, invalidArgumentsErrorWithDetails("cannot download shared-link root with `--path`", pathErrorDetails("/")) } - opts.path = path + opts.path = dropboxPath } if opts.path != "" && opts.recursive { diff --git a/cmd/share_link_download_test.go b/cmd/share_link_download_test.go index b1dd4a6..47f7804 100644 --- a/cmd/share_link_download_test.go +++ b/cmd/share_link_download_test.go @@ -16,6 +16,7 @@ package cmd import ( "bytes" + "errors" "fmt" "io" "os" @@ -777,7 +778,7 @@ func TestShareLinkDownloadReturnsAPIErrors(t *testing.T) { }) err := shareLinkDownload(newShareLinkDownloadTestCommand(nil, nil), []string{"https://example.com/link", "-"}) - if err != wantErr { + if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want original API error", err) } } diff --git a/cmd/share_link_info_test.go b/cmd/share_link_info_test.go index c009f90..8362dc4 100644 --- a/cmd/share_link_info_test.go +++ b/cmd/share_link_info_test.go @@ -2,6 +2,7 @@ package cmd import ( "bytes" + "errors" "fmt" "strings" "testing" @@ -213,7 +214,7 @@ func TestShareLinkInfoReturnsAPIError(t *testing.T) { }) err := shareLinkInfo(&cobra.Command{}, []string{"https://www.dropbox.com/s/abc123"}) - if err != wantErr { + if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want API error", err) } } diff --git a/cmd/share_link_raw.go b/cmd/share_link_raw.go index fdd4e02..0935863 100644 --- a/cmd/share_link_raw.go +++ b/cmd/share_link_raw.go @@ -18,6 +18,7 @@ import ( "encoding/json" "fmt" "io" + "reflect" "time" "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox" @@ -108,7 +109,7 @@ func executeSharingRawRequest(cfg dropbox.Config, req dropbox.Request, parseErro func parseCreateSharedLinkWithSettingsError(err error) error { var appErr sharing.CreateSharedLinkWithSettingsAPIError parsed := auth.ParseError(err, &appErr) - if parsed == &appErr { + if samePointer(parsed, &appErr) { return appErr } return parsed @@ -117,12 +118,27 @@ func parseCreateSharedLinkWithSettingsError(err error) error { func parseModifySharedLinkSettingsError(err error) error { var appErr sharing.ModifySharedLinkSettingsAPIError parsed := auth.ParseError(err, &appErr) - if parsed == &appErr { + if samePointer(parsed, &appErr) { return appErr } return parsed } +func samePointer(value, target any) bool { + valueRef := reflect.ValueOf(value) + targetRef := reflect.ValueOf(target) + if !valueRef.IsValid() || !targetRef.IsValid() { + return false + } + if valueRef.Kind() != reflect.Ptr || targetRef.Kind() != reflect.Ptr { + return false + } + if valueRef.Type() != targetRef.Type() { + return false + } + return valueRef.Pointer() == targetRef.Pointer() +} + func parseSharedLinkMetadata(resp []byte) (sharing.IsSharedLinkMetadata, error) { var tagged struct { Tag string `json:".tag"` diff --git a/cmd/share_link_raw_test.go b/cmd/share_link_raw_test.go index acc4089..5523034 100644 --- a/cmd/share_link_raw_test.go +++ b/cmd/share_link_raw_test.go @@ -18,6 +18,7 @@ import ( "encoding/json" "io" "net/http" + "reflect" "strings" "testing" @@ -183,12 +184,7 @@ func TestRawCreateSharedLinkErrorParserReturnsValueError(t *testing.T) { Content: `{ "error_summary": "shared_link_already_exists/", "error": { ".tag": "shared_link_already_exists" } }`, }) - if _, ok := err.(sharing.CreateSharedLinkWithSettingsAPIError); !ok { - t.Fatalf("error type = %T, want value CreateSharedLinkWithSettingsAPIError", err) - } - if _, ok := err.(*sharing.CreateSharedLinkWithSettingsAPIError); ok { - t.Fatalf("error type = %T, want non-pointer CreateSharedLinkWithSettingsAPIError", err) - } + requireExactErrorType(t, err, sharing.CreateSharedLinkWithSettingsAPIError{}) } func TestRawModifySharedLinkErrorParserReturnsValueError(t *testing.T) { @@ -197,10 +193,12 @@ func TestRawModifySharedLinkErrorParserReturnsValueError(t *testing.T) { Content: `{ "error_summary": "settings_error/", "error": { ".tag": "settings_error" } }`, }) - if _, ok := err.(sharing.ModifySharedLinkSettingsAPIError); !ok { - t.Fatalf("error type = %T, want value ModifySharedLinkSettingsAPIError", err) - } - if _, ok := err.(*sharing.ModifySharedLinkSettingsAPIError); ok { - t.Fatalf("error type = %T, want non-pointer ModifySharedLinkSettingsAPIError", err) + requireExactErrorType(t, err, sharing.ModifySharedLinkSettingsAPIError{}) +} + +func requireExactErrorType(t *testing.T, err error, want any) { + t.Helper() + if got, want := reflect.TypeOf(err), reflect.TypeOf(want); got != want { + t.Fatalf("error type = %v, want exact type %v", got, want) } } diff --git a/cmd/share_link_revoke.go b/cmd/share_link_revoke.go index db91383..3f4de48 100644 --- a/cmd/share_link_revoke.go +++ b/cmd/share_link_revoke.go @@ -103,15 +103,15 @@ func parseShareLinkRevokeOptions(cmd *cobra.Command, args []string) (shareLinkRe return opts, invalidArgumentsErrorWithDetails("`--path` requires a non-empty path", flagErrorDetails("path")) } - path, err := validatePath(pathArg) + dropboxPath, err := validatePath(pathArg) if err != nil { return opts, err } - if path == "" { + if dropboxPath == "" { return opts, invalidArgumentsErrorWithDetails("cannot revoke shared links for Dropbox root", pathErrorDetails("/")) } - opts.path = path + opts.path = dropboxPath return opts, nil } diff --git a/cmd/share_link_revoke_test.go b/cmd/share_link_revoke_test.go index 54ef528..a7a4e75 100644 --- a/cmd/share_link_revoke_test.go +++ b/cmd/share_link_revoke_test.go @@ -119,7 +119,7 @@ func TestShareLinkRevokeReturnsAPIError(t *testing.T) { }) err := shareLinkRevoke(&cobra.Command{}, []string{"https://www.dropbox.com/s/abc123"}) - if err != wantErr { + if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want API error", err) } } @@ -319,7 +319,7 @@ func TestShareLinkRevokePathReturnsListError(t *testing.T) { } err := shareLinkRevoke(cmd, nil) - if err != wantErr { + if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want list error", err) } } @@ -343,6 +343,9 @@ func TestShareLinkRevokePathReturnsRevokeError(t *testing.T) { } err := shareLinkRevoke(cmd, nil) + if err == nil { + t.Fatal("expected wrapped revoke error") + } if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want wrapped revoke error", err) } diff --git a/cmd/share_link_update_test.go b/cmd/share_link_update_test.go index 1491d5f..5d45935 100644 --- a/cmd/share_link_update_test.go +++ b/cmd/share_link_update_test.go @@ -16,6 +16,7 @@ package cmd import ( "bytes" + "errors" "fmt" "os" "strings" @@ -619,7 +620,7 @@ func TestShareLinkUpdateReturnsAPIErrors(t *testing.T) { } err := shareLinkUpdate(cmd, []string{"https://example.com/link"}) - if err != wantErr { + if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want original API error", err) } }