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

Resolve panic reflect.Value.Interface on zero value #396

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

davidhankins
Copy link
Contributor

PROBLEM

In HTTP API services, a user may specify a _filter= specifying a field name and an empty string (field == "").

The ToORM() codec translates the empty string to a nil pointer.

ProcessStringCondition() then attempts to take a reflect.Value.Interface on this nil pointer, causing a panic.

SOLUTION

Detect the nil pointer value, and return a nil value. The gorm codec translates this to a NULL value in SQL syntax. This resolves the panic, but results in an SQL query that matches no rows (field == NULL).

DISCUSSION

It's arguable what the user intent is here, considering that there is existing Atlas filter syntax for field == null (resulting in the correct identity SQL expression field IS NULL). The explicit empty string value cannot exist in our interpretation because the codec translates it to NULL reliably; a user that POSTs an object with a string field set "" will result in a table row set to NULL.

But we might want to consider that field == "" should be an alias for the NULL identity expression; the user intent is probably to find rows where the field is "" in their experience of the object.

From one point of view, it is correct that there are never any rows in our table with fields of "" value. From another point of view, there are rows in our table with fields not equal to "" value and that query should have results (the field != "" syntax seems like it should work).

PROBLEM
=======

In HTTP API services, a user may specify a `_filter=` specifying a
field name and an empty string (`field == ""`).

The `ToORM()` codec translates the empty string to a `nil` pointer.

`ProcessStringCondition()` then attempts to take a
`reflect.Value.Interface` on this nil pointer, causing a panic.

SOLUTION
========

Detect the nil pointer value, and return a nil value. The gorm codec
translates this to a `NULL` value in SQL syntax. This resolves the
panic, but results in an SQL query that matches no rows
(`field == NULL`).

DISCUSSION
==========

It's arguable what the user intent is here, considering that there is
existing Atlas filter syntax for `field == null` (resulting in the
correct identity SQL expression `field IS NULL`). The explicit empty
string value cannot exist in our interpretation because the codec
translates it to NULL reliably; a user that POSTs an object with a
string field set `""` will result in a table row set to NULL.

But we might want to consider that `field == ""` should be an alias for
the NULL identity expression; the user intent is probably to find rows
where the field is `""` in their experience of the object.

From one point of view, it is correct that there are never any rows in
our table with fields of `""` value. From another point of view, there
are rows in our table with fields not equal to `""` value and that query
should have results (the `field != ""` syntax seems like it should work).
@daniel-garcia daniel-garcia merged commit 7670b4b into infobloxopen:master Nov 14, 2023
2 checks passed
davidhankins added a commit to davidhankins/atlas-app-toolkit that referenced this pull request Dec 21, 2023
PROBLEM
=======

In HTTP API services, a user may specify a `_filter=` specifying a
field name and an empty string (`field == ""`).

The `ToORM()` codec translates the empty string to a `nil` pointer.

`ProcessStringCondition()` then attempts to take a
`reflect.Value.Interface` on this nil pointer, causing a panic.

SOLUTION
========

Detect the nil pointer value, and return a nil value. The gorm codec
translates this to a `NULL` value in SQL syntax. This resolves the
panic, but results in an SQL query that matches no rows
(`field == NULL`).

DISCUSSION
==========

It's arguable what the user intent is here, considering that there is
existing Atlas filter syntax for `field == null` (resulting in the
correct identity SQL expression `field IS NULL`). The explicit empty
string value cannot exist in our interpretation because the codec
translates it to NULL reliably; a user that POSTs an object with a
string field set `""` will result in a table row set to NULL.

But we might want to consider that `field == ""` should be an alias for
the NULL identity expression; the user intent is probably to find rows
where the field is `""` in their experience of the object.

From one point of view, it is correct that there are never any rows in
our table with fields of `""` value. From another point of view, there
are rows in our table with fields not equal to `""` value and that query
should have results (the `field != ""` syntax seems like it should work).
daniel-garcia pushed a commit that referenced this pull request Jan 9, 2024
PROBLEM
=======

In HTTP API services, a user may specify a `_filter=` specifying a
field name and an empty string (`field == ""`).

The `ToORM()` codec translates the empty string to a `nil` pointer.

`ProcessStringCondition()` then attempts to take a
`reflect.Value.Interface` on this nil pointer, causing a panic.

SOLUTION
========

Detect the nil pointer value, and return a nil value. The gorm codec
translates this to a `NULL` value in SQL syntax. This resolves the
panic, but results in an SQL query that matches no rows
(`field == NULL`).

DISCUSSION
==========

It's arguable what the user intent is here, considering that there is
existing Atlas filter syntax for `field == null` (resulting in the
correct identity SQL expression `field IS NULL`). The explicit empty
string value cannot exist in our interpretation because the codec
translates it to NULL reliably; a user that POSTs an object with a
string field set `""` will result in a table row set to NULL.

But we might want to consider that `field == ""` should be an alias for
the NULL identity expression; the user intent is probably to find rows
where the field is `""` in their experience of the object.

From one point of view, it is correct that there are never any rows in
our table with fields of `""` value. From another point of view, there
are rows in our table with fields not equal to `""` value and that query
should have results (the `field != ""` syntax seems like it should work).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants