HTTP Response Cache #37
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThis update refactors HTTP handler logic to use typed payload structs and a new response abstraction with ETag-based caching. Handler constructors now require file paths for JSON fixtures. Several new payload struct files are introduced. Legacy response, error, and request handling utilities are removed and replaced by new response logic. The Makefile's ROOT_NETWORK variable is updated. The experience fixture JSON root key is corrected from "date" to "data". Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Handler
participant Parser
participant Response
Client->>Router: HTTP Request (e.g., /profile)
Router->>Handler: Call Handler with file path
Handler->>Parser: ParseJsonFile(filePath)
Parser-->>Handler: Typed payload struct or error
Handler->>Response: MakeResponseFrom(version, w, r)
alt If client cache is valid
Handler->>Response: HasCache()
Response-->>Handler: true
Handler->>Response: RespondWithNotModified()
Response-->>Client: HTTP 304 Not Modified
else
Handler->>Response: RespondOk(payload)
Response-->>Client: HTTP 200 OK + JSON
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (20)
✨ 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/cache-response
branch
from
7de6f96 to
40baa0d
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: 5
🧹 Nitpick comments (6)
handler/payload/talks.go (1)
15-16: Consider using time.Time for timestamp fields.The
CreatedAtandUpdatedAtfields are currently strings, which may not be optimal for timestamp handling. Consider usingtime.Timefor better type safety and easier date operations.- CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"`Don't forget to import the
timepackage if you make this change.handler/payload/profile.go (1)
12-12: Consider email validation for data integrity.The
handler/payload/projects.go (1)
14-15: Consider using time.Time for timestamp fields.Similar to other payload structs, the timestamp fields should consider using
time.Timeinstead of strings for better type safety and date handling capabilities.- CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"`handler/payload/experience.go (2)
14-15: Consider using time.Time for date fields.The
StartDateandEndDatefields would benefit from usingtime.Timeinstead of strings for better date handling and validation.- StartDate string `json:"start_date"` - EndDate string `json:"end_date"` + StartDate time.Time `json:"start_date"` + EndDate time.Time `json:"end_date"`
19-19: Consider using slice for Skills field.The
Skillsfield is currently a single string. Depending on your data structure, you might want to consider using[]stringto represent multiple skills more clearly.- Skills string `json:"skills"` + Skills []string `json:"skills"`This would require updating your JSON data format accordingly.
pkg/http/response.go (1)
24-24: Consider making cache duration configurable.The cache control duration is hardcoded to 1 hour (3600 seconds). Consider making this configurable to allow different caching strategies for different endpoints.
You could add a parameter to
MakeResponseFromor create additional constructor methods:-func MakeResponseFrom(salt string, writer baseHttp.ResponseWriter, request *baseHttp.Request) *Response { +func MakeResponseFrom(salt string, writer baseHttp.ResponseWriter, request *baseHttp.Request) *Response { + return MakeResponseWithCache(salt, writer, request, 3600) +} + +func MakeResponseWithCache(salt string, writer baseHttp.ResponseWriter, request *baseHttp.Request, maxAge int) *Response {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Makefile(1 hunks)boost/router.go(5 hunks)handler/experience.go(1 hunks)handler/payload/experience.go(1 hunks)handler/payload/profile.go(1 hunks)handler/payload/projects.go(1 hunks)handler/payload/social.go(1 hunks)handler/payload/talks.go(1 hunks)handler/profile.go(1 hunks)handler/projects.go(1 hunks)handler/response.go(0 hunks)handler/social.go(1 hunks)handler/talks.go(1 hunks)pkg/handler.go(0 hunks)pkg/http/message.go(0 hunks)pkg/http/response.go(1 hunks)pkg/parser.go(1 hunks)pkg/request/request.go(0 hunks)pkg/response/response.go(0 hunks)storage/fixture/experience.json(1 hunks)
💤 Files with no reviewable changes (5)
- handler/response.go
- pkg/http/message.go
- pkg/request/request.go
- pkg/response/response.go
- pkg/handler.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
boost/router.go (5)
handler/profile.go (1)
MakeProfileHandler(15-19)handler/experience.go (1)
MakeExperienceHandler(15-19)handler/projects.go (1)
MakeProjectsHandler(15-19)handler/social.go (1)
MakeSocialHandler(15-19)handler/talks.go (1)
MakeTalksHandler(15-19)
handler/social.go (4)
pkg/http/schema.go (1)
ApiError(10-13)pkg/parser.go (1)
ParseJsonFile(9-29)handler/payload/social.go (1)
SocialResponse(3-6)pkg/http/response.go (2)
InternalError(68-73)MakeResponseFrom(18-38)
pkg/http/response.go (1)
pkg/http/schema.go (1)
ApiError(10-13)
🔇 Additional comments (16)
pkg/parser.go (1)
9-29: Excellent generic JSON parser implementation!The function correctly handles generics, provides good error context, and follows Go best practices. The zero value initialization and error wrapping are particularly well done.
storage/fixture/experience.json (1)
3-3: Good standardization of JSON structure.Renaming the key from "date" to "data" makes more semantic sense for an array of experience records and aligns with the new typed payload structures.
Makefile (1)
23-23: Network configuration updated appropriately.The addition of quotes and network name change appears to be a proper configuration update for the development environment.
handler/payload/social.go (1)
3-14: Well-structured payload definitions.The
SocialResponseandSocialDatastructs are properly defined with appropriate JSON tags and field types. This aligns well with the type-safe refactoring approach.boost/router.go (1)
32-32: Good architectural improvement with parameterized file paths.The explicit file path parameters make the handlers more flexible and testable. However, there's a potential typo in the handler constructors.
Please verify that the handler constructors don't have a typo. Based on the relevant code snippets, they appear to use
filePahinstead offilePath:#!/bin/bash # Description: Check for potential typo in handler constructor parameter names # Expected: Should find instances of "filePah" that should likely be "filePath" echo "Checking for 'filePah' parameter name in handler files:" rg "filePah" --type go -A 2 -B 2Also applies to: 42-42, 52-52, 62-62, 72-72
handler/payload/talks.go (1)
1-6: LGTM! Well-structured response payload.The
TalksResponsestruct follows the consistent pattern established across other payload files with proper JSON tags and clear field naming.handler/payload/profile.go (1)
1-6: LGTM! Clean and consistent payload structure.The
ProfileResponsestruct follows the established pattern with proper JSON tags and clear field organization.handler/payload/projects.go (1)
1-6: LGTM! Consistent payload structure.The
ProjectsResponsestruct maintains consistency with other payload definitions and has proper JSON tags.handler/payload/experience.go (1)
1-6: LGTM! Well-structured experience payload.The
ExperienceResponsestruct follows the consistent pattern and has proper field organization.handler/profile.go (2)
22-28: LGTM! Well-implemented generic JSON parsing.The use of
pkg.ParseJsonFilewith generic types provides good type safety and the error handling with proper logging is appropriate.
30-42: Excellent caching implementation.The HTTP caching logic using
MakeResponseFromis well-implemented with proper ETag handling and 304 Not Modified responses. The error handling ensures graceful degradation when JSON marshaling fails.handler/social.go (1)
30-42: LGTM: Clean HTTP caching implementation.The new response handling with ETag-based caching is well-implemented. The logic correctly checks for cache hits and responds with 304 Not Modified when appropriate, or returns the full JSON response otherwise.
handler/projects.go (1)
30-42: Consistent implementation with other handlers.The HTTP response caching logic is consistent with the pattern established in other handlers, which is good for maintainability.
pkg/http/response.go (3)
44-52: LGTM: Clean JSON response handling.The
RespondOkmethod correctly sets headers, writes the status code, and encodes the payload as JSON. The error handling allows callers to handle JSON encoding failures appropriately.
54-62: LGTM: Proper ETag cache validation.The
HasCachemethod correctly implements ETag-based cache validation by comparing the client'sIf-None-Matchheader with the stored ETag.
18-38: Fix ETag formatting inconsistency.There's an issue with ETag formatting and trimming. The
etagvariable in the closure (line 35) uses the original formatted value with quotes, whiler.etagstores the trimmed version, causing a mismatch.Apply this diff to fix the ETag handling:
func MakeResponseFrom(salt string, writer baseHttp.ResponseWriter, request *baseHttp.Request) *Response { - etag := fmt.Sprintf( - `"%s"`, - strings.TrimSpace(salt), - ) + trimmedSalt := strings.TrimSpace(salt) + etag := fmt.Sprintf(`"%s"`, trimmedSalt) cacheControl := "public, max-age=3600" return &Response{ writer: writer, request: request, - etag: strings.TrimSpace(etag), + etag: etag, cacheControl: cacheControl, headers: func(w baseHttp.ResponseWriter) { w.Header().Set("Content-Type", "application/json") w.Header().Set("X-Content-Type-Options", "nosniff") w.Header().Set("Cache-Control", cacheControl) w.Header().Set("ETag", etag) }, } }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (1)
handler/social.go (1)
12-12: Fix typo in field name (same issue as ProfileHandler).The same typo exists here:
filePahshould befilePathfor consistency and readability.- filePah string + filePath string-func MakeSocialHandler(filePah string) SocialHandler { +func MakeSocialHandler(filePath string) SocialHandler {- filePah: filePah, + filePath: filePath,- data, err := pkg.ParseJsonFile[payload.SocialResponse](h.filePah) + data, err := pkg.ParseJsonFile[payload.SocialResponse](h.filePath)Also applies to: 15-15, 17-17, 22-22
🧹 Nitpick comments (6)
boost/router.go (1)
31-39: Repeated boilerplate – factor into a helper to keepRouterlean.Each method differs only by route, constructor, and fixture filename. A tiny helper would cut the noise:
func (r *Router) mount(path string, factory func(string) handler.ApiHandler, file string) { h := factory(filepath.Join(base, file)) r.Mux.HandleFunc(path, r.PipelineFor(h.Handle)) }Call it from
InitRoutes()instead of keeping five near-identical methods.Also applies to: 41-49, 51-59, 61-69, 71-79
handler/payload/profile.go (1)
3-14: Structs LGTM – add godoc & consider omitempty tags.The types map cleanly to JSON. Minor polish:
- Add package-level comments for exported structs to satisfy
golint.- If zero values are acceptable, append
,omitemptyto reduce payload size.No blockers.
handler/payload/social.go (1)
3-14: Well-formed payload definitions.Nothing critical spotted. Same godoc /
omitemptynit applies here.handler/payload/projects.go (1)
8-16: Considertime.Timefor date fields.
CreatedAt/UpdatedAtasstringlimit validation and sorting.
Usingtime.Time(with RFC3339 in JSON) improves type-safety:- CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"`If consumers rely on plain strings, ignore this.
handler/payload/talks.go (1)
8-17: Consider stronger typing for specific fields.While the struct design follows good patterns, consider using more specific types:
UUIDfield could use a UUID library type for validationCreatedAt/UpdatedAtcould usetime.Timefor better date/time handlingURLfield could benefit from url.URL type validationCurrent string types work well for JSON serialization but miss opportunities for compile-time type safety and validation.
handler/payload/experience.go (1)
8-20: Consider stronger typing for date and structured fields.Similar to other payload structs, consider more specific types:
StartDate/EndDatecould usetime.Timeor a custom date typeUUIDfield could use a UUID library typeSkillsfield might benefit from a slice type if it represents multiple skillsThis would provide better type safety and validation while maintaining JSON serialization compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Makefile(1 hunks)boost/router.go(5 hunks)handler/experience.go(1 hunks)handler/payload/experience.go(1 hunks)handler/payload/profile.go(1 hunks)handler/payload/projects.go(1 hunks)handler/payload/social.go(1 hunks)handler/payload/talks.go(1 hunks)handler/profile.go(1 hunks)handler/projects.go(1 hunks)handler/response.go(0 hunks)handler/social.go(1 hunks)handler/talks.go(1 hunks)pkg/handler.go(0 hunks)pkg/http/message.go(0 hunks)pkg/http/response.go(1 hunks)pkg/parser.go(1 hunks)pkg/request/request.go(0 hunks)pkg/response/response.go(0 hunks)storage/fixture/experience.json(1 hunks)
💤 Files with no reviewable changes (5)
- handler/response.go
- pkg/http/message.go
- pkg/handler.go
- pkg/request/request.go
- pkg/response/response.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
boost/router.go (5)
handler/profile.go (1)
MakeProfileHandler(15-19)handler/experience.go (1)
MakeExperienceHandler(15-19)handler/projects.go (1)
MakeProjectsHandler(15-19)handler/social.go (1)
MakeSocialHandler(15-19)handler/talks.go (1)
MakeTalksHandler(15-19)
handler/profile.go (4)
pkg/http/schema.go (1)
ApiError(10-13)pkg/parser.go (1)
ParseJsonFile(9-29)handler/payload/profile.go (1)
ProfileResponse(3-6)pkg/http/response.go (2)
InternalError(68-73)MakeResponseFrom(18-38)
pkg/http/response.go (1)
pkg/http/schema.go (1)
ApiError(10-13)
🔇 Additional comments (12)
storage/fixture/experience.json (1)
3-3: Key rename looks correct – verify consumers are updated.
"data"now matches the struct tag used inpayload.ExperienceResponse. 👍
Double-check that every place previously expecting"date"(tests, clients, docs) has been updated.pkg/parser.go (1)
9-29: LGTM! Well-designed generic JSON parsing utility.The function demonstrates good practices with proper error wrapping, clear variable initialization, and helpful comments. The generic type parameter enables type-safe JSON parsing across the codebase.
handler/profile.go (1)
30-42: Excellent implementation of ETag-based caching.The response handling demonstrates good practices:
- Proper ETag-based caching with conditional responses
- Clean separation of cache validation and response generation
- Appropriate error logging without exposing internal details
- Structured response handling with the new abstraction
handler/social.go (1)
30-42: Consistent implementation of the new response pattern.The handler follows the same excellent patterns as ProfileHandler with proper caching, error handling, and structured responses. Good consistency across the refactored handlers.
handler/projects.go (1)
30-42: The caching and response logic looks well-implemented.The ETag-based caching implementation correctly checks for client cache validity and responds with HTTP 304 when appropriate. The error handling for JSON marshaling is also reasonable - logging the error without returning an API error prevents the handler from failing due to serialization issues.
handler/talks.go (1)
30-42: The refactoring and caching implementation is solid.The transition to typed payload parsing and ETag-based caching is well-executed. The error handling appropriately logs marshaling errors without failing the entire request.
handler/experience.go (1)
30-42: Excellent refactoring with proper caching implementation.The move to typed payloads and ETag-based caching significantly improves the handler's reliability and performance. The error handling strategy is appropriate for this use case.
pkg/http/response.go (5)
18-38: Well-designed response constructor with proper caching headers.The
MakeResponseFromfunction correctly implements ETag generation and sets appropriate cache control headers. The 1-hour cache duration and public caching are reasonable defaults for static content.
54-62: Correct ETag-based cache validation implementation.The
HasCache()method properly compares the client'sIf-None-Matchheader with the server's ETag, following HTTP caching specifications.
44-52: Proper JSON response handling with appropriate headers.The
RespondOkmethod correctly sets headers, status code, and handles JSON encoding. Usingjson.NewEncoder().Encode()is the right approach for streaming JSON responses.
64-66: Simple and correct 304 Not Modified response.The
RespondWithNotModifiedmethod appropriately sends only the status code for 304 responses, which should not include a response body.
68-73: Verify that theApiErrortype is properly imported.The
InternalErrorfunction referencesApiErrorbut I don't see an explicit import. Ensure this type is available in the current package scope.#!/bin/bash # Check if ApiError is defined in this package or properly imported ast-grep --pattern 'type ApiError struct { $$$ }' # Also check for imports of packages that might contain ApiError rg -A 3 -B 3 "ApiError"
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Summary by CodeRabbit
New Features
Refactor
Chores