implement conditional requests #352
Conversation
Southclaws
commented
Jan 11, 2025
Southclaws
commented
- currently only applies to single thread get
- still exploring internal API design for this
- will maybe look at proper caching layer too
- currently only applies to single thread get - still exploring internal API design for this - will maybe look at proper caching layer too
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive HTTP caching mechanism across the application's backend and frontend. 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 (
|
- server fetch is force-cache, 60s revalidation - client fetch is default behaviour which will trigger HTTP Conditional Requests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
app/resources/post/thread_cache/cache.go (1)
26-40: Consider enhancing error handling and documentationThe
IsNotModifiedimplementation could be improved in several ways:
- Add documentation explaining the function's behavior with examples
- Consider implementing ETag support alongside the timestamp check
- Use more specific error handling for database errors (e.g., not found vs other errors)
+// IsNotModified checks if a post has been modified since the last request. +// It supports conditional requests using If-Modified-Since headers. +// +// Example: +// notModified, err := cache.IsNotModified(ctx, query, postID) +// if err != nil { +// return err +// } +// if notModified { +// return 304 // Not Modified +// } func (c *Cache) IsNotModified(ctx context.Context, cq opt.Optional[cachecontrol.Query], id xid.ID) (bool, error) { query, ok := cq.Get() if !ok { return false, nil } r, err := c.db.Post.Query().Select(post.FieldUpdatedAt).Where(post.ID(id)).Only(ctx) if err != nil { - return false, fault.Wrap(err, fctx.With(ctx)) + if ent.IsNotFound(err) { + return false, fault.Wrap(err, fctx.With(ctx, "post_not_found")) + } + return false, fault.Wrap(err, fctx.With(ctx, "database_error")) } notModified := query.NotModified(r.UpdatedAt) return notModified, nil }app/resources/cachecontrol/query.go (1)
59-61: Enhance time parsing robustnessThe time parsing implementation could be more robust by supporting multiple date formats commonly used in HTTP headers.
func parseConditionalRequestTime(in openapi.IfModifiedSince) (time.Time, error) { - return time.Parse(time.RFC1123, in) + // Try parsing with different HTTP date formats + formats := []string{ + time.RFC1123, + time.RFC1123Z, + time.RFC850, + time.ANSIC, + } + + var parseErr error + for _, format := range formats { + if t, err := time.Parse(format, in); err == nil { + return t, nil + } else { + parseErr = err + } + } + return time.Time{}, parseErr }web/src/api/server.ts (1)
25-40: Consider dynamic revalidation periodsWhile the current implementation uses a fixed 60-second revalidation period, consider making it dynamic based on content type or user interaction patterns.
The revalidation period could be adjusted based on:
- Content type (e.g., shorter for frequently updated content)
- User interaction patterns (e.g., longer for static content)
- Time of day (e.g., shorter during peak hours)
web/src/api/common.ts (1)
14-14: Add documentation for cache optionsConsider adding JSDoc comments explaining the available cache options and their implications.
+/** + * Cache options for requests: + * - "default": Use browser's default cache behavior + * - "force-cache": Force cache use without revalidation + * - "no-cache": Validate cached response with server + * - "no-store": Don't use cache + * - "only-if-cached": Use cache only, fail if not cached + * - "reload": Bypass cache for request + */ cache?: RequestCache;app/transports/http/bindings/threads.go (1)
236-242: Consider increasing the Cache-Control max-age value.The current max-age=1 is very short and might lead to unnecessary revalidation requests. Consider increasing this value based on your content update patterns.
- CacheControl: "max-age=1", + CacheControl: "max-age=300", // 5 minutes, adjust based on your needsweb/src/api/openapi-client/threads.ts (1)
161-162: Replacevoidwithundefinedin union types.Using
voidin union types can be confusing. In TypeScript,undefinedis more idiomatic for representing the absence of a value in this context.export type ThreadGetQueryError = - | void + | undefined | UnauthorisedResponse | NotFoundResponse | InternalServerErrorResponse; export const useThreadGet = < TError = - | void + | undefined | UnauthorisedResponse | NotFoundResponse | InternalServerErrorResponse, >Also applies to: 171-172
🧰 Tools
🪛 Biome (1.9.4)
[error] 161-162: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
app/transports/http/openapi/server_gen.go (1)
26799-26812: Consider using a safer string conversion for header values.While the implementation is functional, using
fmt.Sprintfor header values might not handle all edge cases correctly.Consider this safer 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)web/src/api/client.ts (1)
8-17: LGTM! Consider making cache behavior configurable.The implementation correctly leverages browser's built-in caching mechanisms and works well with SWR. However, consider making the cache behavior configurable through the Options type for cases where different caching strategies might be needed.
export const fetcher = async <T>(opts: Options): Promise<T> => { const request = buildRequest({ ...opts, - cache: "default", + cache: opts.cache ?? "default", });
📜 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 (9)
app/resources/cachecontrol/query.go(1 hunks)app/resources/post/thread_cache/cache.go(1 hunks)app/resources/resources.go(2 hunks)app/transports/http/bindings/threads.go(5 hunks)app/transports/http/openapi/server_gen.go(7 hunks)web/src/api/client.ts(1 hunks)web/src/api/common.ts(3 hunks)web/src/api/openapi-client/threads.ts(2 hunks)web/src/api/server.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
web/src/api/openapi-client/threads.ts
[error] 161-162: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 171-172: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (10)
app/resources/resources.go (1)
43-43: LGTM! Clean dependency injection setup.The thread cache integration follows the established dependency injection pattern.
Also applies to: 76-76
app/transports/http/bindings/threads.go (1)
219-226: LGTM! Clean implementation of conditional requests.The implementation correctly handles HTTP conditional requests using If-None-Match and If-Modified-Since headers.
app/transports/http/openapi/server_gen.go (6)
3076-3081: LGTM! Well-defined type aliases for HTTP caching headers.The type definitions for
IfModifiedSinceandIfNoneMatchare correctly defined as string aliases, following HTTP header naming conventions.
3808-3813: LGTM! Well-structured parameter additions for cache control.The new fields are properly defined as optional pointers with appropriate JSON tags and clear documentation.
11462-11486: LGTM! Robust header handling implementation.The implementation correctly sets conditional request headers with proper error handling and parameter styling.
21906-21936: LGTM! Thorough header parsing with validation.The implementation includes proper validation of header value counts and clear error messages.
22284-22293: LGTM! Well-structured response types for cache headers.The response types properly encapsulate all necessary cache control headers.
30319-30694: Generated swagger specification.Skipping review of generated code.
web/src/api/client.ts (2)
8-19: Verify cache implementation scope.The PR objectives mention this is for single-threaded GET requests, but the current implementation applies to all requests. We should either:
- Add a check to only enable caching for GET requests, or
- Document why the broader implementation is intentional.
Let's verify the request types being cached:
✅ Verification successful
Cache implementation correctly aligns with GET-only objective
The current implementation using
cache: "default"is correct because:
- Browser's default caching behavior naturally only applies to GET requests
- Non-GET requests bypass cache by default in browsers
- The implementation is well documented, explaining integration with SWR and browser's HTTP Conditional Requests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for request method checks in the codebase rg -A 5 "method.*GET" --type tsLength of output: 26211
13-15: Please clarify the server-side performance impact.The comment mentions that the experience is "slowed down a bit by the server side behaviour" but doesn't provide details. This information would be valuable for future maintenance and optimization.
Let's check the server-side implementation:
✅ Verification successful
Server-side caching trade-offs are well documented
The server-side implementation uses a 60-second cache with force-cache strategy. This can result in either:
- Best case: Single database read with a 304 Not Modified response
- Worst case: Two database reads when content changes or on first visit
The slight performance impact is a conscious trade-off between data freshness and server load, which will improve once Next.js adds native support for HTTP Conditional Requests and ETag headers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for server-side caching implementation rg -l "ETag|If-None-Match|If-Modified-Since" --type goLength of output: 134
Script:
#!/bin/bash # Check the implementation details cat app/resources/cachecontrol/query.go # Look for any performance-related comments rg -i "performance|slow" -C 3 --type goLength of output: 11064
Script:
#!/bin/bash # Find and check server.ts files fd server.ts # If found, check their content for performance-related comments rg -i "performance|slow|cache|etag" -C 3 --type typescriptLength of output: 232
Script:
#!/bin/bash # Check the content of the server.ts file cat web/src/api/server.tsLength of output: 2382
| func (q Query) NotModified(resourceUpdated time.Time) bool { | ||
| // truncate the resourceUpdated to the nearest second because the actual | ||
| // HTTP header is already truncated but the database time is in nanoseconds. | ||
| // If we didn't do this, the resource updated will always be slightly ahead. | ||
| truncated := resourceUpdated.Truncate(time.Second) | ||
|
|
||
| if ms, ok := q.ModifiedSince.Get(); ok { | ||
|
|
||
| // If the resource update time is ahead of the HTTP Last-Modified check, | ||
| // modified = 1, meaning the resource has been modified since the last | ||
| // request and should be returned from the DB, instead of a 304 status. | ||
| modified := truncated.Compare(ms) | ||
|
|
||
| return modified <= 0 | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing ETag comparison
The NotModified method currently only checks If-Modified-Since. Consider implementing ETag comparison for better caching support.
func (q Query) NotModified(resourceUpdated time.Time) bool {
// truncate the resourceUpdated to the nearest second because the actual
// HTTP header is already truncated but the database time is in nanoseconds.
// If we didn't do this, the resource updated will always be slightly ahead.
truncated := resourceUpdated.Truncate(time.Second)
+ // Check ETag first if available
+ if etag, ok := q.ETag.Get(); ok {
+ // TODO: Implement ETag comparison logic
+ // This would require the resource to provide its current ETag
+ // return resourceEtag == etag
+ }
+
if ms, ok := q.ModifiedSince.Get(); ok {
// If the resource update time is ahead of the HTTP Last-Modified check,
// modified = 1, meaning the resource has been modified since the last
// request and should be returned from the DB, instead of a 304 status.
modified := truncated.Compare(ms)
return modified <= 0
}
return false
}Committable suggestion skipped: line range outside the PR's diff.
