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

Use json.RawMessage type to avoid unnecessary json.Unmarshal #255

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

rtorrero
Copy link
Contributor

This PR changes the SaptuneDiscoveryPayload struct to avoid the usage of interface{} type and json.Unmarshal. Instead we use a json.RawMessage that prevents the payload from being quoted.

@rtorrero rtorrero marked this pull request as ready for review September 19, 2023 11:01
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @rtorrero ,
What if the output of the command for some error is not json?
The Unmarshal would catch that

@rtorrero
Copy link
Contributor Author

Hey @rtorrero , What if the output of the command for some error is not json? The Unmarshal would catch that

It shouldn't matter, since we aren't doing anything with the JSON itself (It should fail once we try to unmarshal or decode it). If I understood it correctly, for us json.RawMessage is just acting as an alias of []byte.

@arbulu89
Copy link
Contributor

Hey @rtorrero , What if the output of the command for some error is not json? The Unmarshal would catch that

It shouldn't matter, since we aren't doing anything with the JSON itself (It should fail once we try to unmarshal or decode it). If I understood it correctly, for us json.RawMessage is just acting as an alias of []byte.

Well, you check at least that it is a json format.
What happens if for some reason we get a non json output from the command?
Would we send that plain text in the payload field?
Because the collector will marshall this as json.
I just want to be sure that we don't panic in that scenario

@rtorrero
Copy link
Contributor Author

Hey @rtorrero , What if the output of the command for some error is not json? The Unmarshal would catch that

It shouldn't matter, since we aren't doing anything with the JSON itself (It should fail once we try to unmarshal or decode it). If I understood it correctly, for us json.RawMessage is just acting as an alias of []byte.

Well, you check at least that it is a json format. What happens if for some reason we get a non json output from the command? Would we send that plain text in the payload field? Because the collector will marshall this as json. I just want to be sure that we don't panic in that scenario

This would be caught by

func (c *Collector) Publish(discoveryType string, payload interface{}) error {
	log.Debugf("Sending %s to data collector", discoveryType)

	requestBody, err := json.Marshal(map[string]interface{}{
		"agent_id":       c.config.AgentID,
		"discovery_type": discoveryType,
		"payload":        payload,
	})
	if err != nil {
		return err
	}
	...

in our collector.

We could catch it here if you prefer, but IMO that defeats a bit the purpose of this PR

@rtorrero rtorrero merged commit deae22f into main Oct 3, 2023
10 checks passed
@rtorrero rtorrero deleted the use-json-rawmessage-type branch October 3, 2023 11:32
@CDimonaco CDimonaco added the enhancement New feature or request label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants