GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add tests for `nullable` type by sonasingh46 · Pull Request #26 · oapi-codegen/runtime

Conversation

@sonasingh46

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

Uh oh!

There was an error while loading. Please reload this page.

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 [email protected]
Co-authored-by: Ashutosh Kumar [email protected]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kdisneur reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@sonasingh46
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
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
jamietanna
jamietanna reviewed Dec 22, 2023
View reviewed changes
Comment thread types/nullable.go Resolved

Uh oh!

There was an error while loading. Please reload this page.
@sonasingh46
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
jamietanna
jamietanna reviewed Jan 2, 2024
View reviewed changes
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

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 2, 2024
View reviewed changes
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

There was a problem hiding this comment.

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())

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 2, 2024
View reviewed changes
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

There was a problem hiding this comment.

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) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 2, 2024
View reviewed changes
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

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 2, 2024
View reviewed changes
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

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sonasingh46 sonasingh46 Jan 3, 2024

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 2, 2024
View reviewed changes
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

There was a problem hiding this comment.

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()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 2, 2024
View reviewed changes
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

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 2, 2024
View reviewed changes
Comment thread types/nullable.go Resolved

Uh oh!

There was an error while loading. Please reload this page.
jamietanna
jamietanna reviewed Jan 2, 2024
View reviewed changes
Comment thread types/nullable_test.go Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jamietanna

jamietanna commented Jan 3, 2024

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jamietanna jamietanna mentioned this pull request Jan 3, 2024
Differentiate between JSON 'key = null' and 'key not set' guregu/null#39
Open
@sonasingh46

sonasingh46 commented Jan 3, 2024

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jamietanna
jamietanna reviewed Jan 8, 2024
View reviewed changes
Comment thread types/nullable.go Resolved

Uh oh!

There was an error while loading. Please reload this page.
jamietanna
jamietanna reviewed Jan 8, 2024
View reviewed changes
Comment thread types/nullable.go Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jamietanna
jamietanna force-pushed the nullable_type branch from bfc1c43 to cbea05b Compare January 9, 2024 11:07
@jamietanna jamietanna changed the title add nullable type Add tests for nullable type Jan 9, 2024
Jamie Tanna Sebastien Guilloux Ashutosh Kumar
Add tests for nullable type
f26cbc7 
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]>
@jamietanna
jamietanna force-pushed the nullable_type branch from cbea05b to f26cbc7 Compare January 9, 2024 11:09
@sonasingh46
refactor unit tests
09846bc 
Signed-off-by: Ashutosh Kumar <[email protected]>
jamietanna
jamietanna reviewed Jan 9, 2024
View reviewed changes
Comment thread types/nullable_test.go Outdated
}
}

// Idempotency tests for nullable and optional

@jamietanna jamietanna Jan 9, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sonasingh46 sonasingh46 Jan 9, 2024

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 9, 2024
View reviewed changes
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

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 9, 2024
View reviewed changes
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

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 9, 2024
View reviewed changes
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

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 9, 2024
View reviewed changes
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

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jamietanna
jamietanna reviewed Jan 9, 2024
View reviewed changes
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

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sonasingh46
add unit test for complex struct
e6e916a 
Signed-off-by: Ashutosh Kumar <[email protected]>
@sonasingh46
sonasingh46 force-pushed the nullable_type branch from ab5218f to e6e916a Compare January 9, 2024 17:41
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.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@sonasingh46 @jamietanna
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.