Propagate JSON unmarshal errors in type conversion functions

[Mischulee]

https://issues.redhat.com/browse/HYPERFLEET-300

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling when presenting cluster, node pool, and adapter status data.
  • Errors from data serialization are now properly surfaced instead of being silently ignored.

• Tests

  • Added test coverage for data serialization failure scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR introduces consistent error handling to presenter functions across the API layer. The functions PresentAdapterStatus, PresentCluster, and PresentNodePool were updated to return (value, error) tuples instead of single values. Each now validates JSON unmarshalling of critical fields (Conditions, Data, Metadata for adapters; Spec, Labels, StatusConditions for clusters and node pools) and propagates errors instead of silently ignoring failures. Corresponding handler functions in cluster, node pool, and adapter status management were updated to check for presentation errors and return GeneralError responses when presentation fails. All tests were updated to verify the new error return values and include additional test cases for malformed JSON scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Multiple presenter functions updated with identical error-handling pattern; verify consistency across all three (PresentAdapterStatus, PresentCluster, PresentNodePool)
• Handler files (cluster.go, cluster_nodepools.go, cluster_status.go, node_pool.go, nodepool_status.go) all follow similar error-propagation patterns—confirm all call sites handle errors correctly
• Test additions for malformed JSON scenarios across all presenter tests; validate coverage is comprehensive
• Error messages and GeneralError wrapping—ensure consistency and clarity in user-facing error communications

Possibly related PRs

• Implement Schema Validation for Cluster and NodePool Specifications #8: Directly modified the same presenter functions (PresentAdapterStatus, PresentCluster, PresentNodePool) to return error tuples and updated call sites to handle errors.

Suggested labels

error-handling, presenters, testing

Suggested reviewers

• rh-amarin
• yingzhanredhat

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main objective: propagating JSON unmarshal errors in type conversion functions (PresentAdapterStatus, PresentCluster, PresentNodePool, etc.).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb50b29 and 5416054.

📒 Files selected for processing (11)

