fix: delete root key returns wrong code#3636
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThe changes update the cache interface and implementation to return an explicit cache hit status in addition to the value and error. All usages and wrappers of the cache's SWR method are updated accordingly. Tests are added and updated to verify cache hit/miss/null semantics. Some handler and service logic now branches on the new cache hit status to provide more accurate error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Service
participant Cache
participant Origin
Caller->>Service: Get(ctx, sess, key)
Service->>Cache: SWR(ctx, key, refreshFromOrigin, op)
alt Cache Hit
Cache-->>Service: value, Hit, nil
Service-->>Caller: value, nil
else Cache Null
Cache-->>Service: nil, Null, nil
Service-->>Caller: StatusNotFound, "key does not exist"
else Cache Miss/Error
Cache->>Origin: refreshFromOrigin(ctx)
Origin-->>Cache: value/error
Cache-->>Service: value, Miss, error
Service-->>Caller: value/error
end
Estimated code review effort3 (~45 minutes) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🧠 Learnings (1)go/apps/api/routes/v2_ratelimit_limit/handler.go (4)Learnt from: chronark Learnt from: ogzhanolguncu Learnt from: ogzhanolguncu Learnt from: Flo4604 🧰 Additional context used🧠 Learnings (1)go/apps/api/routes/v2_ratelimit_limit/handler.go (4)Learnt from: chronark Learnt from: ogzhanolguncu Learnt from: ogzhanolguncu Learnt from: Flo4604 ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (5)
✨ Finishing Touches
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. 🪧 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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go(2 hunks)go/apps/api/routes/v2_ratelimit_limit/handler.go(2 hunks)go/internal/services/keys/get.go(3 hunks)go/pkg/cache/cache.go(3 hunks)go/pkg/cache/cache_test.go(4 hunks)go/pkg/cache/interface.go(1 hunks)go/pkg/cache/middleware/tracing.go(1 hunks)go/pkg/cache/noop.go(1 hunks)go/pkg/cache/simulation_test.go(1 hunks)go/pkg/cache/swr_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
go/internal/services/keys/get.go (2)
Learnt from: chronark
PR: #3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.
Learnt from: Flo4604
PR: #3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.464Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
go/pkg/cache/swr_test.go (1)
Learnt from: Flo4604
PR: #3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.464Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
go/apps/api/routes/v2_ratelimit_limit/handler.go (2)
Learnt from: chronark
PR: #2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The cloudflareRatelimiter type definition in apps/api/src/pkg/env.ts should not have its interface changed; it should keep the limit method returning Promise<{ success: boolean }> without additional error properties.
Learnt from: ogzhanolguncu
PR: #2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: When querying or updating namespaces in the Unkey dashboard, always scope the operations to the current workspace using eq(table.workspaceId, ctx.workspace.id) to prevent cross-workspace access.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
go/pkg/cache/simulation_test.go (1)
109-109: LGTM! Clean type inference usage.The removal of explicit type parameters from
cache.Newis a good simplification that leverages Go's type inference while maintaining type safety through the explicitly typedcache.Config[uint64, uint64].go/apps/api/routes/v2_apis_delete_api/cache_validation_test.go (2)
49-53: Correctly updated to test new cache hit status.The test properly captures and asserts the cache hit status from the updated SWR method. The assertion
cache.Hitafter the initial retrieval correctly verifies a cache hit.
74-75: Proper verification of cache invalidation.The test correctly verifies that after API deletion, the cache returns
cache.Null, confirming proper cache invalidation behavior.go/apps/api/routes/v2_ratelimit_limit/handler.go (1)
73-73: Correctly updated to capture cache hit status.The SWR call is properly updated to capture the new cache hit status return value.
go/pkg/cache/cache_test.go (1)
18-18: LGTM! Consistent type inference usage.All
cache.Newcalls have been consistently updated to remove explicit type parameters, leveraging Go's type inference while maintaining type safety through the explicitly typed configuration structs.Also applies to: 36-36, 59-59, 83-83
go/pkg/cache/interface.go (1)
21-21: Core interface fix for cache hit status reporting.This interface change is the fundamental fix that addresses the PR objective. By adding the
hit CacheHitreturn value, the SWR method now properly exposes the cache entry status, enabling callers to distinguish between cache hits, misses, and explicit nulls. This resolves the issue where cache misses were returning null values without proper detection.go/internal/services/keys/get.go (3)
11-11: Import addition looks correct.The cache package import is needed to access the
CacheHittype for the updated SWR method signature.
64-66: Proper handling of the updated SWR method signature.The method now correctly captures the additional
hitparameter from the updated SWR signature, maintaining the existing error handling logic.
83-89: Excellent fix for the core issue described in the PR.This explicit handling of
cache.Nullstatus directly addresses the bug where cache misses were not properly detected. When a key doesn't exist and was previously cached as null, the system now correctly returnsStatusNotFoundinstead of silently returning a null value.go/pkg/cache/noop.go (1)
25-28: Correct implementation of the updated SWR signature for noop cache.The method signature properly includes the additional
CacheHitreturn value, and returningMissis the correct behavior for a no-operation cache that never actually stores data.go/pkg/cache/middleware/tracing.go (1)
87-102: Proper propagation of cache hit status through tracing middleware.The middleware correctly updates the SWR signature to include the
CacheHitreturn value and properly propagates it from the underlying cache implementation. The tracing logic remains appropriately unchanged.go/pkg/cache/cache.go (3)
242-242: Updated method signature correctly includes cache hit status.The SWR method now properly returns
CacheHitstatus alongside the cached value and error, enabling callers to distinguish between different cache states.
250-250: Proper propagation of cached entry hit status.For both fresh and stale cache hits, the method correctly returns
e.Hitfrom the cached entry, preserving the original cache hit status that was stored with the data.Also applies to: 263-263
284-301: Well-implemented cache hit status determination for cache misses.The logic correctly handles different scenarios:
- Returns
Missstatus when errors occur during refresh- Determines hit status based on the operation:
WriteValue→Hit,WriteNull→Null, default →Miss- This ensures that null cache entries (when keys don't exist) are properly identified with
NullstatusThis directly addresses the core issue described in the PR objectives.
go/pkg/cache/swr_test.go (1)
17-157: Comprehensive test suite excellently validates the cache hit status functionality.This test suite thoroughly covers all important SWR cache scenarios:
- Cache miss to hit transition: Tests initial miss that properly caches as
Hitstatus- Fresh cache hits: Validates that fresh entries return cached data with correct hit status
- Null cache hits: Critical test for the PR's main fix - ensures null entries (non-existent keys) return
Nullstatus- Stale behavior: Confirms stale entries return cached data while triggering background refresh
- Expiration handling: Tests cache miss behavior after stale period
- Error scenarios: Validates that errors during refresh return
MissstatusThe use of mock clock for time-based testing is excellent practice, and the assertions properly validate the three-tuple return (value, hit status, error).
This test suite provides strong confidence that the cache hit status feature works correctly and addresses the original bug described in the PR objectives.
|
|
Merge activity
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (07/22/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
|
Ah ill fix merge conflicts |
…e-correct-error-in-v2
Flo4604
deleted the
eng-1942-a-deleted-root-key-should-return-the-correct-error-in-v2
branch
2 participants
What does this PR do?
Fixes #3625
The issue was that swr didn't tell us the status code of the cache entry so if we did a miss and set the key to null bc it doesnt exist we would just return a null value and not detect that.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor