mirror of
https://github.com/bitechdev/ResolveSpec.git
synced 2026-09-17 05:42:34 +00:00
fix(restheadspec,resolvespec): stop casting SqlNull-wrapped and citext columns to TEXT in filters
reflect.Type.Kind() on spectypes.SqlNull[T] wrappers (SqlInt16/32/64, SqlFloat64, SqlBool, SqlString, and embedders like SqlTimeStamp) always reports reflect.Struct, never the wrapped T. ValidateAndAdjustFilterForColumnType treated those as "complex" columns and forced CAST(col AS TEXT) on eq/gt/lt filters, e.g. CAST(atdetail.rid_parent AS TEXT) = '90446096', which can't use the index on rid_parent. Add spectypes.UnwrapKind to see through SqlNull wrappers to the underlying Kind, and use it in GetColumnTypeFromModel so numeric/string SqlNull columns are recognized correctly and compared natively. Also stop unconditionally casting to TEXT for LIKE/ILIKE and add reflection.IsCitextColumn: citext columns are already case-insensitive, so casting them to TEXT flips to case-sensitive matching and defeats a citext index.
This commit is contained in:
@@ -0,0 +1,139 @@
|
||||
package restheadspec
|
||||
|
||||
import (
|
||||
"reflect"
|
||||
"testing"
|
||||
|
||||
"github.com/bitechdev/ResolveSpec/pkg/common"
|
||||
"github.com/bitechdev/ResolveSpec/pkg/spectypes"
|
||||
)
|
||||
|
||||
// atdetailModel mirrors the real-world model that triggered this regression:
|
||||
// rid_parent is a nullable bigint foreign key, backed by spectypes.SqlInt64
|
||||
// (a SqlNull[int64] alias). An eq filter on it was being rendered as
|
||||
// CAST(atdetail.rid_parent AS TEXT) = '90446096', which can't use the index
|
||||
// on rid_parent. Name is a citext column, which must never be cast to TEXT
|
||||
// either (that would switch to case-sensitive matching and lose its index).
|
||||
type atdetailModel struct {
|
||||
RidParent spectypes.SqlInt64 `json:"rid_parent" bun:"rid_parent"`
|
||||
Name string `json:"name" bun:"name,type:citext"`
|
||||
}
|
||||
|
||||
func TestValidateAndAdjustFilterForColumnType_SqlNullNumeric(t *testing.T) {
|
||||
h := &Handler{}
|
||||
model := atdetailModel{}
|
||||
|
||||
filter := &common.FilterOption{Column: "rid_parent", Operator: "eq", Value: "90446096"}
|
||||
info := h.ValidateAndAdjustFilterForColumnType(filter, model)
|
||||
|
||||
if info.NeedsCast {
|
||||
t.Fatalf("expected NeedsCast=false for a numeric SqlInt64 column with a numeric value, got true")
|
||||
}
|
||||
if !info.IsNumericType {
|
||||
t.Fatalf("expected IsNumericType=true for a SqlInt64 column")
|
||||
}
|
||||
if v, ok := filter.Value.(int64); !ok || v != 90446096 {
|
||||
t.Fatalf("expected filter.Value to be converted to int64(90446096), got %#v", filter.Value)
|
||||
}
|
||||
}
|
||||
|
||||
func TestApplyFilter_SqlNullNumeric_NoCastKeepsIndexUsable(t *testing.T) {
|
||||
h := &Handler{}
|
||||
model := atdetailModel{}
|
||||
|
||||
filter := common.FilterOption{Column: "rid_parent", Operator: "eq", Value: "90446096"}
|
||||
castInfo := h.ValidateAndAdjustFilterForColumnType(&filter, model)
|
||||
|
||||
q := &jsonCapQuery{}
|
||||
h.applyFilter(q, filter, "public.atdetail", castInfo.NeedsCast, "AND", model)
|
||||
|
||||
c := q.only(t)
|
||||
const want = "atdetail.rid_parent = ?"
|
||||
if c.query != want {
|
||||
t.Fatalf("query = %q, want %q (must not CAST a numeric column to TEXT)", c.query, want)
|
||||
}
|
||||
if !reflect.DeepEqual(c.args, []interface{}{int64(90446096)}) {
|
||||
t.Fatalf("args = %#v", c.args)
|
||||
}
|
||||
}
|
||||
|
||||
// TestFieldFilterHeader_SqlNullNumeric_EndToEnd reproduces the exact reported
|
||||
// regression: a request carrying the header
|
||||
//
|
||||
// x-fieldfilter-rid_parent: 90446096
|
||||
//
|
||||
// against a model whose rid_parent field is a nullable bigint (spectypes.SqlInt64).
|
||||
// Before the fix, this parsed to a filter that got CAST(atdetail.rid_parent AS TEXT) = '90446096',
|
||||
// making the query unable to use the index on rid_parent. It must now parse to
|
||||
// a native "atdetail.rid_parent = ?" comparison with an int64 argument.
|
||||
func TestFieldFilterHeader_SqlNullNumeric_EndToEnd(t *testing.T) {
|
||||
h := NewHandler(nil, nil)
|
||||
model := atdetailModel{}
|
||||
|
||||
req := &MockRequest{
|
||||
headers: map[string]string{
|
||||
"x-fieldfilter-rid_parent": "90446096",
|
||||
},
|
||||
queryParams: map[string]string{},
|
||||
}
|
||||
|
||||
options := h.parseOptionsFromHeaders(req, model)
|
||||
if len(options.Filters) != 1 {
|
||||
t.Fatalf("expected 1 filter parsed from x-fieldfilter-rid_parent, got %d: %+v", len(options.Filters), options.Filters)
|
||||
}
|
||||
|
||||
filter := options.Filters[0]
|
||||
if filter.Column != "rid_parent" || filter.Operator != "eq" {
|
||||
t.Fatalf("unexpected parsed filter: %+v", filter)
|
||||
}
|
||||
if filter.Value != "90446096" {
|
||||
t.Fatalf("expected raw header string value before type validation, got %#v", filter.Value)
|
||||
}
|
||||
|
||||
// This is the exact step that decided whether to CAST: ValidateAndAdjustFilterForColumnType
|
||||
// used to see reflect.Struct for the SqlInt64-wrapped column and cast to TEXT.
|
||||
castInfo := h.ValidateAndAdjustFilterForColumnType(&filter, model)
|
||||
if castInfo.NeedsCast {
|
||||
t.Fatalf("regression: numeric SqlInt64 column x-fieldfilter-rid_parent got NeedsCast=true, " +
|
||||
"which renders CAST(atdetail.rid_parent AS TEXT) = '90446096' and defeats the column's index")
|
||||
}
|
||||
|
||||
q := &jsonCapQuery{}
|
||||
h.applyFilter(q, filter, "public.atdetail", castInfo.NeedsCast, filter.LogicOperator, model)
|
||||
|
||||
c := q.only(t)
|
||||
const want = "atdetail.rid_parent = ?"
|
||||
if c.query != want {
|
||||
t.Fatalf("SQL condition = %q, want %q (no CAST, so the rid_parent index can still be used)", c.query, want)
|
||||
}
|
||||
if !reflect.DeepEqual(c.args, []interface{}{int64(90446096)}) {
|
||||
t.Fatalf("args = %#v, want [int64(90446096)]", c.args)
|
||||
}
|
||||
}
|
||||
|
||||
func TestApplyFilter_Citext_NeverCastForEqOrIlike(t *testing.T) {
|
||||
h := &Handler{}
|
||||
model := atdetailModel{}
|
||||
|
||||
t.Run("eq", func(t *testing.T) {
|
||||
filter := common.FilterOption{Column: "name", Operator: "eq", Value: "Acme"}
|
||||
castInfo := h.ValidateAndAdjustFilterForColumnType(&filter, model)
|
||||
if castInfo.NeedsCast {
|
||||
t.Fatalf("citext column must never need a CAST")
|
||||
}
|
||||
q := &jsonCapQuery{}
|
||||
h.applyFilter(q, filter, "public.atdetail", castInfo.NeedsCast, "AND", model)
|
||||
if c := q.only(t); c.query != "atdetail.name = ?" {
|
||||
t.Fatalf("query = %q", c.query)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("ilike", func(t *testing.T) {
|
||||
filter := common.FilterOption{Column: "name", Operator: "ilike", Value: "%acme%"}
|
||||
q := &jsonCapQuery{}
|
||||
h.applyFilter(q, filter, "public.atdetail", false, "AND", model)
|
||||
if c := q.only(t); c.query != "atdetail.name ILIKE ?" {
|
||||
t.Fatalf("query = %q, want no CAST for a citext column", c.query)
|
||||
}
|
||||
})
|
||||
}
|
||||
@@ -2325,6 +2325,13 @@ func (h *Handler) applyFilter(query common.SelectQuery, filter common.FilterOpti
|
||||
qualifiedColumn = fmt.Sprintf("CAST(%s AS TEXT)", rawQualifiedColumn)
|
||||
}
|
||||
|
||||
// citext columns already compare case-insensitively; casting to TEXT for
|
||||
// LIKE/ILIKE would switch to case-sensitive matching and defeat a citext index.
|
||||
likeColumn := rawQualifiedColumn
|
||||
if !reflection.IsCitextColumn(model, filter.Column) {
|
||||
likeColumn = fmt.Sprintf("CAST(%s AS TEXT)", rawQualifiedColumn)
|
||||
}
|
||||
|
||||
switch strings.ToLower(filter.Operator) {
|
||||
case "eq", "equals":
|
||||
return applyWhere(fmt.Sprintf("%s = ?", qualifiedColumn), filter.Value)
|
||||
@@ -2339,11 +2346,14 @@ func (h *Handler) applyFilter(query common.SelectQuery, filter common.FilterOpti
|
||||
case "lte", "less_than_equals", "le":
|
||||
return applyWhere(fmt.Sprintf("%s <= ?", qualifiedColumn), filter.Value)
|
||||
case "like":
|
||||
// Always cast to TEXT for LIKE/ILIKE to support date/time/timestamp columns
|
||||
return applyWhere(fmt.Sprintf("CAST(%s AS TEXT) LIKE ?", rawQualifiedColumn), filter.Value)
|
||||
// Cast to TEXT for LIKE to support date/time/timestamp columns; citext
|
||||
// columns are compared natively (see likeColumn above).
|
||||
return applyWhere(fmt.Sprintf("%s LIKE ?", likeColumn), filter.Value)
|
||||
case "ilike":
|
||||
// Always cast to TEXT for LIKE/ILIKE to support date/time/timestamp columns
|
||||
return applyWhere(fmt.Sprintf("CAST(%s AS TEXT) ILIKE ?", rawQualifiedColumn), filter.Value)
|
||||
// Cast to TEXT for ILIKE to support date/time/timestamp columns; citext
|
||||
// columns are compared natively (see likeColumn above) since citext is
|
||||
// already case-insensitive.
|
||||
return applyWhere(fmt.Sprintf("%s ILIKE ?", likeColumn), filter.Value)
|
||||
case "in":
|
||||
cond, inArgs := common.BuildInCondition(qualifiedColumn, filter.Value)
|
||||
if cond == "" {
|
||||
@@ -2421,8 +2431,12 @@ func (h *Handler) applyOrFilterGroup(query common.SelectQuery, filters []*common
|
||||
|
||||
op := strings.ToLower(filter.Operator)
|
||||
if op == "like" || op == "ilike" {
|
||||
// Always cast to TEXT for LIKE/ILIKE to support date/time/timestamp columns
|
||||
qualifiedColumn = fmt.Sprintf("CAST(%s AS TEXT)", rawQualifiedColumn)
|
||||
// Cast to TEXT for LIKE/ILIKE to support date/time/timestamp columns.
|
||||
// citext columns are left native: they're already case-insensitive and
|
||||
// casting would defeat a citext index.
|
||||
if !reflection.IsCitextColumn(model, filter.Column) {
|
||||
qualifiedColumn = fmt.Sprintf("CAST(%s AS TEXT)", rawQualifiedColumn)
|
||||
}
|
||||
} else if castInfo[i].NeedsCast {
|
||||
// Apply casting to text if needed for non-numeric columns or non-numeric values
|
||||
qualifiedColumn = fmt.Sprintf("CAST(%s AS TEXT)", rawQualifiedColumn)
|
||||
|
||||
@@ -1466,6 +1466,12 @@ func (h *Handler) ValidateAndAdjustFilterForColumnType(filter *common.FilterOpti
|
||||
return ColumnCastInfo{NeedsCast: false, IsNumericType: false}
|
||||
}
|
||||
|
||||
// Never cast citext columns to TEXT: CAST(col AS TEXT) swaps in case-sensitive
|
||||
// comparison semantics and prevents PostgreSQL from using a citext index.
|
||||
if reflection.IsCitextColumn(model, filter.Column) {
|
||||
return ColumnCastInfo{NeedsCast: false, IsNumericType: false}
|
||||
}
|
||||
|
||||
colType := reflection.GetColumnTypeFromModel(model, filter.Column)
|
||||
if colType == reflect.Invalid {
|
||||
// Column not found in model, no casting needed
|
||||
|
||||
Reference in New Issue
Block a user