• pkg/api/presenters/adapter_status.go (3 hunks)
• pkg/api/presenters/adapter_status_test.go (7 hunks)
• pkg/api/presenters/cluster.go (2 hunks)
• pkg/api/presenters/cluster_test.go (7 hunks)
• pkg/api/presenters/node_pool.go (2 hunks)
• pkg/api/presenters/node_pool_test.go (8 hunks)
• pkg/handlers/cluster.go (4 hunks)
• pkg/handlers/cluster_nodepools.go (3 hunks)
• pkg/handlers/cluster_status.go (2 hunks)
• pkg/handlers/node_pool.go (4 hunks)
• pkg/handlers/nodepool_status.go (2 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

pkg/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Use structured logging via pkg/logger/logger.go with Extra() method for adding context fields

Files:

• pkg/api/presenters/node_pool.go
• pkg/handlers/node_pool.go
• pkg/api/presenters/cluster_test.go
• pkg/handlers/nodepool_status.go
• pkg/handlers/cluster_nodepools.go
• pkg/api/presenters/cluster.go
• pkg/handlers/cluster_status.go
• pkg/api/presenters/adapter_status_test.go
• pkg/handlers/cluster.go
• pkg/api/presenters/adapter_status.go
• pkg/api/presenters/node_pool_test.go

pkg/{handlers,services}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Retrieve database sessions from context via db.NewContext() rather than creating new GORM connections

Files:

• pkg/handlers/node_pool.go
• pkg/handlers/nodepool_status.go
• pkg/handlers/cluster_nodepools.go
• pkg/handlers/cluster_status.go
• pkg/handlers/cluster.go

pkg/handlers/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

pkg/handlers/**/*.go: Automatically convert errors to OpenAPI error responses using errors.SendError() in HTTP handlers
Parse HTTP request bodies using json.NewDecoder(r.Body) in handler functions
Write HTTP responses using json.NewEncoder(w) for consistency with request parsing
Ensure all HTTP handlers follow the pattern: parse request, call service/DAO, handle errors, send response
Implement handlers as methods on handler structs that accept (w http.ResponseWriter, r *http.Request) parameters
Set HTTP status codes correctly: 201 Created for POST, 200 OK for GET, 204 No Content for DELETE operations

Files:

• pkg/handlers/node_pool.go
• pkg/handlers/nodepool_status.go
• pkg/handlers/cluster_nodepools.go
• pkg/handlers/cluster_status.go
• pkg/handlers/cluster.go

pkg/{dao,handlers}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Support pagination in list operations via ListArgs parameter with configurable limit and offset

Files:

• pkg/handlers/node_pool.go
• pkg/handlers/nodepool_status.go
• pkg/handlers/cluster_nodepools.go
• pkg/handlers/cluster_status.go
• pkg/handlers/cluster.go

pkg/{handlers,services,dao}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Return ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields in error handling

Files:

• pkg/handlers/node_pool.go
• pkg/handlers/nodepool_status.go
• pkg/handlers/cluster_nodepools.go
• pkg/handlers/cluster_status.go
• pkg/handlers/cluster.go

pkg/{db/transaction_middleware.go,handlers}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Store request operation IDs in context for tracing and debugging purposes in error logs

Files:

• pkg/handlers/node_pool.go
• pkg/handlers/nodepool_status.go
• pkg/handlers/cluster_nodepools.go
• pkg/handlers/cluster_status.go
• pkg/handlers/cluster.go

🧠 Learnings (9)

📚 Learning: 2025-12-12T11:04:29.971Z

Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and node pool specifications as JSONB columns to allow flexible cloud provider configurations

Applied to files:

• pkg/api/presenters/node_pool.go
• pkg/handlers/cluster_nodepools.go
• pkg/api/presenters/node_pool_test.go

📚 Learning: 2025-12-12T11:04:29.971Z

Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Automatically convert errors to OpenAPI error responses using errors.SendError() in HTTP handlers

Applied to files:

• pkg/handlers/node_pool.go
• pkg/handlers/nodepool_status.go
• pkg/handlers/cluster_status.go
• pkg/handlers/cluster.go

📚 Learning: 2025-12-12T11:04:29.971Z

Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{db/migrations,dao}/adapter_status.go : Store adapter statuses with observed_generation field to track which resource generation each adapter has processed

Applied to files:

• pkg/handlers/nodepool_status.go
• pkg/handlers/cluster_status.go
• pkg/api/presenters/adapter_status_test.go
• pkg/api/presenters/adapter_status.go

📚 Learning: 2025-12-12T11:04:29.971Z

Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/services/**/*.go : Calculate aggregate status phase from adapter conditions using Ready/Failed/NotReady states

Applied to files:

• pkg/handlers/nodepool_status.go
• pkg/handlers/cluster_status.go
• pkg/api/presenters/adapter_status_test.go
• pkg/api/presenters/adapter_status.go

📚 Learning: 2025-12-12T11:04:29.971Z

Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/services/{cluster_service,nodepool_service}.go : Increment generation field on each cluster or node pool spec update to enable optimistic concurrency control

Applied to files:

• pkg/handlers/cluster_nodepools.go

📚 Learning: 2025-12-12T11:04:29.971Z

Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Enforce foreign key relationships between node pools and clusters via owner_references constraint

Applied to files:

• pkg/handlers/cluster_nodepools.go
• pkg/api/presenters/node_pool_test.go

📚 Learning: 2025-12-12T11:04:29.971Z

Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Ensure all HTTP handlers follow the pattern: parse request, call service/DAO, handle errors, send response

Applied to files:

• pkg/handlers/cluster.go

📚 Learning: 2025-12-12T11:04:29.971Z

Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Implement handlers as methods on handler structs that accept (w http.ResponseWriter, r *http.Request) parameters

Applied to files:

• pkg/handlers/cluster.go

📚 Learning: 2025-12-12T11:04:29.971Z

Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/handlers/**/*.go : Set HTTP status codes correctly: 201 Created for POST, 200 OK for GET, 204 No Content for DELETE operations

Applied to files:

• pkg/handlers/cluster.go

🧬 Code graph analysis (10)

pkg/api/presenters/node_pool.go (2)

pkg/api/node_pool_types.go (1)

• NodePool (12-43)

pkg/api/status_types.go (1)

• ResourceCondition (28-37)

pkg/handlers/node_pool.go (2)

pkg/api/presenters/node_pool.go (1)

• PresentNodePool (57-155)

pkg/errors/errors.go (1)

• GeneralError (190-192)

pkg/api/presenters/cluster_test.go (2)

pkg/api/presenters/cluster.go (1)

• PresentCluster (51-137)

pkg/api/cluster_types.go (1)

• Cluster (11-34)

pkg/handlers/nodepool_status.go (2)

pkg/api/presenters/adapter_status.go (1)

• PresentAdapterStatus (76-133)

pkg/errors/errors.go (1)

• GeneralError (190-192)

pkg/api/presenters/cluster.go (2)

pkg/api/cluster_types.go (1)

• Cluster (11-34)

pkg/api/status_types.go (1)

• ResourceCondition (28-37)

pkg/handlers/cluster_status.go (2)

pkg/api/presenters/adapter_status.go (1)

• PresentAdapterStatus (76-133)

pkg/errors/errors.go (1)

• GeneralError (190-192)

pkg/api/presenters/adapter_status_test.go (3)

pkg/api/presenters/adapter_status.go (1)

• PresentAdapterStatus (76-133)

pkg/api/adapter_status_types.go (1)

• AdapterStatus (11-30)

pkg/api/metadata_types.go (1)

• Metadata (28-34)

pkg/handlers/cluster.go (2)

pkg/api/presenters/cluster.go (1)

• PresentCluster (51-137)

pkg/errors/errors.go (1)

• GeneralError (190-192)

pkg/api/presenters/adapter_status.go (3)

pkg/api/adapter_status_types.go (1)

• AdapterStatus (11-30)

pkg/api/status_types.go (1)

• AdapterCondition (42-48)

pkg/api/metadata_types.go (1)

• Metadata (28-34)

pkg/api/presenters/node_pool_test.go (2)

pkg/api/presenters/node_pool.go (1)

• PresentNodePool (57-155)

pkg/api/node_pool_types.go (1)

• NodePool (12-43)

🔇 Additional comments (28)

pkg/api/presenters/adapter_status.go (1)

76-132: LGTM!

The error propagation for JSON unmarshalling is correctly implemented. The function now returns descriptive errors for Conditions, Data, and Metadata fields, with proper error wrapping using %w. The pattern is consistent with other presenters in this PR.

pkg/api/presenters/cluster.go (1)

51-136: LGTM!

The error handling for JSON unmarshalling of Spec, Labels, and StatusConditions is implemented consistently with the other presenters. Error messages are context-specific and properly wrapped.

pkg/api/presenters/node_pool.go (1)

57-154: LGTM!

The error propagation follows the same pattern established in the other presenters. JSON unmarshalling failures for Spec, Labels, and StatusConditions are now properly propagated with descriptive error messages.

pkg/handlers/node_pool.go (4)

49-53: LGTM!

The Create handler correctly wraps the presentation error with GeneralError, following the established error handling pattern in the codebase.

---

91-95: LGTM!

The Patch handler properly handles presentation errors with consistent error messaging.

---

118-122: LGTM!

The List handler appropriately handles presentation errors for each item, returning early on the first failure. This prevents partial results from being returned when presentation fails.

---

162-166: LGTM!

The Get handler correctly propagates presentation errors with appropriate error wrapping.

pkg/handlers/cluster.go (4)

50-54: LGTM!

The Create handler correctly handles presentation errors, following the pattern established across the codebase. As per coding guidelines, errors are converted to OpenAPI error responses using errors.GeneralError().

---

88-92: LGTM!

The Patch handler properly propagates presentation errors with consistent error messaging.

---

120-124: LGTM!

The List handler correctly handles per-item presentation errors, returning early on failure to prevent partial/inconsistent results.

---

150-154: LGTM!

The Get handler appropriately wraps presentation errors with GeneralError.

pkg/api/presenters/node_pool_test.go (2)

203-204: LGTM!

Existing tests correctly updated to handle the new (result, error) return signature.

---

439-497: LGTM!

The new malformed JSON tests comprehensively cover all three error paths (Spec, Labels, StatusConditions). Each test verifies that:

1. An error is returned (non-nil)
2. The error message contains the expected substring

This provides good coverage for the new error propagation behavior.

pkg/handlers/nodepool_status.go (2)

44-48: LGTM! Error handling properly propagates presentation failures.

The error check correctly short-circuits the loop and returns a GeneralError when presentation fails, preventing partial responses with potentially inconsistent data. Based on learnings, this follows the pattern of automatically converting errors to OpenAPI error responses.

---

105-109: LGTM! Consistent error propagation in Create handler.

The Create handler now properly validates the presentation result before returning, ensuring clients receive meaningful error responses for data integrity issues rather than potentially malformed responses.

pkg/api/presenters/adapter_status_test.go (4)

269-270: LGTM! Test correctly updated for new function signature.

The test properly unpacks the (result, error) tuple and asserts the error is nil for successful presentation.

---

439-460: LGTM! Good coverage for malformed Conditions error path.

The test correctly verifies that malformed JSON in the Conditions field produces an error with the expected message substring.

---

462-483: LGTM! Good coverage for malformed Data error path.

The test correctly verifies that malformed JSON in the Data field produces an error with the expected message substring.

---

485-507: LGTM! Good coverage for malformed Metadata error path.

The test correctly verifies that malformed JSON in the Metadata field produces an error with the expected message substring.

pkg/handlers/cluster_nodepools.go (3)

60-64: LGTM! Consistent error handling in List.

The loop correctly handles presentation errors and returns early, preventing partial/inconsistent list responses.

---

120-124: LGTM! Error handling added to Get handler.

The Get handler now properly validates presentation before returning the response.

---

163-167: LGTM! Create handler follows consistent pattern.

The error handling for presentation matches the pattern used across other handlers in this PR.

pkg/handlers/cluster_status.go (2)

44-48: LGTM! Consistent error handling pattern.

The List handler correctly propagates presentation errors using GeneralError, matching the pattern established across the codebase. Based on learnings, this aligns with the guideline to automatically convert errors to OpenAPI error responses.

---

105-109: LGTM! Create handler properly validates presentation.

The error handling ensures clients receive appropriate error responses when presentation fails rather than potentially corrupted data.

pkg/api/presenters/cluster_test.go (4)

206-207: LGTM! Test correctly updated for new PresentCluster signature.

The test properly handles the (result, error) return type and verifies success.

---

411-428: LGTM! Good coverage for malformed Spec error path.

The test correctly verifies that invalid JSON in the Spec field produces an error containing the expected message.

---

430-447: LGTM! Good coverage for malformed Labels error path.

The test correctly verifies that invalid JSON in the Labels field produces an error containing the expected message.

---

449-466: LGTM! Good coverage for malformed StatusConditions error path.

The test correctly verifies that invalid JSON in the StatusConditions field produces an error containing the expected message.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ciaranRoche]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ciaranRoche

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ciaranRoche]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment