support refactored json format#33
Conversation
WalkthroughThe changes refactor the file extension mapping logic in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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 |
![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
🧹 Nitpick comments (2)
map.go (2)
21-21: Consider making this immutable or documenting its mutability.Since
extensionFormatscontains static values that shouldn't change at runtime, consider either:
- Using a constant with a compile-time array, or
- Adding a comment explaining why it's a variable if runtime modification is intended
Alternative: Use a constant-like pattern
If Go version supports it, or add a comment:
+// extensionFormats defines auxiliary file extensions that should be mapped for each main asset. +// These files are expected to exist alongside the main asset file. var extensionFormats = []string{".asc", ".sha", ".torrent"}
141-144: Consider edge case: empty file extension.The code appends
"."unconditionally at line 141 before appendingfile.Extensionat line 142. Iffile.Extensionis empty or unexpected, this could create malformed keys like"board/distro.".While the JSON structure likely ensures
Extensionis always populated, consider adding defensive validation earlier in the loop for robustness.Optional: Add validation for empty extension
for _, file := range data.Assets { + if file.Extension == "" { + log.WithFields(log.Fields{ + "board": file.BoardSlug, + }).Warning("Missing file extension; skipping entry") + continue + } + // Because download mapping a full URL, redirecting, and finding a server again is redundant,
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
map.go(2 hunks)map_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- map_test.go
🔇 Additional comments (1)
map.go (1)
132-144: Document JSON format changes and add file existence validation.This refactoring auto-generates auxiliary file mappings (
.asc,.sha,.torrent) instead of including them in the JSON source, but lacks documentation and validation:
JSON format undocumented: The README does not document the expected JSON structure or that auxiliary files should be excluded. The code references GitHub issue #129 but provides no user-facing documentation of this breaking change.
No file existence validation: The code assumes all auxiliary files exist at
path + ext(e.g.,file.img.xz.sha) without validation. Missing files will cause 404 errors for users requesting auxiliary mappings.No migration guidance: If existing JSON producers still include auxiliary entries, they create incorrect duplicate mappings. No backwards compatibility or migration strategy is documented.
| // Also add entries for .asc .sha and .torrent | ||
| for _, ext := range extensionFormats { | ||
| if strings.HasSuffix(file.Extension, ext) { | ||
| continue | ||
| } | ||
|
|
||
| if strings.HasSuffix(file.Extension, ".sha") { | ||
| sb.WriteString("sha") | ||
| } else if strings.HasSuffix(file.Extension, ".asc") { | ||
| sb.WriteString("asc") | ||
| } else if strings.HasSuffix(file.Extension, ".torrent") { | ||
| sb.WriteString("torrent") | ||
| } else { | ||
| sb.WriteString(file.Extension) | ||
| m[sb.String()+ext] = u.Path + ext | ||
| } |
There was a problem hiding this comment.
Validate or document that JSON should exclude auxiliary file entries.
The refactored logic assumes the input JSON contains only main asset files (e.g., img.xz), not auxiliary files (e.g., img.xz.asc, img.xz.sha). It then auto-generates mappings for the auxiliary extensions.
However, if auxiliary file entries are present in the JSON, this creates incorrect mappings. For example, processing an entry with extension="img.xz.asc" would generate mappings to non-existent paths like .../file.img.xz.asc.sha and .../file.img.xz.asc.torrent.
The continue at line 134 only partially mitigates this by skipping the matching extension.
Consider adding validation to warn or skip entries whose extensions end with any value in extensionFormats, as these should no longer appear in the refactored JSON format.
🔎 Suggested validation to detect auxiliary files in JSON
for _, file := range data.Assets {
+ // Validate that auxiliary files are not included in the JSON
+ for _, ext := range extensionFormats {
+ if strings.HasSuffix(file.Extension, ext) {
+ log.WithFields(log.Fields{
+ "extension": file.Extension,
+ "board": file.BoardSlug,
+ }).Warning("Auxiliary file entry detected in JSON; skipping to avoid incorrect mappings")
+ continue
+ }
+ }
+
// Because download mapping a full URL, redirecting, and finding a server again is redundant,Note: You'll need to adjust the control flow to skip to the next file in the outer loop, possibly using a labeled continue or a validation function.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In map.go around lines 132 to 139, the code auto-generates auxiliary mappings
(.asc, .sha, .torrent) but doesn’t guard against input JSON entries that are
already auxiliary files (e.g., extension="img.xz.asc"), which causes incorrect
mappings; add validation at the start of processing each file to detect if
file.Extension ends with any value in extensionFormats and, if so, log a warning
(including file path/extension) and skip that file (use a labeled continue or a
small helper that returns a bool to skip the outer loop iteration), so only
primary asset entries are used to generate auxiliary mappings.
2 participants
No description provided.