Skip to content

Navigation Menu

Sign in
Sign up

Add tests for nullable type - #26

Open
sonasingh46 wants to merge 3 commits into
oapi-codegen:main from
sonasingh46:nullable_type
Open

Add tests for nullable type #26
sonasingh46 wants to merge 3 commits into
oapi-codegen:main from
sonasingh46:nullable_type

Conversation

@sonasingh46

@sonasingh46 sonasingh46 commented Dec 20, 2023 •
edited by jamietanna
Loading

Copy link
Copy Markdown

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 sebastien.guilloux@elastic.co
Co-authored-by: Ashutosh Kumar ashutosh.kumar@elastic.co

kdisneur reacted with thumbs up emoji
sonasingh46 requested a review from a team as a code owner December 20, 2023 17:28
@sonasingh46 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 sonasingh46 changed the title (削除) (wip)add nullable type (削除ここまで) (追記) (wip)add optional type (追記ここまで) Dec 22, 2023
Comment thread types/nullable.go Outdated
sonasingh46 marked this pull request as ready for review December 22, 2023 13:42
@sonasingh46 sonasingh46 changed the title (削除) (wip)add optional type (削除ここまで) (追記) add optional type (追記ここまで) Dec 22, 2023
@sonasingh46 sonasingh46 changed the title (削除) add optional type (削除ここまで) (追記) add nullable type (追記ここまで) Dec 27, 2023
Comment thread types/nullable.go Outdated
// MarshalJSON implements the Marshaler interface.
func (t Nullable[T]) MarshalJSON() ([]byte, error) {
if t.IsNull() {
return nullBytes, nil

@jamietanna jamietanna Jan 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Comment thread types/nullable_test.go Outdated
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())

@jamietanna jamietanna Jan 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread types/nullable.go Outdated
return t.Set
}

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

@jamietanna jamietanna Jan 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Think this should be:

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

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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?

Comment thread types/nullable.go Outdated
Comment on lines +42 to +50
// 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
}

@jamietanna jamietanna Jan 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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}

Comment thread types/nullable_test.go Outdated
},
}
for _, tt := range tests {
t.Run(tt.name, func(t1 *testing.T) {

@jamietanna jamietanna Jan 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

typo.. thanks.
Will fix that.

Comment thread types/nullable_test.go Outdated
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()

@jamietanna jamietanna Jan 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mind adding newlines for readability?

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

Comment thread types/nullable_test.go Outdated
t.Run(tt.name, func(t1 *testing.T) {
var obj SimplePointerInt
err := json.Unmarshal(tt.jsonInput, &obj)
assert.NoError(t, err)

@jamietanna jamietanna Jan 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May be worth replacing this and similar ones with:

Suggested change
assert.NoError(t, err)
require.NoError(t, err)

So we're failing earlier

Comment thread types/nullable.go Outdated
Comment thread types/nullable_test.go

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Author

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.

Comment thread types/nullable.go Outdated
Comment thread types/nullable.go Outdated
@jamietanna jamietanna changed the title (削除) add nullable type (削除ここまで) (追記) Add tests for nullable type (追記ここまで) Jan 9, 2024
Jamie Tanna Sebastien Guilloux Ashutosh Kumar 
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 <sebastien.guilloux@elastic.co>
Co-authored-by: Ashutosh Kumar <ashutosh.kumar@elastic.co>
Signed-off-by: Ashutosh Kumar <ashutosh.kumar@elastic.co>
Comment thread types/nullable_test.go Outdated
}
}

// Idempotency tests for nullable and optional

@jamietanna jamietanna Jan 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@sonasingh46 sonasingh46 Jan 9, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Comment thread types/nullable_test.go Outdated

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

@jamietanna jamietanna Jan 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not re-use err?

Comment thread types/nullable_test.go Outdated
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) {

@jamietanna jamietanna Jan 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread types/nullable_test.go Outdated
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")

@jamietanna jamietanna Jan 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please revert this, we're using Truef on purpose

Comment thread types/nullable_test.go Outdated
require.NoError(t, err)

tt.assert(t, obj)
tt.assert(obj, t)

@jamietanna jamietanna Jan 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

newline was added on purpose

Comment thread types/nullable_test.go Outdated
type testCase struct {
name string
jsonInput []byte
assert func(obj ComplexNullable, t *testing.T)

@jamietanna jamietanna Jan 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
assert func(objComplexNullable, t *testing.T)
assert func(t *testing.T, objComplexNullable) 

Signed-off-by: Ashutosh Kumar <ashutosh.kumar@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jamietanna jamietanna jamietanna left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /