From 2c0e952f8124192833e8c9584a361446cfeea073 Mon Sep 17 00:00:00 2001 From: Islam Aleiv Date: Fri, 21 Aug 2026 11:27:09 +0200 Subject: [PATCH 1/7] Graphql warnings extension --- client/db.go | 45 +++++++ client/db_test.go | 141 ++++++++++++++++++++++ docs/website/references/http/openapi.json | 92 ++++++++++++++ http/handler_store.go | 10 ++ internal/db/request.go | 6 + internal/db/subscriptions.go | 6 + internal/extensions/context.go | 74 ++++++++++++ internal/extensions/context_test.go | 84 +++++++++++++ 8 files changed, 458 insertions(+) create mode 100644 client/db_test.go create mode 100644 internal/extensions/context.go create mode 100644 internal/extensions/context_test.go diff --git a/client/db.go b/client/db.go index eef839bd92..690e3ec867 100644 --- a/client/db.go +++ b/client/db.go @@ -421,6 +421,45 @@ type GQLResult struct { // // It will be nil if any errors were raised during execution. Data any `json:"data"` + + // Extensions holds extra information about the request, such as warnings. + // + // It is nil when there is nothing to report, and the field is then left out + // of the response. + Extensions *GQLExtensions `json:"extensions,omitempty"` +} + +// GQLExtensions sits alongside data and errors in a GQL response. It carries anything +// we want to tell the caller that is not a result and not an error. +// +// Callers ignore entries they do not know about. +type GQLExtensions struct { + // Warnings holds anything the caller should know about a request that worked. + Warnings []GQLWarning `json:"warnings,omitempty"` +} + +// IsEmpty returns true if there is nothing to send. +// +// An empty value must be left out of the response, not sent as an empty object. +// Sending `"extensions":{}` would change the shape of every response. +func (e *GQLExtensions) IsEmpty() bool { + return e == nil || len(e.Warnings) == 0 +} + +// GQLWarning describes something that happened during a request that worked. It is not +// an error, and it does not mean the results are wrong. +type GQLWarning struct { + // Code names the warning. Callers check this, and it does not change once + // released. + Code string `json:"code"` + + // Message explains the warning to a person. + // + // The wording can change at any time, so do not parse it. + Message string `json:"message"` + + // Detail holds values belonging to this warning. Optional. + Detail map[string]any `json:"detail,omitempty"` } // gqlError represents an error that was encountered during a GQL request. @@ -439,6 +478,8 @@ type gqlResult struct { Errors []gqlError `json:"errors,omitempty"` // Data contains the result data Data any `json:"data"` + // Extensions contains the result extensions + Extensions *GQLExtensions `json:"extensions,omitempty"` } func (res *GQLResult) UnmarshalJSON(data []byte) error { @@ -449,6 +490,7 @@ func (res *GQLResult) UnmarshalJSON(data []byte) error { return err } res.Data = out.Data + res.Extensions = out.Extensions res.Errors = make([]error, len(out.Errors)) for i, e := range out.Errors { res.Errors[i] = ReviveError(e.Message) @@ -458,6 +500,9 @@ func (res *GQLResult) UnmarshalJSON(data []byte) error { func (res GQLResult) MarshalJSON() ([]byte, error) { out := gqlResult{Data: res.Data} + if !res.Extensions.IsEmpty() { + out.Extensions = res.Extensions + } out.Errors = make([]gqlError, len(res.Errors)) for i, e := range res.Errors { out.Errors[i] = gqlError{Message: e.Error()} diff --git a/client/db_test.go b/client/db_test.go new file mode 100644 index 0000000000..7e34925bca --- /dev/null +++ b/client/db_test.go @@ -0,0 +1,141 @@ +// Copyright 2026 Democratized Data Foundation +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package client + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" +) + +// GQLResult does not use the default struct marshalling. It copies itself field by +// field through a private mirror, so a field can exist on the type and still be missing +// from the JSON. These tests catch that. +// +// It matters because the Go client never serializes anything. A half applied change +// passes there and fails on every other client. + +func TestGQLResultMarshal_WithWarning_RoundTrips(t *testing.T) { + input := GQLResult{ + Data: map[string]any{"Users": []any{}}, + Extensions: &GQLExtensions{ + Warnings: []GQLWarning{ + { + Code: "test_warning", + Message: "something worth knowing happened", + Detail: map[string]any{"requested": 10, "returned": 6}, + }, + }, + }, + } + + data, err := json.Marshal(input) + require.NoError(t, err) + + var output GQLResult + err = json.Unmarshal(data, &output) + require.NoError(t, err) + + require.NotNil(t, output.Extensions) + require.Len(t, output.Extensions.Warnings, 1) + + warning := output.Extensions.Warnings[0] + require.Equal(t, "test_warning", warning.Code) + require.Equal(t, "something worth knowing happened", warning.Message) + + // UnmarshalJSON calls dec.UseNumber, so numbers come back as json.Number, not + // float64. Same as everything under `data`. + require.Equal(t, json.Number("10"), warning.Detail["requested"]) + require.Equal(t, json.Number("6"), warning.Detail["returned"]) +} + +func TestGQLResultMarshal_WithMultipleWarnings_PreservesOrder(t *testing.T) { + input := GQLResult{ + Extensions: &GQLExtensions{ + Warnings: []GQLWarning{ + {Code: "first", Message: "one"}, + {Code: "second", Message: "two"}, + }, + }, + } + + data, err := json.Marshal(input) + require.NoError(t, err) + + var output GQLResult + err = json.Unmarshal(data, &output) + require.NoError(t, err) + + require.Len(t, output.Extensions.Warnings, 2) + require.Equal(t, "first", output.Extensions.Warnings[0].Code) + require.Equal(t, "second", output.Extensions.Warnings[1].Code) +} + +func TestGQLResultMarshal_WithoutExtensions_OmitsField(t *testing.T) { + input := GQLResult{Data: map[string]any{"Users": []any{}}} + + data, err := json.Marshal(input) + require.NoError(t, err) + + var raw map[string]json.RawMessage + err = json.Unmarshal(data, &raw) + require.NoError(t, err) + + require.NotContains(t, raw, "extensions") +} + +func TestGQLResultMarshal_WithEmptyExtensions_OmitsField(t *testing.T) { + // An empty value must not be sent as `"extensions":{}`. Otherwise every response + // changes shape as soon as anything allocates an accumulator. + input := GQLResult{ + Data: map[string]any{"Users": []any{}}, + Extensions: &GQLExtensions{}, + } + + data, err := json.Marshal(input) + require.NoError(t, err) + + var raw map[string]json.RawMessage + err = json.Unmarshal(data, &raw) + require.NoError(t, err) + + require.NotContains(t, raw, "extensions") +} + +func TestGQLResultUnmarshal_WithUnknownExtensionKey_IsIgnored(t *testing.T) { + // An older client must ignore an entry it does not know about instead of failing + // the whole response. + data := []byte(`{ + "data": null, + "extensions": { + "warnings": [{"code": "known", "message": "hi", "unknownField": 1}], + "unknownKey": {"anything": true} + } + }`) + + var output GQLResult + err := json.Unmarshal(data, &output) + require.NoError(t, err) + + require.NotNil(t, output.Extensions) + require.Len(t, output.Extensions.Warnings, 1) + require.Equal(t, "known", output.Extensions.Warnings[0].Code) +} + +func TestGQLResultUnmarshal_WithoutExtensions_LeavesNil(t *testing.T) { + var output GQLResult + err := json.Unmarshal([]byte(`{"data": null}`), &output) + require.NoError(t, err) + + require.Nil(t, output.Extensions) + require.True(t, output.Extensions.IsEmpty()) +} diff --git a/docs/website/references/http/openapi.json b/docs/website/references/http/openapi.json index fdd2c8d28d..cecd41646e 100644 --- a/docs/website/references/http/openapi.json +++ b/docs/website/references/http/openapi.json @@ -2031,6 +2031,29 @@ "type": "object" }, "type": "array" + }, + "extensions": { + "properties": { + "warnings": { + "items": { + "properties": { + "code": { + "type": "string" + }, + "detail": { + "additionalProperties": true, + "type": "object" + }, + "message": { + "type": "string" + } + }, + "type": "object" + }, + "type": "array" + } + }, + "type": "object" } }, "type": "object" @@ -2058,6 +2081,29 @@ "type": "object" }, "type": "array" + }, + "extensions": { + "properties": { + "warnings": { + "items": { + "properties": { + "code": { + "type": "string" + }, + "detail": { + "additionalProperties": true, + "type": "object" + }, + "message": { + "type": "string" + } + }, + "type": "object" + }, + "type": "array" + } + }, + "type": "object" } }, "type": "object" @@ -2103,6 +2149,29 @@ "type": "object" }, "type": "array" + }, + "extensions": { + "properties": { + "warnings": { + "items": { + "properties": { + "code": { + "type": "string" + }, + "detail": { + "additionalProperties": true, + "type": "object" + }, + "message": { + "type": "string" + } + }, + "type": "object" + }, + "type": "array" + } + }, + "type": "object" } }, "type": "object" @@ -2130,6 +2199,29 @@ "type": "object" }, "type": "array" + }, + "extensions": { + "properties": { + "warnings": { + "items": { + "properties": { + "code": { + "type": "string" + }, + "detail": { + "additionalProperties": true, + "type": "object" + }, + "message": { + "type": "string" + } + }, + "type": "object" + }, + "type": "array" + } + }, + "type": "object" } }, "type": "object" diff --git a/http/handler_store.go b/http/handler_store.go index 66dbf78e5c..58bcdaa2e0 100644 --- a/http/handler_store.go +++ b/http/handler_store.go @@ -754,6 +754,16 @@ func (h *storeHandler) bindRoutes(router *Router) { }), ), "data": openapi3.NewObjectSchema().WithAnyAdditionalProperties(), + // Left out unless there is something to report. See client.GQLExtensions. + "extensions": openapi3.NewObjectSchema().WithProperties(map[string]*openapi3.Schema{ + "warnings": openapi3.NewArraySchema().WithItems( + openapi3.NewObjectSchema().WithProperties(map[string]*openapi3.Schema{ + "code": openapi3.NewStringSchema(), + "message": openapi3.NewStringSchema(), + "detail": openapi3.NewObjectSchema().WithAnyAdditionalProperties(), + }), + ), + }), }) collectionArraySchema := openapi3.NewArraySchema() diff --git a/internal/db/request.go b/internal/db/request.go index f8ad26993a..4e1b37d356 100644 --- a/internal/db/request.go +++ b/internal/db/request.go @@ -14,6 +14,7 @@ import ( "context" "github.com/sourcenetwork/defradb/client" + "github.com/sourcenetwork/defradb/internal/extensions" "github.com/sourcenetwork/defradb/internal/identity" "github.com/sourcenetwork/defradb/internal/planner" ) @@ -47,6 +48,10 @@ func (db *DB) execRequest(ctx context.Context, request string, options *client.G return res } + // Warnings raised while running the request are collected here and returned in + // the `extensions` field of the response. + ctx = extensions.WithAccumulator(ctx) + planner := planner.New( ctx, identity.FromContext(ctx), @@ -63,5 +68,6 @@ func (db *DB) execRequest(ctx context.Context, request string, options *client.G res.GQL.Errors = append(res.GQL.Errors, err) } res.GQL.Data = results + res.GQL.Extensions = extensions.Collect(ctx) return res } diff --git a/internal/db/subscriptions.go b/internal/db/subscriptions.go index b7b6598194..cad6a8db65 100644 --- a/internal/db/subscriptions.go +++ b/internal/db/subscriptions.go @@ -17,6 +17,7 @@ import ( "github.com/sourcenetwork/defradb/client/request" "github.com/sourcenetwork/defradb/errors" "github.com/sourcenetwork/defradb/event" + "github.com/sourcenetwork/defradb/internal/extensions" "github.com/sourcenetwork/defradb/internal/identity" "github.com/sourcenetwork/defradb/internal/planner" ) @@ -78,6 +79,10 @@ func (db *DB) handleSubscription(ctx context.Context, r *request.Request) (<-cha } ctx := InitContext(ctx, txn) + // One accumulator per event, not per subscription. A shared one would + // make each event repeat every warning before it. + ctx = extensions.WithAccumulator(ctx) + p := planner.New( ctx, identity.FromContext(ctx), @@ -131,6 +136,7 @@ func (db *DB) handleSubscription(ctx context.Context, r *request.Request) (<-cha continue } res.Data = result + res.Extensions = extensions.Collect(ctx) select { case <-ctx.Done(): diff --git a/internal/extensions/context.go b/internal/extensions/context.go new file mode 100644 index 0000000000..900718593b --- /dev/null +++ b/internal/extensions/context.go @@ -0,0 +1,74 @@ +// Copyright 2026 Democratized Data Foundation +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +// Package extensions collects warnings about a request so they can be returned in the +// `extensions` field of the GQL response. +// +// The accumulator is kept on the context. Warnings come from the planner and the +// fetchers, far below the code that builds the response, and returning them up through +// every layer in between would change a lot of code that does not care about them. +package extensions + +import ( + "context" + "sync" + + "github.com/sourcenetwork/defradb/client" +) + +// accumulatorContextKey is the key type for the request accumulator. +type accumulatorContextKey struct{} + +// accumulator holds the warnings reported during one request. +// +// More than one goroutine can report, so access is locked. +type accumulator struct { + mu sync.Mutex + warnings []client.GQLWarning +} + +// WithAccumulator returns a context carrying a new, empty accumulator. +// +// Call it once per request, and once per subscription event. One accumulator shared +// across a subscription would make each event repeat every warning before it. +func WithAccumulator(ctx context.Context) context.Context { + return context.WithValue(ctx, accumulatorContextKey{}, &accumulator{}) +} + +// AddWarning records a warning on the context's accumulator. +// +// It does nothing if there is no accumulator, so a caller on a path that was never +// wired up reports nothing instead of failing. +func AddWarning(ctx context.Context, warning client.GQLWarning) { + acc, ok := ctx.Value(accumulatorContextKey{}).(*accumulator) + if !ok { + return + } + + acc.mu.Lock() + defer acc.mu.Unlock() + acc.warnings = append(acc.warnings, warning) +} + +// Collect returns the warnings recorded on the context, or nil if there are none. +func Collect(ctx context.Context) *client.GQLExtensions { + acc, ok := ctx.Value(accumulatorContextKey{}).(*accumulator) + if !ok { + return nil + } + + acc.mu.Lock() + defer acc.mu.Unlock() + if len(acc.warnings) == 0 { + return nil + } + + return &client.GQLExtensions{Warnings: acc.warnings} +} diff --git a/internal/extensions/context_test.go b/internal/extensions/context_test.go new file mode 100644 index 0000000000..fa3942e2e7 --- /dev/null +++ b/internal/extensions/context_test.go @@ -0,0 +1,84 @@ +// Copyright 2026 Democratized Data Foundation +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package extensions + +import ( + "context" + "sync" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/sourcenetwork/defradb/client" +) + +func TestCollect_WithoutAccumulator_ReturnsNil(t *testing.T) { + require.Nil(t, Collect(context.Background())) +} + +func TestAddWarning_WithoutAccumulator_DoesNothing(t *testing.T) { + ctx := context.Background() + AddWarning(ctx, client.GQLWarning{Code: "ignored"}) + + require.Nil(t, Collect(ctx)) +} + +func TestCollect_WithEmptyAccumulator_ReturnsNil(t *testing.T) { + ctx := WithAccumulator(context.Background()) + + require.Nil(t, Collect(ctx)) +} + +func TestCollect_WithWarnings_PreservesOrder(t *testing.T) { + ctx := WithAccumulator(context.Background()) + AddWarning(ctx, client.GQLWarning{Code: "first"}) + AddWarning(ctx, client.GQLWarning{Code: "second"}) + + result := Collect(ctx) + require.NotNil(t, result) + require.Len(t, result.Warnings, 2) + require.Equal(t, "first", result.Warnings[0].Code) + require.Equal(t, "second", result.Warnings[1].Code) +} + +// A subscription builds a new context for each event, so each event gets its own +// accumulator. If they shared one, the second event would repeat the first's warning. +func TestWithAccumulator_NestedContext_IsolatesWarnings(t *testing.T) { + first := WithAccumulator(context.Background()) + AddWarning(first, client.GQLWarning{Code: "first"}) + + second := WithAccumulator(first) + AddWarning(second, client.GQLWarning{Code: "second"}) + + firstResult := Collect(first) + require.Len(t, firstResult.Warnings, 1) + require.Equal(t, "first", firstResult.Warnings[0].Code) + + secondResult := Collect(second) + require.Len(t, secondResult.Warnings, 1) + require.Equal(t, "second", secondResult.Warnings[0].Code) +} + +func TestAddWarning_FromMultipleGoroutines_RecordsAll(t *testing.T) { + ctx := WithAccumulator(context.Background()) + + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func() { + defer wg.Done() + AddWarning(ctx, client.GQLWarning{Code: "concurrent"}) + }() + } + wg.Wait() + + require.Len(t, Collect(ctx).Warnings, 50) +} From d9e9e03e79f4effc0c35b8aeaea22da729d6cb8c Mon Sep 17 00:00:00 2001 From: Islam Aleiv Date: Sun, 23 Aug 2026 20:11:31 +0200 Subject: [PATCH 2/7] Polish --- client/db.go | 26 ++++++++++++++++++- client/db_test.go | 39 +++++++++++++++++++++++++++++ internal/extensions/context.go | 6 ++++- internal/extensions/context_test.go | 14 +++++++++++ 4 files changed, 83 insertions(+), 2 deletions(-) diff --git a/client/db.go b/client/db.go index 690e3ec867..7b5b581790 100644 --- a/client/db.go +++ b/client/db.go @@ -442,8 +442,20 @@ type GQLExtensions struct { // // An empty value must be left out of the response, not sent as an empty object. // Sending `"extensions":{}` would change the shape of every response. +// +// This asks the encoder rather than checking each field, so a field added later is +// covered without editing here. Checking fields by hand means the next field added is +// silently dropped from every response until someone remembers to update this. +// +// A value the encoder cannot handle is treated as empty. The warning is then missing, +// which is better than failing the whole response over a diagnostic. func (e *GQLExtensions) IsEmpty() bool { - return e == nil || len(e.Warnings) == 0 + if e == nil { + return true + } + + data, err := json.Marshal(e) + return err != nil || string(data) == "{}" } // GQLWarning describes something that happened during a request that worked. It is not @@ -459,6 +471,13 @@ type GQLWarning struct { Message string `json:"message"` // Detail holds values belonging to this warning. Optional. + // + // Everything here is sent to the caller and may also be logged, so do not put + // secrets, credentials or identity material in it. + // + // Take care with counts and identifiers drawn from documents the caller is not + // allowed to read. Saying how many documents were examined can tell the caller + // about documents that access control hid from them. Detail map[string]any `json:"detail,omitempty"` } @@ -491,6 +510,11 @@ func (res *GQLResult) UnmarshalJSON(data []byte) error { } res.Data = out.Data res.Extensions = out.Extensions + // A peer may send `"extensions":{}`, which decodes to a non nil empty value. Callers + // are told the field is nil when there is nothing to report, so make that true. + if res.Extensions.IsEmpty() { + res.Extensions = nil + } res.Errors = make([]error, len(out.Errors)) for i, e := range out.Errors { res.Errors[i] = ReviveError(e.Message) diff --git a/client/db_test.go b/client/db_test.go index 7e34925bca..9b26543bd4 100644 --- a/client/db_test.go +++ b/client/db_test.go @@ -139,3 +139,42 @@ func TestGQLResultUnmarshal_WithoutExtensions_LeavesNil(t *testing.T) { require.Nil(t, output.Extensions) require.True(t, output.Extensions.IsEmpty()) } + +// The empty non nil slice is the case a field by field check gets wrong: the slice is +// not the zero value, but it still encodes to nothing. +func TestGQLExtensionsIsEmpty_WithEmptyWarningSlice_IsEmpty(t *testing.T) { + extensions := &GQLExtensions{Warnings: []GQLWarning{}} + + require.True(t, extensions.IsEmpty()) +} + +func TestGQLExtensionsIsEmpty_WithNilReceiver_IsEmpty(t *testing.T) { + var extensions *GQLExtensions + + require.True(t, extensions.IsEmpty()) +} + +func TestGQLExtensionsIsEmpty_WithAWarning_IsNotEmpty(t *testing.T) { + extensions := &GQLExtensions{Warnings: []GQLWarning{{Code: "test_warning"}}} + + require.False(t, extensions.IsEmpty()) +} + +// A peer may send an empty extensions object. Callers are told the field is nil when +// there is nothing to report, so decoding must make that true rather than hand back a +// non nil value with nothing in it. +func TestGQLResultUnmarshal_WithEmptyExtensions_LeavesNil(t *testing.T) { + var output GQLResult + err := json.Unmarshal([]byte(`{"data": null, "extensions": {}}`), &output) + require.NoError(t, err) + + require.Nil(t, output.Extensions) +} + +func TestGQLResultUnmarshal_WithOnlyUnknownExtensionKeys_LeavesNil(t *testing.T) { + var output GQLResult + err := json.Unmarshal([]byte(`{"data": null, "extensions": {"unknownKey": 1}}`), &output) + require.NoError(t, err) + + require.Nil(t, output.Extensions) +} diff --git a/internal/extensions/context.go b/internal/extensions/context.go index 900718593b..71491df923 100644 --- a/internal/extensions/context.go +++ b/internal/extensions/context.go @@ -18,6 +18,7 @@ package extensions import ( "context" + "slices" "sync" "github.com/sourcenetwork/defradb/client" @@ -58,6 +59,9 @@ func AddWarning(ctx context.Context, warning client.GQLWarning) { } // Collect returns the warnings recorded on the context, or nil if there are none. +// +// The returned slice is a copy. Handing out the accumulator's own slice would let a +// later AddWarning change a result the caller is already holding. func Collect(ctx context.Context) *client.GQLExtensions { acc, ok := ctx.Value(accumulatorContextKey{}).(*accumulator) if !ok { @@ -70,5 +74,5 @@ func Collect(ctx context.Context) *client.GQLExtensions { return nil } - return &client.GQLExtensions{Warnings: acc.warnings} + return &client.GQLExtensions{Warnings: slices.Clone(acc.warnings)} } diff --git a/internal/extensions/context_test.go b/internal/extensions/context_test.go index fa3942e2e7..3a3091d823 100644 --- a/internal/extensions/context_test.go +++ b/internal/extensions/context_test.go @@ -82,3 +82,17 @@ func TestAddWarning_FromMultipleGoroutines_RecordsAll(t *testing.T) { require.Len(t, Collect(ctx).Warnings, 50) } + +func TestCollect_ThenAddWarning_DoesNotChangeTheEarlierResult(t *testing.T) { + ctx := WithAccumulator(context.Background()) + AddWarning(ctx, client.GQLWarning{Code: "first"}) + + collected := Collect(ctx) + require.Len(t, collected.Warnings, 1) + + AddWarning(ctx, client.GQLWarning{Code: "second"}) + + require.Len(t, collected.Warnings, 1) + require.Equal(t, "first", collected.Warnings[0].Code) + require.Len(t, Collect(ctx).Warnings, 2) +} From 5c9365809c28773f58f08029cd690edfee58f17c Mon Sep 17 00:00:00 2001 From: Islam Aleiv Date: Mon, 24 Aug 2026 13:47:55 +0200 Subject: [PATCH 3/7] Polish --- client/db.go | 48 ++++++++++++++++++------------------------------ 1 file changed, 18 insertions(+), 30 deletions(-) diff --git a/client/db.go b/client/db.go index 7b5b581790..2d73a9e054 100644 --- a/client/db.go +++ b/client/db.go @@ -424,31 +424,25 @@ type GQLResult struct { // Extensions holds extra information about the request, such as warnings. // - // It is nil when there is nothing to report, and the field is then left out - // of the response. + // It is nil when there is nothing to report, and is then left out of the response. Extensions *GQLExtensions `json:"extensions,omitempty"` } -// GQLExtensions sits alongside data and errors in a GQL response. It carries anything -// we want to tell the caller that is not a result and not an error. +// GQLExtensions sits next to data and errors in a response. It holds anything we want +// to tell the caller that is neither a result nor an error. // -// Callers ignore entries they do not know about. +// Callers skip anything in here they do not recognise. type GQLExtensions struct { - // Warnings holds anything the caller should know about a request that worked. + // Warnings holds things the caller should know about a request that worked. Warnings []GQLWarning `json:"warnings,omitempty"` } -// IsEmpty returns true if there is nothing to send. +// IsEmpty returns true if there is nothing to send. An empty value is left out of the +// response rather than sent as `{}`. // -// An empty value must be left out of the response, not sent as an empty object. -// Sending `"extensions":{}` would change the shape of every response. -// -// This asks the encoder rather than checking each field, so a field added later is -// covered without editing here. Checking fields by hand means the next field added is -// silently dropped from every response until someone remembers to update this. -// -// A value the encoder cannot handle is treated as empty. The warning is then missing, -// which is better than failing the whole response over a diagnostic. +// It turns the value into JSON and looks at the result, so a field added later is +// covered without changing this. A value that cannot be turned into JSON counts as +// empty, so a bad warning is dropped instead of breaking the whole response. func (e *GQLExtensions) IsEmpty() bool { if e == nil { return true @@ -458,26 +452,20 @@ func (e *GQLExtensions) IsEmpty() bool { return err != nil || string(data) == "{}" } -// GQLWarning describes something that happened during a request that worked. It is not -// an error, and it does not mean the results are wrong. +// GQLWarning describes something that happened during a request that still worked. type GQLWarning struct { - // Code names the warning. Callers check this, and it does not change once - // released. + // Code names the warning. Callers check this. It does not change once released. Code string `json:"code"` - // Message explains the warning to a person. - // - // The wording can change at any time, so do not parse it. + // Message explains the warning to a person. The wording can change, so do not + // read it in code. Message string `json:"message"` - // Detail holds values belonging to this warning. Optional. - // - // Everything here is sent to the caller and may also be logged, so do not put - // secrets, credentials or identity material in it. + // Detail holds values that belong to this warning. Optional. // - // Take care with counts and identifiers drawn from documents the caller is not - // allowed to read. Saying how many documents were examined can tell the caller - // about documents that access control hid from them. + // It is sent to the caller and may be logged, so keep secrets and keys out of it. + // Be careful with counts too: saying how many documents were looked at can tell + // the caller about documents they are not allowed to see. Detail map[string]any `json:"detail,omitempty"` } From 03b96f1c7147fbaeeace4a417760e48f379ccf2b Mon Sep 17 00:00:00 2001 From: Islam Aleiv Date: Thu, 27 Aug 2026 00:15:24 +0200 Subject: [PATCH 4/7] warn on bad queries --- client/db.go | 9 + internal/planner/vector_search.go | 81 ++++-- tests/action/assert_request.go | 32 +++ tests/action/request.go | 7 + tests/action/subscription_request.go | 1 + .../index/vector_fallback_warning_test.go | 250 ++++++++++++++++++ .../integration/index/vector_metrics_test.go | 15 +- .../with_similarity_vector_index_test.go | 19 +- 8 files changed, 392 insertions(+), 22 deletions(-) create mode 100644 tests/integration/index/vector_fallback_warning_test.go diff --git a/client/db.go b/client/db.go index 2d73a9e054..1af898f695 100644 --- a/client/db.go +++ b/client/db.go @@ -469,6 +469,15 @@ type GQLWarning struct { Detail map[string]any `json:"detail,omitempty"` } +// Warning codes. Callers match on these, so they do not change once released. +const ( + // WarningCodeVectorIndexUnused means a similarity query read the whole collection even though + // the field it scored has a vector index. The results are correct, but the query costs more as + // the collection grows. The `reason` detail says which part of the query shape ruled the index + // out. + WarningCodeVectorIndexUnused = "VECTOR_INDEX_UNUSED" +) + // gqlError represents an error that was encountered during a GQL request. // // This is only used for marshalling to keep our responses spec compliant. diff --git a/internal/planner/vector_search.go b/internal/planner/vector_search.go index 05fa9475aa..414d05e288 100644 --- a/internal/planner/vector_search.go +++ b/internal/planner/vector_search.go @@ -16,35 +16,84 @@ import ( "github.com/sourcenetwork/defradb/internal/db/fetcher" "github.com/sourcenetwork/defradb/internal/db/id" "github.com/sourcenetwork/defradb/internal/db/vectorindex" + "github.com/sourcenetwork/defradb/internal/extensions" "github.com/sourcenetwork/defradb/internal/keys" "github.com/sourcenetwork/defradb/internal/planner/mapper" ) +// Sent as the `reason` detail of a [client.WarningCodeVectorIndexUnused] warning. Callers match on +// them, so they do not change once released. +const ( + reasonNoLimit = "noLimit" + // Lifting this is https://github.com/sourcenetwork/defradb/issues/5071 + reasonFilter = "filter" + reasonNotOrderedBySimilarityDesc = "notOrderedBySimilarityDesc" + // Lifting this is https://github.com/sourcenetwork/defradb/issues/5072 + reasonMultipleSimilarityFields = "multipleSimilarityFields" +) + +// warnVectorIndexUnused reports that the query reads the whole collection even though the field has +// a vector index. The results are still correct, so this is a warning and not an error. +func (n *selectNode) warnVectorIndexUnused(sim *mapper.Similarity, reason string) { + fieldName := sim.SimilarityTarget.Field.Name + extensions.AddWarning(n.planner.ctx, client.GQLWarning{ + Code: client.WarningCodeVectorIndexUnused, + Message: "similarity query on field '" + fieldName + + "' did not use the vector index and read the whole collection", + Detail: map[string]any{ + "field": fieldName, + "reason": reason, + }, + }) +} + // tryRouteSimilarityToVectorIndex narrows an otherwise-full scan to the k nearest documents when the // query is a nearest-neighbour search (a single `_similarity` ordered descending, with a limit) and // the field has a ready vector index. It feeds the graph search results to the scan as document // prefixes; the similarity/order/limit nodes are left to score, sort and cap as usual. When the query // does not match, it leaves the full-scan path in place. func (n *selectNode) tryRouteSimilarityToVectorIndex(origScan *scanNode) error { - if n.selectReq.Limit == nil || n.selectReq.Limit.Limit <= 0 { + // The index is looked up before the query shape is checked. Without one there is nothing to fall + // back from, so warning would be noise. + sims := n.similarityFields() + if len(sims) == 0 { return nil } - // A filter can drop some of the k nearest, so the graph would need to return more than k to - // backfill. That is filtered KNN: https://github.com/sourcenetwork/defradb/issues/5071 - if n.filter != nil { + if len(sims) > 1 { + // Which one drives the search is ambiguous, so the query full-scans. Letting the query say + // which it means is https://github.com/sourcenetwork/defradb/issues/5072 + // + // Reported against the first field that has an index, since the warning is one per query. + for _, sim := range sims { + if _, ok := n.readyVectorIndexOnField(sim.SimilarityTarget.Field.Name); ok { + n.warnVectorIndexUnused(sim, reasonMultipleSimilarityFields) + return nil + } + } return nil } + sim := sims[0] - sim := n.singleSimilarityField() - if sim == nil { + index, ok := n.readyVectorIndexOnField(sim.SimilarityTarget.Field.Name) + if !ok { return nil } - if !n.isOrderedBySimilarityDesc(sim) { + + if n.selectReq.Limit == nil || n.selectReq.Limit.Limit <= 0 { + n.warnVectorIndexUnused(sim, reasonNoLimit) return nil } - - index, ok := n.readyVectorIndexOnField(sim.SimilarityTarget.Field.Name) - if !ok { + // A filter can drop some of the k nearest, so the graph would need to return more than k to + // backfill. That is filtered KNN: https://github.com/sourcenetwork/defradb/issues/5071 + // + // Read from the scan, not n.filter: initSource moves the filter there and leaves n.filter nil + // unless the collection has migrations. + if origScan.filter != nil { + n.warnVectorIndexUnused(sim, reasonFilter) + return nil + } + if !n.isOrderedBySimilarityDesc(sim) { + n.warnVectorIndexUnused(sim, reasonNotOrderedBySimilarityDesc) return nil } // readyVectorIndexOnField only returns a vector index, so GetVector always succeeds here. @@ -78,16 +127,12 @@ func (n *selectNode) tryRouteSimilarityToVectorIndex(origScan *scanNode) error { return nil } -// singleSimilarityField returns the sole `_similarity` field, or nil if there are none or several: -// with two, which one drives the search is ambiguous, so such a query keeps the full-scan path. -func (n *selectNode) singleSimilarityField() *mapper.Similarity { - var found *mapper.Similarity +// similarityFields returns every `_similarity` field on the request, in the order they appear. +func (n *selectNode) similarityFields() []*mapper.Similarity { + var found []*mapper.Similarity for _, field := range n.selectReq.Fields { if sim, ok := field.(*mapper.Similarity); ok { - if found != nil { - return nil - } - found = sim + found = append(found, sim) } } return found diff --git a/tests/action/assert_request.go b/tests/action/assert_request.go index 1af9b0d66d..6914022683 100644 --- a/tests/action/assert_request.go +++ b/tests/action/assert_request.go @@ -81,6 +81,35 @@ func (a *assertStack) String() string { return b.String() } +// assertWarnings asserts the warnings in the `extensions` field of a response. +// +// Expecting none is the default, so a test that never mentions warnings still fails if the code +// starts emitting one. Only the code and the details given are compared: message wording is free +// to change, and a test can pin just the details it cares about. +func assertWarnings(t testing.TB, actual *client.GQLExtensions, expected []client.GQLWarning) { + var warnings []client.GQLWarning + if actual != nil { + warnings = actual.Warnings + } + + if len(expected) == 0 { + require.Empty(t, warnings, "expected no warnings") + return + } + + require.Len(t, warnings, len(expected), "unexpected number of warnings: %v", warnings) + for i, exp := range expected { + got := warnings[i] + require.Equal(t, exp.Code, got.Code, "unexpected warning code") + require.NotEmpty(t, got.Message, "warning %s has no message", got.Code) + for key, expValue := range exp.Detail { + actualValue, ok := got.Detail[key] + require.True(t, ok, "warning %s has no detail %q", got.Code, key) + require.Equal(t, expValue, actualValue, "unexpected detail %q on warning %s", key, got.Code) + } + } +} + // assertRequestResults asserts the results of a GQL request. func assertRequestResults( s *state.State, @@ -90,6 +119,7 @@ func assertRequestResults( asserter ResultAsserter, nodeID int, ordered bool, + expectedWarnings []client.GQLWarning, ) bool { s.CurrentAssertingNodeID = nodeID // we skip assertion benchmark because you don't specify expected result for benchmark. @@ -97,6 +127,8 @@ func assertRequestResults( return true } + assertWarnings(s.T, result.Extensions, expectedWarnings) + if expectedResults == nil && result.Data == nil { return false } diff --git a/tests/action/request.go b/tests/action/request.go index 448aab8c2a..7961c6a6e0 100644 --- a/tests/action/request.go +++ b/tests/action/request.go @@ -82,6 +82,12 @@ type Request struct { // Asserter is an optional custom result asserter. Asserter ResultAsserter + // The warnings expected in the `extensions` field of the response. Optional. + // + // Leaving it empty asserts the response carries none. Only the Code and any Detail entries + // given are compared. + ExpectedWarnings []client.GQLWarning + // Any error expected from the action. Optional. // // String can be a partial, and the test will pass if an error is returned that @@ -145,6 +151,7 @@ nodeLoop: a.Asserter, nodeID, !a.NonOrderedResults, + a.ExpectedWarnings, ) } diff --git a/tests/action/subscription_request.go b/tests/action/subscription_request.go index 2dad03d622..26f71e08ec 100644 --- a/tests/action/subscription_request.go +++ b/tests/action/subscription_request.go @@ -124,6 +124,7 @@ func (a *SubscriptionRequest) Execute() { nil, nodeID, true, + nil, ) assertExpectedErrorRaised(a.s.T, a.ExpectedError, expectedErrorRaised) diff --git a/tests/integration/index/vector_fallback_warning_test.go b/tests/integration/index/vector_fallback_warning_test.go new file mode 100644 index 0000000000..91c3362c00 --- /dev/null +++ b/tests/integration/index/vector_fallback_warning_test.go @@ -0,0 +1,250 @@ +// Copyright 2026 Democratized Data Foundation +// +// This file is part of the DefraDB test suite. +// +// The DefraDB test suite is licensed under either: +// +// (1) GNU Affero General Public License v3 +// (2) Business Source License 1.1 +// +// See tests/LICENSE for details. + +package index + +import ( + "testing" + + "github.com/sourcenetwork/defradb/client" + "github.com/sourcenetwork/defradb/tests/action" + testUtils "github.com/sourcenetwork/defradb/tests/integration" +) + +// vectorWarningSetup builds a collection with a cosine vector index and four documents. Every test +// here uses the same data and differs only in the shape of the request. +func vectorWarningSetup() []any { + return []any{ + &action.AddCollection{ + SDL: `type User { + name: String + age: Int + vector: [Float32!] @vectorIndex(dimensions: 3, HNSW: {metric: COSINE}) + }`, + }, + &action.AddDoc{DocMap: map[string]any{"name": "x", "age": 10, "vector": []float32{1, 0, 0}}}, + &action.AddDoc{DocMap: map[string]any{"name": "y", "age": 20, "vector": []float32{0, 1, 0}}}, + &action.AddDoc{DocMap: map[string]any{"name": "xy", "age": 30, "vector": []float32{0.9, 0.4, 0}}}, + &action.AddDoc{DocMap: map[string]any{"name": "z", "age": 40, "vector": []float32{0, 0, 1}}}, + } +} + +// unusedIndexWarning builds the warning a fallback is expected to report. +func unusedIndexWarning(reason string) []client.GQLWarning { + return []client.GQLWarning{ + { + Code: client.WarningCodeVectorIndexUnused, + Detail: map[string]any{ + "field": "vector", + "reason": reason, + }, + }, + } +} + +// The control. Without it, tests that only check the fallbacks would pass even if every query +// warned. +func TestVectorIndexWarning_QueryUsesIndex_ReportsNoWarning(t *testing.T) { + test := testUtils.TestCase{ + Actions: append(vectorWarningSetup(), + &action.Request{ + Request: `query { + User(order: {_alias: {sim: DESC}}, limit: 2){ + name + sim: SIMILARITY(vector: {vector: [1, 0, 0]}) + } + }`, + Results: map[string]any{ + "User": []map[string]any{ + {"name": "x", "sim": testUtils.CosineSimilarity([]float64{1, 0, 0}, []float64{1, 0, 0})}, + {"name": "xy", "sim": testUtils.CosineSimilarity([]float64{0.9, 0.4, 0}, []float64{1, 0, 0})}, + }, + }, + }, + ), + } + + testUtils.ExecuteTestCase(t, test) +} + +// Ascending asks for the farthest documents, which the graph cannot serve. This is the easiest +// mistake to make: anyone thinking in distances rather than similarities reaches for ASC. +func TestVectorIndexWarning_OrderedAscending_ReportsWarning(t *testing.T) { + test := testUtils.TestCase{ + Actions: append(vectorWarningSetup(), + &action.Request{ + Request: `query { + User(order: {_alias: {sim: ASC}}, limit: 2){ + name + sim: SIMILARITY(vector: {vector: [1, 0, 0]}) + } + }`, + // "y" and "z" both score 0, so their relative order is not defined. + NonOrderedResults: true, + Results: map[string]any{ + "User": []map[string]any{ + {"name": "z", "sim": testUtils.CosineSimilarity([]float64{0, 0, 1}, []float64{1, 0, 0})}, + {"name": "y", "sim": testUtils.CosineSimilarity([]float64{0, 1, 0}, []float64{1, 0, 0})}, + }, + }, + ExpectedWarnings: unusedIndexWarning("notOrderedBySimilarityDesc"), + }, + ), + } + + testUtils.ExecuteTestCase(t, test) +} + +// Without a limit there is no k for the graph to return, so the whole collection is read. +func TestVectorIndexWarning_NoLimit_ReportsWarning(t *testing.T) { + test := testUtils.TestCase{ + Actions: append(vectorWarningSetup(), + &action.Request{ + Request: `query { + User(order: {_alias: {sim: DESC}}){ + name + sim: SIMILARITY(vector: {vector: [1, 0, 0]}) + } + }`, + // "y" and "z" both score 0, so their relative order is not defined. + NonOrderedResults: true, + Results: map[string]any{ + "User": []map[string]any{ + {"name": "x", "sim": testUtils.CosineSimilarity([]float64{1, 0, 0}, []float64{1, 0, 0})}, + {"name": "xy", "sim": testUtils.CosineSimilarity([]float64{0.9, 0.4, 0}, []float64{1, 0, 0})}, + {"name": "y", "sim": testUtils.CosineSimilarity([]float64{0, 1, 0}, []float64{1, 0, 0})}, + {"name": "z", "sim": testUtils.CosineSimilarity([]float64{0, 0, 1}, []float64{1, 0, 0})}, + }, + }, + ExpectedWarnings: unusedIndexWarning("noLimit"), + }, + ), + } + + testUtils.ExecuteTestCase(t, test) +} + +// Lifting this is https://github.com/sourcenetwork/defradb/issues/5071 +func TestVectorIndexWarning_WithFilter_ReportsWarning(t *testing.T) { + test := testUtils.TestCase{ + Actions: append(vectorWarningSetup(), + &action.Request{ + Request: `query { + User(filter: {age: {_gt: 15}}, order: {_alias: {sim: DESC}}, limit: 2){ + name + sim: SIMILARITY(vector: {vector: [1, 0, 0]}) + } + }`, + Results: map[string]any{ + "User": []map[string]any{ + {"name": "xy", "sim": testUtils.CosineSimilarity([]float64{0.9, 0.4, 0}, []float64{1, 0, 0})}, + {"name": "y", "sim": testUtils.CosineSimilarity([]float64{0, 1, 0}, []float64{1, 0, 0})}, + }, + }, + ExpectedWarnings: unusedIndexWarning("filter"), + }, + ), + } + + testUtils.ExecuteTestCase(t, test) +} + +// Filtering the k nearest after the fact can reject all of them and return nothing while matching +// documents exist. Only "z" matches age > 35 and it is the farthest from the query, so asking the +// graph for k=1 would find "x", drop it, and return an empty result. +func TestVectorIndexWarning_SelectiveFilter_StillReturnsMatchingDocs(t *testing.T) { + test := testUtils.TestCase{ + Actions: append(vectorWarningSetup(), + &action.Request{ + Request: `query { + User(filter: {age: {_gt: 35}}, order: {_alias: {sim: DESC}}, limit: 1){ + name + sim: SIMILARITY(vector: {vector: [1, 0, 0]}) + } + }`, + Results: map[string]any{ + "User": []map[string]any{ + {"name": "z", "sim": testUtils.CosineSimilarity([]float64{0, 0, 1}, []float64{1, 0, 0})}, + }, + }, + ExpectedWarnings: unusedIndexWarning("filter"), + }, + ), + } + + testUtils.ExecuteTestCase(t, test) +} + +// With two similarity fields, which one drives the search is ambiguous, so the query full-scans. +func TestVectorIndexWarning_MultipleSimilarityFields_ReportsWarning(t *testing.T) { + test := testUtils.TestCase{ + Actions: append(vectorWarningSetup(), + &action.Request{ + Request: `query { + User(order: {_alias: {sim: DESC}}, limit: 2){ + name + sim: SIMILARITY(vector: {vector: [1, 0, 0]}) + other: SIMILARITY(vector: {vector: [0, 1, 0]}) + } + }`, + Results: map[string]any{ + "User": []map[string]any{ + { + "name": "x", + "sim": testUtils.CosineSimilarity([]float64{1, 0, 0}, []float64{1, 0, 0}), + "other": testUtils.CosineSimilarity([]float64{1, 0, 0}, []float64{0, 1, 0}), + }, + { + "name": "xy", + "sim": testUtils.CosineSimilarity([]float64{0.9, 0.4, 0}, []float64{1, 0, 0}), + "other": testUtils.CosineSimilarity([]float64{0.9, 0.4, 0}, []float64{0, 1, 0}), + }, + }, + }, + ExpectedWarnings: unusedIndexWarning("multipleSimilarityFields"), + }, + ), + } + + testUtils.ExecuteTestCase(t, test) +} + +// Nothing to fall back from, so no warning. Otherwise every similarity query in a schema without +// vector indexes would report one. +func TestVectorIndexWarning_NoVectorIndexOnField_ReportsNoWarning(t *testing.T) { + test := testUtils.TestCase{ + Actions: []any{ + &action.AddCollection{ + SDL: `type User { + name: String + vector: [Float32!] + }`, + }, + &action.AddDoc{DocMap: map[string]any{"name": "x", "vector": []float32{1, 0, 0}}}, + &action.AddDoc{DocMap: map[string]any{"name": "y", "vector": []float32{0, 1, 0}}}, + &action.Request{ + Request: `query { + User(order: {_alias: {sim: ASC}}, limit: 1){ + name + sim: SIMILARITY(vector: {vector: [1, 0, 0]}) + } + }`, + Results: map[string]any{ + "User": []map[string]any{ + {"name": "y", "sim": testUtils.CosineSimilarity([]float64{0, 1, 0}, []float64{1, 0, 0})}, + }, + }, + }, + }, + } + + testUtils.ExecuteTestCase(t, test) +} diff --git a/tests/integration/index/vector_metrics_test.go b/tests/integration/index/vector_metrics_test.go index 057a1f6870..d9fa6b96fb 100644 --- a/tests/integration/index/vector_metrics_test.go +++ b/tests/integration/index/vector_metrics_test.go @@ -156,11 +156,22 @@ func TestVectorIndex_SameQueryUsingIndexAndFullScan_ReturnsSameResults(t *testin Asserter: testUtils.NewExplainAsserter().WithIndexFetches(1).WithDocFetches(2), }, - // The same order, reached without the index. - &action.Request{Request: fullScanReq, Results: map[string]any{"User": expected}}, + // The same order, reached without the index. Dropping the limit is what forces the + // full scan, so this reports the unused-index warning. + &action.Request{ + Request: fullScanReq, + Results: map[string]any{"User": expected}, + ExpectedWarnings: []client.GQLWarning{ + {Code: client.WarningCodeVectorIndexUnused, Detail: map[string]any{"reason": "noLimit"}}, + }, + }, &action.Request{ Request: makeExplainQuery(fullScanReq), Asserter: testUtils.NewExplainAsserter().WithIndexFetches(0).WithDocFetches(3), + // Explaining a query still plans it, so the warning is reported here too. + ExpectedWarnings: []client.GQLWarning{ + {Code: client.WarningCodeVectorIndexUnused, Detail: map[string]any{"reason": "noLimit"}}, + }, }, }, } diff --git a/tests/integration/query/simple/with_similarity_vector_index_test.go b/tests/integration/query/simple/with_similarity_vector_index_test.go index 8bbaf42de1..aae44274ec 100644 --- a/tests/integration/query/simple/with_similarity_vector_index_test.go +++ b/tests/integration/query/simple/with_similarity_vector_index_test.go @@ -14,6 +14,7 @@ package simple import ( "testing" + "github.com/sourcenetwork/defradb/client" "github.com/sourcenetwork/defradb/tests/action" testUtils "github.com/sourcenetwork/defradb/tests/integration" ) @@ -211,6 +212,7 @@ func TestQuerySimple_WithSimilarityOnVectorIndex_AscendingOrderFullScans(t *test {"name": "far", "sim": testUtils.CosineSimilarity([]float64{0.1, 1, 0}, []float64{1, 0, 0})}, }, }, + ExpectedWarnings: ascendingFallbackWarning(), }, &action.Request{ Request: `query @explain(type: execute) { @@ -218,7 +220,8 @@ func TestQuerySimple_WithSimilarityOnVectorIndex_AscendingOrderFullScans(t *test sim: SIMILARITY(vector: {vector: [1, 0, 0]}) } }`, - Asserter: testUtils.NewExplainAsserter().WithIndexFetches(0).WithDocFetches(4), + Asserter: testUtils.NewExplainAsserter().WithIndexFetches(0).WithDocFetches(4), + ExpectedWarnings: ascendingFallbackWarning(), }, )...), } @@ -312,10 +315,22 @@ func TestQuerySimple_WithSimilarityOnVectorIndex_NoOrderDoesNotUseIndex(t *testi sim: SIMILARITY(vector: {vector: [1, 0, 0]}) } }`, - Asserter: testUtils.NewExplainAsserter().WithIndexFetches(0), + Asserter: testUtils.NewExplainAsserter().WithIndexFetches(0), + ExpectedWarnings: ascendingFallbackWarning(), }, }, } testUtils.ExecuteTestCase(t, test) } + +// ascendingFallbackWarning is the warning reported when a similarity query on an indexed field is +// not ordered by that similarity descending, so it reads the whole collection. +func ascendingFallbackWarning() []client.GQLWarning { + return []client.GQLWarning{ + { + Code: client.WarningCodeVectorIndexUnused, + Detail: map[string]any{"field": "vector", "reason": "notOrderedBySimilarityDesc"}, + }, + } +} From fe1ee582c40454a9b43b94b42972147de79e8970 Mon Sep 17 00:00:00 2001 From: Islam Aleiv Date: Thu, 27 Aug 2026 09:35:32 +0200 Subject: [PATCH 5/7] Polish --- internal/planner/vector_search.go | 2 ++ tests/action/assert_request.go | 20 ++++++++++++++++--- .../integration/index/vector_metrics_test.go | 4 ++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/internal/planner/vector_search.go b/internal/planner/vector_search.go index 414d05e288..ce174eaf50 100644 --- a/internal/planner/vector_search.go +++ b/internal/planner/vector_search.go @@ -99,6 +99,8 @@ func (n *selectNode) tryRouteSimilarityToVectorIndex(origScan *scanNode) error { // readyVectorIndexOnField only returns a vector index, so GetVector always succeeds here. vectorDesc, _ := index.GetVector() + // No warning here: the vector is malformed rather than the query shape being wrong, so there is + // nothing to rewrite. The full-scan path reports its own error. query, ok := similarityQueryVector(sim.Vector) if !ok { return nil diff --git a/tests/action/assert_request.go b/tests/action/assert_request.go index 6914022683..12b4b9efc6 100644 --- a/tests/action/assert_request.go +++ b/tests/action/assert_request.go @@ -86,7 +86,12 @@ func (a *assertStack) String() string { // Expecting none is the default, so a test that never mentions warnings still fails if the code // starts emitting one. Only the code and the details given are compared: message wording is free // to change, and a test can pin just the details it cares about. -func assertWarnings(t testing.TB, actual *client.GQLExtensions, expected []client.GQLWarning) { +func assertWarnings( + t testing.TB, + clientType state.ClientType, + actual *client.GQLExtensions, + expected []client.GQLWarning, +) { var warnings []client.GQLWarning if actual != nil { warnings = actual.Warnings @@ -102,10 +107,19 @@ func assertWarnings(t testing.TB, actual *client.GQLExtensions, expected []clien got := warnings[i] require.Equal(t, exp.Code, got.Code, "unexpected warning code") require.NotEmpty(t, got.Message, "warning %s has no message", got.Code) + // Details are part of the API, so an unexpected one is a change a test should catch. + require.Len(t, got.Detail, len(exp.Detail), "unexpected details on warning %s: %v", got.Code, got.Detail) for key, expValue := range exp.Detail { actualValue, ok := got.Detail[key] require.True(t, ok, "warning %s has no detail %q", got.Code, key) - require.Equal(t, expValue, actualValue, "unexpected detail %q on warning %s", key, got.Code) + // isResultsEqual, not require.Equal: the serializing clients decode numbers as + // json.Number, so a number detail would not match its Go value. + require.True( + t, + isResultsEqual(clientType, expValue, actualValue), + "unexpected detail %q on warning %s: expected %v, got %v", + key, got.Code, expValue, actualValue, + ) } } } @@ -127,7 +141,7 @@ func assertRequestResults( return true } - assertWarnings(s.T, result.Extensions, expectedWarnings) + assertWarnings(s.T, s.ClientType, result.Extensions, expectedWarnings) if expectedResults == nil && result.Data == nil { return false diff --git a/tests/integration/index/vector_metrics_test.go b/tests/integration/index/vector_metrics_test.go index d9fa6b96fb..425c192d8c 100644 --- a/tests/integration/index/vector_metrics_test.go +++ b/tests/integration/index/vector_metrics_test.go @@ -162,7 +162,7 @@ func TestVectorIndex_SameQueryUsingIndexAndFullScan_ReturnsSameResults(t *testin Request: fullScanReq, Results: map[string]any{"User": expected}, ExpectedWarnings: []client.GQLWarning{ - {Code: client.WarningCodeVectorIndexUnused, Detail: map[string]any{"reason": "noLimit"}}, + {Code: client.WarningCodeVectorIndexUnused, Detail: map[string]any{"field": "vector", "reason": "noLimit"}}, }, }, &action.Request{ @@ -170,7 +170,7 @@ func TestVectorIndex_SameQueryUsingIndexAndFullScan_ReturnsSameResults(t *testin Asserter: testUtils.NewExplainAsserter().WithIndexFetches(0).WithDocFetches(3), // Explaining a query still plans it, so the warning is reported here too. ExpectedWarnings: []client.GQLWarning{ - {Code: client.WarningCodeVectorIndexUnused, Detail: map[string]any{"reason": "noLimit"}}, + {Code: client.WarningCodeVectorIndexUnused, Detail: map[string]any{"field": "vector", "reason": "noLimit"}}, }, }, }, From 6b7beb36ba49dc0bac25664d6b807d458ac26523 Mon Sep 17 00:00:00 2001 From: Islam Aleiv Date: Thu, 27 Aug 2026 12:02:29 +0200 Subject: [PATCH 6/7] polish --- internal/db/request.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/db/request.go b/internal/db/request.go index c9bfb94d2b..5f56cd4fdd 100644 --- a/internal/db/request.go +++ b/internal/db/request.go @@ -14,8 +14,8 @@ import ( "context" "github.com/sourcenetwork/defradb/client" - "github.com/sourcenetwork/defradb/internal/extensions" "github.com/sourcenetwork/defradb/client/request" + "github.com/sourcenetwork/defradb/internal/extensions" "github.com/sourcenetwork/defradb/internal/identity" "github.com/sourcenetwork/defradb/internal/planner" ) From 71f8ce337e6e9af25e0b944874254e73d61055c5 Mon Sep 17 00:00:00 2001 From: Islam Aleiv Date: Tue, 1 Sep 2026 11:40:02 +0200 Subject: [PATCH 7/7] fix: Use current vector index directive syntax in warning tests The vector index directive changed from `@vectorIndex(dimensions: N, HNSW: {...})` to `@index(vector: {dimensions: N, hnsw: {...}})` on develop. These tests were written before that merge and still used the old form, so every schema in them failed to parse with `Unknown directive "vectorIndex"`. --- tests/integration/index/vector_fallback_warning_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/index/vector_fallback_warning_test.go b/tests/integration/index/vector_fallback_warning_test.go index 91c3362c00..7f01f50303 100644 --- a/tests/integration/index/vector_fallback_warning_test.go +++ b/tests/integration/index/vector_fallback_warning_test.go @@ -27,7 +27,7 @@ func vectorWarningSetup() []any { SDL: `type User { name: String age: Int - vector: [Float32!] @vectorIndex(dimensions: 3, HNSW: {metric: COSINE}) + vector: [Float32!] @index(vector: {dimensions: 3, hnsw: {metric: COSINE}}) }`, }, &action.AddDoc{DocMap: map[string]any{"name": "x", "age": 10, "vector": []float32{1, 0, 0}}},