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

Fix handling string containing object for NewJSONInput and use table tests for JSON #365

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

taimoorgit
Copy link
Contributor

@taimoorgit taimoorgit commented Feb 5, 2024

Issues

Part of: OpsLevel/kubectl-opslevel#244 / https://github.com/OpsLevel/product/issues/8

Changelog

  • NewJSONInput will stop adding extra backslashes to unnecessarily escape "{\"foo\": \"bar\"}". This is the only functional change.
  • Use table tests for all constructors (NewJSONInput, NewJSON, NewJSONSchema)
  • Add documentation for the 3 constructors and types
  • Make a changie entry

Before/after

Before I had to make this change in k8s sync in order for syncing object/array properties that are defined in .metadata.annotations (handled as a string):

		// TODO: hack to get around NewJSONInput not reading strings properly.
		var value opslevel.JsonString
		if strings.HasPrefix(val, "{") && strings.HasSuffix(val, "}") {
			value = opslevel.JsonString(val)
		} else {
			valuePtr, err := opslevel.NewJSONInput(val)
			if err != nil {
				log.Error().Err(err).Msgf("[%s] Failed parsing property: '%s'", service.Name, def)
				continue
			}
			value = *valuePtr
		}

This if clause is no longer necessary.

Sorry, something went wrong.

@taimoorgit taimoorgit self-assigned this Feb 5, 2024
@taimoorgit taimoorgit added the bug Something isn't working label Feb 5, 2024
@taimoorgit taimoorgit force-pushed the ta/jsonstring-obj-array branch from 95423ed to d08c9fc Compare February 5, 2024 17:13
@taimoorgit taimoorgit changed the title JsonString - fix constructor case on objects/arrays wrapped in a string Fix handling string containing object for NewJSONInput and use table tests for JSON Feb 5, 2024
res, err := ol.NewJSONSchema(validStringContainingJSON)
resVal := *res
autopilot.Ok(t, err)
autopilot.Equals(t, resVal["name"], "Thomas")
Copy link
Contributor Author

@taimoorgit taimoorgit Feb 5, 2024

Choose a reason for hiding this comment

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

Removed because all these equals checks are done by reflect.DeepEqual anyways, which is what autopilot.Equals is doing.

@@ -10,7 +10,34 @@ import (
"github.com/rocktavious/autopilot/v2023"
)

var validStringContainingJSON = `{"name":"Thomas","isIntern":false,"age":45,"access":{"aws":"admin","okta":"admin"},"tags":["org:engineering","team:platform"]}`
Copy link
Contributor Author

@taimoorgit taimoorgit Feb 5, 2024

Choose a reason for hiding this comment

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

Removed the "age":45, here because it is annoying to have reflect.DeepEqual fail to compare a int to a float64. This is handled by JSON marshal, we don't get any benefits from testing this on our own. We just need to make sure that keys and values are not being discarded.

@taimoorgit taimoorgit force-pushed the ta/jsonstring-obj-array branch 2 times, most recently from cb5ef18 to b5326b5 Compare February 5, 2024 18:31
@taimoorgit taimoorgit force-pushed the ta/jsonstring-obj-array branch from b5326b5 to 24eaa55 Compare February 5, 2024 18:34
Copy link
Contributor

@davidbloss davidbloss left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@taimoorgit taimoorgit merged commit bc804ea into main Feb 5, 2024
4 checks passed
@taimoorgit taimoorgit deleted the ta/jsonstring-obj-array branch February 5, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants