Fix: handle previously ignored errors in Go backend utilities#5
Fix: handle previously ignored errors in Go backend utilities#5
Conversation
Stop discarding errors with blank identifiers in core utilities. Add proper error checking for snowflake node initialization, database URL password parsing, storage backend cleanup, and HTTP form parsing. Co-authored-by: Cursor <cursoragent@cursor.com>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if cleanupErr := k.dataStore.Delete(ctx, dataKey); cleanupErr != nil { | ||
| return 0, fmt.Errorf("cleanup failed after version check error: %w", errors.Join(err, cleanupErr)) | ||
| } | ||
| // continue - cleanup succeeded, original error is cleared |
There was a problem hiding this comment.
Original error silently dropped when cleanup succeeds
High Severity
When GetLatestAndPredecessor fails but Delete cleanup succeeds, the original error is silently discarded and execution continues with zero-valued latestKey/prevKey. The old code always returned an error in this case. Now, the function proceeds to use uninitialized data for optimistic concurrency checks, which could lead to incorrect version comparisons, data corruption, or silent acceptance of invalid writes.
Additional Locations (1)
The ':=' operator on line 58 created a new local 'node' variable inside the 'if node == nil' block, shadowing the package-level 'node' declared on line 48. This left the package-level variable permanently nil, causing every call to GenerateShortUID to re-attempt initialization and always fall back to UUID generation instead of using snowflake IDs. Fix by declaring 'err' separately with 'var err error' and using '=' for the node assignment so it writes to the package-level variable. Co-authored-by: Ryan Aznar <ryan@aznar.io>
The condition `!set` was inverted — `userInfo.Password()` returns `(password, set)` where `set` is true when a password is present. Using `!set` meant `dbCfg.Pwd` was assigned only when the password was *not* set (getting an empty string), and was skipped when a real password existed. This effectively dropped all database passwords from URL-based configurations. Changed the condition to `set` and fixed the indentation of the inner if block. Co-authored-by: Ryan Aznar <ryan@aznar.io>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| if err != nil { | ||
| fmt.Println("failed to create snowflake node:", err) | ||
| } else if node == nil { | ||
| fmt.Println("snowflake node is nil after initialization") |
There was a problem hiding this comment.
Uses fmt.Println instead of structured logger
Low Severity
Error output uses fmt.Println instead of a structured logger. The rest of the codebase (e.g., pkg/web/context.go in this same PR) uses log.New(...) for proper logging with levels, context, and structured fields. fmt.Println output goes to stdout without timestamps, log levels, or any way to filter or route it, making it inconsistent and harder to observe in production.
2 participants
Summary
Several places in the Go backend were silently discarding errors using the blank identifier (
_ = err). This PR fixes four such locations to properly handle or surface these errors.pkg/util/shortid_generator.go: Handle snowflake node initialization error instead of silently ignoring itpkg/services/sqlstore/database_config.go: Check whether the password was actually set in the database URL before assigning itpkg/storage/unified/resource/storage_backend.go: Propagate version-check errors through the cleanup path rather than discarding thempkg/web/context.go: Log HTTP form parse errors instead of silently swallowing themTest plan
pkg/util/shortid_generator_test.gopasspkg/services/sqlstore/database_config_test.gopasspkg/storage/unified/resource/storage_backend_test.gopassMade with Cursor
Note
Medium Risk
Mostly low-impact error-handling changes, but it touches request parsing and unified storage write/cleanup paths where altered error propagation could change behavior under failure conditions.
Overview
Improves error handling and observability by removing several
_ = ...patterns that previously swallowed failures.Database URL parsing now only assigns
dbCfg.Pwdwhen a password is actually present, web request form parsing logsParseForm/ParseMultipartFormfailures, andGenerateShortUIDsurfaces snowflake node init failures via stderr output.In unified storage writes, version-check failures now attempt cleanup and return a combined error if cleanup also fails, rather than always discarding cleanup errors.
Written by Cursor Bugbot for commit 1fa29ad. This will update automatically on new commits. Configure here.