Add fixtures #35
Add fixtures #35
Conversation
WalkthroughThis update introduces a major refactor of the application's initialization, routing, and handler architecture. It removes legacy user management code, introduces modular route handlers serving JSON fixture data, and establishes a new middleware pipeline for authentication and authorization. The application setup is streamlined via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Middleware
participant Handler
participant Storage
Client->>Router: HTTP GET /api/{resource}
Router->>Middleware: PipelineFor(handler)
Middleware->>Middleware: UsernameCheck
Middleware->>Middleware: TokenCheck
Middleware->>Handler: Call Handle()
Handler->>Storage: Read fixture JSON
Storage-->>Handler: JSON content
Handler-->>Client: JSON response
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gocanto
force-pushed
the
feature/add-fixtures
branch
from
fd4743f to
6a75064
Compare
gocanto
force-pushed
the
feature/add-fixtures
branch
from
6a75064 to
34fdbc3
Compare
gocanto
force-pushed
the
feature/add-fixtures
branch
from
02bfe20 to
3145f5d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (8)
handler/response.go (1)
8-17: Well-structured JSON response utility.The function follows good practices by setting the correct Content-Type header, handling write errors appropriately, and providing meaningful error messages.
Consider adding a status code parameter for future flexibility:
-func writeJSON(content []byte, w baseHttp.ResponseWriter) error { +func writeJSON(content []byte, w baseHttp.ResponseWriter, statusCode ...int) error { w.Header().Set("Content-Type", "application/json") - w.WriteHeader(baseHttp.StatusOK) + + status := baseHttp.StatusOK + if len(statusCode) > 0 { + status = statusCode[0] + } + w.WriteHeader(status)This would maintain backward compatibility while allowing for error responses in the future.
pkg/http/middleware/username.go (1)
17-17: Consider making the username configurable.The hardcoded username
"gocanto"reduces flexibility and makes testing more difficult. Consider reading this from environment configuration.- if username != "gocanto" { + if username != m.Env.App.Username { // Assuming you add Username to env confighandler/talks.go (2)
14-18: Consider making the file path configurable.The hardcoded file path
"./storage/fixture/talks.json"reduces flexibility and makes testing harder. Consider accepting the path as a parameter or reading from configuration.-func MakeTalks() Talks { - return Talks{ - content: "./storage/fixture/talks.json", - } +func MakeTalks(filePath string) Talks { + return Talks{ + content: filePath, + } }
20-34: Consider validating file existence at startup.The handler reads the file on every request. Consider validating file existence during initialization to fail fast if the fixture file is missing.
Additionally, you could add a startup validation method:
+func (h Talks) Validate() error { + if _, err := os.Stat(h.content); os.IsNotExist(err) { + return fmt.Errorf("talks fixture file not found: %s", h.content) + } + return nil +}storage/fixture/experience.json (1)
15-15: Consider standardizing skills field formatting.The skills fields have inconsistent punctuation - some end with periods, others don't. Consider standardizing the format for consistency.
"skills": "Executive Leadership, Strategic Planning, Engineering Management, Cross-functional Team Leadership, Technical Architecture."Should be consistent across all entries (either all with periods or all without).
Also applies to: 28-28, 41-41, 54-54, 67-67, 80-80, 93-93
boost/helpers.go (1)
9-11: Consider passing Router by pointerTaking
Routerby value and then taking its address could be inefficient for large structs. Consider accepting a pointer instead.-func (a *App) SetRouter(router Router) { - a.router = &router +func (a *App) SetRouter(router *Router) { + a.router = routerboost/router.go (1)
31-79: Consider abstracting route registration patternThe route registration methods follow an identical pattern. Consider creating a helper method to reduce code duplication.
+func (r *Router) registerRoute(path string, handlerFactory func() interface{ Handle(baseHttp.ResponseWriter, *baseHttp.Request) *http.ApiError }) { + handler := handlerFactory() + resolver := r.PipelineFor(handler.Handle) + r.Mux.HandleFunc("GET "+path, resolver) +} + func (r *Router) Profile() { - abstract := handler.MakeProfileHandler() - - resolver := r.PipelineFor( - abstract.Handle, - ) - - r.Mux.HandleFunc("GET /profile", resolver) + r.registerRoute("/profile", func() interface{ Handle(baseHttp.ResponseWriter, *baseHttp.Request) *http.ApiError } { + return handler.MakeProfileHandler() + }) }main.go (1)
10-10: Consider avoiding global variables for better testability.Using a global
appvariable can make unit testing difficult and creates hidden dependencies.Consider returning the app from a setup function instead:
-var app *boost.App - -func init() { +func setupApp() *boost.App { secrets, validate := boost.Ignite("./.env") - app = boost.MakeApp(secrets, validate) + return boost.MakeApp(secrets, validate) } func main() { + app := setupApp() defer app.CloseDB()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.env.example(0 hunks)boost/app.go(1 hunks)boost/factory.go(3 hunks)boost/helpers.go(1 hunks)boost/ignite.go(1 hunks)boost/router.go(1 hunks)cli/main.go(1 hunks)database/seeder/main.go(3 hunks)database/seeder/seeds/factory.go(1 hunks)env/app.go(0 hunks)handler/experience.go(1 hunks)handler/profile.go(1 hunks)handler/projects.go(1 hunks)handler/response.go(1 hunks)handler/social.go(1 hunks)handler/talks.go(1 hunks)handler/user/admin.go(0 hunks)handler/user/create.go(0 hunks)handler/user/repository.go(0 hunks)handler/user/schema.go(0 hunks)main.go(1 hunks)pkg/auth/token.go(1 hunks)pkg/http/handler.go(1 hunks)pkg/http/message.go(1 hunks)pkg/http/middleware/pipeline.go(1 hunks)pkg/http/middleware/token.go(1 hunks)pkg/http/middleware/username.go(1 hunks)pkg/http/schema.go(1 hunks)pkg/middleware/middlewares.go(0 hunks)pkg/middleware/schema.go(0 hunks)storage/fixture/experience.json(1 hunks)storage/fixture/profile.json(1 hunks)storage/fixture/projects.json(1 hunks)storage/fixture/social.json(1 hunks)storage/fixture/talks.json(1 hunks)
💤 Files with no reviewable changes (8)
- env/app.go
- .env.example
- pkg/middleware/schema.go
- handler/user/schema.go
- handler/user/admin.go
- handler/user/create.go
- pkg/middleware/middlewares.go
- handler/user/repository.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
handler/experience.go
24-24: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
handler/projects.go
24-24: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
handler/profile.go
24-24: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
🔇 Additional comments (26)
storage/fixture/profile.json (1)
1-10: Well-structured fixture data with appropriate obfuscation.The JSON structure is valid and the profile data is appropriately anonymized using an example domain. The reversed email format (
otnacog@example.com) maintains consistency with the handle while using a safe example domain.database/seeder/seeds/factory.go (1)
26-26: Good consistency improvement.Lowercasing "db" in the error message follows Go error message conventions.
boost/ignite.go (1)
9-9: Function renaming improves clarity.The rename from
SparktoIgnitebetter describes the initialization purpose of this function.Verify that all references to the old
Sparkfunction have been updated throughout the codebase:#!/bin/bash # Description: Check for any remaining references to the old Spark function name # Expected: No remaining references to boost.Spark echo "Searching for remaining references to boost.Spark..." rg "boost\.Spark" --type go echo "Searching for Spark function calls..." rg "Spark\(" --type gopkg/auth/token.go (1)
10-11: Token struct simplification aligns with authentication refactor.Removing the
Usernamefield simplifies token-based authentication by eliminating username dependency, which aligns with the broader authentication refactoring described in the summary.Verify that all code depending on the
Usernamefield has been updated:#!/bin/bash # Description: Check for any remaining references to Token.Username field # Expected: No remaining references to the removed Username field echo "Searching for Token struct field access..." rg "\.Username" --type go -A 2 -B 2 echo "Searching for Token struct initialization with Username..." rg "Username.*:" --type go -A 2 -B 2cli/main.go (1)
19-19: Verify function rename consistency across codebase.The function call change from
boost.Sparktoboost.Ignitelooks correct, but ensure this rename is applied consistently throughout the entire codebase.#!/bin/bash # Description: Verify all occurrences of boost.Spark have been updated to boost.Ignite # Expected: No occurrences of boost.Spark should remain # Search for any remaining boost.Spark calls rg "boost\.Spark" --type godatabase/seeder/main.go (2)
16-16: LGTM on function rename.The change from
boost.Sparktoboost.Igniteis consistent with the broader refactoring.
33-33: Good stylistic consistency improvements.The standardization of "DB" to "db" in comments and log messages improves consistency across the codebase.
Also applies to: 37-37, 117-117
storage/fixture/talks.json (1)
1-36: Well-structured fixture data with minor validation suggestions.The JSON structure is clean and consistent. Consider validating the following:
- UUID format - All UUIDs appear properly formatted but should be verified
- URL accessibility - The engineers.sg URLs should be validated for availability
- Date consistency - Dates follow YYYY-MM-DD format consistently
#!/bin/bash # Description: Validate the fixture data format and accessibility # Expected: Valid UUIDs, accessible URLs, proper JSON structure # Validate JSON structure echo "Validating JSON structure..." cat storage/fixture/talks.json | jq . > /dev/null && echo "✓ Valid JSON" || echo "✗ Invalid JSON" # Check UUID format (basic pattern check) echo "Checking UUID formats..." cat storage/fixture/talks.json | jq -r '.data[].uuid' | grep -E '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' | wc -l # Test URL accessibility (first URL only to avoid heavy operations) echo "Testing first URL accessibility..." curl -s --head "$(cat storage/fixture/talks.json | jq -r '.data[0].url')" | head -1storage/fixture/social.json (1)
1-41: ```shell
#!/bin/bashTest all URLs for reachability
echo "Testing all URLs in social.json..."
cat storage/fixture/social.json | jq -r '.data[].url' | while read url; do
echo "Testing: $url"
curl -s --head --max-time 5 "$url" | head -1
done</details> <details> <summary>pkg/http/message.go (1)</summary> `8-13`: **Clean implementation of internal error helper function.** The function correctly formats error messages and uses the appropriate HTTP status code. The import aliasing prevents naming conflicts with the local `http` package. ```shell #!/bin/bash # Description: Verify ApiError type definition and usage patterns # Expected: ApiError struct should be defined, function should be used consistently # Find ApiError struct definition echo "Searching for ApiError type definition..." ast-grep --pattern 'type ApiError struct { $$$ }' # Check usage patterns of InternalError function echo "Checking InternalError usage patterns..." rg "InternalError" --type go -A 2 -B 1pkg/http/middleware/pipeline.go (1)
12-21: LGTM! Clean middleware pipeline implementation.The middleware chaining logic is correct, with clear documentation explaining the reverse order application. The implementation follows Go best practices.
pkg/http/handler.go (1)
9-27: Good error handling pattern.The handler wrapper provides consistent JSON error responses and proper HTTP status codes. The JSON encoding error handling is well implemented.
boost/factory.go (2)
53-53: Good consistency improvement in error messages.The capitalization changes improve consistency in error message formatting across the factory functions.
Also applies to: 117-117
64-67: LGTM! Token simplification aligns with refactor.The removal of the Username field from the token initialization is consistent with the broader authentication refactor mentioned in the summary.
storage/fixture/experience.json (1)
10-11: Verify future end date.The end date "April, 2025" appears to be in the future. Ensure this is intentional and not a typo.
handler/social.go (2)
14-18: LGTM: Clean constructor patternThe constructor function follows a consistent pattern with other handlers in the system. The hardcoded path approach is appropriate for fixture-based data serving.
20-34: LGTM: Solid error handling patternThe Handle method demonstrates good error handling practices:
- Proper file reading with error checking
- Structured API error responses
- Consistent return pattern with nil indicating success
- Integration with the writeJSON helper for response handling
handler/profile.go (2)
14-18: LGTM: Consistent handler patternThe constructor follows the same clean pattern established across all fixture handlers, maintaining consistency in the codebase architecture.
20-34: LGTM: Consistent error handling implementationThe Handle method maintains the same robust error handling pattern established in other handlers, ensuring consistent API behavior across all endpoints.
pkg/http/schema.go (3)
5-13: LGTM: Well-designed error response typesThe separation between
ErrorResponsefor JSON serialization andApiErrorfor internal error handling is well thought out. The struct field tags ensure proper JSON marshaling.
15-21: LGTM: Robust error interface implementationThe Error() method includes proper nil safety checks, preventing potential panics when dealing with nil ApiError pointers. The fallback to "Internal Server Error" is appropriate.
23-25: LGTM: Clean function type definitionsThe
ApiHandlerandMiddlewarefunction types establish a clear contract for the HTTP layer, enabling clean middleware composition and error handling throughout the system.boost/helpers.go (1)
23-29: LGTM: Clean getter methodsThe getter methods provide appropriate access to private App fields while maintaining encapsulation.
boost/router.go (3)
11-15: LGTM: Well-structured Router designThe Router struct encapsulates the necessary dependencies (environment, mux, pipeline) for handling route registration and middleware composition.
17-29: LGTM: Solid middleware pipeline compositionThe PipelineFor method properly composes the middleware chain with token authentication and username validation. The pipeline order (username check, then token check) is logical for authentication flow.
38-38: Verify API path consistencyThe route paths don't include the
/apiprefix mentioned in the AI summary. Please confirm if these should be/api/profile,/api/experience, etc., or if the current paths are intentional.#!/bin/bash # Description: Check if other files reference these routes with /api prefix # Expected: Find references to confirm the correct path pattern rg -A 2 -B 2 "/api/(profile|experience|projects|social|talks)" --type goAlso applies to: 48-48, 58-58, 68-68, 78-78
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
boost/app.go (1)
21-41: Add error handling for component initialization.The initialization functions
MakeLogs,MakeSentry, andMakeDbConnectioncould potentially fail, but their errors are not being handled. This could lead to runtime panics.
🧹 Nitpick comments (1)
boost/app.go (1)
43-55: Consider calling router methods directly on the pointer.While the nil check is good, you can avoid dereferencing the router pointer by calling methods directly:
func (a *App) Boot() { if a.router == nil { panic("Router is not set") } - - router := *a.router - - router.Profile() - router.Experience() - router.Projects() - router.Social() - router.Talks() + + a.router.Profile() + a.router.Experience() + a.router.Projects() + a.router.Social() + a.router.Talks()This is more idiomatic Go and avoids unnecessary dereferencing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
boost/app.go(1 hunks)boost/helpers.go(1 hunks)handler/experience.go(1 hunks)handler/profile.go(1 hunks)handler/projects.go(1 hunks)handler/social.go(1 hunks)handler/talks.go(1 hunks)pkg/http/handler.go(1 hunks)pkg/http/middleware/token.go(1 hunks)pkg/http/middleware/username.go(1 hunks)storage/fixture/projects.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- storage/fixture/projects.json
- pkg/http/handler.go
- pkg/http/middleware/username.go
- handler/social.go
- handler/experience.go
- handler/projects.go
- handler/profile.go
- handler/talks.go
- pkg/http/middleware/token.go
- boost/helpers.go
🔇 Additional comments (2)
boost/app.go (2)
7-7: LGTM! Imports are appropriate for the new functionality.The added imports for middleware and net/http are necessary for the middleware pipeline and HTTP multiplexer setup.
Also applies to: 9-9
13-18: Excellent encapsulation with private fields.Converting all struct fields to private provides better encapsulation and controlled access to the App's internal components.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores