apply cache headers to some most used routes #354
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of cache control and request handling mechanisms across multiple components of the application. The changes focus on simplifying cache query creation, improving header management, and standardizing response structures. Key modifications include introducing a new Changes
Possibly related PRs
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
app/resources/post/thread_cache/cache.go (1)
23-26: Consider logging database errors for debuggingThe error handling has been simplified to return false, but this might hide database errors. Consider logging the error before returning false to aid in debugging.
func (c *Cache) IsNotModified(ctx context.Context, cq cachecontrol.Query, id xid.ID) bool { r, err := c.db.Post.Query().Select(post.FieldUpdatedAt).Where(post.ID(id)).Only(ctx) if err != nil { + log.Printf("failed to query post update time: %v", err) return false }app/transports/http/middleware/headers/ua.go (1)
15-16: Enhance middleware documentationThe comment could be more descriptive about which specific headers are stored and how they're used downstream.
-// WithHeaderContext stores in the request context header info. +// WithHeaderContext stores HTTP request headers in the context for downstream use. +// Headers stored include: User-Agent, If-None-Match, If-Modified-Since, and other +// cache-related headers used for conditional requests.app/resources/profile/profile_cache/cache.go (2)
26-28: Add error logging for failed queriesSimilar to the thread cache, consider logging database errors to aid in debugging.
if err != nil { + log.Printf("failed to query account update time: %v", err) return false }
11-11: Remove unused importThe
postpackage is imported but not used after fixing the field selection.- "github.com/Southclaws/storyden/internal/ent/post"app/services/reqinfo/info.go (2)
22-41: Consider adding error logging for time parsing failures.While the error handling is appropriate, logging the parsing error could help with debugging cache-related issues in production.
if err != nil { + logger.Debug("failed to parse If-Modified-Since header", zap.Error(err)) ifModifiedSince = opt.NewEmpty[time.Time]() }
69-71: Consider supporting additional time formats.HTTP clients might send dates in different formats. Consider supporting additional RFC formats like RFC850 and ANSI C's asctime() format as per HTTP/1.1 specification.
func parseConditionalRequestTime(in string) (time.Time, error) { - return time.Parse(time.RFC1123, in) + formats := []string{ + time.RFC1123, + time.RFC850, + time.ANSIC, + } + for _, format := range formats { + if t, err := time.Parse(format, in); err == nil { + return t, nil + } + } + return time.Time{}, fmt.Errorf("failed to parse time: %s", in) }app/transports/http/bindings/info.go (1)
80-82: Consider increasing the cache duration for banners.The current 1-hour cache duration (
max-age=3600) for banners seems short. Since banners are also static assets that don't change frequently, consider increasing this to a longer duration, such as 1 week (max-age=604800) or 1 month (max-age=2592000).app/transports/http/bindings/assets.go (1)
40-42: Consider implementing conditional cache durations based on asset types.While a 1-year cache duration is good for static assets, consider implementing different cache durations based on the asset type using the
MIMEinformation. For example:
- Images/Icons: 1 year
- Documents: 1 week
- Dynamic content: 1 day
Headers: openapi.AssetGetOKResponseHeaders{ - CacheControl: "public, max-age=31536000", + CacheControl: getCacheControlForMIME(a.MIME), },Add a helper function:
func getCacheControlForMIME(mime string) string { switch { case strings.HasPrefix(mime, "image/"): return "public, max-age=31536000" case strings.HasPrefix(mime, "application/pdf"): return "public, max-age=604800" default: return "public, max-age=86400" } }app/transports/http/openapi/server_gen.go (1)
22291-22293: Consider optimizing header value conversionWhile the current implementation is correct, we can optimize the string conversion. Since the header fields are already strings, fmt.Sprint is unnecessary.
Consider this simpler approach:
-w.Header().Set("Cache-Control", fmt.Sprint(response.Headers.CacheControl)) -w.Header().Set("ETag", fmt.Sprint(response.Headers.ETag)) -w.Header().Set("Last-Modified", fmt.Sprint(response.Headers.LastModified)) +w.Header().Set("Cache-Control", response.Headers.CacheControl) +w.Header().Set("ETag", response.Headers.ETag) +w.Header().Set("Last-Modified", response.Headers.LastModified)Also applies to: 22598-22600, 22869-22871, 22915-22917, 23001-23003, 24820-24822, 24929-24931, 26175-26177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/openapi.yamlis excluded by!**/*.yaml
📒 Files selected for processing (20)
app/resources/cachecontrol/query.go(1 hunks)app/resources/post/thread_cache/cache.go(1 hunks)app/resources/profile/profile.go(4 hunks)app/resources/profile/profile_cache/cache.go(1 hunks)app/resources/resources.go(2 hunks)app/services/authentication/provider/webauthn/webauthn.go(3 hunks)app/services/reqinfo/info.go(1 hunks)app/transports/http/bindings/accounts.go(6 hunks)app/transports/http/bindings/admin.go(2 hunks)app/transports/http/bindings/assets.go(1 hunks)app/transports/http/bindings/info.go(2 hunks)app/transports/http/bindings/profiles.go(4 hunks)app/transports/http/bindings/threads.go(2 hunks)app/transports/http/middleware/headers/ua.go(1 hunks)app/transports/http/middleware/middleware.go(1 hunks)app/transports/http/middleware/useragent/ua.go(0 hunks)app/transports/http/openapi/server_gen.go(12 hunks)app/transports/http/router.go(3 hunks)web/src/api/openapi-client/accounts.ts(1 hunks)web/src/api/openapi-client/profiles.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/transports/http/middleware/useragent/ua.go
🧰 Additional context used
🪛 Biome (1.9.4)
web/src/api/openapi-client/profiles.ts
[error] 101-102: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 349-350: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
web/src/api/openapi-client/accounts.ts
[error] 44-45: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (20)
app/transports/http/middleware/middleware.go (1)
8-8: LGTM!Clean replacement of the user agent middleware with the new headers middleware.
Also applies to: 20-20
app/resources/cachecontrol/query.go (2)
16-20: LGTM! Interface simplification improves type safety.The change from pointer parameters to
opt.Optionalparameters is a good improvement that:
- Makes the optionality more explicit in the type system
- Eliminates potential nil pointer issues
- Simplifies the function interface
28-31: LGTM! Clear documentation of time truncation.The updated comments clearly explain why time truncation is necessary for HTTP header compatibility.
app/services/reqinfo/info.go (1)
15-18: LGTM! Well-structured request info encapsulation.The
Infostruct cleanly encapsulates both user agent and cache query information.app/transports/http/router.go (1)
35-35: LGTM! Clean middleware replacement.The replacement of the user agent middleware with the new headers middleware is clean and maintains the correct middleware order.
Also applies to: 45-45
app/resources/profile/profile.go (1)
23-23: LGTM! Consistent implementation of update tracking.The addition of the
Updatedfield and its consistent implementation across all profile creation paths properly supports cache validation.Also applies to: 49-49, 88-88, 104-104
app/transports/http/bindings/info.go (1)
53-55: LGTM! Good choice of cache duration for icons.The 1-year cache duration is appropriate for icons as they are static assets that rarely change. This will significantly improve performance for returning users.
app/services/authentication/provider/webauthn/webauthn.go (1)
21-21: LGTM! Good refactoring of device name retrieval.The migration from
useragenttoreqinfopackage is a good architectural decision that consolidates request information handling into a single package.Also applies to: 112-112, 139-139
app/resources/resources.go (1)
46-46: LGTM! Good addition of profile caching support.The integration of
profile_cachealigns well with the PR's objective of implementing caching mechanisms across the application.Also applies to: 94-94
app/transports/http/bindings/admin.go (2)
100-102: LGTM!The response structure change aligns with the standardized response format used across the API.
132-134: LGTM!The response structure change aligns with the standardized response format used across the API.
app/transports/http/bindings/threads.go (1)
219-219: LGTM!The cache validation logic is simplified by using
reqinfo.GetCacheQuery, which provides a cleaner way to handle cache control.app/transports/http/bindings/profiles.go (2)
103-105: LGTM!The cache validation logic is consistent with the pattern used in
ThreadGet.
115-121: Review the cache control header value.The cache control header is set to "public, max-age=1", which means the response will be cached for only 1 second. Consider if this short cache duration is intentional or if it should be increased for better performance.
app/transports/http/bindings/accounts.go (3)
34-34: LGTM: Profile cache integrationThe profile cache field and its initialization in the constructor are properly implemented.
Also applies to: 46-46, 57-57
80-82: Review the cache duration settingThe cache control max-age is set to 1 second, which might be too aggressive and could lead to unnecessary requests. Consider increasing this value based on your application's requirements for data freshness vs performance.
Would you like me to help determine a more optimal cache duration based on your user activity patterns?
Also applies to: 90-96
237-239: LGTM: Avatar caching implementationThe cache control header for avatars is well configured:
- Public cache directive allows CDNs and browsers to cache the content
- 1-hour duration is reasonable for avatar images
app/transports/http/openapi/server_gen.go (3)
22065-22069: Well-structured cache header definitions!The new response header structs for Account, Asset, and Profile endpoints are consistently defined with all necessary cache control fields (CacheControl, ETag, LastModified).
Also applies to: 22077-22081, 22092-22096, 22217-22221
22299-22304: Correct implementation of 304 Not Modified responses!The 304 response structs and their handlers are properly implemented:
- Empty structs as 304 responses should have no body
- Consistent pattern across Account, Profile, and ProfileFollowing endpoints
Also applies to: 26183-26188, 26367-26373
30327-30706: Base64 encoded swagger spec updatedThe swagger specification has been updated to include the new cache-related response types and headers.
| @@ -344,12 +346,14 @@ export type ProfileFollowingGetQueryResult = NonNullable< | |||
| Awaited<ReturnType<typeof profileFollowingGet>> | |||
| >; | |||
| export type ProfileFollowingGetQueryError = | |||
| | void | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace void with undefined in union type.
Using void in a union type can be confusing. Consider using undefined instead to better represent the absence of an error.
Apply this diff:
- | void
+ | undefined📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | void | |
| | undefined |
🧰 Tools
🪛 Biome (1.9.4)
[error] 349-350: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
| @@ -98,12 +98,14 @@ | |||
| Awaited<ReturnType<typeof profileGet>> | |||
| >; | |||
| export type ProfileGetQueryError = | |||
| | void | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace void with undefined in union type.
Using void in a union type can be confusing. Consider using undefined instead to better represent the absence of an error.
Apply this diff:
- | void
+ | undefined📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | void | |
| | undefined |
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-102: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
| | void | ||
| | UnauthorisedResponse |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace void with undefined in union types
Using void in union types can be confusing as it represents the absence of a return value rather than an optional value. Consider using undefined instead.
Apply this change:
export type AccountGetQueryError =
- | void
+ | undefined
| UnauthorisedResponse
| NotFoundResponse
| InternalServerErrorResponse;Also applies to: 51-53
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-45: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
Southclaws
force-pushed
the
apply-cache-control-widely
branch
from
4e7f1a4 to
4975e05
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/resources/post/thread_cache/cache.go (1)
23-31: Consider logging database errors.The simplified error handling returns
falsewhen database errors occur. While this makes the API simpler, it could hide important database issues from callers. Consider logging the error before returningfalseto aid in debugging.func (c *Cache) IsNotModified(ctx context.Context, cq cachecontrol.Query, id xid.ID) bool { r, err := c.db.Post.Query().Select(post.FieldUpdatedAt).Where(post.ID(id)).Only(ctx) if err != nil { + // TODO: Add logging with appropriate level (warn/error) + // log.Ctx(ctx).Warn().Err(err).Msg("failed to query post update time") return false }app/transports/http/bindings/threads.go (1)
Line range hint
234-236: Consider making cache max-age configurable.The cache control value is hardcoded to "max-age=1". Consider making this configurable to allow different caching strategies for different environments or use cases.
app/transports/http/bindings/accounts.go (1)
93-94: Consider standardizing cache control values.Different endpoints use different cache-control values:
- Account profile: "public, max-age=1"
- Account avatar: "public, max-age=3600"
Consider standardizing these values or documenting why they differ.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/openapi.yamlis excluded by!**/*.yaml
📒 Files selected for processing (20)
app/resources/cachecontrol/query.go(1 hunks)app/resources/post/thread_cache/cache.go(1 hunks)app/resources/profile/profile.go(4 hunks)app/resources/profile/profile_cache/cache.go(1 hunks)app/resources/resources.go(2 hunks)app/services/authentication/provider/webauthn/webauthn.go(3 hunks)app/services/reqinfo/info.go(1 hunks)app/transports/http/bindings/accounts.go(6 hunks)app/transports/http/bindings/admin.go(2 hunks)app/transports/http/bindings/assets.go(1 hunks)app/transports/http/bindings/info.go(2 hunks)app/transports/http/bindings/profiles.go(4 hunks)app/transports/http/bindings/threads.go(2 hunks)app/transports/http/middleware/headers/ua.go(1 hunks)app/transports/http/middleware/middleware.go(1 hunks)app/transports/http/middleware/useragent/ua.go(0 hunks)app/transports/http/openapi/server_gen.go(12 hunks)app/transports/http/router.go(3 hunks)web/src/api/openapi-client/accounts.ts(1 hunks)web/src/api/openapi-client/profiles.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/transports/http/middleware/useragent/ua.go
🚧 Files skipped from review as they are similar to previous changes (11)
- app/transports/http/middleware/middleware.go
- app/resources/resources.go
- app/services/authentication/provider/webauthn/webauthn.go
- app/resources/profile/profile_cache/cache.go
- app/transports/http/bindings/assets.go
- app/transports/http/middleware/headers/ua.go
- app/transports/http/router.go
- app/transports/http/bindings/info.go
- app/transports/http/bindings/admin.go
- app/services/reqinfo/info.go
- app/transports/http/bindings/profiles.go
🧰 Additional context used
🪛 Biome (1.9.4)
web/src/api/openapi-client/accounts.ts
[error] 44-45: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
web/src/api/openapi-client/profiles.ts
[error] 101-102: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 349-350: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (10)
web/src/api/openapi-client/profiles.ts (1)
101-101: Consider updating OpenAPI schema to useundefinedinstead ofvoid.The file is auto-generated by orval, and the additions of
voidto error types might get overwritten if modified directly. Consider updating the OpenAPI schema to useundefinedinstead ofvoidin union types for better type semantics.Note: Since this is an auto-generated file, any direct modifications will be lost on the next generation. The change needs to be made in the OpenAPI schema source.
Also applies to: 108-108, 349-349, 356-356
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-102: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
web/src/api/openapi-client/accounts.ts (1)
44-44: Consider updating OpenAPI schema to useundefinedinstead ofvoid.The file is auto-generated by orval, and the additions of
voidto error types might get overwritten if modified directly. Consider updating the OpenAPI schema to useundefinedinstead ofvoidin union types for better type semantics.Note: Since this is an auto-generated file, any direct modifications will be lost on the next generation. The change needs to be made in the OpenAPI schema source.
Also applies to: 51-51
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-45: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
app/resources/cachecontrol/query.go (2)
16-20: LGTM! Good use of opt.Optional.The change from pointers to
opt.Optionalimproves type safety and makes the optionality more explicit in the API.
28-31: Great documentation of the time truncation rationale!The comments clearly explain why truncation is necessary for consistency between HTTP headers and database timestamps.
app/resources/profile/profile.go (1)
23-23: LGTM! Consistent initialization of the Updated field.The Updated field is properly initialized in both ProfileFromModel and ProfileFromAccount functions.
Also applies to: 88-88, 104-104
app/transports/http/bindings/threads.go (1)
219-221: LGTM! Clean cache validation implementation.Good use of the reqinfo package to simplify cache query handling.
app/transports/http/bindings/accounts.go (1)
237-239: LGTM! Appropriate cache duration for avatars.The longer cache duration (3600s) for avatars is appropriate as they change less frequently than profile data.
app/transports/http/openapi/server_gen.go (3)
22065-22069: Well-structured cache header definitions!The new response header structures are consistently defined with all necessary caching fields (CacheControl, ETag, LastModified) across different response types. This promotes good HTTP caching practices.
Also applies to: 22077-22081, 22092-22096, 22217-22221
22299-22304: Correct implementation of 304 Not Modified responsesThe 304 response implementations correctly follow HTTP standards by:
- Using empty response bodies
- Setting only the status code
- Implementing the response interface consistently
Also applies to: 26183-26188, 26367-26373
30327-30706: Generated swagger specThis is a machine-generated swagger specification. No manual review needed.
| w.Header().Set("Cache-Control", fmt.Sprint(response.Headers.CacheControl)) | ||
| w.Header().Set("ETag", fmt.Sprint(response.Headers.ETag)) | ||
| w.Header().Set("Last-Modified", fmt.Sprint(response.Headers.LastModified)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate and sanitize header values before setting
The current implementation directly sets header values using fmt.Sprint without validation. Consider:
- Validating header values to prevent header injection
- Using header-specific formatting functions
- Adding value length limits
Apply this pattern for header setting:
-w.Header().Set("Cache-Control", fmt.Sprint(response.Headers.CacheControl))
-w.Header().Set("ETag", fmt.Sprint(response.Headers.ETag))
-w.Header().Set("Last-Modified", fmt.Sprint(response.Headers.LastModified))
+if err := validateHeaderValue(response.Headers.CacheControl); err != nil {
+ return fmt.Errorf("invalid Cache-Control header: %v", err)
+}
+w.Header().Set("Cache-Control", response.Headers.CacheControl)
+
+if err := validateHeaderValue(response.Headers.ETag); err != nil {
+ return fmt.Errorf("invalid ETag header: %v", err)
+}
+w.Header().Set("ETag", response.Headers.ETag)
+
+if err := validateHeaderValue(response.Headers.LastModified); err != nil {
+ return fmt.Errorf("invalid Last-Modified header: %v", err)
+}
+w.Header().Set("Last-Modified", response.Headers.LastModified)Add a helper function:
func validateHeaderValue(value string) error {
if len(value) > 4096 { // RFC suggests reasonable limits
return errors.New("header value too long")
}
if strings.ContainsAny(value, "\r\n") {
return errors.New("header value contains CRLF")
}
return nil
}Also applies to: 22598-22600, 22869-22871, 22915-22917, 23001-23003, 24820-24822, 24929-24931, 26175-26177
No description provided.