Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optjson.go better handling of zero value #24695

Open
ksykulev opened this issue Dec 12, 2024 · 2 comments
Open

optjson.go better handling of zero value #24695

ksykulev opened this issue Dec 12, 2024 · 2 comments
Labels
~backend Backend-related issue. bug Something isn't working as documented ~engineering-initiated Engineering-initiated story, such as a bug, refactor, or contributor experience improvement. #g-mdm MDM product group :incoming New issue in triage process. :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. ~released bug This bug was found in a stable release.

Comments

@ksykulev
Copy link
Contributor

ksykulev commented Dec 12, 2024

💥  Actual behavior

Given a struct such as

type testStruct struct {
  SomeID optjson.Any[uint] `json:"some_id"`
}

If you were to initialize this struct without specifying SomeID

ts := testStruct{}

Marshaling this object will product

{ "some_id": null }

🧑‍💻  Steps to reproduce

Discovered during #24658
You can reproduce by creating a patch request to the policies endpoint with a struct that does not define ScriptID or SoftwareTitleID

modifyGlobalPolicyRequest{
		ModifyPolicyPayload: fleet.ModifyPolicyPayload{
			Name:        ptr.String("TestQuery4"),
			Query:       ptr.String("select * from osquery_info;"),
			Description: ptr.String("Some description updated"),
			Resolution:  ptr.String("some global resolution updated"),
		},
	}

Notice how the json that is generated for this struct looks like:

{
		"PolicyID": 0,
		"name": "TestQuery4",
		"query": "select * from osquery_info;",
		"description": "Some description updated",
		"resolution": "some global resolution updated",
		"platform": null,
		"critical": null,
		"calendar_events_enabled": null,
                "script_id": null,
                "software_title_id": null
	}

🕯️ More info (optional)

https://victoronsoftware.com/posts/go-json-unmarshal/

🛠️ To fix

Is there a way to change optjson.go to not output the key if the Set is equal to false. Can we use the omitempty tag to get the intended behavior.

SomeID optjson.Any[uint] `json:"some_id,omitempty"`
@ksykulev ksykulev added #g-endpoint-ops Endpoint ops product group :incoming New issue in triage process. :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. bug Something isn't working as documented ~backend Backend-related issue. ~engineering-initiated Engineering-initiated story, such as a bug, refactor, or contributor experience improvement. ~released bug This bug was found in a stable release. labels Dec 12, 2024
@mna
Copy link
Member

mna commented Dec 16, 2024

@ksykulev The problem is Go's support for omitempty is limited to non-struct, so when you have a struct type that implements a custom marshaler (MarshalJSON), as is the case for the optjson types, MarshalJSON will be called, and once it's called you have to return some valid JSON representation (hence, null). You cannot make Go skip the current field at this point, the key is already written out and it calls the type only to get the value.

The good thing is that this was raised a while ago as a bug on Go's tracker: golang/go#10648, which then created a proposal to add an omitzero tag option that would skip calling the marshaler if the struct was the zero-value: golang/go#45669 and it was accepted just recently (!) so it will be in Go's next version (1.24, as noted in the changelog). golang/go#45669 (comment)

@sharon-fdm sharon-fdm added #g-mdm MDM product group and removed #g-endpoint-ops Endpoint ops product group labels Dec 18, 2024
@sharon-fdm
Copy link
Collaborator

Probably MDM has better context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
~backend Backend-related issue. bug Something isn't working as documented ~engineering-initiated Engineering-initiated story, such as a bug, refactor, or contributor experience improvement. #g-mdm MDM product group :incoming New issue in triage process. :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. ~released bug This bug was found in a stable release.
Development

No branches or pull requests

3 participants