feat:[AH-2685]: fix npm error handling while upload#90
Open
abhinavcode wants to merge 1 commit into
Open
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughStandardized error handling in NPM package parsing logic by replacing fmt.Errorf calls with usererror.BadRequestf for JSON parsing failures, updated error wrapping format from %w to %s, and introduced optimized attachment processing functions with aligned error handling patterns. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
registry/app/pkg/npm/local.go (1)
483-483:⚠️ Potential issue | 🟠 Major
processAttachmentsOptimizederrors arefmt.Errorf, defeating the PR's standardisation goalThe PR converts all
parseAndUploadNPMPackagefield-parse failures tousererror.BadRequestfto ensure client-input errors surface as HTTP 400. However:
- Line 483 still uses
fmt.Errorf("failed to process attachments: %w", err)instead ofusererror.BadRequestf.- Every error path inside the newly-introduced
processAttachmentsOptimized(lines 512, 515, 526, 533, 539, 542, 553, 571, 577) uses plainfmt.Errorf.Because the HTTP error middleware likely inspects the error chain via
errors.Asfor*usererror.Error, errors that originate insideprocessAttachmentsOptimized(e.g.,"expected '{' at start of _attachments","no attachment data found") will be classified as server errors and return HTTP 500 to the client instead of HTTP 400.Only errors propagated through
processBase64DataOptimized(which does useusererror.BadRequestf) survive as 400 because of the%wchain at line 483.🔧 Proposed fix — convert `processAttachmentsOptimized` errors to `usererror.BadRequestf`
- return types.FileInfo{}, fmt.Errorf("failed to parse _attachments: %w", err) + return types.FileInfo{}, usererror.BadRequestf("failed to parse _attachments: %s", err.Error()) ... - return types.FileInfo{}, fmt.Errorf("expected '{' at start of _attachments") + return types.FileInfo{}, usererror.BadRequestf("expected '{' at start of _attachments") ... - return types.FileInfo{}, fmt.Errorf("failed to parse JSON: %w", err) + return types.FileInfo{}, usererror.BadRequestf("failed to parse JSON: %s", err.Error()) ... - return types.FileInfo{}, fmt.Errorf("expected string key in _attachments") + return types.FileInfo{}, usererror.BadRequestf("expected string key in _attachments") ... - return types.FileInfo{}, fmt.Errorf("failed to parse attachment %s: %w", attachmentKey, err) + return types.FileInfo{}, usererror.BadRequestf("failed to parse attachment %s: %s", attachmentKey, err.Error()) ... - return types.FileInfo{}, fmt.Errorf("expected '{' for attachment %s", attachmentKey) + return types.FileInfo{}, usererror.BadRequestf("expected '{' for attachment %s", attachmentKey) ... - return types.FileInfo{}, fmt.Errorf("failed to parse attachment %s fields: %w", attachmentKey, err) + return types.FileInfo{}, usererror.BadRequestf("failed to parse attachment %s fields: %s", attachmentKey, err.Error()) ... - return types.FileInfo{}, fmt.Errorf("failed to skip field %s: %w", field, err) + return types.FileInfo{}, usererror.BadRequestf("failed to skip field %s: %s", field, err.Error()) ... - return types.FileInfo{}, fmt.Errorf("no attachment data found") + return types.FileInfo{}, usererror.BadRequestf("no attachment data found")And line 483 can be simplified to direct propagation since all inner errors will already be
usererror:- return types.FileInfo{}, fmt.Errorf("failed to process attachments: %w", err) + return types.FileInfo{}, errAlso applies to: 512-577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/npm/local.go` at line 483, The errors produced inside processAttachmentsOptimized (and any fmt.Errorf wrappers returned from parseAndUploadNPMPackage) must be converted to usererror.BadRequestf so client input failures surface as HTTP 400: replace all fmt.Errorf(...) usages inside processAttachmentsOptimized (the internal messages like "expected '{' at start of _attachments", "no attachment data found", etc.) with usererror.BadRequestf and ensure processBase64DataOptimized continues to return usererror errors; then simplify the upstream return in parseAndUploadNPMPackage that currently wraps with fmt.Errorf("failed to process attachments: %w", err) to just propagate the error (or return err) since the inner errors will already be usererror.BadRequestf.
🧹 Nitpick comments (1)
registry/app/pkg/npm/local.go (1)
523-523: Inconsistentio.EOFcomparison — prefererrors.IsLines 523 and 550 use
err == io.EOF(and line 651 inNewOptimizedJSONStringStreamReaderuseserr != io.EOF), while line 406 correctly useserrors.Is(err, io.EOF). Direct==comparisons miss wrapped EOF errors and are non-idiomatic in modern Go.♻️ Proposed fix
- if err == io.EOF || strings.Contains(err.Error(), "EOF") { + if errors.Is(err, io.EOF) || strings.Contains(err.Error(), "EOF") {Apply the same change at line 550, and in
NewOptimizedJSONStringStreamReader:- if err != nil && err != io.EOF { + if err != nil && !errors.Is(err, io.EOF) { ... - if err == io.EOF { + if errors.Is(err, io.EOF) {Also applies to: 550-550
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/npm/local.go` at line 523, Several EOF checks use direct equality (err == io.EOF / err != io.EOF) which misses wrapped errors; replace these with errors.Is(err, io.EOF) and !errors.Is(err, io.EOF) respectively (e.g., the comparisons inside the stream-reading logic and in NewOptimizedJSONStringStreamReader), and add the standard "errors" import if missing so wrapped EOFs are detected consistently and idiomatically.
🤖 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/npm/local.go`:
- Around line 596-614: The code reading raw bytes from decoder.Buffered() (via
streamReader.ReadByte()) after decoder.Token() doesn't tolerate optional JSON
whitespace; change the logic around the streamReader.ReadByte() calls (the block
that checks for ':' and then '"' using streamReader) to first loop-reading and
discarding ASCII whitespace (space, tab, CR, LF) until a non-whitespace byte is
found, then assert that the first non-whitespace is ':'; after consuming ':',
again skip any whitespace before asserting the next non-whitespace is '"' for
the string start; keep the same usererror.BadRequestf messages but report the
actual non-whitespace byte when expectations fail and reference attachmentKey in
the error as before.
---
Outside diff comments:
In `@registry/app/pkg/npm/local.go`:
- Line 483: The errors produced inside processAttachmentsOptimized (and any
fmt.Errorf wrappers returned from parseAndUploadNPMPackage) must be converted to
usererror.BadRequestf so client input failures surface as HTTP 400: replace all
fmt.Errorf(...) usages inside processAttachmentsOptimized (the internal messages
like "expected '{' at start of _attachments", "no attachment data found", etc.)
with usererror.BadRequestf and ensure processBase64DataOptimized continues to
return usererror errors; then simplify the upstream return in
parseAndUploadNPMPackage that currently wraps with fmt.Errorf("failed to process
attachments: %w", err) to just propagate the error (or return err) since the
inner errors will already be usererror.BadRequestf.
---
Nitpick comments:
In `@registry/app/pkg/npm/local.go`:
- Line 523: Several EOF checks use direct equality (err == io.EOF / err !=
io.EOF) which misses wrapped errors; replace these with errors.Is(err, io.EOF)
and !errors.Is(err, io.EOF) respectively (e.g., the comparisons inside the
stream-reading logic and in NewOptimizedJSONStringStreamReader), and add the
standard "errors" import if missing so wrapped EOFs are detected consistently
and idiomatically.
Comment on lines
596
to
614
| startByte, err := streamReader.ReadByte() | ||
| if err != nil { | ||
| return types.FileInfo{}, fmt.Errorf("failed to upload attachment %s: Error while reading : character: %w", | ||
| attachmentKey, err) | ||
| return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: Error while reading : character: %s", | ||
| attachmentKey, err.Error()) | ||
| } | ||
| if startByte != ':' { | ||
| return types.FileInfo{}, fmt.Errorf("failed to upload attachment %s: Expected : character, got %c", | ||
| return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: Expected : character, got %c", | ||
| attachmentKey, startByte) | ||
| } | ||
|
|
||
| // Now expecting `"`, marking start of JSON string | ||
| startByte, err = streamReader.ReadByte() | ||
| if err != nil { | ||
| return types.FileInfo{}, fmt.Errorf("failed to upload"+ | ||
| " attachment %s: Error while reading \" character: %w", attachmentKey, err) | ||
| return types.FileInfo{}, usererror.BadRequestf("failed to upload"+ | ||
| " attachment %s: Error while reading \" character: %s", attachmentKey, err.Error()) | ||
| } | ||
| if startByte != '"' { | ||
| return types.FileInfo{}, fmt.Errorf("failed to upload"+ | ||
| return types.FileInfo{}, usererror.BadRequestf("failed to upload"+ | ||
| " attachment %s: Expected \" character, got %c", attachmentKey, startByte) |
There was a problem hiding this comment.
Raw byte parsing after decoder.Buffered() doesn't tolerate whitespace in the JSON stream
After decoder.Token() returns the key "data", decoder.Buffered() returns a reader starting at the first byte after the key's closing ". Per RFC 8259, insignificant whitespace is legal around : and before the value. If the client sends pretty-printed JSON (e.g., "data" : "base64..."), streamReader.ReadByte() at line 596 returns a space character, not :, and the upload fails with a misleading usererror.BadRequestf("...Expected : character, got ").
Standard npm clients emit compact JSON, so this is low-risk in practice, but a defensive fix is straightforward:
🛡️ Proposed fix — skip optional whitespace before `:` and `"`
- // Expecting `:` character first
- startByte, err := streamReader.ReadByte()
- if err != nil {
- return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: Error while reading : character: %s",
- attachmentKey, err.Error())
- }
- if startByte != ':' {
- return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: Expected : character, got %c",
- attachmentKey, startByte)
- }
-
- // Now expecting `"`, marking start of JSON string
- startByte, err = streamReader.ReadByte()
- if err != nil {
- return types.FileInfo{}, usererror.BadRequestf("failed to upload"+
- " attachment %s: Error while reading \" character: %s", attachmentKey, err.Error())
- }
- if startByte != '"' {
- return types.FileInfo{}, usererror.BadRequestf("failed to upload"+
- " attachment %s: Expected \" character, got %c", attachmentKey, startByte)
- }
+ // Skip optional whitespace, then expect `:`
+ for {
+ startByte, err := streamReader.ReadByte()
+ if err != nil {
+ return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: error reading colon separator: %s",
+ attachmentKey, err.Error())
+ }
+ if startByte == ':' {
+ break
+ }
+ if startByte != ' ' && startByte != '\t' && startByte != '\n' && startByte != '\r' {
+ return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: expected ':' separator, got %c",
+ attachmentKey, startByte)
+ }
+ }
+ // Skip optional whitespace, then expect `"`
+ for {
+ startByte, err := streamReader.ReadByte()
+ if err != nil {
+ return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: error reading opening quote: %s",
+ attachmentKey, err.Error())
+ }
+ if startByte == '"' {
+ break
+ }
+ if startByte != ' ' && startByte != '\t' && startByte != '\n' && startByte != '\r' {
+ return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: expected '\"' opening quote, got %c",
+ attachmentKey, startByte)
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/pkg/npm/local.go` around lines 596 - 614, The code reading raw
bytes from decoder.Buffered() (via streamReader.ReadByte()) after
decoder.Token() doesn't tolerate optional JSON whitespace; change the logic
around the streamReader.ReadByte() calls (the block that checks for ':' and then
'"' using streamReader) to first loop-reading and discarding ASCII whitespace
(space, tab, CR, LF) until a non-whitespace byte is found, then assert that the
first non-whitespace is ':'; after consuming ':', again skip any whitespace
before asserting the next non-whitespace is '"' for the string start; keep the
same usererror.BadRequestf messages but report the actual non-whitespace byte
when expectations fail and reference attachmentKey in the error as before.
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
Bug Fixes
Performance