Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/naabu/main.go(1 hunks)internal/pdcp/writer.go(1 hunks)
🔇 Additional comments (1)
cmd/naabu/main.go (1)
159-162: Call site correctly updated for the new signature.The removal of error handling here correctly reflects the updated
SetAssetIDsignature. However, note that this means users who provide an invalid--asset-idflag will only receive a warning log (if they're watching) with no clear indication that their custom asset ID was rejected. See the concern raised ininternal/pdcp/writer.golines 100-105 regarding the validation behavior.
| func (u *UploadWriter) SetAssetID(id string) { | ||
| if !xidRegex.MatchString(id) { | ||
| return fmt.Errorf("invalid asset id provided") | ||
| gologger.Warning().Msgf("invalid asset id provided (unknown xid format): %s", id) | ||
| } | ||
| u.assetGroupID = id | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Inconsistent validation: warns about invalid asset IDs but still assigns them.
The method validates the asset ID format (line 101) and logs a warning if invalid (line 102), but then proceeds to assign the invalid ID anyway (line 104). This creates an inconsistent state where:
- The invalid asset ID will likely cause API failures when used in the PATCH endpoint URL (line 233:
fmt.Sprintf(appendEndpoint, u.assetGroupID)) - Users receive only a warning that might be missed, with no way to handle the invalid input
- The PR title suggests "allowing custom asset id", but the strict regex validation (
^[a-z0-9]{20}$) remains, creating confusion about the intended behavior
Recommended solutions:
- If the goal is to allow any custom asset ID: Remove the validation entirely and trust the server to handle invalid IDs.
- If validation is necessary: Don't assign invalid IDs—either keep the error return or set
u.assetGroupIDto an empty string (allowing the server to generate a new ID).
Apply this diff for solution 2 (recommended for backward compatibility):
func (u *UploadWriter) SetAssetID(id string) {
if !xidRegex.MatchString(id) {
gologger.Warning().Msgf("invalid asset id provided (unknown xid format): %s", id)
+ return
}
u.assetGroupID = id
}This ensures invalid IDs are rejected early rather than causing cryptic API failures later.
📝 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.
| func (u *UploadWriter) SetAssetID(id string) { | |
| if !xidRegex.MatchString(id) { | |
| return fmt.Errorf("invalid asset id provided") | |
| gologger.Warning().Msgf("invalid asset id provided (unknown xid format): %s", id) | |
| } | |
| u.assetGroupID = id | |
| return nil | |
| } | |
| func (u *UploadWriter) SetAssetID(id string) { | |
| if !xidRegex.MatchString(id) { | |
| gologger.Warning().Msgf("invalid asset id provided (unknown xid format): %s", id) | |
| return | |
| } | |
| u.assetGroupID = id | |
| } |
🤖 Prompt for AI Agents
In internal/pdcp/writer.go around lines 100 to 105, the method currently logs a
warning for invalid asset IDs but still assigns them; change it so invalid IDs
are not assigned to u.assetGroupID. Specifically, keep the regex check and
warning, but when the ID does not match, do not set u.assetGroupID — instead set
it to the empty string (so the server can generate an ID) and log the warning;
only assign id to u.assetGroupID when the regex passes.
Summary by CodeRabbit