Polish

Author: islamaliev

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)

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)
+}

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)}
 }

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)
+}