Skip to content

Commit

Permalink
Resolve panic reflect.Value.Interface on zero value (#396)
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
davidhankins authored Nov 14, 2023
1 parent 3c74ac5 commit 7670b4b
Showing 1 changed file with 14 additions and 0 deletions.
14 changes: 14 additions & 0 deletions gorm/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,12 @@ func (converter *DefaultFilteringConditionConverter) StringConditionToGorm(ctx c
return converter.insensitiveCaseStringConditionToGorm(neg, dbName, o), []interface{}{value}, assocToJoin, nil
}

// N.B. if the user specifies a value that the codec translates to NULL
// (e.g. `field1 == ""` for string columns) instead of using the explicit
// support for identity (`field1 == null`), the results of this syntax may
// not match user expectations - `(col_name = NULL)` will match no rows,
// not even rows with NULL values. Did the user intend to match rows with
// NULL values (`field1 IS NULL`)?
return fmt.Sprintf("%s(%s %s ?)", neg, dbName, o), []interface{}{value}, assocToJoin, nil
}

Expand Down Expand Up @@ -226,6 +232,14 @@ func (p *DefaultFilteringConditionProcessor) ProcessStringCondition(ctx context.
if !ormId.IsValid() {
return nil, fmt.Errorf("Cannot find field %s in %s", part, objType)
}
// For type values where the codec translates a NULL value in
// SQL, we receive a pointer of nil value. E.g. `""`.
switch ormId.Kind() {
case reflect.Ptr, reflect.UnsafePointer:
if ormId.IsNil() {
return nil, nil
}
}
return reflect.Indirect(ormId).Interface(), nil

}
Expand Down

0 comments on commit 7670b4b

Please sign in to comment.