Ensure Snapshot.Disks marshals as [] instead of null - #94
Merged
Conversation
docs/API.md says fields may briefly read as "empty arrays" right after startup, but the []Disk zero value marshals to JSON null (Disks has no omitempty, unlike Network), which trips strict JSON consumers iterating disks. Initialize the slice on Collector construction and normalize a nil result from DiskCollector.Collect (empty/failed collection) so Snapshot.Disks is always a non-nil slice, matching the documented behavior. Closes #70
SonarCloud's quality gate flagged the nil-normalization branch added for #70 as untested new code: DiskCollector.Collect now always returns a non-nil slice on success, which left fastTick's nil check (covering a failed collection, e.g. /proc/mounts unreadable) with no test exercising it directly.
|
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
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.



📖 Description
docs/API.mddocuments that fields may briefly read as "empty arrays" right after process start, before the first collection tick completes. In practice,Snapshot.Disks []Diskhas noomitempty(unlikeNetwork), and its Go zero value (nil) marshals to JSONnull, not[]. Strict JSON consumers iteratingdiskswould trip onnull. This also applied after any tick where disk collection failed or found no eligible mountpoints (DiskCollector.Collectusedvar disks []Disk, which staysnilwhen nothing is appended).Fix: keep
Snapshot.Disksa non-nil (possibly empty) slice everywhere:internal/collector/disk.go:DiskCollector.Collectnow allocates withmake([]Disk, 0, len(order))instead of a nilvar disks []Disk.internal/collector/collector.go: theCollector's initiallatestsnapshot now starts withDisks: []Disk{}(covers the window before the first fast tick), andfastTicknormalizes a nil result fromDiskCollector.Collect(e.g. on a read error) before storing it.This is a docs-consistency fix, not a JSON shape change — the field itself (
disks) is unchanged, only its zero-state value goes fromnullto[], which is whatdocs/API.mdalready promised.🎫 Issues
Closes #70
👩💻 Reviewer Notes
Changes are confined to
internal/collector. No API surface, config, or packaging changes.📑 Test Plan
TestCollector_Disks_NeverMarshalsAsNullininternal/collector/collector_test.go, which JSON-marshals aSnapshotboth before the firstfastTickand after, asserting"disks"is never"null".TestDiskCollector_Collect_NoMountsReturnsEmptyNotNilSliceininternal/collector/disk_test.gocovering the case where no mountpoint qualifies (all excluded pseudo filesystems).TestDiskCollector_Collect_SkipsFailingStatfsto assert the returned slice is non-nil.go build ./...,go vet ./...,go test ./... -race -cover, andgolangci-lint runall pass locally.✅ Checklist
General
go test ./... -race -coverpasses locally).go vet ./...andgolangci-lint runare clean.ARCHITECTURE.mdif this changes a documented design decision. (not applicable — no design change)REST API / configuration / packaging
docs/API.mdto reflect a REST API change. (not needed — the doc already describes this exact behavior; the code now matches it)/api/v1/...response shapes, or a new API version (/api/v2/...) was introduced instead.README.md/packaging/pimonitor.example.yaml— (not applicable)packaging/install.shor the systemd units — (not applicable)⏭ Next Steps
None.