feat: [AH-2542]: added readme api for nuget#95
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds NuGet package README retrieval: new controller method and response type, HTTP handler and route, Registry/GetReadme interface and implementations (local + proxy stub), URL helper and catalog field, and related wiring across controller, handler, registry, and types. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as HTTP Handler
participant Controller as Controller
participant NoProxy as NoProxyWrapper
participant Registry as NuGet Registry
participant Meta as Artifact Metadata
Client->>Handler: GET /nuget/{id}/{version}/readme
Handler->>Handler: extract artifact info
Handler->>Controller: GetReadme(ctx, info)
Controller->>NoProxy: invoke operation for registries
loop per registry
NoProxy->>Registry: GetReadme(ctx, info)
alt local registry
Registry->>Meta: unmarshal artifact metadata
Meta-->>Registry: return README content
Registry-->>NoProxy: README string
else proxy/other
Registry-->>NoProxy: error (invalid request / not implemented)
end
end
NoProxy-->>Controller: GetReadmeResponse
Controller-->>Handler: return response
Handler->>Client: 200 + README content (or error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
registry/app/pkg/nuget/local.go (1)
330-336: Consider matching the README filename more strictly.
HasSuffix(strings.ToLower(header.Name), "readme.md")will also match files likedocs/myreadme.md. If you only want the README file itself, usingpath.Base+EqualFoldavoids accidental matches while staying case-insensitive.🔧 Possible tweak
-import ( +import ( "context" "encoding/json" "encoding/xml" "errors" "fmt" "io" "net/http" + "path" "regexp" "strconv" "strings" @@ - } else if strings.HasSuffix(strings.ToLower(header.Name), "readme.md") { + } else if strings.EqualFold(path.Base(header.Name), "readme.md") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 330 - 336, Replace the loose suffix check for README with a case-insensitive base-name comparison: instead of strings.HasSuffix(strings.ToLower(header.Name), "readme.md"), use strings.EqualFold(path.Base(header.Name), "readme.md") so only a top-level README file matches; update imports to include "path" if necessary and keep the call to c.parseReadme(zr) unchanged while ensuring header.Name is the value being checked.registry/app/api/controller/pkg/nuget/get_readme.go (1)
39-44: Nit: prefer named fields in struct literals for clarity.Using positional initialization for
BaseResponsemakes the code harder to read and fragile if fields are reordered.♻️ Suggested improvement
return &GetReadmeResponse{ - BaseResponse{ - fmt.Errorf("invalid registry type: expected nuget.Registry"), - nil, - }, "", + BaseResponse: BaseResponse{ + Error: fmt.Errorf("invalid registry type: expected nuget.Registry"), + ResponseHeaders: nil, + }, + ReadmeContent: "", }Apply the same pattern to all
GetReadmeResponseandBaseResponseliterals in this file (lines 48-53, 62-67, 73-78, 82-87).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/api/controller/pkg/nuget/get_readme.go` around lines 39 - 44, Replace positional struct literals with named-field literals for GetReadmeResponse and its embedded BaseResponse (e.g., change the return that constructs GetReadmeResponse{BaseResponse{fmt.Errorf(...), nil}, ""} to use field names like GetReadmeResponse{ BaseResponse: BaseResponse{ Err: fmt.Errorf(...), Data: nil }, Readme: "" }). Update every similar instance in this file (all GetReadmeResponse and BaseResponse literals referenced in the review) so each struct literal uses explicit field names matching the BaseResponse and GetReadmeResponse type definitions.
🤖 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/nuget/get_readme.go`:
- Around line 56-60: The ResponseHeaders instance in GetReadme (the headers
variable of type commons.ResponseHeaders) leaves Code at zero which can cause
WriteToResponse() to skip writing the status; set Code to http.StatusOK (200)
when constructing headers so the status is explicit and consistent with other
handlers (e.g., DownloadPackage) and with ResponseHeaders.WriteToResponse().
In `@registry/app/api/handler/nuget/handler.go`:
- Around line 98-129: GetReadme improperly maps all controller errors to a
hardcoded 404 and writes raw error text and a forced 200 response; change it to
use the same error/response flow as other NuGet handlers: after calling
h.controller.GetReadme(ctx, *nugetInfo) invoke h.HandleError(ctx, writer,
response.GetError()) when response.Error/response.GetError() is non-nil (instead
of http.Error), and when successful call
response.ResponseHeaders.WriteToResponse(writer) (so headers and status are
written together) before writing response.ReadmeContent; refer to the GetReadme
function, h.GetPackageArtifactInfo, nugettype.ArtifactInfo,
h.controller.GetReadme, response.GetError, h.HandleError and
response.ResponseHeaders.WriteToResponse to locate and update the code.
In `@registry/app/pkg/nuget/proxy.go`:
- Around line 520-526: The proxy GetReadme method currently always returns a
"not implemented" error; replace this stub in func (r *proxy) GetReadme so proxy
registries either fetch the readme from the upstream registry or explicitly
signal "not supported": implement an upstream fetch by delegating to the proxy's
upstream client/transport (e.g., call the existing upstream fetch method on r
such as r.fetchUpstream / r.upstreamClient.GetReadme with ctx and the provided
ArtifactInfo, return the fetched string on success, translate an upstream 404
into a nil/empty/no-readme response, and map other upstream errors into
appropriate errcodes), or if proxy readmes are unsupported return a clear "not
supported" error (not errcode.ErrCodeInvalidRequest) such as
errcode.ErrCodeNotSupported and ensure metadata generation hides readme URLs for
proxy artifacts; update error handling to stop using the current
errcode.ErrCodeInvalidRequest.WithDetail(fmt.Errorf("not implemented")).
---
Nitpick comments:
In `@registry/app/api/controller/pkg/nuget/get_readme.go`:
- Around line 39-44: Replace positional struct literals with named-field
literals for GetReadmeResponse and its embedded BaseResponse (e.g., change the
return that constructs GetReadmeResponse{BaseResponse{fmt.Errorf(...), nil}, ""}
to use field names like GetReadmeResponse{ BaseResponse: BaseResponse{ Err:
fmt.Errorf(...), Data: nil }, Readme: "" }). Update every similar instance in
this file (all GetReadmeResponse and BaseResponse literals referenced in the
review) so each struct literal uses explicit field names matching the
BaseResponse and GetReadmeResponse type definitions.
In `@registry/app/pkg/nuget/local.go`:
- Around line 330-336: Replace the loose suffix check for README with a
case-insensitive base-name comparison: instead of
strings.HasSuffix(strings.ToLower(header.Name), "readme.md"), use
strings.EqualFold(path.Base(header.Name), "readme.md") so only a top-level
README file matches; update imports to include "path" if necessary and keep the
call to c.parseReadme(zr) unchanged while ensuring header.Name is the value
being checked.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
registry/app/api/controller/pkg/nuget/controller.goregistry/app/api/controller/pkg/nuget/get_readme.goregistry/app/api/controller/pkg/nuget/response.goregistry/app/api/handler/nuget/handler.goregistry/app/api/router/packages/route.goregistry/app/pkg/nuget/helper.goregistry/app/pkg/nuget/local.goregistry/app/pkg/nuget/proxy.goregistry/app/pkg/nuget/registry.goregistry/app/pkg/types/nuget/types.go
| headers := &commons.ResponseHeaders{ | ||
| Headers: map[string]string{ | ||
| "Content-Type": "text/markdown; charset=utf-8", | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other nuget controller methods set ResponseHeaders to see if Code is typically set
rg -n "ResponseHeaders{" --type=go -C3 -g '**/controller/pkg/nuget/**'Repository: abhinavcode/harness
Length of output: 158
🏁 Script executed:
# Also check the ResponseHeaders struct definition
cat -n registry/app/pkg/commons/request.go | head -40Repository: abhinavcode/harness
Length of output: 1338
🏁 Script executed:
# Look at the specific file mentioned in the review
cat -n registry/app/api/controller/pkg/nuget/get_readme.go | sed -n '50,70p'Repository: abhinavcode/harness
Length of output: 545
🏁 Script executed:
# Search for ResponseHeaders usage with escaped pattern
rg -n "ResponseHeaders" --type=go -C3 'registry/app/api/controller/pkg/nuget/'Repository: abhinavcode/harness
Length of output: 4402
🏁 Script executed:
# Check what files exist in the nuget controller directory
fd -t f --type f 'registry/app/api/controller/pkg/nuget/' | head -20Repository: abhinavcode/harness
Length of output: 520
🏁 Script executed:
# Search for how ResponseHeaders is typically initialized across the codebase
rg -n "Code\s*:" --type=go -B2 -A2 'registry/app/api/controller/pkg/'Repository: abhinavcode/harness
Length of output: 1912
🏁 Script executed:
# Find where ResponseHeaders.Code is used in handler/middleware code
rg -n "ResponseHeaders|\.Code" --type=go -C2 'registry/app/api/handler\|registry/app/api/middleware' | head -60Repository: abhinavcode/harness
Length of output: 156
🏁 Script executed:
# Search for where BaseResponse is processed
rg -n "BaseResponse" --type=go -C3 'registry/app/api/handler' | head -80Repository: abhinavcode/harness
Length of output: 6438
🏁 Script executed:
# Look for HTTP status code handling
rg -n "http.StatusOK\|w.WriteHeader\|statusCode\|StatusCode" --type=go 'registry/app/api' -C2 | grep -A2 -B2 "ResponseHeaders\|Code"Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Find nuget handler files
fd -type f --full-path '*/handler/nuget' --type fRepository: abhinavcode/harness
Length of output: 233
🏁 Script executed:
# Search for where responses are converted to HTTP responses
rg -n "WriteHeader\|StatusCode\|Code.*http" --type=go 'registry/app/api/handler' -C3 | head -100Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Look at the handler structure and how it processes controller responses
fd -type f -name "*.go" 'registry/app/api/handler' | xargs -I {} basename {} | sort | uniqRepository: abhinavcode/harness
Length of output: 233
🏁 Script executed:
# Find nuget handler files
find registry/app/api/handler -type f -name "*.go" 2>/dev/null | grep nugetRepository: abhinavcode/harness
Length of output: 1008
🏁 Script executed:
# Look for all handler directories
find registry/app/api/handler -type d 2>/dev/nullRepository: abhinavcode/harness
Length of output: 502
🏁 Script executed:
# Search for how controller responses are handled - look for function that handles BaseResponse
rg -n "GetError\|ResponseHeaders" --type=go 'registry/app/api' -C2 | head -150Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Look at the nuget handler to see how it processes responses
cat -n registry/app/api/handler/nuget/handler.goRepository: abhinavcode/harness
Length of output: 4976
🏁 Script executed:
# Look at one of the handler methods to understand the pattern
cat -n registry/app/api/handler/nuget/download_package.go | head -100Repository: abhinavcode/harness
Length of output: 2596
🏁 Script executed:
# Search for where the Code field from ResponseHeaders is used
rg -n "\.Code" --type=go 'registry/app/api/handler/nuget' -C3Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Find the WriteHeadersToResponse and WriteToResponse method definitions
rg -n "WriteHeadersToResponse\|WriteToResponse" --type=go -B2 -A10 'registry/app'Repository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Also check the commons package for these methods
cat -n registry/app/pkg/commons/request.goRepository: abhinavcode/harness
Length of output: 3184
🏁 Script executed:
# Double-check if other nuget controller methods also omit Code without setting ResponseHeaders to nil
rg -n "ResponseHeaders\{" --type=go 'registry/app/api/controller/pkg/nuget/' -C3Repository: abhinavcode/harness
Length of output: 585
Set Code: 200 explicitly for clarity and consistency with other handler patterns.
The handler layer does write HTTP 200 OK (via writer.WriteHeader(http.StatusOK) on line 127 of registry/app/api/handler/nuget/handler.go), so the current code functions correctly. However, the Code field is left at zero, which creates confusion given that ResponseHeaders.WriteToResponse() only writes the status if Code != 0. Other handlers like DownloadPackage use the WriteToResponse() pattern, which would fail silently if Code remained zero. For consistency and clarity, explicitly set Code: 200 in the controller response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/api/controller/pkg/nuget/get_readme.go` around lines 56 - 60,
The ResponseHeaders instance in GetReadme (the headers variable of type
commons.ResponseHeaders) leaves Code at zero which can cause WriteToResponse()
to skip writing the status; set Code to http.StatusOK (200) when constructing
headers so the status is explicit and consistent with other handlers (e.g.,
DownloadPackage) and with ResponseHeaders.WriteToResponse().
| func (h *handler) GetReadme(writer http.ResponseWriter, request *http.Request) { | ||
| ctx := request.Context() | ||
|
|
||
| // Get artifact info from request | ||
| info, err := h.GetPackageArtifactInfo(request) | ||
| if err != nil { | ||
| http.Error(writer, fmt.Sprintf("Failed to get artifact info: %v", err), http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| nugetInfo, ok := info.(*nugettype.ArtifactInfo) | ||
| if !ok { | ||
| http.Error(writer, "Invalid artifact info type", http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| // Call controller to get readme | ||
| response := h.controller.GetReadme(ctx, *nugetInfo) | ||
| if response.Error != nil { | ||
| http.Error(writer, response.Error.Error(), http.StatusNotFound) | ||
| return | ||
| } | ||
|
|
||
| // Write response headers | ||
| if response.ResponseHeaders != nil { | ||
| response.ResponseHeaders.WriteHeadersToResponse(writer) | ||
| } | ||
|
|
||
| // Write readme content | ||
| writer.WriteHeader(http.StatusOK) | ||
| _, _ = writer.Write([]byte(response.ReadmeContent)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ResponseHeaders type definition and WriteHeadersToResponse implementation
rg -n "type ResponseHeaders|WriteHeadersToResponse" -C 5 --type=goRepository: abhinavcode/harness
Length of output: 8039
🏁 Script executed:
# Search for other NuGet handlers to see standard error handling patterns
rg -n "func \(h \*handler\) Get" -C 3 --type=go | head -100Repository: abhinavcode/harness
Length of output: 7184
🏁 Script executed:
# Look for standard error mapping patterns in NuGet handlers
rg -n "TranslatedUserError|StatusCodeMapper|GetPackageMetadata|GetServiceEndpoint" -C 3 --type=go | head -150Repository: abhinavcode/harness
Length of output: 10135
🏁 Script executed:
# Find the response type/structure for GetReadme
rg -n "GetReadme" -C 5 --type=go | grep -A 20 "func.*GetReadme"Repository: abhinavcode/harness
Length of output: 6266
🏁 Script executed:
# Find GetReadmeResponse type definition
rg -n "type GetReadmeResponse|ReadmeContent|ResponseHeaders" registry/app/api/controller/pkg/nuget/ -C 3 --type=goRepository: abhinavcode/harness
Length of output: 4914
🏁 Script executed:
# Look at GetPackageMetadata handler implementation to see standard pattern
cat -n registry/app/api/handler/nuget/package_metadata.goRepository: abhinavcode/harness
Length of output: 1860
🏁 Script executed:
# Check GetServiceEndpoint handler implementation
cat -n registry/app/api/handler/nuget/service_endpoint.goRepository: abhinavcode/harness
Length of output: 1847
🏁 Script executed:
# Find how other handlers map controller responses to HTTP responses
rg -n "controller\." registry/app/api/handler/nuget/ -A 5 --type=go | head -200Repository: abhinavcode/harness
Length of output: 9256
🏁 Script executed:
# Check WriteToResponse vs WriteHeadersToResponse difference
rg -n "func.*WriteToResponse|func.*WriteHeadersToResponse" registry/app/pkg/commons/request.go -A 10Repository: abhinavcode/harness
Length of output: 551
🏁 Script executed:
# Find HandleError implementation
rg -n "func.*HandleError" registry/app/api/handler/nuget/ -A 8 --type=goRepository: abhinavcode/harness
Length of output: 45
🏁 Script executed:
# Look at the complete get_readme.go controller to see full flow
cat -n registry/app/api/controller/pkg/nuget/get_readme.goRepository: abhinavcode/harness
Length of output: 2962
🏁 Script executed:
# Search for HandleError in the handler base class
rg -n "HandleError" registry/app/api/handler/ -C 3 --type=go | head -100Repository: abhinavcode/harness
Length of output: 6677
🏁 Script executed:
# Check the handler base implementation
find registry/app/api/handler -name "handler.go" -o -name "base.go" | xargs grep -l "HandleError" 2>/dev/nullRepository: abhinavcode/harness
Length of output: 195
🏁 Script executed:
# Look at the handler struct definition in nuget
rg -n "type handler struct" registry/app/api/handler/nuget/ -A 10 --type=goRepository: abhinavcode/harness
Length of output: 770
🏁 Script executed:
# Find HandleError in packages.Handler
cat -n registry/app/api/handler/packages/handler.go | grep -A 15 "func.*HandleError"Repository: abhinavcode/harness
Length of output: 1901
Use standard error handling pattern consistent with other NuGet handlers.
GetReadme returns a hardcoded 404 for all controller errors (line 117), masking authentication/validation/internal failures with the same status code. It also ignores any status in response.ResponseHeaders (the controller never sets Code, so WriteHeadersToResponse won't write one), then unconditionally writes 200 OK (line 127). This bypasses proper error mapping and exposes raw internal error text to clients.
Follow the pattern used by GetPackageMetadata, GetServiceEndpoint, and other handlers: call h.HandleError(ctx, w, response.GetError()) instead of http.Error, which routes errors through TranslatedUserError for correct status mapping. Then call response.ResponseHeaders.WriteToResponse(w) to write headers and status together before writing body.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/api/handler/nuget/handler.go` around lines 98 - 129, GetReadme
improperly maps all controller errors to a hardcoded 404 and writes raw error
text and a forced 200 response; change it to use the same error/response flow as
other NuGet handlers: after calling h.controller.GetReadme(ctx, *nugetInfo)
invoke h.HandleError(ctx, writer, response.GetError()) when
response.Error/response.GetError() is non-nil (instead of http.Error), and when
successful call response.ResponseHeaders.WriteToResponse(writer) (so headers and
status are written together) before writing response.ReadmeContent; refer to the
GetReadme function, h.GetPackageArtifactInfo, nugettype.ArtifactInfo,
h.controller.GetReadme, response.GetError, h.HandleError and
response.ResponseHeaders.WriteToResponse to locate and update the code.
| func (r *proxy) GetReadme( | ||
| ctx context.Context, | ||
| _ nugettype.ArtifactInfo, | ||
| ) (string, error) { | ||
| log.Error().Ctx(ctx).Msg("Not implemented") | ||
| return "", errcode.ErrCodeInvalidRequest.WithDetail(fmt.Errorf("not implemented")) | ||
| } |
There was a problem hiding this comment.
Readme retrieval always fails for proxy registries.
The new endpoint is now routable for NuGet, but the proxy implementation hard-fails with “not implemented,” which means readme fetches for upstream/proxy registries will consistently error. If proxy registries are meant to support readmes, this needs a real upstream fetch; if not, consider explicitly signaling “not supported” and hiding readme URLs for proxy metadata to avoid broken links.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/pkg/nuget/proxy.go` around lines 520 - 526, The proxy GetReadme
method currently always returns a "not implemented" error; replace this stub in
func (r *proxy) GetReadme so proxy registries either fetch the readme from the
upstream registry or explicitly signal "not supported": implement an upstream
fetch by delegating to the proxy's upstream client/transport (e.g., call the
existing upstream fetch method on r such as r.fetchUpstream /
r.upstreamClient.GetReadme with ctx and the provided ArtifactInfo, return the
fetched string on success, translate an upstream 404 into a nil/empty/no-readme
response, and map other upstream errors into appropriate errcodes), or if proxy
readmes are unsupported return a clear "not supported" error (not
errcode.ErrCodeInvalidRequest) such as errcode.ErrCodeNotSupported and ensure
metadata generation hides readme URLs for proxy artifacts; update error handling
to stop using the current
errcode.ErrCodeInvalidRequest.WithDetail(fmt.Errorf("not implemented")).
2 participants
Summary by CodeRabbit
New Features
Bug Fixes / Improvements