fix: [CODE-5090]: fix forked repo output when upstream is soft deleted#99
Open
abhinavcode wants to merge 1 commit into
Open
fix: [CODE-5090]: fix forked repo output when upstream is soft deleted#99abhinavcode wants to merge 1 commit into
abhinavcode wants to merge 1 commit into
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.
📝 WalkthroughWalkthroughModified GetRepoOutput to treat missing upstream forks as non-fatal by resetting ForkID to 0 when fork lookup returns ErrResourceNotFound; other errors still propagate. Added gitnessstore import alias to check for the specific error. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/controller/repo/helper.go (1)
150-173:⚠️ Potential issue | 🟠 Major
GetRepoOutputWithAccesshas the same bug — soft-deleted upstream still causes a fatal error here.The PR fixes the soft-delete case in
GetRepoOutputbut the siblingGetRepoOutputWithAccessuses an identicalrepoFinder.FindByIDlookup (Line 160) with noErrResourceNotFoundguard. Any caller routed through this path will still receive an error when the upstream is soft-deleted.🐛 Proposed fix — apply the same guard in `GetRepoOutputWithAccess`
func GetRepoOutputWithAccess( ctx context.Context, repoFinder refcache.RepoFinder, isPublic bool, repo *types.Repository, ) (*RepositoryOutput, error) { + repoCopy := *repo + var upstreamRepo *types.RepositoryCore - if repo.ForkID != 0 { - var err error - - upstreamRepo, err = repoFinder.FindByID(ctx, repo.ForkID) - if err != nil { - return nil, fmt.Errorf("failed to find repo fork %d: %w", repo.ForkID, err) + if repoCopy.ForkID != 0 { + var err error + upstreamRepo, err = repoFinder.FindByID(ctx, repoCopy.ForkID) + if errors.Is(err, gitnessstore.ErrResourceNotFound) { + repoCopy.ForkID = 0 + } else if err != nil { + return nil, fmt.Errorf("failed to find repo fork %d: %w", repoCopy.ForkID, err) } } return &RepositoryOutput{ - Repository: *repo, + Repository: repoCopy, IsPublic: isPublic, - Importing: slices.Contains(importingStates, repo.State), - Archived: repo.State == enum.RepoStateArchived, + Importing: slices.Contains(importingStates, repoCopy.State), + Archived: repoCopy.State == enum.RepoStateArchived, Upstream: upstreamRepo, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/controller/repo/helper.go` around lines 150 - 173, GetRepoOutputWithAccess currently calls repoFinder.FindByID for the upstream (in the upstreamRepo lookup) and returns an error if that call fails; mirror the fix applied to GetRepoOutput by special-casing the soft-delete/not-found error: call repoFinder.FindByID(ctx, repo.ForkID) inside the existing block, and if it returns refcache.ErrResourceNotFound (or the package's not-found sentinel) treat upstreamRepo as nil and clear the error, otherwise propagate the error as before; update the error handling around the upstreamRepo assignment in GetRepoOutputWithAccess accordingly.
🧹 Nitpick comments (1)
app/api/controller/repo/helper.go (1)
132-138: Mutatingrepo.ForkIDon the caller's pointer is a hidden side effect.
repois*types.Repository, sorepo.ForkID = 0modifies the caller's object, not just a local copy. The output struct is self-consistent becauseRepository: *repoon line 142 copies the already-mutated value, but any code in the caller that inspectsrepo.ForkIDafter this call will silently observe 0.Consider clearing only the local view by working on a copy, or explicitly documenting this mutation contract:
♻️ Proposed fix — operate on a shallow copy to avoid mutating the caller
func GetRepoOutput( ctx context.Context, publicAccess publicaccess.Service, repoFinder refcache.RepoFinder, repo *types.Repository, ) (*RepositoryOutput, error) { isPublic, err := publicAccess.Get(ctx, enum.PublicResourceTypeRepo, repo.Path) if err != nil { return nil, fmt.Errorf("failed to check if repo is public: %w", err) } + // Work on a local copy so we don't mutate the caller's struct. + repoCopy := *repo var upstreamRepo *types.RepositoryCore - if repo.ForkID != 0 { - upstreamRepo, err = repoFinder.FindByID(ctx, repo.ForkID) + if repoCopy.ForkID != 0 { + upstreamRepo, err = repoFinder.FindByID(ctx, repoCopy.ForkID) if errors.Is(err, gitnessstore.ErrResourceNotFound) { - repo.ForkID = 0 + repoCopy.ForkID = 0 } else if err != nil { - return nil, fmt.Errorf("failed to find repo fork %d: %w", repo.ForkID, err) + return nil, fmt.Errorf("failed to find repo fork %d: %w", repoCopy.ForkID, err) } } return &RepositoryOutput{ - Repository: *repo, + Repository: repoCopy, IsPublic: isPublic, - Importing: slices.Contains(importingStates, repo.State), - Archived: repo.State == enum.RepoStateArchived, + Importing: slices.Contains(importingStates, repoCopy.State), + Archived: repoCopy.State == enum.RepoStateArchived, Upstream: upstreamRepo, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/controller/repo/helper.go` around lines 132 - 138, The code mutates the caller's Repository by setting repo.ForkID = 0 when the upstream is not found; avoid this hidden side effect by working on a shallow copy instead: copy the incoming pointer into a local value (e.g., r := *repo), use r.ForkID for the FindByID logic and set r.ForkID = 0 only on the local copy, and return the copy (or use its fields) instead of modifying the original; alternatively, if mutation is intended, add an explicit comment and update the function contract to document that repo.ForkID may be cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/api/controller/repo/helper.go`:
- Around line 150-173: GetRepoOutputWithAccess currently calls
repoFinder.FindByID for the upstream (in the upstreamRepo lookup) and returns an
error if that call fails; mirror the fix applied to GetRepoOutput by
special-casing the soft-delete/not-found error: call repoFinder.FindByID(ctx,
repo.ForkID) inside the existing block, and if it returns
refcache.ErrResourceNotFound (or the package's not-found sentinel) treat
upstreamRepo as nil and clear the error, otherwise propagate the error as
before; update the error handling around the upstreamRepo assignment in
GetRepoOutputWithAccess accordingly.
---
Nitpick comments:
In `@app/api/controller/repo/helper.go`:
- Around line 132-138: The code mutates the caller's Repository by setting
repo.ForkID = 0 when the upstream is not found; avoid this hidden side effect by
working on a shallow copy instead: copy the incoming pointer into a local value
(e.g., r := *repo), use r.ForkID for the FindByID logic and set r.ForkID = 0
only on the local copy, and return the copy (or use its fields) instead of
modifying the original; alternatively, if mutation is intended, add an explicit
comment and update the function contract to document that repo.ForkID may be
cleared.
abhinavcode
force-pushed
the
mg/fork/fix_repo_output_deleted_upstream
branch
from
f2fb205 to
c700e10
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/controller/repo/helper.go (1)
152-175:⚠️ Potential issue | 🟠 Major
GetRepoOutputWithAccesshas the same bug and needs the same fix.The fork-lookup in
GetRepoOutputWithAccess(lines 162-165) still hard-errors onErrResourceNotFound, so any code path that calls this function for a forked repo with a soft-deleted upstream will continue to fail. The fix applied toGetRepoOutputshould be mirrored here. This affects 8 call sites across repo creation, forking, importing, and public access operations.🐛 Proposed fix for `GetRepoOutputWithAccess`
func GetRepoOutputWithAccess( ctx context.Context, repoFinder refcache.RepoFinder, isPublic bool, repo *types.Repository, ) (*RepositoryOutput, error) { + repoClone := *repo + var upstreamRepo *types.RepositoryCore if repo.ForkID != 0 { var err error upstreamRepo, err = repoFinder.FindByID(ctx, repo.ForkID) - if err != nil { + if errors.Is(err, gitnessstore.ErrResourceNotFound) { + repoClone.ForkID = 0 + } else if err != nil { return nil, fmt.Errorf("failed to find repo fork %d: %w", repo.ForkID, err) } } return &RepositoryOutput{ - Repository: *repo, + Repository: repoClone, IsPublic: isPublic, Importing: slices.Contains(importingStates, repo.State), Archived: repo.State == enum.RepoStateArchived, Upstream: upstreamRepo, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/controller/repo/helper.go` around lines 152 - 175, GetRepoOutputWithAccess currently hard-fails when repoFinder.FindByID returns ErrResourceNotFound for a forked repo; update the upstream lookup in GetRepoOutputWithAccess to mirror GetRepoOutput: call repoFinder.FindByID, and if it returns ErrResourceNotFound treat it as non-fatal (leave upstreamRepo nil) while returning other errors as before. Locate the block that calls repoFinder.FindByID (inside GetRepoOutputWithAccess) and change error handling to check for refcache.ErrResourceNotFound (or the same sentinel used in GetRepoOutput) and only wrap/return the error for unexpected errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/api/controller/repo/helper.go`:
- Around line 152-175: GetRepoOutputWithAccess currently hard-fails when
repoFinder.FindByID returns ErrResourceNotFound for a forked repo; update the
upstream lookup in GetRepoOutputWithAccess to mirror GetRepoOutput: call
repoFinder.FindByID, and if it returns ErrResourceNotFound treat it as non-fatal
(leave upstreamRepo nil) while returning other errors as before. Locate the
block that calls repoFinder.FindByID (inside GetRepoOutputWithAccess) and change
error handling to check for refcache.ErrResourceNotFound (or the same sentinel
used in GetRepoOutput) and only wrap/return the error for unexpected errors.
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