feat(private-api): make the three global reads cacheable - #61
Conversation
pro-members, announcements and post-tips return the same body for every caller, but none of them sent a Cache-Control. A response without one is treated as uncacheable, so clients refetched all three on every page view. - CachePolicy holds one policy per endpoint, with the opt-in kept explicit: most private-api reads are per-user and must stay uncacheable. - CacheWhenOk attaches a policy via OnStarting so it is applied only when the response ends as a 200. Handlers attach it before awaiting the upstream and Pipe() can still turn a transport failure into 504/500 afterwards; caching that would pin an error in front of a healthy endpoint for the whole max-age. - post-tips gains a GET twin. It is a plain read keyed by author/permlink, but a POST response is uncacheable by definition. The POST stays for clients that have not moved over. Announcements get the longest window since they are a compile-time constant, post-tips the shortest since tips arrive at any time.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: deb1f8803e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| app.MapGet("/private-api/waves/trending/authors", PrivateApi.WavesTrendingAuthors); | ||
| // Cacheable twin of the POST /private-api/post-tips below; both hit the | ||
| // same upstream. Registered as GET so the response can be reused. | ||
| app.MapGet("/private-api/post-tips/{author}/{permlink}", PrivateApi.TipsGet); |
There was a problem hiding this comment.
When the parity harness compares this build with the legacy Node image, driver.py derives the case /private-api/post-tips/x/x::get from this route. The legacy service has no matching GET route and returns its HTML fallback, while this handler returns the mock upstream JSON; because this case is absent from KNOWN_DIVERGENCES, every documented Node-vs-C# parity diff now reports a mismatch and exits nonzero. Add the deterministic route addition to the divergence list so the harness remains usable.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThe PR makes three global private-API reads explicitly cacheable while preventing unsuccessful responses from being cached.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs | Adds the cacheable post-tips GET handler and safely constructs upstream paths by escaping segment data and rejecting dot segments. |
| dotnet/EcencyApi/Infrastructure/HttpContextExtensions.cs | Adds deferred Cache-Control application so only final 200 responses receive the selected policy. |
| dotnet/EcencyApi/Infrastructure/CachePolicy.cs | Defines explicit shared-cache policies for the three caller-independent resources. |
| dotnet/EcencyApi/Handlers/Routes.cs | Registers the GET post-tips route without changing the existing POST route. |
| dotnet/EcencyApi.Tests/PostTipsPathTests.cs | Covers ordinary identifiers, escaped structural characters, dot-segment rejection, and inert missing values. |
| dotnet/EcencyApi.Tests/CachePolicyTests.cs | Pins policy shape, successful-response gating, and relative cache durations. |
Sequence Diagram
sequenceDiagram
participant Client
participant API as Vision API
participant Upstream
Client->>API: GET global resource
API->>API: Register CacheWhenOk(policy)
API->>Upstream: Request resource
Upstream-->>API: Response
alt Status is 200
API->>API: Add public Cache-Control
else Non-200 status
API->>API: Omit Cache-Control
end
API-->>Client: Response
Reviews (2): Last reviewed commit: "review: escape tips path segments and re..." | Re-trigger Greptile
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds centralized cache policies, applies them only when responses finish with status 200, and enables caching for announcements, pro-members, and a new GET post-tips endpoint. ChangesPrivate API caching
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Routes
participant PrivateApi
participant ApiClient
Client->>Routes: GET /private-api/post-tips/{author}/{permlink}
Routes->>PrivateApi: invoke TipsGet
PrivateApi->>ApiClient: GET post-tips/{author}/{permlink}
ApiClient-->>PrivateApi: upstream response
PrivateApi-->>Client: response with Cache-Control when status is 200
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
dotnet/EcencyApi.Tests/CachePolicyTests.cs (1)
1-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd integration coverage for the deferred header and GET route.
These tests validate only
CachePolicy; they cannot catchCacheWhenOk/OnStartingwiring failures or a miswiredRoutes→TipsGetendpoint. Add or point to tests covering final 200, 500/504, unmatched routes, and the new GET request.Based on the PR objective to include runtime verification for successful, error, unmatched-route, and control responses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dotnet/EcencyApi.Tests/CachePolicyTests.cs` around lines 1 - 56, Add integration tests covering the GET route wired through Routes to TipsGet and the CacheWhenOk/OnStarting response pipeline. Verify final 200 responses include the expected cache header, 500/504 responses do not, unmatched routes remain unaffected, and the new GET request returns the expected control response; reuse existing integration test fixtures and helpers where available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs`:
- Around line 286-292: Update TipsGet to URI-component-encode both author and
permlink before interpolating them into the post-tips upstream path. Preserve
the existing route extraction, caching, and Upstream.Pipe flow while ensuring
reserved characters cannot alter the request path.
---
Nitpick comments:
In `@dotnet/EcencyApi.Tests/CachePolicyTests.cs`:
- Around line 1-56: Add integration tests covering the GET route wired through
Routes to TipsGet and the CacheWhenOk/OnStarting response pipeline. Verify final
200 responses include the expected cache header, 500/504 responses do not,
unmatched routes remain unaffected, and the new GET request returns the expected
control response; reuse existing integration test fixtures and helpers where
available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 24b78562-a969-4307-834f-efd446896460
📒 Files selected for processing (7)
dotnet/EcencyApi.Tests/CachePolicyTests.csdotnet/EcencyApi/Handlers/Announcements.csdotnet/EcencyApi/Handlers/PrivateApi.Core.csdotnet/EcencyApi/Handlers/PrivateApi.Misc.csdotnet/EcencyApi/Handlers/Routes.csdotnet/EcencyApi/Infrastructure/CachePolicy.csdotnet/EcencyApi/Infrastructure/HttpContextExtensions.cs
Author and permlink were interpolated into the upstream path unescaped, so a value carrying a slash, question mark or hash was re-parsed as URL structure and addressed a different upstream resource, with this service's credentials attached. Both review bots flagged it; it applies to the existing POST as much as to the new GET, so both now go through one helper. Escaping alone is not enough for dot segments. `.` and `..` are unreserved, so EscapeDataString passes them through, and hand-encoding does not help either because Uri decodes %2E back to `.` before it removes dot segments — verified directly, `post-tips/../p` and `post-tips/%2E%2E/p` both resolve one level up. They are rejected with a 400 instead. Real authors and permlinks are unreserved characters, which EscapeDataString leaves byte-identical, so live traffic is unchanged. Also records the new GET route in the parity harness KNOWN_DIVERGENCES: the reference build has no such route and answers with its unmatched-GET template page, which is a deterministic mismatch that would otherwise fail every run.
|
Thanks all. Triage: Greptile (P1) and CodeRabbit (Major), unescaped route values — valid, fixed, and worse than reported. Author and permlink were interpolated into the upstream path raw, so a Worth recording that escaping alone does not fix the dot-segment case Greptile named. So they are rejected with a 400 rather than escaped. Both behaviours are now pinned by tests, including that legitimate authors and permlinks are unreserved characters and come through byte-identical, so live traffic is unchanged. Codex (P2), parity
|
1 participant
pro-members,announcementsandpost-tipsreturn the same body for every caller and need no auth, but none of them sent aCache-Control. A response without one is treated as uncacheable, so every client refetched all three on each page view even though the data had not moved.Changes
CachePolicy— one policy per endpoint. The opt-in is deliberately per endpoint rather than a default: most private-api reads are per-user and must stay uncacheable, andpublichere means shared caches may store the response, so a wrong opt-in would let one visitor's response reach another.CacheWhenOk— attaches a policy throughOnStarting, so it lands only if the response finishes as a200. Handlers attach it before awaiting the upstream, andPipe()can still turn a transport failure into504/500after that point; caching one would pin an error in front of a healthy endpoint for the fullmax-age.GET /private-api/post-tips/{author}/{permlink}— a GET twin of the existing POST, same upstream call and same body. Tips are a plain read keyed by author and permlink, but a POST response is uncacheable by definition. The POST is untouched so current clients keep working. Both params are single route segments, so neither can smuggle a slash into the upstream path.Windows differ by how the data actually changes: announcements are a compile-time constant in this repo so they only move on deploy, tips can arrive at any moment.
Verification
dotnet testgreen (68). Unit tests pin the opt-in rule and the policy shapes, but they cannot coverOnStarting, so the header behaviour was checked against a running instance:GET /private-api/announcements200+public, max-age=1800, stale-while-revalidate=86400GET /private-api/pro-members(upstream unauthenticated locally, so500)Cache-ControlGET /private-api/post-tips/{a}/{p}GET /private-api/post-tips/only-one-segment200template page, i.e. correctly unmatchedGET /private-api/rewarded-communities(control, no policy)Cache-ControlPOST /private-api/post-tips(control)Cache-ControlThe
500rows are the useful ones: they are the error path proving the policy is suppressed when the response is not a 200.Follow-up
A companion vision-next SDK change is needed for clients to benefit from the
post-tipsGET and to align thepro-membersstaleTime. Tracked in ecency/vision-next#1272. Worth confirming whether mobile calls these before the POST is ever retired.Summary by CodeRabbit
New Features
Bug Fixes
Tests