diff --git a/pkg/reflection/model_utils.go b/pkg/reflection/model_utils.go index 966cb7b..1d9fb26 100644 --- a/pkg/reflection/model_utils.go +++ b/pkg/reflection/model_utils.go @@ -9,6 +9,7 @@ import ( "time" "github.com/bitechdev/ResolveSpec/pkg/modelregistry" + "github.com/bitechdev/ResolveSpec/pkg/spectypes" ) type PrimaryKeyNameProvider interface { @@ -728,19 +729,19 @@ func GetColumnTypeFromModel(model interface{}, colName string) reflect.Kind { // Parse JSON tag (format: "name,omitempty") parts := strings.Split(jsonTag, ",") if parts[0] == sourceColName { - return field.Type.Kind() + return spectypes.UnwrapKind(field.Type) } } // Check field name (case-insensitive) if strings.EqualFold(field.Name, sourceColName) { - return field.Type.Kind() + return spectypes.UnwrapKind(field.Type) } // Check snake_case conversion snakeCaseName := ToSnakeCase(field.Name) if snakeCaseName == sourceColName { - return field.Type.Kind() + return spectypes.UnwrapKind(field.Type) } } diff --git a/pkg/reflection/model_utils_test.go b/pkg/reflection/model_utils_test.go index 0041a81..99ade55 100644 --- a/pkg/reflection/model_utils_test.go +++ b/pkg/reflection/model_utils_test.go @@ -3,6 +3,8 @@ package reflection import ( "reflect" "testing" + + "github.com/bitechdev/ResolveSpec/pkg/spectypes" ) // Test models for GORM @@ -1047,6 +1049,22 @@ func TestGetColumnTypeFromModel(t *testing.T) { } } +// SqlNull-wrapped columns (e.g. nullable bigint foreign keys) must report the +// wrapped value's Kind, not reflect.Struct, so numeric eq/gt/lt filters don't +// get an unnecessary CAST(... AS TEXT) that defeats the column's index. +type SqlNullFKModel struct { + RidParent spectypes.SqlInt64 `bun:"rid_parent" json:"rid_parent"` +} + +func TestGetColumnTypeFromModel_SqlNullWrapper(t *testing.T) { + model := SqlNullFKModel{RidParent: spectypes.NewSqlInt64(90446096)} + + result := GetColumnTypeFromModel(model, "rid_parent") + if result != reflect.Int64 { + t.Errorf("GetColumnTypeFromModel(rid_parent) = %v, want %v (SqlInt64 must unwrap to its numeric Kind)", result, reflect.Int64) + } +} + // ============= Tests for relation functions ============= // Models for relation testing diff --git a/pkg/reflection/spectypes_helpers.go b/pkg/reflection/spectypes_helpers.go index 4cf7c81..1a32b97 100644 --- a/pkg/reflection/spectypes_helpers.go +++ b/pkg/reflection/spectypes_helpers.go @@ -145,8 +145,16 @@ func IsJSONColumn(model interface{}, colName string) bool { // tagDeclaresJSON reports whether an ORM struct tag declares a json/jsonb column // type, e.g. `bun:"meta,type:jsonb"` or `gorm:"column:meta;type:json"`. func tagDeclaresJSON(tag string) bool { + return columnTypeTagValue(tag) == "json" || strings.HasPrefix(columnTypeTagValue(tag), "json(") || + columnTypeTagValue(tag) == "jsonb" || strings.HasPrefix(columnTypeTagValue(tag), "jsonb(") +} + +// columnTypeTagValue extracts the lower-cased value of a `type:` entry from a +// bun or gorm struct tag, e.g. `bun:"name,type:citext"` -> "citext". Returns "" +// if the tag carries no `type:` entry. +func columnTypeTagValue(tag string) string { if tag == "" { - return false + return "" } for _, part := range strings.FieldsFunc(tag, func(r rune) bool { return r == ',' || r == ';' || r == ' ' @@ -155,12 +163,22 @@ func tagDeclaresJSON(tag string) bool { if !found { continue } - value = strings.ToLower(strings.TrimSpace(value)) - // Match "json" and "jsonb", including parametrised forms just in case. - if value == "json" || value == "jsonb" || - strings.HasPrefix(value, "json(") || strings.HasPrefix(value, "jsonb(") { - return true - } + return strings.ToLower(strings.TrimSpace(value)) } - return false + return "" +} + +// IsCitextColumn reports whether colName carries an explicit `type:citext` +// bun/gorm tag. citext columns must never be CAST(... AS TEXT) for comparisons: +// that swaps in case-sensitive text semantics and defeats any citext index. +func IsCitextColumn(model interface{}, colName string) bool { + f, ok := getColumnStructField(model, colName) + if !ok { + return false + } + tagVal := columnTypeTagValue(f.Tag.Get("bun")) + if tagVal == "" { + tagVal = columnTypeTagValue(f.Tag.Get("gorm")) + } + return tagVal == "citext" } diff --git a/pkg/resolvespec/handler.go b/pkg/resolvespec/handler.go index c23fb26..6d98569 100644 --- a/pkg/resolvespec/handler.go +++ b/pkg/resolvespec/handler.go @@ -1937,10 +1937,10 @@ func (h *Handler) buildFilterCondition(filter common.FilterOption, model interfa condition = fmt.Sprintf("%s <= ?", filter.Column) args = []interface{}{filter.Value} case "like": - condition = fmt.Sprintf("CAST(%s AS TEXT) LIKE ?", filter.Column) + condition = fmt.Sprintf("%s LIKE ?", likeColumn(filter.Column, model)) args = []interface{}{filter.Value} case "ilike": - condition = fmt.Sprintf("CAST(%s AS TEXT) ILIKE ?", filter.Column) + condition = fmt.Sprintf("%s ILIKE ?", likeColumn(filter.Column, model)) args = []interface{}{filter.Value} case "in": condition, args = common.BuildInCondition(filter.Column, filter.Value) @@ -1973,6 +1973,18 @@ func (h *Handler) buildFilterCondition(filter common.FilterOption, model interfa return condition, args } +// likeColumn returns the column expression to use for LIKE/ILIKE. citext +// columns are compared natively — they're already case-insensitive, and +// CAST(... AS TEXT) would switch to case-sensitive matching and defeat a +// citext index. Every other column is cast to TEXT so LIKE/ILIKE also works +// against date/time/timestamp and numeric columns. +func likeColumn(column string, model interface{}) string { + if reflection.IsCitextColumn(model, column) { + return column + } + return fmt.Sprintf("CAST(%s AS TEXT)", column) +} + func (h *Handler) applyFilter(query common.SelectQuery, filter common.FilterOption, model interface{}) common.SelectQuery { // Determine which method to use based on LogicOperator useOrLogic := strings.EqualFold(filter.LogicOperator, "OR") @@ -2007,10 +2019,10 @@ func (h *Handler) applyFilter(query common.SelectQuery, filter common.FilterOpti condition = fmt.Sprintf("%s <= ?", filter.Column) args = []interface{}{filter.Value} case "like": - condition = fmt.Sprintf("CAST(%s AS TEXT) LIKE ?", filter.Column) + condition = fmt.Sprintf("%s LIKE ?", likeColumn(filter.Column, model)) args = []interface{}{filter.Value} case "ilike": - condition = fmt.Sprintf("CAST(%s AS TEXT) ILIKE ?", filter.Column) + condition = fmt.Sprintf("%s ILIKE ?", likeColumn(filter.Column, model)) args = []interface{}{filter.Value} case "in": condition, args = common.BuildInCondition(filter.Column, filter.Value) diff --git a/pkg/restheadspec/filter_cast_test.go b/pkg/restheadspec/filter_cast_test.go new file mode 100644 index 0000000..d1c249f --- /dev/null +++ b/pkg/restheadspec/filter_cast_test.go @@ -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) + } + }) +} diff --git a/pkg/restheadspec/handler.go b/pkg/restheadspec/handler.go index ae30338..01fbe6b 100644 --- a/pkg/restheadspec/handler.go +++ b/pkg/restheadspec/handler.go @@ -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) diff --git a/pkg/restheadspec/headers.go b/pkg/restheadspec/headers.go index 35e70eb..cd09453 100644 --- a/pkg/restheadspec/headers.go +++ b/pkg/restheadspec/headers.go @@ -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 diff --git a/pkg/spectypes/type_names.go b/pkg/spectypes/type_names.go index 7885f8b..af7fa60 100644 --- a/pkg/spectypes/type_names.go +++ b/pkg/spectypes/type_names.go @@ -97,3 +97,27 @@ func IsJSONType(t reflect.Type) bool { n, ok := SQLTypeName(t) return ok && (n == "jsonb" || n == "json") } + +// UnwrapKind returns the reflect.Kind to use when reasoning about a column's +// comparability (numeric vs. string vs. other) for filter building. Plain Go +// types return their own Kind unchanged. spectypes.SqlNull[T] wrappers (and +// types that embed one, such as SqlTimeStamp/SqlDate/SqlTime) always report +// reflect.Struct for their own Kind even when T is an int64 or string, which +// would otherwise make numeric/text columns look "complex" and force an +// unnecessary CAST(... AS TEXT) that defeats native column indexes. For those +// wrappers, UnwrapKind returns the Kind of the wrapped value T instead. +func UnwrapKind(t reflect.Type) reflect.Kind { + for t != nil && t.Kind() == reflect.Pointer { + t = t.Elem() + } + if t == nil { + return reflect.Invalid + } + if t.Kind() != reflect.Struct || t.PkgPath() != pkgPath { + return t.Kind() + } + if f, ok := t.FieldByName("Val"); ok { + return f.Type.Kind() + } + return t.Kind() +}