fix: add nil checks for req.Filters to prevent panic on empty request…#109
Open
CN-Antonio wants to merge 1 commit into
Open
fix: add nil checks for req.Filters to prevent panic on empty request…#109CN-Antonio wants to merge 1 commit into
CN-Antonio wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CN-Antonio The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @CN-Antonio! It looks like this is your first PR to Project-HAMi/HAMi-WebUI 🎉 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds nil-request guards to node, card, and container handlers, and adds readiness state from node/pod repos through usecases into ChangesNil pointer guards for empty request bodies
Informer readiness reporting
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPHandler as "/readyz Handler"
participant NodeService
participant NodeUsecase
participant PodUseCase
participant NodeRepo
participant PodRepo
Client->>HTTPHandler: GET /readyz
HTTPHandler->>NodeService: Ready()
NodeService->>NodeUsecase: Ready()
NodeUsecase->>NodeRepo: Ready()
NodeRepo-->>NodeUsecase: bool
NodeService->>PodUseCase: Ready()
PodUseCase->>PodRepo: Ready()
PodRepo-->>PodUseCase: bool
NodeService-->>HTTPHandler: combined bool
alt ready
HTTPHandler-->>Client: 200 "ok"
else not ready
HTTPHandler-->>Client: 503 "not ready"
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
… bodies
When the Go backend receives a request with empty body {}, protobuf
deserialization sets req.Filters to nil. The service layer then accesses
filters.DeviceId etc. without nil checks, causing nil pointer dereference.
This affects /v1/summary, /v1/nodes, /v1/gpus, /v1/containers and their
detail endpoints. The built-in Vue frontend is unaffected because it always
includes the filters field.
Changes:
- service/node.go: nil guards on GetSummary, GetAllNodes, GetNode
- service/card.go: nil guards on GetAllGPUs, GetAllGPUTypes, GetGPU
- service/container.go: nil guard on GetAllContainers
- data/node.go: add Ready() flag set after informer sync
- data/pod.go: add Ready() flag set after informer sync
- biz/node.go: add Ready() to NodeRepo interface
- biz/pod.go: add Ready() to PodRepo interface
- server/http.go: /readyz returns 503 when informers not synced
Fixes Project-HAMi#108
Signed-off-by: CN-Antonio <24762952+CN-Antonio@users.noreply.github.com>
CN-Antonio
force-pushed
the
fix/nil-pointer-panic-on-empty-filters
branch
from
418514b to
91b10e0
Compare
![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 (3)
server/internal/service/card.go (1)
24-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame manual nil-guard duplicated twice in this file — simplify with
GetFilters().Both
GetAllGPUsandGetAllGPUTypesrepeat the identicalif req != nil { filters = req.Filters }pattern. The generated getterreq.GetFilters()already nil-checks the receiver, so this can be simplified and de-duplicated.♻️ Proposed simplification
func (s *CardService) GetAllGPUs(ctx context.Context, req *pb.GetAllGpusReq) (*pb.GPUsReply, error) { - var filters *pb.GetAllGpusReq_Filters - if req != nil { - filters = req.Filters - } + filters := req.GetFilters() if filters == nil { filters = &pb.GetAllGpusReq_Filters{} }func (s *CardService) GetAllGPUTypes(ctx context.Context, req *pb.GetAllGpusReq) (*pb.GPUsReply, error) { ... - var filters *pb.GetAllGpusReq_Filters - if req != nil { - filters = req.Filters - } + filters := req.GetFilters() if filters == nil { filters = &pb.GetAllGpusReq_Filters{} }Since both methods take the same request type, consider extracting a small local helper (e.g.
defaultGpusFilters(req *pb.GetAllGpusReq) *pb.GetAllGpusReq_Filters) to avoid the duplicated fallback logic entirely.Also applies to: 124-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/service/card.go` around lines 24 - 30, The request filter fallback logic in GetAllGPUs and GetAllGPUTypes is duplicated and manually nil-checks the request before reading Filters. Replace the repeated `if req != nil { filters = req.Filters }` pattern with the generated `GetFilters()` accessor on the request type, and factor the defaulting logic into a small local helper such as `defaultGpusFilters` so both methods share the same fallback behavior without duplication.server/internal/service/container.go (1)
48-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify nil-guard using generated
GetFilters()getter.Same pattern as node.go/card.go:
req.GetFilters()already nil-checks the receiver, making the manualif req != nilblock unnecessary.♻️ Proposed simplification
func (s *ContainerService) GetAllContainers(ctx context.Context, req *pb.GetAllContainersReq) (*pb.ContainersReply, error) { - var filters *pb.GetAllContainersReq_Filters - if req != nil { - filters = req.Filters - } + filters := req.GetFilters() if filters == nil { filters = &pb.GetAllContainersReq_Filters{} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/service/container.go` around lines 48 - 54, Simplify the nil-guard in GetAllContainers by using the generated req.GetFilters() getter instead of manually checking req and assigning req.Filters. Update the container service code path to mirror the existing node.go/card.go pattern, keeping the fallback to an empty pb.GetAllContainersReq_Filters when the getter returns nil.server/internal/service/node.go (1)
34-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify nil-guard using generated
GetFilters()getter.Standard protoc-gen-go messages generate nil-safe getters (
func (x *T) GetFilters() *T_Filters { if x != nil { return x.Filters }; return nil }), so the manualif req != nil { filters = req.Filters }block is redundant —req.GetFilters()already handles a nilreq.♻️ Proposed simplification
func (s *NodeService) GetSummary(ctx context.Context, req *pb.GetSummaryReq) (*pb.DeviceSummaryReply, error) { - var filters *pb.GetSummaryReq_Filters - if req != nil { - filters = req.Filters - } + filters := req.GetFilters() var res = &pb.DeviceSummaryReply{} if filters == nil { filters = &pb.GetSummaryReq_Filters{} }func (s *NodeService) GetAllNodes(ctx context.Context, req *pb.GetAllNodesReq) (*pb.NodesReply, error) { - var filters *pb.GetAllNodesReq_Filters - if req != nil { - filters = req.Filters - } + filters := req.GetFilters() if filters == nil { filters = &pb.GetAllNodesReq_Filters{} }Please confirm the generated getters exist for
GetSummaryReq/GetAllNodesReqbefore applying (standard for protoc-gen-go output, but worth a quick check).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/service/node.go` around lines 34 - 55, Simplify the nil handling in NodeService.GetSummary and NodeService.GetAllNodes by using the generated nil-safe GetFilters() accessor on GetSummaryReq and GetAllNodesReq instead of manually checking req for nil and reading req.Filters. Verify the protoc-gen-go generated getters exist for these request types, then replace the temporary filters setup with the getter-based flow so the methods remain nil-safe and shorter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/internal/data/node.go`:
- Line 33: The readiness state in nodeRepo is being set from local cache
population even when informer sync fails, so /readyz can become true too early.
Update the nodeRepo readiness logic around init(), Ready(), and updateLocalNodes
so readiness is only enabled when the initial sync succeeds, not just when data
is populated. Use the existing syncedOK handling in nodeRepo to gate Ready() and
keep ready false if cache sync fails.
---
Nitpick comments:
In `@server/internal/service/card.go`:
- Around line 24-30: The request filter fallback logic in GetAllGPUs and
GetAllGPUTypes is duplicated and manually nil-checks the request before reading
Filters. Replace the repeated `if req != nil { filters = req.Filters }` pattern
with the generated `GetFilters()` accessor on the request type, and factor the
defaulting logic into a small local helper such as `defaultGpusFilters` so both
methods share the same fallback behavior without duplication.
In `@server/internal/service/container.go`:
- Around line 48-54: Simplify the nil-guard in GetAllContainers by using the
generated req.GetFilters() getter instead of manually checking req and assigning
req.Filters. Update the container service code path to mirror the existing
node.go/card.go pattern, keeping the fallback to an empty
pb.GetAllContainersReq_Filters when the getter returns nil.
In `@server/internal/service/node.go`:
- Around line 34-55: Simplify the nil handling in NodeService.GetSummary and
NodeService.GetAllNodes by using the generated nil-safe GetFilters() accessor on
GetSummaryReq and GetAllNodesReq instead of manually checking req for nil and
reading req.Filters. Verify the protoc-gen-go generated getters exist for these
request types, then replace the temporary filters setup with the getter-based
flow so the methods remain nil-safe and shorter.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fb1c1a2-c8e0-4b66-b329-0d06fbac801f
📒 Files selected for processing (8)
server/internal/biz/node.goserver/internal/biz/pod.goserver/internal/data/node.goserver/internal/data/pod.goserver/internal/server/http.goserver/internal/service/card.goserver/internal/service/container.goserver/internal/service/node.go
| log *log.Helper | ||
| mutex sync.RWMutex | ||
| providers []provider.Provider | ||
| ready bool |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
nodeRepo.Ready() should stay false when cache sync fails. syncedOK is only used for logging in init(), while Ready() still flips on when updateLocalNodes populates data. That can expose /readyz as ready even after a failed informer sync; gate readiness on sync success as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/internal/data/node.go` at line 33, The readiness state in nodeRepo is
being set from local cache population even when informer sync fails, so /readyz
can become true too early. Update the nodeRepo readiness logic around init(),
Ready(), and updateLocalNodes so readiness is only enabled when the initial sync
succeeds, not just when data is populated. Use the existing syncedOK handling in
nodeRepo to gate Ready() and keep ready false if cache sync fails.
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
1 participant
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.
… bodies
When the Go backend receives a request with empty body {}, protobuf deserialization sets req.Filters to nil. The service layer then accesses filters.DeviceId etc. without nil checks, causing nil pointer dereference.
This affects /v1/summary, /v1/nodes, /v1/gpus, /v1/containers and their detail endpoints. The built-in Vue frontend is unaffected because it always includes the filters field.
Changes:
Fixes #108
Summary by CodeRabbit
Ready()method./readyzendpoint to return200 okonly when node data is ready; otherwise it returns503 not ready.nilrequests andnilfilters in GPU, container, and node endpoints.