commit json status write after checking the fields picker

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 - Minor UI improvements (updated dark primary btn contrast).
 
+- Writes the status header for JSON responses only if the fields picker succeed or has acceptable fallback.
+    _This is to allow custom response status code in reaction to failed json writes._
+
 - Bumped `golang.org/x/*` dependencies to silence security scanners ([#7829](https://github.com/pocketbase/pocketbase/discussions/7829)).
 
 

tools/picker/pick.go

@@ -3,12 +3,16 @@ package picker
 import (
 	"encoding/json/jsontext"
 	"encoding/json/v2"
+	"errors"
+	"fmt"
 	"strings"
 
 	"github.com/pocketbase/pocketbase/tools/search"
 	"github.com/pocketbase/pocketbase/tools/tokenizer"
 )
 
+var ErrInvalidModifierData = errors.New("failed to apply some of the field modifiers for the provided data")
+
 // Pick converts data into a []any, map[string]any, etc. (using json marshal->unmarshal)
 // containing only the fields from the parsed rawFields expression.
 //
@@ -16,6 +20,10 @@ import (
 // Nested fields should be listed with dot-notation.
 // Fields value modifiers are also supported using the `:modifier(args)` format (see Modifiers).
 //
+// In case some data fails to apply against the registered modifiers
+// a wrapped [ErrInvalidModifierData] is returned that you can inspect and
+// decide to ignore or propagate further up the execution chain.
+//
 // Example:
 //
 //	data := map[string]any{"a": 1, "b": 2, "c": map[string]any{"c1": 11, "c2": 22}}
@@ -41,7 +49,9 @@ func Pick(data any, rawFields string) (any, error) {
 	}
 
 	var decoded any
-	if err := json.Unmarshal(encoded, &decoded); err != nil {
+
+	err = json.Unmarshal(encoded, &decoded)
+	if err != nil {
 		return nil, err
 	}
 	// ---
@@ -55,10 +65,16 @@ func Pick(data any, rawFields string) (any, error) {
 
 	if isSearchResult {
 		if decodedMap, ok := decoded.(map[string]any); ok {
-			pickParsedFields(decodedMap["items"], parsedFields)
+			err = pickParsedFields(decodedMap["items"], parsedFields)
+			if err != nil {
+				return nil, fmt.Errorf("%w: %w", ErrInvalidModifierData, err)
+			}
 		}
 	} else {
-		pickParsedFields(decoded, parsedFields)
+		err = pickParsedFields(decoded, parsedFields)
+		if err != nil {
+			return nil, fmt.Errorf("%w: %w", ErrInvalidModifierData, err)
+		}
 	}
 
 	return decoded, nil
@@ -94,7 +110,10 @@ func parseFields(rawFields string) (map[string]Modifier, error) {
 func pickParsedFields(data any, fields map[string]Modifier) error {
 	switch v := data.(type) {
 	case map[string]any:
-		pickMapFields(v, fields)
+		err := pickMapFields(v, fields)
+		if err != nil {
+			return err
+		}
 	case []map[string]any:
 		for _, item := range v {
 			if err := pickMapFields(item, fields); err != nil {
@@ -112,7 +131,7 @@ func pickParsedFields(data any, fields map[string]Modifier) error {
 
 		for _, item := range v {
 			if err := pickMapFields(item.(map[string]any), fields); err != nil {
-				return nil
+				return err
 			}
 		}
 	}

tools/picker/pick_test.go

@@ -2,12 +2,26 @@ package picker_test
 
 import (
 	"encoding/json/v2"
+	"errors"
 	"testing"
 
 	"github.com/pocketbase/pocketbase/tools/picker"
 	"github.com/pocketbase/pocketbase/tools/search"
 )
 
+type brokenModifier struct {
+}
+
+func (m *brokenModifier) Modify(val any) (any, error) {
+	return nil, errors.New("test_error")
+}
+
+func init() {
+	picker.Modifiers["broken"] = func(args ...string) (picker.Modifier, error) {
+		return &brokenModifier{}, nil
+	}
+}
+
 func TestPickFields(t *testing.T) {
 	scenarios := []struct {
 		name        string
@@ -223,6 +237,13 @@ func TestPickFields(t *testing.T) {
 			false,
 			`{"id":"123","rel":{"id":"456","sub":{"id":"789"},"title":"rel_title"}}`,
 		},
+		{
+			"with modifer.Modify error",
+			map[string]any{"a": 1},
+			"*:broken",
+			true,
+			`{"a":1}`,
+		},
 		{
 			"invalid excerpt modifier",
 			map[string]any{"a": 1, "b": 2, "c": "test"},

tools/router/event.go

@@ -189,23 +189,24 @@ const jsonFieldsParam = "fields"
 // Note that invalid UTF8 characters are mangled for compatibility
 // with earlier versions and to prevent unnecessary causing a response error.
 func (e *Event) JSON(status int, data any) error {
+	// try to pick only the requested fields (currently allowed only for "success" responses)
+	rawFields := e.Request.URL.Query().Get(jsonFieldsParam)
+	if rawFields != "" && status >= 200 && status <= 299 {
+		modified, err := picker.Pick(data, rawFields)
+		if err == nil {
+			data = modified
+		} else if !errors.Is(err, picker.ErrInvalidModifierData) {
+			// @todo ignore for now modifier data errors to avoid introducing
+			// breaking changes but once the router is merged with core consider
+			// at least logging for dev purposes
+			return err
+		}
+	}
+
 	e.setResponseHeaderIfEmpty(headerContentType, "application/json")
 	e.Response.WriteHeader(status)
 
-	rawFields := e.Request.URL.Query().Get(jsonFieldsParam)
-
-	// error response or no fields to pick
-	if rawFields == "" || status < 200 || status > 299 {
-		return json.MarshalWrite(e.Response, data, jsontext.AllowInvalidUTF8(true))
-	}
-
-	// pick only the requested fields
-	modified, err := picker.Pick(data, rawFields)
-	if err != nil {
-		return err
-	}
-
-	return json.MarshalWrite(e.Response, modified, jsontext.AllowInvalidUTF8(true))
+	return json.MarshalWrite(e.Response, data, jsontext.AllowInvalidUTF8(true))
 }
 
 // XML writes an XML response.

tools/router/event_test.go

@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	validation "github.com/pocketbase/ozzo-validation/v4"
+	"github.com/pocketbase/pocketbase/tools/picker"
 	"github.com/pocketbase/pocketbase/tools/router"
 )
 
@@ -220,6 +221,8 @@ func TestEventRemoteIP(t *testing.T) {
 }
 
 func TestFindUploadedFiles(t *testing.T) {
+	t.Parallel()
+
 	scenarios := []struct {
 		filename        string
 		expectedPattern string
@@ -273,6 +276,8 @@ func TestFindUploadedFiles(t *testing.T) {
 }
 
 func TestFindUploadedFilesMissing(t *testing.T) {
+	t.Parallel()
+
 	body := new(bytes.Buffer)
 	mp := multipart.NewWriter(body)
 	mp.Close()
@@ -293,6 +298,8 @@ func TestFindUploadedFilesMissing(t *testing.T) {
 }
 
 func TestEventSetGet(t *testing.T) {
+	t.Parallel()
+
 	event := router.Event{}
 
 	// get before any set (ensures that doesn't panic)
@@ -324,6 +331,8 @@ func TestEventSetGet(t *testing.T) {
 }
 
 func TestEventSetAllGetAll(t *testing.T) {
+	t.Parallel()
+
 	data := map[string]any{
 		"a": 123,
 		"b": 456,
@@ -351,6 +360,8 @@ func TestEventSetAllGetAll(t *testing.T) {
 }
 
 func TestEventString(t *testing.T) {
+	t.Parallel()
+
 	scenarios := []testResponseWriteScenario[string]{
 		{
 			name:            "no explicit content-type",
@@ -380,6 +391,8 @@ func TestEventString(t *testing.T) {
 }
 
 func TestEventHTML(t *testing.T) {
+	t.Parallel()
+
 	scenarios := []testResponseWriteScenario[string]{
 		{
 			name:            "no explicit content-type",
@@ -409,6 +422,8 @@ func TestEventHTML(t *testing.T) {
 }
 
 func TestEventJSON(t *testing.T) {
+	t.Parallel()
+
 	body := map[string]any{
 		"a": 123,
 		"b": true,
@@ -456,7 +471,93 @@ func TestEventJSON(t *testing.T) {
 	}
 }
 
+func TestEventJSONPickError(t *testing.T) {
+	t.Parallel()
+
+	req, err := http.NewRequest(http.MethodGet, "/?fields=a:excerpt(-1)", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	rec := httptest.NewRecorder()
+
+	event := &router.Event{
+		Request:  req,
+		Response: &router.ResponseWriter{ResponseWriter: rec},
+	}
+
+	err = event.JSON(200, map[string]any{"a": "test"})
+	if err == nil {
+		t.Fatal("Expected JSON to return modifier args err, got nil")
+	}
+
+	// ensure that no explicit status code was written yet by attempting to write one
+	// (should do nothing if it was already written)
+	rec.WriteHeader(567)
+
+	result := rec.Result()
+	defer result.Body.Close()
+
+	if result.StatusCode != 567 {
+		t.Fatalf("Expected custom status code, got %d", result.StatusCode)
+	}
+}
+
+type brokenModifier struct {
+}
+
+func (m *brokenModifier) Modify(val any) (any, error) {
+	return nil, errors.New("test_error")
+}
+
+func TestEventJSONIgnoredError(t *testing.T) {
+	t.Parallel()
+
+	picker.Modifiers["broken_ignore"] = func(args ...string) (picker.Modifier, error) {
+		return &brokenModifier{}, nil
+	}
+
+	req, err := http.NewRequest(http.MethodGet, "/?fields=a:broken_ignore", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	rec := httptest.NewRecorder()
+
+	event := &router.Event{
+		Request:  req,
+		Response: &router.ResponseWriter{ResponseWriter: rec},
+	}
+
+	err = event.JSON(200, map[string]any{"a": "test"})
+	if err != nil {
+		t.Fatalf("Expected nil JSON result, got error %v", err)
+	}
+
+	// (should do nothing if it was already written)
+	rec.WriteHeader(567)
+
+	result := rec.Result()
+	defer result.Body.Close()
+
+	rawBody, err := io.ReadAll(result.Body)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if result.StatusCode != http.StatusOK {
+		t.Fatalf("Expected %d status code, got %d", http.StatusOK, result.StatusCode)
+	}
+
+	expected := `{"a":"test"}`
+	if string(rawBody) != expected {
+		t.Fatalf("Expected json\n%s\ngot\n%s", expected, rawBody)
+	}
+}
+
 func TestEventXML(t *testing.T) {
+	t.Parallel()
+
 	scenarios := []testResponseWriteScenario[string]{
 		{
 			name:            "no explicit content-type",
@@ -486,6 +587,8 @@ func TestEventXML(t *testing.T) {
 }
 
 func TestEventStream(t *testing.T) {
+	t.Parallel()
+
 	scenarios := []testResponseWriteScenario[string]{
 		{
 			name:            "stream",
@@ -506,6 +609,8 @@ func TestEventStream(t *testing.T) {
 }
 
 func TestEventBlob(t *testing.T) {
+	t.Parallel()
+
 	scenarios := []testResponseWriteScenario[[]byte]{
 		{
 			name:            "blob",
@@ -526,6 +631,8 @@ func TestEventBlob(t *testing.T) {
 }
 
 func TestEventNoContent(t *testing.T) {
+	t.Parallel()
+
 	s := testResponseWriteScenario[any]{
 		name:            "no content",
 		status:          234,
@@ -542,6 +649,8 @@ func TestEventNoContent(t *testing.T) {
 }
 
 func TestEventFlush(t *testing.T) {
+	t.Parallel()
+
 	rec := httptest.NewRecorder()
 
 	event := &router.Event{
@@ -556,6 +665,8 @@ func TestEventFlush(t *testing.T) {
 }
 
 func TestEventRedirect(t *testing.T) {
+	t.Parallel()
+
 	scenarios := []testResponseWriteScenario[any]{
 		{
 			name:           "non-30x status",
@@ -580,6 +691,8 @@ func TestEventRedirect(t *testing.T) {
 }
 
 func TestEventFileFS(t *testing.T) {
+	t.Parallel()
+
 	// stub test files
 	// ---
 	dir, err := os.MkdirTemp("", "EventFileFS")
@@ -676,6 +789,8 @@ func TestEventFileFS(t *testing.T) {
 }
 
 func TestEventError(t *testing.T) {
+	t.Parallel()
+
 	err := new(router.Event).Error(123, "message_test", map[string]any{"a": validation.Required, "b": "test"})
 
 	result, _ := json.Marshal(err, json.Deterministic(true))
@@ -687,6 +802,8 @@ func TestEventError(t *testing.T) {
 }
 
 func TestEventBadRequestError(t *testing.T) {
+	t.Parallel()
+
 	err := new(router.Event).BadRequestError("message_test", map[string]any{"a": validation.Required, "b": "test"})
 
 	result, _ := json.Marshal(err, json.Deterministic(true))
@@ -698,6 +815,8 @@ func TestEventBadRequestError(t *testing.T) {
 }
 
 func TestEventNotFoundError(t *testing.T) {
+	t.Parallel()
+
 	err := new(router.Event).NotFoundError("message_test", map[string]any{"a": validation.Required, "b": "test"})
 
 	result, _ := json.Marshal(err, json.Deterministic(true))
@@ -709,6 +828,8 @@ func TestEventNotFoundError(t *testing.T) {
 }
 
 func TestEventForbiddenError(t *testing.T) {
+	t.Parallel()
+
 	err := new(router.Event).ForbiddenError("message_test", map[string]any{"a": validation.Required, "b": "test"})
 
 	result, _ := json.Marshal(err, json.Deterministic(true))
@@ -720,6 +841,8 @@ func TestEventForbiddenError(t *testing.T) {
 }
 
 func TestEventUnauthorizedError(t *testing.T) {
+	t.Parallel()
+
 	err := new(router.Event).UnauthorizedError("message_test", map[string]any{"a": validation.Required, "b": "test"})
 
 	result, _ := json.Marshal(err, json.Deterministic(true))
@@ -731,6 +854,8 @@ func TestEventUnauthorizedError(t *testing.T) {
 }
 
 func TestEventTooManyRequestsError(t *testing.T) {
+	t.Parallel()
+
 	err := new(router.Event).TooManyRequestsError("message_test", map[string]any{"a": validation.Required, "b": "test"})
 
 	result, _ := json.Marshal(err, json.Deterministic(true))
@@ -742,6 +867,8 @@ func TestEventTooManyRequestsError(t *testing.T) {
 }
 
 func TestEventInternalServerError(t *testing.T) {
+	t.Parallel()
+
 	err := new(router.Event).InternalServerError("message_test", map[string]any{"a": validation.Required, "b": "test"})
 
 	result, _ := json.Marshal(err, json.Deterministic(true))
@@ -753,6 +880,8 @@ func TestEventInternalServerError(t *testing.T) {
 }
 
 func TestEventBindBody(t *testing.T) {
+	t.Parallel()
+
 	type testDstStruct struct {
 		A int    `json:"a" xml:"a" form:"a"`
 		B int    `json:"b" xml:"b" form:"b"`