add FileContentsLimit feature to file contents service#78
add FileContentsLimit feature to file contents service#78mscasso-scanoss merged 7 commits intomainfrom
Conversation
|
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 a configurable file-contents size limit: config files updated, server config gains Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as APIService
participant Config as Config
participant Script as TestSupport
Client->>API: GET /file-contents?md5=<md5>
API->>Config: read Scanning.FileContentsLimit (MB)
API->>Script: request file contents for <md5>
Script-->>API: stream file contents (bytes)
API->>API: compute len(output) and limitBytes (MB->bytes)
alt output > limitBytes and limitBytes > 0
API-->>Client: 413 Request Entity Too Large (application/json) {"error":"...exceeds the maximum allowed limit..."}
else
API-->>Client: 200 OK (original content response with detected charset/headers)
end
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 |
![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
🧹 Nitpick comments (1)
pkg/service/filecontents_service_test.go (1)
121-121: Avoid unconditional zero-value override ofFileContentsLimitin table cases.Cases that omit
fileContentsLimitcurrently force it to0, which changes semantics from config-default behavior and narrows coverage unintentionally.♻️ Suggested refactor
tests := []struct { name string input map[string]string binary string telemetry bool - fileContentsLimit int64 + fileContentsLimit *int64 want int }{ @@ - myConfig.Scanning.FileContentsLimit = test.fileContentsLimit + if test.fileContentsLimit != nil { + myConfig.Scanning.FileContentsLimit = *test.fileContentsLimit + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/filecontents_service_test.go` at line 121, The test unconditionally sets myConfig.Scanning.FileContentsLimit = test.fileContentsLimit which forces a zero-value override for cases that intended to rely on the default; change the test table and assignment so the override is applied only when the case explicitly specifies a value (e.g. make test.fileContentsLimit a *int or add a boolean like fileContentsLimitSet) and set myConfig.Scanning.FileContentsLimit only when that pointer/flag is non-nil/true, leaving the default otherwise; update all table entries accordingly to preserve semantics and test coverage for default behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/service/filecontents_service.go`:
- Around line 85-96: The current guard checks limitBytes after using
exec.CommandContext(...).Output(), which buffers full stdout and can cause
memory pressure; change the implementation that uses exec.CommandContext to call
cmd.StdoutPipe(), start the command with cmd.Start(), then read the stdout
through an io.LimitedReader (or read and count bytes while copying into a buffer
up to limitBytes+1) so you can detect overflow before allocating the full
response; if the read exceeds s.config.Scanning.FileContentsLimit (limitBytes)
return the 413 response (as in the existing block that references limitBytes,
md5, ContentTypeKey, ApplicationJSON) and ensure you call cmd.Wait() and handle
command errors properly.
---
Nitpick comments:
In `@pkg/service/filecontents_service_test.go`:
- Line 121: The test unconditionally sets myConfig.Scanning.FileContentsLimit =
test.fileContentsLimit which forces a zero-value override for cases that
intended to rely on the default; change the test table and assignment so the
override is applied only when the case explicitly specifies a value (e.g. make
test.fileContentsLimit a *int or add a boolean like fileContentsLimitSet) and
set myConfig.Scanning.FileContentsLimit only when that pointer/flag is
non-nil/true, leaving the default otherwise; update all table entries
accordingly to preserve semantics and test coverage for default behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac9b1845-0e80-45e6-b5f5-8a406125758b
📒 Files selected for processing (6)
config/app-config-dev.jsonconfig/app-config-prod.jsonpkg/config/server_config.gopkg/service/filecontents_service.gopkg/service/filecontents_service_test.gotest-support/scanoss.sh
agustingroh
left a comment
agustingroh
left a comment
There was a problem hiding this comment.
LGTM, please update the CHANGELOG.md file with latest changes
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 `@pkg/service/filecontents_service.go`:
- Line 86: The comment on the line describing FileContentsLimit is missing a
space after the '//' and breaks gocritic commentFormatting; update the comment
to start with "// " (for example "// unlimited for FileContentsLimit <= 0") so
the linter passes and the comment is properly formatted near the
FileContentsLimit reference in filecontents_service.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f70509f-fffd-4c33-807b-74ae61f1fe8c
📒 Files selected for processing (2)
CHANGELOG.mdpkg/service/filecontents_service.go
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
pkg/service/filecontents_service.go
Outdated
| http.Error(w, "ERROR recovering file contents", http.StatusInternalServerError) | ||
| return | ||
| } | ||
| limitBytes := s.config.Scanning.FileContentsLimit * 1024 * 1024 |
There was a problem hiding this comment.
this should be computed at initial startup?
There was a problem hiding this comment.
To simplify the user's life, the limit is expressed in MB in the configuration file. Here I'm just adjusting the units for the comparison. I think it is clearer from a code reading perspective to do it here
pkg/service/filecontents_service.go
Outdated
| } | ||
| limitBytes := s.config.Scanning.FileContentsLimit * 1024 * 1024 | ||
| // unlimited for FileContentsLimit <= 0 | ||
| if limitBytes > 0 && int64(len(output)) > limitBytes { |
There was a problem hiding this comment.
len(output) should only be calculated once
pkg/service/filecontents_service.go
Outdated
| zs.Warnf("File contents size %d bytes exceeds limit %d MB for md5 %s", len(output), s.config.Scanning.FileContentsLimit, md5) | ||
| w.Header().Set(ContentTypeKey, ApplicationJSON) | ||
| w.WriteHeader(http.StatusRequestEntityTooLarge) | ||
| resp := map[string]string{ |
There was a problem hiding this comment.
It's probably ok to just return this in plain text?
There was a problem hiding this comment.
This is the log entry. The user will receive the error in json format.
CHANGELOG.md
Outdated
| ### Added | ||
| - Added configurable file contents size limit (`SCANOSS_FILE_CONTENTS_LIMIT`). | ||
| - Limits the maximum file size returned by the `file_contents` endpoint (default: 50 MB). | ||
| - Returns HTTP 400 when the file exceeds the configured limit. |
There was a problem hiding this comment.
Add in the exact error code
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/service/filecontents_service.go (1)
85-85:⚠️ Potential issue | 🟠 MajorFix gocritic comment formatting at Line 85 (CI is failing).
The comment must include a space after
//to satisfycommentFormatting.Suggested fix
- //Convert the configured limit from MB to bytes. + // Convert the configured limit from MB to bytes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/filecontents_service.go` at line 85, There is a gocritic commentFormatting failure for the inline comment "//Convert the configured limit from MB to bytes." — update that comment to include a space after the slashes (e.g. "// Convert the configured limit from MB to bytes.") wherever it appears in pkg/service/filecontents_service.go (near the code handling file size limit logic) so CI's commentFormatting check passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/service/filecontents_service.go`:
- Line 85: There is a gocritic commentFormatting failure for the inline comment
"//Convert the configured limit from MB to bytes." — update that comment to
include a space after the slashes (e.g. "// Convert the configured limit from MB
to bytes.") wherever it appears in pkg/service/filecontents_service.go (near the
code handling file size limit logic) so CI's commentFormatting check passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15daa80f-2c50-4aaa-ba85-a8601d5b2a9d
📒 Files selected for processing (2)
CHANGELOG.mdpkg/service/filecontents_service.go
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
mscasso-scanoss
force-pushed
the
mscasso/feat/sp-4187-add-file-contents-limit
branch
from
8815788 to
95a74c5
Compare
eeisegn
left a comment
eeisegn
left a comment
There was a problem hiding this comment.
please update the error code in the change log to 413 before merging
3 participants
Summary by CodeRabbit
New Features
Tests
Documentation