Add tests for `nullable` type #26

sonasingh46 · 2023-12-20T17:28:44Z

We have spun out a separate package, oapi-codegen/nullable as a step
towards oapi-codegen/oapi-codegen#1039.

Until we have implemented #27, we cannot add an explicit type alias in
this package, so we can at least add some tests to cover additional
functionality and expectations that the package should have when
interplaying with oapi-codegen.

Co-authored-by: Sebastien Guilloux [email protected]
Co-authored-by: Ashutosh Kumar [email protected]

types/nullable.go

jamietanna · 2024-01-02T16:50:05Z

types/nullable.go

+// MarshalJSON implements the  Marshaler interface.
+func (t Nullable[T]) MarshalJSON() ([]byte, error) {
+	if t.IsNull() {
+		return nullBytes, nil


We need to replace this with:

Suggested change

return nullBytes, nil

return []byte("null"), nil

Right now, we've got the risk that someone could do something like:

func TestF(t *testing.T) { var n Nullable[string] n.Null = true b, err := n.MarshalJSON() fmt.Printf("b: %v\n", b) fmt.Printf("b: %s\n", b) fmt.Printf("err: %v\n", err) b[1] = '_' b_, err := n.MarshalJSON() fmt.Printf("b: %v\n", b_) fmt.Printf("b: %s\n", b_) fmt.Printf("err: %v\n", err) }

Which outputs:

b: [110 117 108 108] b: null err: <nil> b: [110 95 108 108] b: n_ll err: <nil>

Note that nullBytes from the package is changed.

We could alternatively perform a protective copy, or in this case, just return a literal

Did not grok this in the first read. Trying to understand it.

Ah! Makes sense now.. So basically some one can alter the nullByte. Good catch.

jamietanna · 2024-01-02T17:05:49Z

types/nullable_test.go

+			assert.NoError(t, err)
+			assert.Equalf(t, tt.wantNull, obj.Name.IsNull(), "IsNull()")
+			assert.Equalf(t, tt.wantSet, obj.Name.IsSet(), "IsSet()")
+			fmt.Println(obj.Name.Get())


Suggested change

fmt.Println(obj.Name.Get())

jamietanna · 2024-01-02T17:09:51Z

types/nullable.go

+	return t.Set
+}
+
+func (t *Nullable[T]) Get() (value T, null bool) {


Think this should be:

Suggested change

func (t *Nullable[T]) Get() (value T, null bool) {

func (t *Nullable[T]) Get() (value *T, set bool) {

On similar line -- now I am thinking we do not need this Get() method too.. Especially when I used in my own implementation -- I did not feel them need to use it especially when it returns 2 values, the code becomes little clumsy.

I think we should add these util functions as and when we need them. wdyt?

jamietanna · 2024-01-02T17:10:04Z

types/nullable.go

+// IsNull returns true if the value is explicitly provided `null` in json
+func (t *Nullable[T]) IsNull() bool {
+	return t.Null
+}
+
+// IsSet returns true if the value is provided in json
+func (t *Nullable[T]) IsSet() bool {
+	return t.Set
+}


Unsure if these are needed if we export the values from the struct?

Hmm.. Agreed.

Not exporting the Set and Null keys is not a good idea as to create a Nullable type it is just easy to have them.

Now, since we have the fields exported the methods really does not do any useful stuff.

I was thinking on that and figured we should have something like the following:

// HasValue returns true if the value is provided in json func (t *Nullable[T]) HasValue() bool { return t.Set && !t.Null }

HasValue() is a good util to have as I felt the need to that while using this type to answer this question:

Is the value provided in JSON as well as is not null.

If this util is not present then one will have to do:

if t.Set && !t.Null {// do something}

vs

if t.HasValue() {// do something}

jamietanna · 2024-01-02T17:12:00Z

types/nullable_test.go

+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t1 *testing.T) {


Suggested change

t.Run(tt.name, func(t1 *testing.T) {

t.Run(tt.name, func(t *testing.T) {

Otherwise we're not passing in the correct type.

Shadowing here is fine (in my opinion - happy to chat if there are things to be aware of)

typo.. thanks.
Will fix that.

jamietanna · 2024-01-02T17:12:15Z

types/nullable_test.go

+			assert: func(obj TestComplex, t *testing.T) {
+				assert.Equalf(t, true, obj.StringList.IsSet(), "string_list should be set")
+				assert.Equalf(t, false, obj.StringList.IsNull(), "string_list should not be null")
+				gotStringList, isNull := obj.StringList.Get()


Mind adding newlines for readability?

Suggested change

gotStringList, isNull := obj.StringList.Get()

gotStringList, isNull := obj.StringList.Get()

jamietanna · 2024-01-02T17:12:38Z

types/nullable_test.go

+		t.Run(tt.name, func(t1 *testing.T) {
+			var obj SimplePointerInt
+			err := json.Unmarshal(tt.jsonInput, &obj)
+			assert.NoError(t, err)


May be worth replacing this and similar ones with:

Suggested change

assert.NoError(t, err)

require.NoError(t, err)

So we're failing earlier

types/nullable.go

types/nullable_test.go

jamietanna · 2024-01-03T08:38:10Z

Was the original implementation of the code adapted from https://github.com/guregu/null ? It looks like it but we've not declared so, as the licence would expect?

sonasingh46 · 2024-01-03T09:07:11Z

Was the original implementation of the code adapted from https://github.com/guregu/null ? It looks like it but we've not declared so, as the licence would expect?

Not really but definitely adapted the nullByte stuff from here. https://github.com/guregu/null/blob/master/string.go#L15
Also I looked at a lot of libs around doing the Nullable distinction and the basic way is kind of same.

So if we need to declare it or do anything needful, sure! Thanks for bringing this.

types/nullable.go

We have spun out a separate package, `oapi-codegen/nullable` as a step towards oapi-codegen/oapi-codegen#1039. Until we have implemented oapi-codegen#27, we cannot add an explicit type alias in this package, so we can at least add some tests to cover additional functionality and expectations that the package should have when interplaying with `oapi-codegen`. Co-authored-by: Sebastien Guilloux <[email protected]> Co-authored-by: Ashutosh Kumar <[email protected]>

Signed-off-by: Ashutosh Kumar <[email protected]>

jamietanna · 2024-01-09T15:40:32Z

types/nullable_test.go

 		})
 	}
 }
+
+// Idempotency tests for nullable and optional


Are these actually Idempotency tests? Or can we find a better name?

I think the word idempotency can be simple removed. I will do that.

jamietanna · 2024-01-09T15:40:46Z

types/nullable_test.go

+
+	var obj2 StringNullableOptional
+	originalJSON2 := []byte(`{"id":"12esd412"}`)
+	err2 := json.Unmarshal(originalJSON2, &obj2)


Why not re-use err?

jamietanna · 2024-01-09T15:41:11Z

types/nullable_test.go

 			},
 		},

 		{
 			name: "when explicitly set to a specific value",
 			json: []byte(`{"replica_count":5}`),
-			assert: func(t *testing.T, obj SimpleIntNullableRequired) {
+			assert: func(obj SimpleIntNullableRequired, t *testing.T) {


🛑 please revert this back, the ordering was changed on purpose

jamietanna · 2024-01-09T15:41:24Z

types/nullable_test.go

 				t.Helper()

-				assert.Truef(t, obj.ReplicaCount.IsNull(), "replica count should be null")
+				assert.Equalf(t, true, obj.ReplicaCount.IsSpecified(), "replica count should be set")


Please revert this, we're using Truef on purpose

jamietanna · 2024-01-09T15:42:20Z

types/nullable_test.go

@@ -437,8 +466,61 @@ func TestNullableRequired_UnmarshalJSON(t *testing.T) {
 			var obj SimpleIntNullableRequired
 			err := json.Unmarshal(tt.json, &obj)
 			require.NoError(t, err)
-
-			tt.assert(t, obj)
+			tt.assert(obj, t)


newline was added on purpose

jamietanna · 2024-01-09T15:43:06Z

types/nullable_test.go

+	type testCase struct {
+		name          string
+		jsonInput     []byte
+		assert        func(obj ComplexNullable, t *testing.T)


Suggested change

assert func(obj ComplexNullable, t *testing.T)

assert func(t *testing.T, obj ComplexNullable)

Signed-off-by: Ashutosh Kumar <[email protected]>

sonasingh46 requested a review from a team as a code owner December 20, 2023 17:28

sonasingh46 changed the title ~~(poc)add nullable type~~ (wip)add nullable type Dec 20, 2023

sonasingh46 marked this pull request as draft December 20, 2023 17:31

sonasingh46 changed the title ~~(wip)add nullable type~~ (wip)add optional type Dec 22, 2023

jamietanna reviewed Dec 22, 2023

View reviewed changes

types/nullable.go Outdated Show resolved Hide resolved

sonasingh46 marked this pull request as ready for review December 22, 2023 13:42

sonasingh46 changed the title ~~(wip)add optional type~~ add optional type Dec 22, 2023

sonasingh46 changed the title ~~add optional type~~ add nullable type Dec 27, 2023

jamietanna reviewed Jan 2, 2024

View reviewed changes

types/nullable.go Outdated Show resolved Hide resolved

jamietanna reviewed Jan 2, 2024

View reviewed changes

types/nullable_test.go Show resolved Hide resolved

jamietanna mentioned this pull request Jan 3, 2024

Differentiate between JSON 'key = null' and 'key not set' guregu/null#39

Open

jamietanna reviewed Jan 8, 2024

View reviewed changes

types/nullable.go Outdated Show resolved Hide resolved

jamietanna reviewed Jan 8, 2024

View reviewed changes

types/nullable.go Outdated Show resolved Hide resolved

jamietanna force-pushed the nullable_type branch from bfc1c43 to cbea05b Compare January 9, 2024 11:07

jamietanna changed the title ~~add nullable type~~ Add tests for nullable type Jan 9, 2024

jamietanna force-pushed the nullable_type branch from cbea05b to f26cbc7 Compare January 9, 2024 11:09

refactor unit tests

09846bc

Signed-off-by: Ashutosh Kumar <[email protected]>

jamietanna reviewed Jan 9, 2024

View reviewed changes

add unit test for complex struct

e6e916a

Signed-off-by: Ashutosh Kumar <[email protected]>

sonasingh46 force-pushed the nullable_type branch from ab5218f to e6e916a Compare January 9, 2024 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for `nullable` type #26

Add tests for `nullable` type #26

sonasingh46 commented Dec 20, 2023 •

edited by jamietanna

Loading

jamietanna Jan 2, 2024

sonasingh46 Jan 3, 2024

sonasingh46 Jan 3, 2024

jamietanna Jan 2, 2024

jamietanna Jan 2, 2024

sonasingh46 Jan 3, 2024

jamietanna Jan 2, 2024

sonasingh46 Jan 3, 2024

jamietanna Jan 2, 2024

sonasingh46 Jan 3, 2024

jamietanna Jan 2, 2024

jamietanna Jan 2, 2024

jamietanna commented Jan 3, 2024

sonasingh46 commented Jan 3, 2024

jamietanna Jan 9, 2024

sonasingh46 Jan 9, 2024

jamietanna Jan 9, 2024

jamietanna Jan 9, 2024

jamietanna Jan 9, 2024

jamietanna Jan 9, 2024

jamietanna Jan 9, 2024

	func (t *Nullable[T]) Get() (value T, null bool) {
	func (t Nullable[T]) Get() (value T, set bool) {

	t.Run(tt.name, func(t1 *testing.T) {
	t.Run(tt.name, func(t *testing.T) {

	gotStringList, isNull := obj.StringList.Get()

	gotStringList, isNull := obj.StringList.Get()

	assert func(obj ComplexNullable, t *testing.T)
	assert func(t *testing.T, obj ComplexNullable)

Add tests for nullable type #26

Are you sure you want to change the base?

Add tests for nullable type #26

Conversation

sonasingh46 commented Dec 20, 2023 • edited by jamietanna Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamietanna commented Jan 3, 2024

sonasingh46 commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add tests for `nullable` type #26

Add tests for `nullable` type #26

sonasingh46 commented Dec 20, 2023 •

edited by jamietanna

Loading