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 JSON marshalling of an ID to be null (stop using unnecessary pointer) #371

Closed
wants to merge 1 commit into from

Conversation

taimoorgit
Copy link
Contributor

@taimoorgit taimoorgit commented Feb 13, 2024

Issues

Extracted from #356

TL;DR

The test expectation is wrong because we shouldn't be using a pointer receiver.

Explanation

These changes in the test expectation is justified. The test expectations before were incorrect.

type IDTester struct {
	Key1 ol.ID  `json:"key1"`
	Key2 ol.ID  `json:"key2,omitempty"`
	Key3 *ol.ID `json:"key3"`
	Key4 *ol.ID `json:"key4,omitempty"`
}

func TestMarshalID(t *testing.T) {
	// Arrange
	id1 := ol.NewID() <------ we're getting a pointer to a blank ID (string)
	id2 := ol.NewID("Z2lkOi8vMTIzNDU2Nzg5")
	case1 := IDTester{}
	case2 := IDTester{
		Key1: *id1, <------ this is now a concrete empty ID (""). key1 is not omit empty, so it should serialize to null.
		Key2: *id1,
		Key3: id1,
		Key4: id1,
	}
	case3 := IDTester{
		Key1: *id2, <------ empty ID (""). again, key 1 is not omit empty, so it should serialize to null.
		Key2: *id2,
		Key3: id2,
		Key4: id2,
	}
	// Act
	buf1, err1 := json.Marshal(case1)
	buf2, err2 := json.Marshal(case2)
	buf3, err3 := json.Marshal(case3)
	// Assert
	autopilot.Ok(t, err1)
	autopilot.Equals(t, `{"key1":null,"key3":null}`, string(buf1))
	autopilot.Ok(t, err2)
	autopilot.Equals(t, `{"key1":null,"key3":null,"key4":null}`, string(buf2))

Changelog

  • Stop using unnecessary pointer
  • Fix test accordingly
  • Make a changie entry

Sorry, something went wrong.

@taimoorgit taimoorgit added the bug Something isn't working label Feb 13, 2024
@taimoorgit taimoorgit self-assigned this Feb 13, 2024
@@ -40,9 +40,9 @@ func TestMarshalID(t *testing.T) {
buf3, err3 := json.Marshal(case3)
// Assert
autopilot.Ok(t, err1)
autopilot.Equals(t, `{"key1":"","key3":null}`, string(buf1))
autopilot.Equals(t, `{"key1":null,"key3":null}`, string(buf1))
Copy link
Collaborator

@rocktavious rocktavious Feb 14, 2024

Choose a reason for hiding this comment

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

null for IDs means "unset" in our API generally but what does "" mean (for create and update mutations)? I'm concerned we are losing the ability to to create input types where "" is the expected JSON for input variables.

What is the underlying impetus for this change? golang has the ability to put functions on pointers or value types - theres no reason that I see restricting this.

@taimoorgit taimoorgit closed this Feb 22, 2024
@taimoorgit taimoorgit deleted the ta/extracted-id-scalar branch February 22, 2024 00:31
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