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.

Sign up for GitHub

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: Make all array kinds nillable #2534

Merged
merged 5 commits into from
Apr 20, 2024
Merged

fix: Make all array kinds nillable #2534

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a71b16
make all array kinds nillable
nasdf Apr 19, 2024
2690568
simplify ScalarArrayKind IsNillable
nasdf Apr 19, 2024
d1737e0
add TestArrayValue_IsNillable
nasdf Apr 19, 2024
8a83889
use correct normal values for normal nil arrays
nasdf Apr 19, 2024
d9953a9
Merge branch 'develop' into nasdf/fix/array-kind-nillable
nasdf Apr 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions client/normal_nil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@ func NewNormalNil(kind FieldKind) (NormalValue, error) {
return NewNormalNillableString(immutable.None[string]()), nil
case FieldKind_NILLABLE_BLOB:
return NewNormalNillableBytes(immutable.None[[]byte]()), nil
case FieldKind_NILLABLE_BOOL_ARRAY:
case FieldKind_BOOL_ARRAY, FieldKind_NILLABLE_BOOL_ARRAY:
Copy link
Collaborator

@fredcarle fredcarle Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

todo: This should be changed to

case FieldKind_BOOL_ARRAY:
    return NewNormalIntNillableArray(immutable.None[[]bool]()), nil
case FieldKind_NILLABLE_BOOL_ARRAY:
    return NewNormalNillableBoolNillableArray(immutable.None[[]immutable.Option[bool]]()), nil

and similarly for the other array types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice spot :)

return NewNormalBoolNillableArray(immutable.None[[]bool]()), nil
case FieldKind_NILLABLE_INT_ARRAY:
case FieldKind_INT_ARRAY, FieldKind_NILLABLE_INT_ARRAY:
return NewNormalIntNillableArray(immutable.None[[]int64]()), nil
case FieldKind_NILLABLE_FLOAT_ARRAY:
case FieldKind_FLOAT_ARRAY, FieldKind_NILLABLE_FLOAT_ARRAY:
return NewNormalFloatNillableArray(immutable.None[[]float64]()), nil
case FieldKind_NILLABLE_STRING_ARRAY:
case FieldKind_STRING_ARRAY, FieldKind_NILLABLE_STRING_ARRAY:
return NewNormalStringNillableArray(immutable.None[[]string]()), nil
default:
return nil, NewCanNotMakeNormalNilFromFieldKind(kind)
Expand Down
6 changes: 5 additions & 1 deletion client/schema_field_description.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,11 @@ func (k ScalarArrayKind) Underlying() string {
}

func (k ScalarArrayKind) IsNillable() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: Please add some integration tests that fail before the production code is fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm actually not sure how the tests did not fail before. I'm still investigating, but I believe its because of encoding / decoding of []int{} vs nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a unit test, but I don't think an integration test can actually catch this problem due to the encoding I mentioned above.

Copy link
Contributor

@AndrewSisley AndrewSisley Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe its because of encoding / decoding of []int{} vs nil.

I think that might be another bug, client.validateFieldSchema does not appear to reject nil values for non-nillable fields. I'll create a new ticket, is kind of out of scope here if that is what was hiding this PR's bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

#2539

return k == FieldKind_NILLABLE_BOOL_ARRAY ||
return k == FieldKind_BOOL_ARRAY ||
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: This could be simplified to return true.

k == FieldKind_INT_ARRAY ||
k == FieldKind_FLOAT_ARRAY ||
k == FieldKind_STRING_ARRAY ||
k == FieldKind_NILLABLE_BOOL_ARRAY ||
k == FieldKind_NILLABLE_INT_ARRAY ||
k == FieldKind_NILLABLE_FLOAT_ARRAY ||
k == FieldKind_NILLABLE_STRING_ARRAY
Expand Down
Loading