stuff #2

Open
Marti wants to merge 23 commits from dev into main
Owner
No description provided.
First-time contributor
  1. Executive Summary
    This PR refactors the authentication flow by extracting JWT middleware into a dedicated middleware/auth.go package, introduces a new social profiles feature with corresponding database migrations, handlers, and queries, and improves route organization using Echo's group middleware. It also upgrades database dependencies, standardizes SQL formatting, and fixes a historical response typo in collection creation.

  2. Critical Issues (High Priority)

  • Nil Pointer Dereference in services/users.go: The line HasLoggedIn: *db_user.HasLoggedIn, directly dereferences a *bool from the database. If the has_logged_in column is NULL, this will cause a runtime panic.
  • Incorrect HTTP Status Codes for DB Errors: In HandlePostUserSocial and HandleDeleteUserSocial, database errors are returned as http.StatusNotFound (404). A 404 semantically means the requested resource doesn't exist, but these are write/delete operations failing. This misleads clients and breaks standard API conventions.
  • Inconsistent User ID Parsing: HandleGetUserSocials bypasses the new centralized parsing logic by calling utils.ParseInt(c.Param("userId")) directly, while other handlers use m.GetPathUserID(c). This breaks consistency and could cause subtle bugs if the parsing logic or error handling changes later.
  1. Suggestions & Improvements
  • Middleware Context Inheritance: In main.go, OptionalJWT runs globally, then RequiredJWT runs on private groups. While functional, RequiredJWT's SuccessHandler will overwrite the context value set by OptionalJWT. Consider explicitly chaining OptionalJWT on priv or using a single unified auth middleware to avoid redundant JWT parsing and context overwrites.
  • Standardize Error Responses: Replace c.String(http.StatusNotFound, "Error adding social...") with echo.NewHTTPError or structured JSON responses. This improves client-side error parsing and aligns with modern REST conventions.
  • Valkey/Redis Compatibility Note: The switch to valkey/valkey in docker-compose.yaml is a good move for licensing, but add a comment in cache/init.go or docker-compose.yaml confirming that go-redis/v9 is verified against Valkey 8.x to prevent future version mismatch surprises.
  • SQL Identifier Quoting: In migrations/20260426160059_dan.sql, the column dan is unquoted. While PostgreSQL accepts it, quoting identifiers ("dan" varchar(32)) is safer practice to avoid conflicts with reserved keywords in future releases.
  • Test Coverage for New Handlers: The new social handlers (HandleGetUserSocials, HandlePostUserSocial, HandleDeleteUserSocial) lack unit tests. Adding tests for validation, success paths, and DB error handling will significantly improve reliability.
  1. Actionable Checklist
  • Fix nil pointer dereference in services/users.go by safely handling db_user.HasLoggedIn (e.g., check for nil or use a helper like utils.BoolPtr before dereferencing).
  • Change HTTP status codes in HandlePostUserSocial and HandleDeleteUserSocial from StatusNotFound to StatusInternalServerError (or map specific DB errors appropriately).
  • Refactor HandleGetUserSocials to use m.GetPathUserID(c) for consistency with other handlers.
  • Verify RequiredJWT middleware correctly handles context inheritance from OptionalJWT and doesn't cause unexpected overwrites or redundant parsing.
  • Add unit tests for the new social handlers covering valid inputs, missing parameters, and database failure scenarios.
  • Quote the dan column name in 20260426160059_dan.sql migration for safer SQL standard compliance.
  • Add a compatibility comment in docker-compose.yaml or cache/init.go noting the Redis-to-Valkey migration and confirm go-redis/v9 support.
  • Replace inline c.String() error responses with echo.NewHTTPError or JSON payloads for consistent API error formatting.
1. **Executive Summary** This PR refactors the authentication flow by extracting JWT middleware into a dedicated `middleware/auth.go` package, introduces a new social profiles feature with corresponding database migrations, handlers, and queries, and improves route organization using Echo's group middleware. It also upgrades database dependencies, standardizes SQL formatting, and fixes a historical response typo in collection creation. 2. **Critical Issues (High Priority)** - **Nil Pointer Dereference in `services/users.go`**: The line `HasLoggedIn: *db_user.HasLoggedIn,` directly dereferences a `*bool` from the database. If the `has_logged_in` column is `NULL`, this will cause a runtime panic. - **Incorrect HTTP Status Codes for DB Errors**: In `HandlePostUserSocial` and `HandleDeleteUserSocial`, database errors are returned as `http.StatusNotFound` (404). A 404 semantically means the requested resource doesn't exist, but these are write/delete operations failing. This misleads clients and breaks standard API conventions. - **Inconsistent User ID Parsing**: `HandleGetUserSocials` bypasses the new centralized parsing logic by calling `utils.ParseInt(c.Param("userId"))` directly, while other handlers use `m.GetPathUserID(c)`. This breaks consistency and could cause subtle bugs if the parsing logic or error handling changes later. 3. **Suggestions & Improvements** - **Middleware Context Inheritance**: In `main.go`, `OptionalJWT` runs globally, then `RequiredJWT` runs on private groups. While functional, `RequiredJWT`'s `SuccessHandler` will overwrite the context value set by `OptionalJWT`. Consider explicitly chaining `OptionalJWT` on `priv` or using a single unified auth middleware to avoid redundant JWT parsing and context overwrites. - **Standardize Error Responses**: Replace `c.String(http.StatusNotFound, "Error adding social...")` with `echo.NewHTTPError` or structured JSON responses. This improves client-side error parsing and aligns with modern REST conventions. - **Valkey/Redis Compatibility Note**: The switch to `valkey/valkey` in `docker-compose.yaml` is a good move for licensing, but add a comment in `cache/init.go` or `docker-compose.yaml` confirming that `go-redis/v9` is verified against Valkey 8.x to prevent future version mismatch surprises. - **SQL Identifier Quoting**: In `migrations/20260426160059_dan.sql`, the column `dan` is unquoted. While PostgreSQL accepts it, quoting identifiers (`"dan" varchar(32)`) is safer practice to avoid conflicts with reserved keywords in future releases. - **Test Coverage for New Handlers**: The new social handlers (`HandleGetUserSocials`, `HandlePostUserSocial`, `HandleDeleteUserSocial`) lack unit tests. Adding tests for validation, success paths, and DB error handling will significantly improve reliability. 4. **Actionable Checklist** - [ ] Fix nil pointer dereference in `services/users.go` by safely handling `db_user.HasLoggedIn` (e.g., check for nil or use a helper like `utils.BoolPtr` before dereferencing). - [ ] Change HTTP status codes in `HandlePostUserSocial` and `HandleDeleteUserSocial` from `StatusNotFound` to `StatusInternalServerError` (or map specific DB errors appropriately). - [ ] Refactor `HandleGetUserSocials` to use `m.GetPathUserID(c)` for consistency with other handlers. - [ ] Verify `RequiredJWT` middleware correctly handles context inheritance from `OptionalJWT` and doesn't cause unexpected overwrites or redundant parsing. - [ ] Add unit tests for the new social handlers covering valid inputs, missing parameters, and database failure scenarios. - [ ] Quote the `dan` column name in `20260426160059_dan.sql` migration for safer SQL standard compliance. - [ ] Add a compatibility comment in `docker-compose.yaml` or `cache/init.go` noting the Redis-to-Valkey migration and confirm `go-redis/v9` support. - [ ] Replace inline `c.String()` error responses with `echo.NewHTTPError` or JSON payloads for consistent API error formatting.
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin dev:dev
git switch dev
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
wysi/server!2
No description provided.