Conversation
The v1alpha1 REST serializer always included spec and status
fields even when they serialized to empty JSON objects.
GetSpec()/GetStatus() return non-nil pointers to empty
structs, which marshal to "{}". json.RawMessage("{}") has
length 2 so omitempty doesn't omit it.
Skip assigning the marshaled bytes when the result is "{}",
matching the pattern used by unversioned.Resource.
Signed-off-by: Bart Smykla <bartek@smykla.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the REST v1alpha1 resource JSON marshaling to omit spec and/or status when they serialize to an empty JSON object ({}), aligning output with omitempty expectations and existing behavior in the unversioned.Resource marshaler.
Changes:
- Update
pkg/core/resources/model/rest/v1alpha1.Resource.MarshalJSON()to skip assigningspec/statuswhen marshaled bytes equal"{}". - Add unit tests covering omission of non-nil empty
specandstatus. - Refresh API-server golden JSON fixtures to reflect the new omission behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/core/resources/model/rest/v1alpha1/resource.go | Skip emitting spec/status when marshaled value is {} so omitempty can omit fields. |
| pkg/core/resources/model/rest/v1alpha1/resource_test.go | Add coverage for non-nil empty spec and status being omitted. |
| pkg/api-server/testdata/resources/crud/*.golden.json | Update expected REST responses to no longer include empty spec/status. |
Reviewer Checklist🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
|
Signed-off-by: Bart Smykla <bartek@smykla.com>
Signed-off-by: Bart Smykla <bartek@smykla.com>
Signed-off-by: Bart Smykla <bartek@smykla.com>
Add optional_spec marker and descriptor field so only
MOTB omits empty spec. Other resources keep spec: {}
as required by their OpenAPI schemas. Status omission
stays global for all resources.
Signed-off-by: Bart Smykla <bartek@smykla.com>
|
Is there a strong reason to do this? |
|
At my brief read of the description, yes it sounds like I would prefer a good default, and in this case always have a I guess I have the same question as @lahabana:
Generally good defaults is fairly easy with simple values like strings, booleans etc. Harder with objects, I'd usually prefer empty objects rather than non-existent field, unless there is good reason of course. |
Motivation
The REST API returns
spec: {}andstatus: {}for resources with empty spec or status. Found during MeshOpenTelemetryBackend testing - MOTB with no spec returnedspec: {}, and MeshMetric on global CP showedstatus: {}.Implementation information
v1alpha1.Resource.MarshalJSON()callscore_model.ToJSON()on non-nil spec/status (every resource'sGetSpec()returns&EmptyStruct{}, never nil). Empty structs marshal to{}, andjson.RawMessage("{}")has length 2 -omitemptyonly omits nil/zero-length slices, so the field is always included.After marshaling spec/status to JSON bytes, check if the result is
"{}"and skip assignment. This leavesjson.RawMessageas nil soomitemptyomits the field. Same pattern already used byunversioned.Resource.MarshalJSON().Round-trip safety verified: every generated
SetSpec(nil)initializes to&EmptyStruct{}, so missingspecandspec: {}converge to the same state after deserialization.Supporting documentation
Found during MeshOpenTelemetryBackend manual testing.