feat(rest-api): return valid JSON from DELETE endpoints#2701
Open
osu wants to merge 5 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughIntroduces a new ChangesDeletion Response Standardization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2701.docs.buildwithfern.com/infra-controller |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-06-19 07:10:17 UTC | Commit: 85a174e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@rest-api/api/pkg/api/handler/dpuextensionservice.go`:
- Line 1125: The deletion handler returns HTTP 204 No Content with a JSON
response body, which violates RFC 7231 that explicitly prohibits message bodies
in 204 responses. In the dpuextensionservice.go file, locate all return
statements using c.JSON(http.StatusNoContent,
model.NewAPIDeletionCompletedResponse()) (appearing at least at lines 1125 and
1653) and change http.StatusNoContent to http.StatusOK to maintain HTTP
specification compliance while still returning the structured deletion response
body.
In `@rest-api/openapi/spec.yaml`:
- Around line 2911-2920: The 202 Accepted response examples in the OpenAPI spec
use a message that indicates completion ("Resource deleted successfully")
instead of reflecting the async/accepted semantics of a 202 response. Update the
example message values in both 202 response examples (one around line 2911-2920
and another around line 3227-3236) that reference the DeletionResponse schema to
use a message that accurately represents an accepted but pending deletion
request, such as "Deletion request has been accepted and is being processed" or
similar wording that conveys the request is queued rather than already
completed.
- Around line 2713-2722: The OpenAPI specification declares content and schemas
for HTTP 204 No Content responses, which violates RFC 7231 since 204 responses
must not contain a message body. Remove the content declaration (including the
application/json schema and DeletionResponse reference) from all 204 response
definitions in DELETE endpoints, or alternatively change the response status
code to 200 OK or 202 Accepted if returning a DeletionResponse body is required.
Ensure consistency between the OpenAPI spec and the handler implementations that
use c.JSON(http.StatusNoContent, ...) to use the same contract across all
affected DELETE endpoints.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3e61b954-2b05-4fe0-bcf5-df529045ccea
📒 Files selected for processing (33)
rest-api/api/pkg/api/handler/allocation.gorest-api/api/pkg/api/handler/deletion_response_test_helper_test.gorest-api/api/pkg/api/handler/dpuextensionservice.gorest-api/api/pkg/api/handler/expectedmachine.gorest-api/api/pkg/api/handler/expectedmachine_test.gorest-api/api/pkg/api/handler/expectedpowershelf.gorest-api/api/pkg/api/handler/expectedrack.gorest-api/api/pkg/api/handler/expectedswitch.gorest-api/api/pkg/api/handler/infinibandpartition.gorest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/instancetype.gorest-api/api/pkg/api/handler/ipblock.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machineinstancetype.gorest-api/api/pkg/api/handler/machinevalidation.gorest-api/api/pkg/api/handler/networksecuritygroup.gorest-api/api/pkg/api/handler/nvlinklogicalpartition.gorest-api/api/pkg/api/handler/operatingsystem.gorest-api/api/pkg/api/handler/site.gorest-api/api/pkg/api/handler/sshkey.gorest-api/api/pkg/api/handler/sshkeygroup.gorest-api/api/pkg/api/handler/subnet.gorest-api/api/pkg/api/handler/taskrule.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/tenantidentity.gorest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/handler/vpcpeering.gorest-api/api/pkg/api/handler/vpcprefix.gorest-api/api/pkg/api/model/deletion.gorest-api/api/pkg/api/model/deletion_test.gorest-api/openapi/spec.yaml
| logger.Info().Msg("finishing API handler") | ||
|
|
||
| return c.NoContent(http.StatusNoContent) | ||
| return c.JSON(http.StatusNoContent, model.NewAPIDeletionCompletedResponse()) |
Contributor
There was a problem hiding this comment.
HTTP 204 with response body violates HTTP specification.
Both deletion handlers now return HTTP 204 No Content with a JSON response body. This directly contradicts RFC 7231 Section 6.3.5, which explicitly states that 204 responses MUST NOT contain a message body. While the OpenAPI specification documents this as intentional design, this creates potential compatibility issues:
- Strict HTTP clients may discard the response body
- Proxies and intermediaries may strip bodies from 204 responses
- API consumers expecting standard HTTP semantics will encounter unexpected behavior
Recommended alternative: Use HTTP 200 OK for synchronous deletions that return a structured response body. This maintains HTTP spec compliance while still providing the desired JSON response:
return c.JSON(http.StatusOK, model.NewAPIDeletionCompletedResponse())If the team has decided to proceed with 204 + body by design, document this deviation and its rationale prominently in API documentation to set client expectations.
Also applies to: 1653-1653
🤖 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 `@rest-api/api/pkg/api/handler/dpuextensionservice.go` at line 1125, The
deletion handler returns HTTP 204 No Content with a JSON response body, which
violates RFC 7231 that explicitly prohibits message bodies in 204 responses. In
the dpuextensionservice.go file, locate all return statements using
c.JSON(http.StatusNoContent, model.NewAPIDeletionCompletedResponse()) (appearing
at least at lines 1125 and 1653) and change http.StatusNoContent to
http.StatusOK to maintain HTTP specification compliance while still returning
the structured deletion response body.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Introduce a typed JSON body for async DELETE requests that return HTTP 202 Accepted. Fixes NVIDIA#2090 Signed-off-by: Hasan Khan <hasank@nvidia.com>
Replace plain-text c.String responses with APIDeletionAcceptedResponse so clients can json.Unmarshal delete outcomes. Signed-off-by: Hasan Khan <hasank@nvidia.com>
Signed-off-by: Hasan Khan <hasank@nvidia.com>
Add DeletionAcceptedResponse schema for async delete endpoints only. Regenerate sdk/standard, update sdk/simple wrappers, and publish docs. Signed-off-by: Hasan Khan <hasank@nvidia.com>
osu
force-pushed
the
feat/2090-delete-json-responses
branch
from
9c0defa to
9b7f486
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rest-api/api/pkg/api/model/deletion_test.go (1)
14-23: ⚡ Quick winConsider adding explicit JSON structure verification.
The round-trip test validates that the JSON tag is correct (a wrong tag would fail unmarshaling), but adding an explicit assertion on the marshaled JSON bytes would make the test more explicit and catch structural issues immediately.
💡 Suggested enhancement
func TestAPIDeletionAcceptedResponse_JSON(t *testing.T) { t.Parallel() payload, err := json.Marshal(NewAPIDeletionAcceptedResponse()) require.NoError(t, err) + assert.JSONEq(t, `{"message":"Deletion request was accepted"}`, string(payload)) var decoded APIDeletionAcceptedResponse require.NoError(t, json.Unmarshal(payload, &decoded)) assert.Equal(t, DeletionRequestAcceptedMessage, decoded.Message) }🤖 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 `@rest-api/api/pkg/api/model/deletion_test.go` around lines 14 - 23, In the TestAPIDeletionAcceptedResponse_JSON function, add an explicit assertion on the marshaled JSON payload bytes to verify the actual JSON structure rather than relying solely on the round-trip test. After marshaling NewAPIDeletionAcceptedResponse() into the payload variable, add an assertion that checks the payload bytes match the expected JSON structure (for example, verifying it contains the Message field with the correct value). This makes the test more explicit and will catch structural issues in the JSON output immediately without depending on unmarshaling to validate it.rest-api/openapi/spec.yaml (2)
22959-22960: 💤 Low valuePrefer
exampleoverexamplesat schema level.The
examplesarray format is valid OpenAPI 3.0, but schema-level examples conventionally use the singularexamplekeyword for a single representative value. The pluralexamplesis typically reserved for media-type–level Example Objects (as correctly used in the response definitions).Suggested fix
- examples: - - message: Deletion request was accepted + example: + message: Deletion request was accepted🤖 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 `@rest-api/openapi/spec.yaml` around lines 22959 - 22960, Replace the `examples` array at the schema level with the singular `example` keyword. Convert the array structure containing the message "Deletion request was accepted" to use the `example` keyword directly with the message value, removing the array wrapping. This aligns with OpenAPI 3.0 conventions where schema-level examples use the singular `example` keyword, reserving the plural `examples` for media-type-level usage.
762-762: ⚡ Quick winStandardize 202 response descriptions across all DELETE endpoints.
Most DELETE operations document the
202response with description"Accepted", while line 1033 uses"Deletion request was accepted". This inconsistency introduces ambiguity—readers cannot determine whether the variation is intentional or an oversight.Prefer a uniform description across all async DELETE responses. Either adopt the more explicit
"Deletion request was accepted"everywhere, or retain the concise"Accepted"consistently. The former clarifies intent; the latter aligns with standard HTTP semantics.Suggested fix
Option A: Align all to the explicit description:
- description: Accepted + description: Deletion request was acceptedOption B: Align line 1033 to match the others:
- description: Deletion request was accepted + description: AcceptedAlso applies to: 1033-1033, 1468-1468, 1901-1901, 2383-2383, 4896-4896, 5226-5226, 5660-5660, 5912-5912, 6960-6960, 8281-8281, 9117-9117, 11499-11499, 12078-12078, 12373-12373
🤖 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 `@rest-api/openapi/spec.yaml` at line 762, Standardize the description for 202 responses across all DELETE endpoints in the OpenAPI specification file. Currently, most DELETE operations use the description "Accepted" while some locations (including line 1033 and others noted in the comment) use "Deletion request was accepted", creating inconsistency. Identify all 202 response descriptions at the specified line numbers (762, 1033, 1468, 1901, 2383, 4896, 5226, 5660, 5912, 6960, 8281, 9117, 11499, 12078, 12373) in the DELETE endpoint definitions and update them all to use the same description—either keep the concise "Accepted" consistently throughout or update all to use the more explicit "Deletion request was accepted" for clarity.
🤖 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.
Nitpick comments:
In `@rest-api/api/pkg/api/model/deletion_test.go`:
- Around line 14-23: In the TestAPIDeletionAcceptedResponse_JSON function, add
an explicit assertion on the marshaled JSON payload bytes to verify the actual
JSON structure rather than relying solely on the round-trip test. After
marshaling NewAPIDeletionAcceptedResponse() into the payload variable, add an
assertion that checks the payload bytes match the expected JSON structure (for
example, verifying it contains the Message field with the correct value). This
makes the test more explicit and will catch structural issues in the JSON output
immediately without depending on unmarshaling to validate it.
In `@rest-api/openapi/spec.yaml`:
- Around line 22959-22960: Replace the `examples` array at the schema level with
the singular `example` keyword. Convert the array structure containing the
message "Deletion request was accepted" to use the `example` keyword directly
with the message value, removing the array wrapping. This aligns with OpenAPI
3.0 conventions where schema-level examples use the singular `example` keyword,
reserving the plural `examples` for media-type-level usage.
- Line 762: Standardize the description for 202 responses across all DELETE
endpoints in the OpenAPI specification file. Currently, most DELETE operations
use the description "Accepted" while some locations (including line 1033 and
others noted in the comment) use "Deletion request was accepted", creating
inconsistency. Identify all 202 response descriptions at the specified line
numbers (762, 1033, 1468, 1901, 2383, 4896, 5226, 5660, 5912, 6960, 8281, 9117,
11499, 12078, 12373) in the DELETE endpoint definitions and update them all to
use the same description—either keep the concise "Accepted" consistently
throughout or update all to use the more explicit "Deletion request was
accepted" for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3033efc5-4c62-40b7-9e11-d1d63e1224c1
⛔ Files ignored due to path filters (16)
rest-api/sdk/standard/api_allocation.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_infini_band_partition.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_instance.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_instance_type.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_ip_block.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_machine.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_network_security_group.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_nv_link_logical_partition.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_operating_system.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_site.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_ssh_key.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_ssh_key_group.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_tenant_account.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_vpc.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_deletion_accepted_response.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (31)
rest-api/api/pkg/api/handler/allocation.gorest-api/api/pkg/api/handler/deletion_response_test_helper_test.gorest-api/api/pkg/api/handler/infinibandpartition.gorest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/instancetype.gorest-api/api/pkg/api/handler/ipblock.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machineinstancetype.gorest-api/api/pkg/api/handler/machinevalidation.gorest-api/api/pkg/api/handler/networksecuritygroup.gorest-api/api/pkg/api/handler/nvlinklogicalpartition.gorest-api/api/pkg/api/handler/operatingsystem.gorest-api/api/pkg/api/handler/site.gorest-api/api/pkg/api/handler/sshkey.gorest-api/api/pkg/api/handler/sshkeygroup.gorest-api/api/pkg/api/handler/subnet.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/handler/vpcprefix.gorest-api/api/pkg/api/model/deletion.gorest-api/api/pkg/api/model/deletion_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/sdk/simple/infinibandpartition.gorest-api/sdk/simple/instance.gorest-api/sdk/simple/nvlinklogicalpartition.gorest-api/sdk/simple/operatingsystem.gorest-api/sdk/simple/sshkeygroup.gorest-api/sdk/simple/vpc.go
✅ Files skipped from review due to trivial changes (8)
- rest-api/api/pkg/api/handler/instancetype.go
- rest-api/sdk/simple/sshkeygroup.go
- rest-api/api/pkg/api/handler/instance_test.go
- rest-api/api/pkg/api/handler/machine.go
- rest-api/sdk/simple/infinibandpartition.go
- rest-api/api/pkg/api/handler/machinevalidation.go
- rest-api/api/pkg/api/handler/instance.go
- rest-api/sdk/simple/operatingsystem.go
🚧 Files skipped from review as they are similar to previous changes (18)
- rest-api/api/pkg/api/handler/site.go
- rest-api/api/pkg/api/handler/tenantaccount.go
- rest-api/sdk/simple/vpc.go
- rest-api/api/pkg/api/handler/operatingsystem.go
- rest-api/api/pkg/api/handler/nvlinklogicalpartition.go
- rest-api/api/pkg/api/handler/vpcprefix.go
- rest-api/api/pkg/api/handler/machineinstancetype.go
- rest-api/api/pkg/api/handler/sshkey.go
- rest-api/sdk/simple/nvlinklogicalpartition.go
- rest-api/api/pkg/api/handler/vpc_test.go
- rest-api/api/pkg/api/handler/networksecuritygroup.go
- rest-api/api/pkg/api/handler/subnet.go
- rest-api/api/pkg/api/handler/sshkeygroup.go
- rest-api/api/pkg/api/handler/allocation.go
- rest-api/sdk/simple/instance.go
- rest-api/api/pkg/api/handler/vpc.go
- rest-api/api/pkg/api/handler/infinibandpartition.go
- rest-api/api/pkg/api/handler/ipblock.go
Assert marshaled JSON bytes in the model test, use schema-level example in OpenAPI, and standardize 202 delete response descriptions. Signed-off-by: Hasan Khan <hasank@nvidia.com>
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.
Description
DELETE success routes in the carbide REST API did not consistently return valid JSON bodies. Many async delete handlers responded with a plain-text string (
Deletion request was accepted) viac.String, which breaks clients that expect tojson.Unmarshalevery API response.Per #2090 and maintainer agreement with the reporter: HTTP 202 Accepted async deletes should return JSON instead of a plain text string; HTTP 204 No Content sync deletes should remain empty (204 does not allow a response body).
This PR introduces
APIDeletionAcceptedResponse({"message":"Deletion request was accepted"}) and updates only the 202 Accepted async delete handlers. 204 No Content sync delete handlers are unchanged (c.NoContent). OpenAPI andsdk/standardare updated for 202 delete endpoints only.Signed-off-by: Hasan Khan hasank@nvidia.com
Type of Change
Related Issues (Optional)
Fixes #2090
Breaking Changes
Testing
Validated locally (Go toolchain from
rest-api/go.mod):go test ./api/pkg/api/model/... -run TestAPIDeletionAcceptedResponse— passedgo build ./api/pkg/api/handler/...— cleango vet ./sdk/simple/...— cleanoasdiff breaking origin/main:rest-api/openapi/spec.yaml :rest-api/openapi/spec.yaml— no breaking changesmake test-api— not run (requires PostgreSQL on :30432)Additional Notes
~47 files changed (down from ~83 in the earlier revision). Clients that previously matched the raw string
"Deletion request was accepted"will still find that value in themessagefield on 202 responses. The generatedsdk/standarddelete clients for 202 endpoints now deserializeDeletionAcceptedResponse;sdk/simplewrappers ignore the body and continue to use the HTTP response for error handling.