Resolve panic reflect.Value.Interface on zero value
#395
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
daniel-garcia
approved these changes
Nov 14, 2023
daniel-garcia
changed the title
reflect.Value.Interface on zero valuereflect.Value.Interface on zero value
davidhankins
force-pushed
the
NORTHSTAR-11355
branch
from
f2d0484 to
27b4dbe
Compare
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).
This was changed in Go 1.18, but it looks like the sources still use the older reference (probably for Go 1.16 compatibility).
davidhankins
force-pushed
the
NORTHSTAR-11355
branch
from
27b4dbe to
5ee54e3
Compare
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
A user may specify a
_filter=specifying a field name and an empty string (field == "").The
ToORM()codec translates the empty string to anilpointer.ProcessStringCondition()then attempts to take areflect.Value.Interfaceon this nil pointer, causing a panic.SOLUTION
Detect the nil pointer value, and return a nil value. The gorm codec translates this to a
NULLvalue 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).