-
Notifications
You must be signed in to change notification settings - Fork 117
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
daniel-garcia
merged 1 commit into
infobloxopen:master
from
davidhankins:handle-nil-filter
Nov 14, 2023
Merged
Resolve panic reflect.Value.Interface on zero value
#396
daniel-garcia
merged 1 commit into
infobloxopen:master
from
davidhankins:handle-nil-filter
Nov 14, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
approved these changes
Nov 14, 2023
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
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.
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 anil
pointer.ProcessStringCondition()
then attempts to take areflect.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 expressionfield 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 (thefield != ""
syntax seems like it should work).