[feat]: [AH-2903]: add support for upload/download for raw files#100
Open
abhinavcode wants to merge 16 commits into
Open
[feat]: [AH-2903]: add support for upload/download for raw files#100abhinavcode wants to merge 16 commits into
abhinavcode wants to merge 16 commits 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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds raw file upload/download support for non‑GENERIC package types, refactors artifact-info resolution with a new buildFileArtifactInfo helper, updates routing for /files to a flat layout, extends LocalBase with UploadRawFile/DownloadRawFile, and propagates a new packageWrapper dependency and file‑support checks. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as Handler\n(GetGenericArtifactInfoV2)
participant Resolver as Space/Registry\n(resolver)
participant TypeCheck as PackageType\n(detector)
participant RawBuilder as buildFileArtifactInfo
participant GenericFlow as GENERIC\n(flow)
participant LocalBase as LocalBase\n(Upload/DownloadRawFile)
participant Storage as File\nStorage
Client->>Handler: HTTP file request
Handler->>Resolver: resolve root space & registry
Handler->>TypeCheck: determine package type
alt Non-GENERIC
TypeCheck->>RawBuilder: delegate to buildFileArtifactInfo
RawBuilder->>RawBuilder: validate path, assemble filePath/fileName
RawBuilder->>LocalBase: UploadRawFile / DownloadRawFile(filePath)
LocalBase->>Storage: upload/download by filePath
Storage-->>LocalBase: SHA256 or FileReader + redirect
LocalBase-->>RawBuilder: headers, sha/url
else GENERIC
TypeCheck->>GenericFlow: delegate to GENERIC flow
GenericFlow->>LocalBase: existing GENERIC upload/download
LocalBase->>Storage: store/retrieve by artifact layout
Storage-->>LocalBase: response
end
Handler-->>Client: respond (status / artifact info / redirect)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
registry/app/api/handler/generic/base.go (1)
373-383:⚠️ Potential issue | 🔴 CriticalRouting condition incorrectly rejects valid non-GENERIC paths with fewer than 7 segments.
The first branch requires
len(splits) >= 7, which is right for GENERIC paths (/pkg/root/reg/files/pkg/version/file). However, non-GENERIC raw file paths only need one path segment afterfiles/, e.g.,/pkg/root/reg/files/artifact.zip→len(splits) == 5. This falls through to the second branch at Line 379, which matches (len >= 4 && splits[3] == "files") and returns a misleading "expected package/version/file" error, completely breaking non-GENERIC single-segment uploads/downloads.The condition should route to
GetGenericArtifactInfoV2wheneverlen(splits) >= 5(at least one path component afterfiles/), and the>= 7guard should only apply to GENERIC type validation withinGetGenericArtifactInfoV2itself (which already does this).🐛 Proposed fix
- if len(splits) >= 7 && splits[0] == "pkg" && splits[3] == "files" { + if len(splits) >= 5 && splits[0] == "pkg" && splits[3] == "files" { artifactInfo, err := h.GetGenericArtifactInfoV2(r) if err != nil { return nil, fmt.Errorf("failed to get generic artifact info: %w", err) } return artifactInfo, nil - } else if len(splits) >= 4 && splits[0] == "pkg" && splits[3] == "files" { - // Missing required path segments (version or filepath) - return nil, usererror.BadRequestf( - "Invalid request path: expected /pkg/{root}/{registry}/files/{package}/{version}/{filepath}, got: %s", - r.URL.Path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/handler/generic/base.go` around lines 373 - 383, The routing check incorrectly requires len(splits) >= 7 before calling GetGenericArtifactInfoV2, which rejects valid non-GENERIC file paths; change the first branch condition to call GetGenericArtifactInfoV2 when len(splits) >= 5 and splits[0] == "pkg" && splits[3] == "files" (so any request with at least one path segment after "files" is routed to GetGenericArtifactInfoV2), and let GetGenericArtifactInfoV2 perform stricter GENERIC-specific validation (remove or update the redundant else-if that repeats splits[3] == "files" so it no longer misreports valid single-segment file requests).registry/app/pkg/generic/local.go (1)
81-83:⚠️ Potential issue | 🟠 Major
GetPackageTypes()advertises onlyGENERIC, butPutFile/DownloadFilehandle non-GENERIC types.The methods explicitly route non-GENERIC package types (lines 92 and 115) to
uploadRawFile()anddownloadRawFile(), yetGetPackageTypes()returns onlyPackageTypeGENERIC. This creates a dispatch inconsistency: theRegister()function inregistry/app/pkg/base/wrapper.go(lines 39-42) usesGetPackageTypes()to populate the routing registry. Non-GENERIC types will not be registered for this registry, causing dispatch logic to incorrectly believe this registry cannot handle those types despite being able to do so.Update
GetPackageTypes()to return all package types that are actually supported by thePutFileandDownloadFileimplementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/generic/local.go` around lines 81 - 83, GetPackageTypes currently only lists artifact.PackageTypeGENERIC but PutFile and DownloadFile route non-GENERIC types to uploadRawFile/downloadRawFile, so update GetPackageTypes() in localRegistry to return every artifact.PackageType that PutFile/DownloadFile actually support (include GENERIC and the other concrete PackageType values used by uploadRawFile/downloadRawFile) so the registry registration (Register) will correctly advertise this backend; reference the GetPackageTypes function and ensure its returned slice includes the same package types handled by PutFile/DownloadFile and their internal helpers uploadRawFile/downloadRawFile.
🧹 Nitpick comments (4)
registry/app/api/handler/generic/base.go (2)
348-353: Redundant empty-field check — unreachable after prior validation.By Line 348,
packageName,version,fileName, andfilePathare guaranteed non-empty:packageNameandversionpassedvalidatePackageVersionV2(Line 318),filePathpassedvalidateFilePath(Line 323), andfileNameis the last element ofremainingSegmentswhose length was checked. Theinfo.Image == "" || ...guard will never fire and should be removed.♻️ Proposed removal
log.Ctx(ctx).Info().Msgf("Dispatch: URI: %s", path) - if info.Image == "" || info.Version == "" || info.FileName == "" || info.FilePath == "" { - log.Ctx(ctx).Warn().Msgf("Invalid request") - return generic2.ArtifactInfo{}, usererror.BadRequestf("Invalid request Image: %q, Version: %q, FileName: %q", - info.Image, info.Version, - info.FileName) - } - if info.Image != "" && info.Version != "" && info.FilePath != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/handler/generic/base.go` around lines 348 - 353, Remove the redundant empty-field guard that checks info.Image, info.Version, info.FileName, and info.FilePath in the function (the block starting with "if info.Image == "" || ..."). Those fields are already guaranteed non-empty by earlier validations (validatePackageVersionV2, validateFilePath and the remainingSegments length check), so delete this conditional and its log/BadRequest return to avoid unreachable code; keep the surrounding logic intact and rely on the prior validators to enforce non-empty values.
262-289: Double space/registry DB lookup when delegating toGetFileArtifactInfoV2.
GetGenericArtifactInfoV2resolvesrootSpace(Line 271) andregistry(Line 277), then for non-GENERIC types immediately delegates toGetFileArtifactInfoV2(Line 288), which resolves both again (Lines 203–215). This results in 4 DB calls instead of 2 on every non-GENERIC request.Consider passing the already-resolved
registryintoGetFileArtifactInfoV2(or extracting a shared private helper), to avoid the duplicate lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/handler/generic/base.go` around lines 262 - 289, GetGenericArtifactInfoV2 currently resolves rootSpace via SpaceFinder.FindByRef and registry via RegistryFinder.FindByRootRef and then immediately calls GetFileArtifactInfoV2 for non-GENERIC types, causing duplicate DB lookups because GetFileArtifactInfoV2 re-resolves the same values; refactor by adding an overload or helper so GetFileArtifactInfoV2 can accept an already-resolved registry (or factor out the shared resolution into a private helper used by both functions), update calls to pass the registry variable when delegating from GetGenericArtifactInfoV2, and remove the redundant FindByRef/FindByRootRef calls in the delegated path.registry/app/pkg/base/base.go (1)
216-219: Double registry lookup on every raw file operation.Both
UploadRawFileandDownloadRawFilere-executeregistryFinder.FindByRootParentID, even thoughGetFileArtifactInfoV2already resolved and embedded theregistryininfo.ArtifactInfo.Registry. Consider passing the already-resolvedregistry.IDdirectly viainfo.RegistryIDto avoid redundant DB round trips.Also applies to: 261-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/base/base.go` around lines 216 - 219, UploadRawFile and DownloadRawFile each re-run registryFinder.FindByRootParentID even though GetFileArtifactInfoV2 already resolved the registry into info.ArtifactInfo.Registry; change the flow to avoid the duplicate DB lookup by propagating the resolved registry ID from GetFileArtifactInfoV2 into the returned info (e.g., set info.RegistryID = info.ArtifactInfo.Registry.ID) and update UploadRawFile and DownloadRawFile to use info.RegistryID instead of calling registryFinder.FindByRootParentID; ensure any existing code paths that still need the full registry object fall back to using the embedded info.ArtifactInfo.Registry when present.registry/app/api/router/packages/route.go (1)
97-111: Missing bandwidth stat tracking for/filesuploads and downloads.The old
/genericroute block appliedTrackBandwidthStatForGenericArtifactsmiddleware for both upload and download. The new/filesblock omits it entirely. Bandwidth metrics will be silently absent for all raw file operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/router/packages/route.go` around lines 97 - 111, The /files route block is missing the TrackBandwidthStatForGenericArtifacts middleware, so add middleware.TrackBandwidthStatForGenericArtifacts(packageHandler) into the middleware chain for raw artifact uploads and downloads: include it in the .With(...) chains that build Get("/*", genericHandler.GetFile) and Head("/*", genericHandler.HeadFile) for downloads and in the chain for Put("/*", genericHandler.PutFile) for uploads (use the same placement pattern as middleware.CheckQuarantineStatus and middleware.RequestPackageAccess around packageHandler and genericHandler).
🤖 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/pkg/base/base.go`:
- Around line 232-242: The call to request.AuthSessionFrom(ctx) in base.go
ignores the returned error and may allow session.Principal.ID to be zero when
calling l.fileManager.UploadFile; change code around AuthSessionFrom to capture
and check the error (AuthSessionFrom) and, if no session is present, either
return an error or enforce a fallback (reject upload or require authentication)
before calling l.fileManager.UploadFile so you never pass a zero principal ID
(session.Principal.ID) into UploadFile; ensure the handler/function returns a
clear error when the session is missing.
In `@registry/app/pkg/generic/local.go`:
- Line 150: The call to c.localBase.UploadRawFile(ctx, info.ArtifactInfo,
info.FilePath, reader, true) hardcodes failOnConflict=true so raw files can
never be overwritten; change this to pass a configurable boolean instead (e.g.,
a field on the incoming info struct or an explicit parameter) and wire that
through to UploadRawFile so callers can opt into overwrite semantics; update the
caller site in registry/app/pkg/generic/local.go (the UploadRawFile invocation)
and any upstream call sites or API surface that creates the info/parameters so
the overwrite flag is plumbed through.
---
Outside diff comments:
In `@registry/app/api/handler/generic/base.go`:
- Around line 373-383: The routing check incorrectly requires len(splits) >= 7
before calling GetGenericArtifactInfoV2, which rejects valid non-GENERIC file
paths; change the first branch condition to call GetGenericArtifactInfoV2 when
len(splits) >= 5 and splits[0] == "pkg" && splits[3] == "files" (so any request
with at least one path segment after "files" is routed to
GetGenericArtifactInfoV2), and let GetGenericArtifactInfoV2 perform stricter
GENERIC-specific validation (remove or update the redundant else-if that repeats
splits[3] == "files" so it no longer misreports valid single-segment file
requests).
In `@registry/app/pkg/generic/local.go`:
- Around line 81-83: GetPackageTypes currently only lists
artifact.PackageTypeGENERIC but PutFile and DownloadFile route non-GENERIC types
to uploadRawFile/downloadRawFile, so update GetPackageTypes() in localRegistry
to return every artifact.PackageType that PutFile/DownloadFile actually support
(include GENERIC and the other concrete PackageType values used by
uploadRawFile/downloadRawFile) so the registry registration (Register) will
correctly advertise this backend; reference the GetPackageTypes function and
ensure its returned slice includes the same package types handled by
PutFile/DownloadFile and their internal helpers uploadRawFile/downloadRawFile.
---
Nitpick comments:
In `@registry/app/api/handler/generic/base.go`:
- Around line 348-353: Remove the redundant empty-field guard that checks
info.Image, info.Version, info.FileName, and info.FilePath in the function (the
block starting with "if info.Image == "" || ..."). Those fields are already
guaranteed non-empty by earlier validations (validatePackageVersionV2,
validateFilePath and the remainingSegments length check), so delete this
conditional and its log/BadRequest return to avoid unreachable code; keep the
surrounding logic intact and rely on the prior validators to enforce non-empty
values.
- Around line 262-289: GetGenericArtifactInfoV2 currently resolves rootSpace via
SpaceFinder.FindByRef and registry via RegistryFinder.FindByRootRef and then
immediately calls GetFileArtifactInfoV2 for non-GENERIC types, causing duplicate
DB lookups because GetFileArtifactInfoV2 re-resolves the same values; refactor
by adding an overload or helper so GetFileArtifactInfoV2 can accept an
already-resolved registry (or factor out the shared resolution into a private
helper used by both functions), update calls to pass the registry variable when
delegating from GetGenericArtifactInfoV2, and remove the redundant
FindByRef/FindByRootRef calls in the delegated path.
In `@registry/app/api/router/packages/route.go`:
- Around line 97-111: The /files route block is missing the
TrackBandwidthStatForGenericArtifacts middleware, so add
middleware.TrackBandwidthStatForGenericArtifacts(packageHandler) into the
middleware chain for raw artifact uploads and downloads: include it in the
.With(...) chains that build Get("/*", genericHandler.GetFile) and Head("/*",
genericHandler.HeadFile) for downloads and in the chain for Put("/*",
genericHandler.PutFile) for uploads (use the same placement pattern as
middleware.CheckQuarantineStatus and middleware.RequestPackageAccess around
packageHandler and genericHandler).
In `@registry/app/pkg/base/base.go`:
- Around line 216-219: UploadRawFile and DownloadRawFile each re-run
registryFinder.FindByRootParentID even though GetFileArtifactInfoV2 already
resolved the registry into info.ArtifactInfo.Registry; change the flow to avoid
the duplicate DB lookup by propagating the resolved registry ID from
GetFileArtifactInfoV2 into the returned info (e.g., set info.RegistryID =
info.ArtifactInfo.Registry.ID) and update UploadRawFile and DownloadRawFile to
use info.RegistryID instead of calling registryFinder.FindByRootParentID; ensure
any existing code paths that still need the full registry object fall back to
using the embedded info.ArtifactInfo.Registry when present.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
registry/app/api/handler/generic/base.goregistry/app/api/router/packages/route.goregistry/app/pkg/base/base.goregistry/app/pkg/base/utils.goregistry/app/pkg/generic/local.go
| Msgf("Uploading raw file for package type: %s, filePath: %s", | ||
| info.Registry.PackageType, info.FilePath) | ||
|
|
||
| headers, sha256, err := c.localBase.UploadRawFile(ctx, info.ArtifactInfo, info.FilePath, reader, true) |
There was a problem hiding this comment.
failOnConflict is hardcoded to true — raw files can never be updated.
Every re-upload of an existing raw file will return a 409 Conflict with no way for callers to opt into an overwrite. If update semantics are needed (e.g., CI pushing a new build artifact to the same path), there is currently no path to do so.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/pkg/generic/local.go` at line 150, The call to
c.localBase.UploadRawFile(ctx, info.ArtifactInfo, info.FilePath, reader, true)
hardcodes failOnConflict=true so raw files can never be overwritten; change this
to pass a configurable boolean instead (e.g., a field on the incoming info
struct or an explicit parameter) and wire that through to UploadRawFile so
callers can opt into overwrite semantics; update the caller site in
registry/app/pkg/generic/local.go (the UploadRawFile invocation) and any
upstream call sites or API surface that creates the info/parameters so the
overwrite flag is plumbed through.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
registry/app/api/handler/generic/base.go (1)
330-334:⚠️ Potential issue | 🔴 CriticalSame
splits[4:]panic risk as inbuildFileArtifactInfo.The GENERIC path in
GetGenericArtifactInfoV2also accessessplits[4:]without a prior length check. Add the same bounds guard (or extract a shared helper) here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/handler/generic/base.go` around lines 330 - 334, GetGenericArtifactInfoV2 currently slices splits[4:] without verifying splits has at least 5 elements, risking a panic; add the same bounds guard used in buildFileArtifactInfo (or extract a helper like validatePathSegments) to check len(splits) >= 5 before computing remainingSegments := splits[4:], and return a usererror.BadRequestf("Invalid request: no file path provided") (or equivalent) when the check fails so the function safely handles short paths.
🤖 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/api/handler/generic/base.go`:
- Around line 196-221: GetFileArtifactInfoV2 forwards URL path segments (splits)
to buildFileArtifactInfo without ensuring there are at least 4 segments, which
can cause a panic when buildFileArtifactInfo slices splits[4:]; update
buildFileArtifactInfo to validate len(splits) (or the specific index access)
before using splits[4:] and return a user-friendly NotFound or BadRequest error
when the path is too short; reference the functions
Handler.GetFileArtifactInfoV2 and Handler.buildFileArtifactInfo and the variable
splits/splits[4:] when making the change.
- Around line 353-356: The fmt.Errorf wrapping around validatePackageVersionV2's
error discards the usererror HTTP 400; instead, when
validatePackageVersionV2(packageName, version) returns an error, propagate it as
a usererror (preserve the *usererror.Error) or convert it to a
usererror.BadRequestf so callers see a 400. Update the branch that currently
returns generic2.ArtifactInfo{} with fmt.Errorf to either (1) detect and return
the original *usererror.Error from validatePackageVersionV2, or (2) always
return usererror.BadRequestf("invalid image name/version/fileName: %q/%q: %v",
packageName, version, err) so the HTTP 400 status is preserved (fix the code
around validatePackageVersionV2 and the fmt.Errorf usage).
In `@registry/app/pkg/base/base.go`:
- Around line 226-250: Move the authentication gate before the file-existence
probe to avoid unauthenticated path probing: call request.AuthSessionFrom(ctx)
first and return usererror.ErrUnauthorized (with the same logging used now) if
unauthenticated, then call l.ExistsByFilePath(ctx, registry.ID, filePath) and
proceed with the existing failOnConflict handling; ensure you preserve the
existing return values (responseHeaders, "", error types) and keep the same log
messages around AuthSessionFrom and ExistsByFilePath so behavior and diagnostics
remain consistent.
---
Duplicate comments:
In `@registry/app/api/handler/generic/base.go`:
- Around line 330-334: GetGenericArtifactInfoV2 currently slices splits[4:]
without verifying splits has at least 5 elements, risking a panic; add the same
bounds guard used in buildFileArtifactInfo (or extract a helper like
validatePathSegments) to check len(splits) >= 5 before computing
remainingSegments := splits[4:], and return a usererror.BadRequestf("Invalid
request: no file path provided") (or equivalent) when the check fails so the
function safely handles short paths.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
registry/app/api/handler/generic/base.goregistry/app/pkg/base/base.goregistry/app/pkg/generic/local.go
🚧 Files skipped from review as they are similar to previous changes (1)
- registry/app/pkg/generic/local.go
Comment on lines
+226
to
+250
| exists, err := l.ExistsByFilePath(ctx, registry.ID, filePath) | ||
| if err != nil { | ||
| log.Ctx(ctx).Error().Err(err). | ||
| Int64("registryID", registry.ID). | ||
| Str("filePath", filePath). | ||
| Msg("Failed to check if file exists") | ||
| return responseHeaders, "", errcode.ErrCodeUnknown.WithDetail(err) | ||
| } | ||
|
|
||
| if exists && failOnConflict { | ||
| log.Ctx(ctx).Warn(). | ||
| Str("filePath", filePath). | ||
| Int64("registryID", registry.ID). | ||
| Msg("File already exists and failOnConflict is true") | ||
| return responseHeaders, "", usererror.Conflict( | ||
| fmt.Sprintf("File already exists at path: %s", filePath)) | ||
| } | ||
|
|
||
| session, ok := request.AuthSessionFrom(ctx) | ||
| if !ok { | ||
| log.Ctx(ctx).Error(). | ||
| Str("filePath", filePath). | ||
| Msg("Authentication required for raw file upload") | ||
| return responseHeaders, "", usererror.ErrUnauthorized | ||
| } |
There was a problem hiding this comment.
Auth check should precede the existence check to prevent unauthenticated path probing.
An unauthenticated caller can distinguish between a 409 Conflict response (file exists) and a 401 Unauthorized response (file doesn't exist or wrong path), leaking file-existence information without any authentication.
🛡️ Proposed fix: move auth check before existence check
+ session, ok := request.AuthSessionFrom(ctx)
+ if !ok {
+ log.Ctx(ctx).Error().
+ Str("filePath", filePath).
+ Msg("Authentication required for raw file upload")
+ return responseHeaders, "", usererror.ErrUnauthorized
+ }
+
// Check if file already exists
exists, err := l.ExistsByFilePath(ctx, registry.ID, filePath)
if err != nil {
...
}
if exists && failOnConflict {
...
}
- session, ok := request.AuthSessionFrom(ctx)
- if !ok {
- ...
- }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/pkg/base/base.go` around lines 226 - 250, Move the
authentication gate before the file-existence probe to avoid unauthenticated
path probing: call request.AuthSessionFrom(ctx) first and return
usererror.ErrUnauthorized (with the same logging used now) if unauthenticated,
then call l.ExistsByFilePath(ctx, registry.ID, filePath) and proceed with the
existing failOnConflict handling; ensure you preserve the existing return values
(responseHeaders, "", error types) and keep the same log messages around
AuthSessionFrom and ExistsByFilePath so behavior and diagnostics remain
consistent.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
registry/app/pkg/base/base.go (1)
215-237:⚠️ Potential issue | 🟠 MajorAuth check should precede the existence check to prevent unauthenticated path probing.
An unauthenticated caller can distinguish between a
409 Conflict(file exists) and a401 Unauthorized(no session), leaking file-existence information. Move the auth check beforeExistsByFilePath.🛡️ Proposed fix: reorder auth and existence checks
func (l *localBase) UploadRawFile( ctx context.Context, info pkg.ArtifactInfo, filePath string, file io.ReadCloser, failOnConflict bool, ) (*commons.ResponseHeaders, string, error) { responseHeaders := &commons.ResponseHeaders{ Headers: make(map[string]string), Code: 0, } + session, ok := request.AuthSessionFrom(ctx) + if !ok { + log.Ctx(ctx).Error(). + Str("filePath", filePath). + Msg("Authentication required for raw file upload") + return responseHeaders, "", usererror.ErrUnauthorized + } exists, err := l.ExistsByFilePath(ctx, info.RegistryID, filePath) if err != nil { log.Ctx(ctx).Error().Err(err). Int64("registryID", info.RegistryID). Str("filePath", filePath). Msg("Failed to check if file exists") return responseHeaders, "", errcode.ErrCodeUnknown.WithDetail(err) } if exists && failOnConflict { log.Ctx(ctx).Warn(). Str("filePath", filePath). Int64("registryID", info.RegistryID). Msg("File already exists and failOnConflict is true") return responseHeaders, "", usererror.Conflict( fmt.Sprintf("File already exists at path: %s", filePath)) } - session, ok := request.AuthSessionFrom(ctx) - if !ok { - log.Ctx(ctx).Error(). - Str("filePath", filePath). - Msg("Authentication required for raw file upload") - return responseHeaders, "", usererror.ErrUnauthorized - } fileInfo, err := l.fileManager.UploadFile(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/base/base.go` around lines 215 - 237, Reorder the authentication and existence checks so the request.AuthSessionFrom(ctx) call happens before calling l.ExistsByFilePath(ctx, info.RegistryID, filePath); specifically, call request.AuthSessionFrom and return usererror.ErrUnauthorized if no session before invoking l.ExistsByFilePath, then proceed to check exists and handle failOnConflict as before; ensure log messages remain appropriate (use the same log lines around AuthSessionFrom and ExistsByFilePath) and preserve the same error/return behavior for the existence-check path.registry/app/api/handler/generic/base.go (2)
248-251:⚠️ Potential issue | 🟠 Major
fmt.Errorfwrapping strips the HTTP 400 status fromusererror.
validatePackageVersionV2returns a*usererror.Error(HTTP 400), but wrapping it withfmt.Errorfdiscards the HTTP status. Callers will receive a generic error, likely causing a 500 instead of 400.🐛 Proposed fix
if err = validatePackageVersionV2(packageName, version); err != nil { - return generic2.ArtifactInfo{}, fmt.Errorf("invalid package name/version/fileName: %q/%q %w", packageName, - version, err) + return generic2.ArtifactInfo{}, usererror.BadRequestf("invalid package name/version/fileName: %q/%q: %v", + packageName, version, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/handler/generic/base.go` around lines 248 - 251, The fmt.Errorf wrapper around the error returned by validatePackageVersionV2 loses the original *usererror.Error semantics (HTTP 400) for callers; instead do not wrap the error with fmt.Errorf—either return the original err directly from the function or attach context using a helper that preserves the usererror type (e.g., usererror.WithMessage or equivalent) so validatePackageVersionV2's *usererror.Error remains detectable by callers.
309-313:⚠️ Potential issue | 🟡 MinorMissing bounds check before
splits[4:]could cause panic on malformed paths.While the router typically provides well-formed paths, if
splitshas fewer than 4 elements,splits[4:]will panic. Add a defensive guard.🛡️ Proposed fix
func (h *Handler) buildFileArtifactInfo( ctx context.Context, registry *regtypes.Registry, rootSpace *coretypes.SpaceCore, rootIdentifier string, registryIdentifier string, path string, splits []string, ) (generic2.ArtifactInfo, error) { // Path format: /pkg/{rootIdentifier}/{registryIdentifier}/files/* // splits[0] = "pkg", splits[1] = rootIdentifier, splits[2] = registryIdentifier, splits[3] = "files" // Remaining path starts at splits[4] + if len(splits) < 4 { + return generic2.ArtifactInfo{}, usererror.BadRequestf( + "Invalid request path: expected /pkg/{root}/{registry}/files/..., got: %s", path) + } remainingSegments := splits[4:]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/handler/generic/base.go` around lines 309 - 313, The code slices splits with splits[4:] without validating length which can panic on malformed paths; add a defensive guard before computing remainingSegments (check len(splits) >= 4) and return a clear error (or handle it) when the path is too short. Update the logic around the remainingSegments assignment (the code that sets remainingSegments := splits[4:]) to first validate splits, and reference the variables splits and remainingSegments (and the surrounding handler/function that computes ArtifactInfo) when making the change.
🧹 Nitpick comments (1)
registry/app/api/handler/generic/base_test.go (1)
405-493: Test verifies parsing logic inline rather than testing handler methods.This test duplicates the path parsing logic instead of testing the actual
GetGenericArtifactInfoV2orbuildFileArtifactInfomethods. While useful as a specification, it won't catch regressions if the handler's parsing logic diverges.Consider refactoring to test the actual handler methods with mock dependencies, or at minimum, extract the parsing logic into a shared helper that both the test and handler use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/handler/generic/base_test.go` around lines 405 - 493, The test duplicates inline path-parsing logic instead of exercising the real parsing used by the handler; extract the parsing into a single helper (e.g., ParseArtifactPath or similar) or reuse the existing handler logic by calling GetGenericArtifactInfoV2/buildFileArtifactInfo, then update the test to call that helper or the handler with minimal mocks; specifically, move the segment-splitting and package/version/filePath derivation out of TestURLPathParsing into a shared function, update GetGenericArtifactInfoV2/buildFileArtifactInfo to call that helper, and replace the inline assertions in TestURLPathParsing with calls to the helper or to the handler (using mocked dependencies) so the test verifies the actual implementation rather than a duplicated copy.
🤖 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/api/handler/generic/base.go`:
- Around line 248-251: The fmt.Errorf wrapper around the error returned by
validatePackageVersionV2 loses the original *usererror.Error semantics (HTTP
400) for callers; instead do not wrap the error with fmt.Errorf—either return
the original err directly from the function or attach context using a helper
that preserves the usererror type (e.g., usererror.WithMessage or equivalent) so
validatePackageVersionV2's *usererror.Error remains detectable by callers.
- Around line 309-313: The code slices splits with splits[4:] without validating
length which can panic on malformed paths; add a defensive guard before
computing remainingSegments (check len(splits) >= 4) and return a clear error
(or handle it) when the path is too short. Update the logic around the
remainingSegments assignment (the code that sets remainingSegments :=
splits[4:]) to first validate splits, and reference the variables splits and
remainingSegments (and the surrounding handler/function that computes
ArtifactInfo) when making the change.
In `@registry/app/pkg/base/base.go`:
- Around line 215-237: Reorder the authentication and existence checks so the
request.AuthSessionFrom(ctx) call happens before calling l.ExistsByFilePath(ctx,
info.RegistryID, filePath); specifically, call request.AuthSessionFrom and
return usererror.ErrUnauthorized if no session before invoking
l.ExistsByFilePath, then proceed to check exists and handle failOnConflict as
before; ensure log messages remain appropriate (use the same log lines around
AuthSessionFrom and ExistsByFilePath) and preserve the same error/return
behavior for the existence-check path.
---
Nitpick comments:
In `@registry/app/api/handler/generic/base_test.go`:
- Around line 405-493: The test duplicates inline path-parsing logic instead of
exercising the real parsing used by the handler; extract the parsing into a
single helper (e.g., ParseArtifactPath or similar) or reuse the existing handler
logic by calling GetGenericArtifactInfoV2/buildFileArtifactInfo, then update the
test to call that helper or the handler with minimal mocks; specifically, move
the segment-splitting and package/version/filePath derivation out of
TestURLPathParsing into a shared function, update
GetGenericArtifactInfoV2/buildFileArtifactInfo to call that helper, and replace
the inline assertions in TestURLPathParsing with calls to the helper or to the
handler (using mocked dependencies) so the test verifies the actual
implementation rather than a duplicated copy.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
registry/app/api/handler/generic/base.goregistry/app/api/handler/generic/base_test.goregistry/app/pkg/base/base.goregistry/app/pkg/base/utils.goregistry/app/pkg/types/generic/types.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/api/controller/pkg/generic/v2.go`:
- Around line 48-50: The code performs an unchecked type assertion on
base.Factory(key) into generic2.Registry before calling methods like
DownloadFile, HeadFile, DeleteFile, and PutFile, which can cause a nil pointer
panic; update each use (the DownloadFile call where genericRegistry is assigned,
and the analogous places for HeadFile, DeleteFile, PutFile) to perform the
two-value assertion (e.g., r, ok := base.Factory(key).(generic2.Registry)), and
if !ok or r == nil, return an appropriate error response (or log and return)
instead of calling methods on the nil interface to avoid dereference panics.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cmd/gitness/wire_gen.goregistry/app/api/controller/pkg/generic/controller.goregistry/app/api/controller/pkg/generic/v2.goregistry/app/api/controller/pkg/generic/wire.goregistry/app/api/interfaces/package_type_factory.goregistry/app/api/interfaces/package_wrapper.goregistry/app/helpers/package_wrapper.goregistry/app/helpers/pkg/cargo.goregistry/app/helpers/pkg/docker.goregistry/app/helpers/pkg/generic.goregistry/app/helpers/pkg/gopkg.goregistry/app/helpers/pkg/helm.goregistry/app/helpers/pkg/huggingface.goregistry/app/helpers/pkg/maven.goregistry/app/helpers/pkg/npm.goregistry/app/helpers/pkg/nuget.goregistry/app/helpers/pkg/python.goregistry/app/helpers/pkg/rpm.goregistry/app/pkg/base/wrapper.go
Comment on lines
48
to
50
| key := base.GetFactoryKey(artifact2.PackageTypeGENERIC, registry.Type) | ||
| genericRegistry, _ := base.Factory(key).(generic2.Registry) | ||
| headers, fileReader, readCloser, redirectURL, err := genericRegistry.DownloadFile(ctx, info, filePath) |
There was a problem hiding this comment.
Nil pointer dereference risk: type assertion result not validated.
The type assertion ignores the ok boolean. If base.Factory(key) returns nil or a type that doesn't implement generic2.Registry, genericRegistry will be nil, causing a panic on genericRegistry.DownloadFile(...).
This same issue exists at lines 108, 151, and 199.
🐛 Proposed fix to handle failed type assertion
key := base.GetFactoryKey(artifact2.PackageTypeGENERIC, registry.Type)
- genericRegistry, _ := base.Factory(key).(generic2.Registry)
+ genericRegistry, ok := base.Factory(key).(generic2.Registry)
+ if !ok || genericRegistry == nil {
+ return &GetArtifactResponse{
+ BaseResponse: BaseResponse{
+ Error: fmt.Errorf("failed to resolve generic registry for type: %s", registry.Type),
+ },
+ }
+ }
headers, fileReader, readCloser, redirectURL, err := genericRegistry.DownloadFile(ctx, info, filePath)Apply the same pattern to HeadFile (line 108), DeleteFile (line 151), and PutFile (line 199).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/api/controller/pkg/generic/v2.go` around lines 48 - 50, The code
performs an unchecked type assertion on base.Factory(key) into generic2.Registry
before calling methods like DownloadFile, HeadFile, DeleteFile, and PutFile,
which can cause a nil pointer panic; update each use (the DownloadFile call
where genericRegistry is assigned, and the analogous places for HeadFile,
DeleteFile, PutFile) to perform the two-value assertion (e.g., r, ok :=
base.Factory(key).(generic2.Registry)), and if !ok or r == nil, return an
appropriate error response (or log and return) instead of calling methods on the
nil interface to avoid dereference panics.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
registry/app/api/handler/generic/base_test.go (1)
446-492: Consider testing actual parsing functions instead of inline logic.This test implements URL path parsing inline rather than invoking production code. While useful as documentation of expected parsing behavior, it won't catch regressions if the actual parsing logic in production code changes.
If there's an existing parsing function (or one to be added), consider refactoring this test to call it directly and validate the returned values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/handler/generic/base_test.go` around lines 446 - 492, The test currently duplicates URL path parsing inline which won’t catch regressions; refactor the test to call the production parsing function (e.g., implement or use a function like ParseRegistryPath/ParsePackagePath or ParseGenericFileRequest that accepts the raw urlPath and returns isGeneric, packageName, version, fileName, filePath, error) and assert its returned values against tt.want* fields instead of re-parsing inside the test; update or add the parsing function in the handler/generic package (exported or package-private) so tests can exercise the real logic and adjust test cases to call that function and check for errors and returned values.
🤖 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/api/handler/generic/base_test.go`:
- Around line 453-455: The test currently logs an error when the segment counts
differ but continues execution, which can cause panics when later accessing
remainingSegments; change the check in the test (the block comparing
len(remainingSegments) and len(tt.wantSegments) that calls t.Errorf) to stop
execution immediately on mismatch—replace the t.Errorf with a fatal-style stop
(e.g., t.Fatalf or t.FailNow) or return from the test so subsequent index access
on remainingSegments is skipped; target the comparison around remainingSegments
and tt.wantSegments in the failing test.
In `@registry/app/pkg/python/local_helper_test.go`:
- Around line 101-104: There are two identical method declarations for
MockLocalBase.ExistsByFilePath which causes a compile error; remove one of the
duplicate implementations so only a single ExistsByFilePath(ctx context.Context,
registryID int64, filePath string) (bool, error) method remains on the
MockLocalBase receiver, preserving the existing implementation that calls
m.Called(ctx, registryID, filePath) and returns args.Bool(0), args.Error(1).
---
Nitpick comments:
In `@registry/app/api/handler/generic/base_test.go`:
- Around line 446-492: The test currently duplicates URL path parsing inline
which won’t catch regressions; refactor the test to call the production parsing
function (e.g., implement or use a function like
ParseRegistryPath/ParsePackagePath or ParseGenericFileRequest that accepts the
raw urlPath and returns isGeneric, packageName, version, fileName, filePath,
error) and assert its returned values against tt.want* fields instead of
re-parsing inside the test; update or add the parsing function in the
handler/generic package (exported or package-private) so tests can exercise the
real logic and adjust test cases to call that function and check for errors and
returned values.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/gitness/wire_gen.goregistry/app/api/handler/generic/base_test.goregistry/app/helpers/pkg/generic.goregistry/app/pkg/python/local_helper_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- registry/app/helpers/pkg/generic.go
- cmd/gitness/wire_gen.go
Comment on lines
+453
to
+455
| if len(remainingSegments) != len(tt.wantSegments) { | ||
| t.Errorf("Expected %d segments, got %d", len(tt.wantSegments), len(remainingSegments)) | ||
| } |
There was a problem hiding this comment.
Missing early return after segment count mismatch.
When the segment count doesn't match expectations, the test logs an error but continues execution. Subsequent index accesses on remainingSegments may produce misleading errors or panic if the count is significantly different.
Proposed fix
if len(remainingSegments) != len(tt.wantSegments) {
t.Errorf("Expected %d segments, got %d", len(tt.wantSegments), len(remainingSegments))
+ return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(remainingSegments) != len(tt.wantSegments) { | |
| t.Errorf("Expected %d segments, got %d", len(tt.wantSegments), len(remainingSegments)) | |
| } | |
| if len(remainingSegments) != len(tt.wantSegments) { | |
| t.Errorf("Expected %d segments, got %d", len(tt.wantSegments), len(remainingSegments)) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/api/handler/generic/base_test.go` around lines 453 - 455, The
test currently logs an error when the segment counts differ but continues
execution, which can cause panics when later accessing remainingSegments; change
the check in the test (the block comparing len(remainingSegments) and
len(tt.wantSegments) that calls t.Errorf) to stop execution immediately on
mismatch—replace the t.Errorf with a fatal-style stop (e.g., t.Fatalf or
t.FailNow) or return from the test so subsequent index access on
remainingSegments is skipped; target the comparison around remainingSegments and
tt.wantSegments in the failing test.
Comment on lines
+101
to
+104
| func (m *MockLocalBase) ExistsByFilePath(ctx context.Context, registryID int64, filePath string) (bool, error) { | ||
| args := m.Called(ctx, registryID, filePath) | ||
| return args.Bool(0), args.Error(1) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP 'func\s+\(m\s+\*MockLocalBase\)\s+ExistsByFilePath\s*\(' registry/app/pkg/python/local_helper_test.goRepository: abhinavcode/harness
Length of output: 296
🏁 Script executed:
# Get the file size to understand scope
wc -l registry/app/pkg/python/local_helper_test.goRepository: abhinavcode/harness
Length of output: 111
🏁 Script executed:
# Inspect both claimed locations
sed -n '100,105p' registry/app/pkg/python/local_helper_test.go
sed -n '145,152p' registry/app/pkg/python/local_helper_test.goRepository: abhinavcode/harness
Length of output: 481
Remove duplicate ExistsByFilePath method declaration.
MockLocalBase declares ExistsByFilePath twice (line 101 and line 148 in the same file), which causes a Go compile error. Keep one declaration and remove the other.
Duplicate method locations
Lines 101-104 and lines 148-151 both contain identical ExistsByFilePath implementations on the MockLocalBase receiver.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/pkg/python/local_helper_test.go` around lines 101 - 104, There
are two identical method declarations for MockLocalBase.ExistsByFilePath which
causes a compile error; remove one of the duplicate implementations so only a
single ExistsByFilePath(ctx context.Context, registryID int64, filePath string)
(bool, error) method remains on the MockLocalBase receiver, preserving the
existing implementation that calls m.Called(ctx, registryID, filePath) and
returns args.Bool(0), args.Error(1).
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
Bug Fixes
Refactor
Tests