feat: [AH-2845]: add cache for registry UUID#92
Open
abhinavcode wants to merge 1 commit into
Open
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR adds UUID-based registry lookup: a new RegistryUUIDCache type and provider, FindByUUID on RegistryFinder with implementation, mock support for FindByUUID, and DI updates to wire the new cache into the registry finder. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RegistryFinder
participant RegistryUUIDCache
participant RegistryRepo
participant Evictor
Caller->>RegistryFinder: FindByUUID(ctx, uuid)
RegistryFinder->>RegistryUUIDCache: Get(uuid.String())
alt cache hit
RegistryUUIDCache-->>RegistryFinder: *types.Registry
RegistryFinder-->>Caller: *types.Registry
else cache miss
RegistryUUIDCache->>RegistryRepo: FindByUUID(ctx, uuid)
RegistryRepo-->>RegistryUUIDCache: *types.Registry
RegistryUUIDCache-->>RegistryFinder: *types.Registry
RegistryFinder-->>Caller: *types.Registry
end
Note right of Evictor: Evictor may notify cache evictions asynchronously
Evictor--xRegistryUUIDCache: Evict(uuid)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 (2)
registry/app/store/cache/reg_uuid.go (1)
51-51: Optional: use "registry" instead of "repo" in the error message♻️ Suggested rename
- return nil, fmt.Errorf("failed to find repo by UUID: %w", err) + return nil, fmt.Errorf("failed to find registry by UUID: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/store/cache/reg_uuid.go` at line 51, Update the error message that currently returns "failed to find repo by UUID: %w" to use "registry" instead of "repo" so it reads "failed to find registry by UUID: %w"; locate the return statement in reg_uuid.go that constructs this fmt.Errorf and change only the wording (keep the %w error wrapping intact) and update any nearby variable names or tests/expectations that assert on the old message if present.registry/app/services/refcache/reg_finder.go (1)
90-92: UUID format is consistent between lookup and eviction — no fix needed, but consider normalizing for robustnessThe cache key mismatch concern does not apply to the current code. Both the lookup path (
repoUUID.String()) and eviction path (repoCore.UUID) use the hyphenated UUID format:uuid.NewString()generates hyphenated UUIDs that are stored verbatim in the database, anduuid.UUID.String()also returns the hyphenated form. The keys match.However, the reliance on implicit format consistency without explicit normalization is fragile. If Registry.UUID is ever read or stored in a different format (e.g., legacy non-hyphenated data), the cache keys would diverge. Consider normalizing UUID lookups explicitly to guard against this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/services/refcache/reg_finder.go` around lines 90 - 92, The lookup currently uses repoUUID.String() while eviction uses repoCore.UUID; to make keys robust, normalize UUIDs to a single canonical form before using as cache keys. Update registryFinder.FindByUUID to build the cache key from a normalized UUID (parse and re-format to the hyphenated canonical form, e.g., via uuid.Parse/uuid.String), and update the eviction path that uses repoCore.UUID to perform the same normalization so both sides use identical key generation (ensure any case/format differences are normalized).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@registry/app/store/cache/reg_uuid.go`:
- Around line 48-54: The error message in registryUUIDCacheGetter.Find uses
"repo" but the method deals with types.Registry; update the fmt.Errorf call in
registryUUIDCacheGetter.Find (the error returned from regSource.GetByUUID) to
use "failed to find registry by UUID" instead of "failed to find repo by UUID"
so domain terminology matches types.Registry and regSource.GetByUUID.
---
Nitpick comments:
In `@registry/app/services/refcache/reg_finder.go`:
- Around line 90-92: The lookup currently uses repoUUID.String() while eviction
uses repoCore.UUID; to make keys robust, normalize UUIDs to a single canonical
form before using as cache keys. Update registryFinder.FindByUUID to build the
cache key from a normalized UUID (parse and re-format to the hyphenated
canonical form, e.g., via uuid.Parse/uuid.String), and update the eviction path
that uses repoCore.UUID to perform the same normalization so both sides use
identical key generation (ensure any case/format differences are normalized).
In `@registry/app/store/cache/reg_uuid.go`:
- Line 51: Update the error message that currently returns "failed to find repo
by UUID: %w" to use "registry" instead of "repo" so it reads "failed to find
registry by UUID: %w"; locate the return statement in reg_uuid.go that
constructs this fmt.Errorf and change only the wording (keep the %w error
wrapping intact) and update any nearby variable names or tests/expectations that
assert on the old message if present.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/gitness/wire_gen.goregistry/app/api/controller/mocks/registry_finder.goregistry/app/services/refcache/reg_finder.goregistry/app/services/refcache/wire.goregistry/app/store/cache.goregistry/app/store/cache/reg_uuid.goregistry/app/store/cache/wire.go
Comment on lines
+48
to
+54
| func (c registryUUIDCacheGetter) Find(ctx context.Context, repoUUID string) (*types.Registry, error) { | ||
| repo, err := c.regSource.GetByUUID(ctx, repoUUID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to find repo by UUID: %w", err) | ||
| } | ||
|
|
||
| return repo, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "GetByUUID" --type=go -C3Repository: abhinavcode/harness
Length of output: 13761
Use "registry" instead of "repo" in the error message for domain consistency
The error message on line 51 uses "repo" but the method returns *types.Registry. Change "failed to find repo by UUID" to "failed to find registry by UUID" to align with the domain terminology.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/store/cache/reg_uuid.go` around lines 48 - 54, The error message
in registryUUIDCacheGetter.Find uses "repo" but the method deals with
types.Registry; update the fmt.Errorf call in registryUUIDCacheGetter.Find (the
error returned from regSource.GetByUUID) to use "failed to find registry by
UUID" instead of "failed to find repo by UUID" so domain terminology matches
types.Registry and regSource.GetByUUID.
abhinavcode
force-pushed
the
feat-AH-2845
branch
from
6be5c26 to
4175e15
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
registry/app/store/cache/reg_uuid.go (1)
48-54:⚠️ Potential issue | 🟡 Minor
"failed to find repo by UUID"should say"registry", not"repo"The error message uses
repoterminology but the method operates on*types.Registry. This was flagged in a previous review and is still unaddressed.🔧 Proposed fix
- return nil, fmt.Errorf("failed to find repo by UUID: %w", err) + return nil, fmt.Errorf("failed to find registry by UUID: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/store/cache/reg_uuid.go` around lines 48 - 54, The error message in registryUUIDCacheGetter.Find incorrectly refers to "repo" but this method deals with registries; update the fmt.Errorf call after regSource.GetByUUID(ctx, repoUUID) to use "failed to find registry by UUID" (or similar), keeping the wrapped error and return behavior unchanged; reference the registryUUIDCacheGetter.Find method, the regSource.GetByUUID call, and the *types.Registry return type when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@registry/app/store/cache/reg_uuid.go`:
- Around line 48-54: The error message in registryUUIDCacheGetter.Find
incorrectly refers to "repo" but this method deals with registries; update the
fmt.Errorf call after regSource.GetByUUID(ctx, repoUUID) to use "failed to find
registry by UUID" (or similar), keeping the wrapped error and return behavior
unchanged; reference the registryUUIDCacheGetter.Find method, the
regSource.GetByUUID call, and the *types.Registry return type when making the
change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/gitness/wire_gen.goregistry/app/api/controller/mocks/registry_finder.goregistry/app/services/refcache/reg_finder.goregistry/app/services/refcache/wire.goregistry/app/store/cache.goregistry/app/store/cache/reg_uuid.goregistry/app/store/cache/wire.go
🚧 Files skipped from review as they are similar to previous changes (2)
- registry/app/store/cache.go
- registry/app/store/cache/wire.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests