Skip to content

Commit 55d6f5a

Browse files
xernobylxernobyl
committed
refactor: make tenant settings PATCH an RFC 7396 merge patch
PATCH now follows JSON Merge Patch (RFC 7396): a member with a value sets that setting, an explicit JSON null removes the key, and an omitted member is left unchanged. Previously an empty string cleared a field and there was no way to remove a key (the JSONB `||` merge could only add or overwrite). - models: add a small generic Optional[T] that distinguishes absent vs null vs value when decoding a merge-patch body; PatchTenantSettingsRequest uses it instead of *string. - service: translate the typed request into keys-to-set and keys-to-remove; reject an explicit empty string (clear via null), keeping the value contract "a valid BCP-47 locale, or null to remove". - repository: Patch applies (settings || set) - removeKeys::text[], so null members delete keys while unmentioned keys survive; writeSettings takes the extra bind. Upsert (full replace) is unchanged. - openapi: declare application/merge-patch+json (application/json still accepted), make target_language nullable, document the null-removes rule. - tests: Optional tri-state unit test; service set/remove/empty-rejected cases; integration PatchNullRemovesField (key deleted, siblings kept) and PatchEmptyStringRejected. ENG-1254
1 parent a10995d commit 55d6f5a

9 files changed

Lines changed: 335 additions & 120 deletions

File tree

‎internal/api/handlers/tenant_settings_handler.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,9 @@ import (
1111
"github.com/formbricks/hub/internal/models"
1212
)
1313

14-
// maxSettingsRequestBodyBytes caps the PUT request body. A settings payload is
15-
// tiny (a short locale string), so 8 KiB is generous; larger bodies are rejected
16-
// with 413 before being read into memory.
14+
// maxSettingsRequestBodyBytes caps the PUT and PATCH request bodies. A settings
15+
// payload is tiny (a short locale string), so 8 KiB is generous; larger bodies are
16+
// rejected with 413 before being read into memory.
1717
const maxSettingsRequestBodyBytes = 8 << 10
1818

1919
// TenantSettingsService defines the business logic for tenant-scoped settings.
@@ -73,10 +73,11 @@ func (h *TenantSettingsHandler) Update(w http.ResponseWriter, r *http.Request) {
7373
response.RespondJSON(w, http.StatusOK, settings)
7474
}
7575

76-
// Patch handles PATCH /v1/tenants/{tenant_id}/settings. It applies a partial
77-
// update: only the fields present in the body change; omitted fields are left
78-
// untouched (send an empty string to clear a field). tenant_id is taken from the
79-
// path, so a request can only ever modify its own tenant's settings.
76+
// Patch handles PATCH /v1/tenants/{tenant_id}/settings. The body is an RFC 7396
77+
// JSON Merge Patch (Content-Type application/merge-patch+json): a member with a
78+
// value sets that setting, a member with JSON null removes it, and an omitted
79+
// member is left unchanged. tenant_id is taken from the path, so a request can
80+
// only ever modify its own tenant's settings.
8081
func (h *TenantSettingsHandler) Patch(w http.ResponseWriter, r *http.Request) {
8182
tenantID := r.PathValue("tenant_id")
8283

‎internal/models/optional.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
package models
2+
3+
import (
4+
"bytes"
5+
"encoding/json"
6+
"fmt"
7+
)
8+
9+
// jsonNull is the JSON null literal, used to detect an explicit null member in a
10+
// merge-patch body.
11+
var jsonNull = []byte("null")
12+
13+
// Optional captures the three states a member can take in an RFC 7396 (JSON
14+
// Merge Patch) request body, which a plain pointer cannot distinguish:
15+
//
16+
// - absent — Present is false (the member was omitted; leave it unchanged)
17+
// - explicit null — Present is true, Value is nil (remove the member)
18+
// - a value — Present is true, Value is non-nil (set the member)
19+
//
20+
// Its zero value is the "absent" state, so a struct field of this type is correct
21+
// until JSON decoding marks it present. It is a decode-only input helper.
22+
type Optional[T any] struct {
23+
Present bool
24+
Value *T
25+
}
26+
27+
// UnmarshalJSON records that the member was present. encoding/json only calls
28+
// this for members that actually appear in the object (including when the value
29+
// is null), so Present stays false for omitted members. A JSON null leaves Value
30+
// nil to signal removal; any other value is decoded into Value.
31+
func (o *Optional[T]) UnmarshalJSON(data []byte) error {
32+
o.Present = true
33+
34+
if bytes.Equal(bytes.TrimSpace(data), jsonNull) {
35+
o.Value = nil
36+
37+
return nil
38+
}
39+
40+
var v T
41+
if err := json.Unmarshal(data, &v); err != nil {
42+
return fmt.Errorf("decode optional value: %w", err)
43+
}
44+
45+
o.Value = &v
46+
47+
return nil
48+
}

‎internal/models/optional_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package models
2+
3+
import (
4+
"encoding/json"
5+
"testing"
6+
)
7+
8+
// TestOptionalUnmarshalJSONTriState pins the three RFC 7396 states Optional must
9+
// distinguish when decoded as a struct member.
10+
func TestOptionalUnmarshalJSONTriState(t *testing.T) {
11+
type doc struct {
12+
Field Optional[string] `json:"field"`
13+
}
14+
15+
t.Run("absent member stays not-present", func(t *testing.T) {
16+
var decoded doc
17+
if err := json.Unmarshal([]byte(`{}`), &decoded); err != nil {
18+
t.Fatalf("unmarshal: %v", err)
19+
}
20+
21+
if decoded.Field.Present || decoded.Field.Value != nil {
22+
t.Fatalf("absent: Present=%v Value=%v, want false/nil", decoded.Field.Present, decoded.Field.Value)
23+
}
24+
})
25+
26+
t.Run("explicit null is present with nil value", func(t *testing.T) {
27+
var decoded doc
28+
if err := json.Unmarshal([]byte(`{"field":null}`), &decoded); err != nil {
29+
t.Fatalf("unmarshal: %v", err)
30+
}
31+
32+
if !decoded.Field.Present || decoded.Field.Value != nil {
33+
t.Fatalf("null: Present=%v Value=%v, want true/nil", decoded.Field.Present, decoded.Field.Value)
34+
}
35+
})
36+
37+
t.Run("value is present with the decoded value", func(t *testing.T) {
38+
var decoded doc
39+
if err := json.Unmarshal([]byte(`{"field":"de-DE"}`), &decoded); err != nil {
40+
t.Fatalf("unmarshal: %v", err)
41+
}
42+
43+
if !decoded.Field.Present || decoded.Field.Value == nil || *decoded.Field.Value != "de-DE" {
44+
t.Fatalf("value: Present=%v Value=%v, want true/de-DE", decoded.Field.Present, decoded.Field.Value)
45+
}
46+
})
47+
48+
t.Run("wrong type errors", func(t *testing.T) {
49+
var decoded doc
50+
if err := json.Unmarshal([]byte(`{"field":123}`), &decoded); err == nil {
51+
t.Fatal("expected an error decoding a number into Optional[string]")
52+
}
53+
})
54+
}

‎internal/models/tenant_settings.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,11 @@ type UpdateTenantSettingsRequest struct {
2828
}
2929

3030
// PatchTenantSettingsRequest is the body for PATCH /v1/tenants/{tenant_id}/settings.
31-
// Unlike PUT (full replace), PATCH is a partial update: only the fields present in
32-
// the body change; omitted fields are left untouched. Pointer fields distinguish
33-
// "absent" (nil — leave unchanged) from "present" (set, including an empty string,
34-
// which clears the field). tenant_id comes from the path, never the body. The
35-
// omitempty tags keep absent fields out of the JSONB merge patch.
31+
// PATCH is an RFC 7396 JSON Merge Patch, not a full replace: for each member, a
32+
// concrete value sets that setting, an explicit JSON null removes it, and an
33+
// omitted member is left unchanged. Optional captures those three states (a plain
34+
// pointer cannot tell absent from null). Values are validated and normalized by
35+
// the service. tenant_id comes from the path, never the body.
3636
type PatchTenantSettingsRequest struct {
37-
TargetLanguage *string `json:"target_language,omitempty" validate:"omitempty,no_null_bytes,max=35"`
37+
TargetLanguage Optional[string] `json:"target_language"`
3838
}

‎internal/repository/tenant_settings_repository.go‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -67,42 +67,55 @@ func (r *TenantSettingsRepository) Upsert(
6767
return r.writeSettings(ctx, tenantID, query, payload)
6868
}
6969

70-
// Patch merges a partial settings object into the tenant's existing settings: keys
71-
// present in patch overwrite, keys absent are left untouched (a top-level JSONB
72-
// merge via `||`). For a tenant with no row yet, the patch becomes the initial
73-
// settings. patch is marshaled with omitempty so only provided fields are sent.
70+
// Patch applies an RFC 7396 JSON Merge Patch to the tenant's settings: keys in
71+
// set are written (a top-level JSONB `||`), keys in removeKeys are deleted
72+
// (`- text[]`), and keys mentioned in neither are left untouched. For a tenant
73+
// with no row yet the set object becomes the initial settings. set and removeKeys
74+
// are disjoint, so the merge-then-remove order does not matter.
7475
func (r *TenantSettingsRepository) Patch(
75-
ctx context.Context, tenantID string, patch *models.PatchTenantSettingsRequest,
76+
ctx context.Context, tenantID string, set models.EnrichmentSettings, removeKeys []string,
7677
) (*models.TenantSettings, error) {
77-
payload, err := json.Marshal(patch)
78+
payload, err := json.Marshal(set)
7879
if err != nil {
7980
return nil, fmt.Errorf("marshal tenant settings patch: %w", err)
8081
}
8182

82-
// `||` merges top-level keys, so settings the patch does not mention survive.
83+
// A nil slice binds as SQL NULL and `jsonb - NULL` is NULL (which would blank
84+
// the column); an empty, non-nil slice binds as an empty text[] and
85+
// `jsonb - '{}'` is a no-op. So never pass nil to the `- $3` removal.
86+
if removeKeys == nil {
87+
removeKeys = []string{}
88+
}
89+
90+
// `||` merges the provided keys (settings the patch does not mention survive);
91+
// `- $3` then deletes the keys the merge patch set to null.
8392
const query = `
8493
INSERT INTO tenant_settings (tenant_id, settings)
8594
VALUES ($1, $2)
8695
ON CONFLICT (tenant_id) DO UPDATE
87-
SET settings = tenant_settings.settings || EXCLUDED.settings, updated_at = NOW()
96+
SET settings = (tenant_settings.settings || EXCLUDED.settings) - $3::text[], updated_at = NOW()
8897
RETURNING tenant_id, settings`
8998

90-
return r.writeSettings(ctx, tenantID, query, payload)
99+
return r.writeSettings(ctx, tenantID, query, payload, removeKeys)
91100
}
92101

93102
// writeSettings runs a settings write (an INSERT ... ON CONFLICT ... RETURNING with
94-
// $1 = tenant_id and $2 = the settings JSONB) inside the shared tenant write lock
95-
// for that tenant. The lock serializes the write against a tenant data purge (a
96-
// purge in progress yields a retryable *huberrors.TenantWriteConflictError before
97-
// any write happens), and the tenant-scoped statement only ever touches this
98-
// tenant's row, so a write can never affect another tenant's settings.
103+
// $1 = tenant_id, $2 = the settings JSONB, and any extraArgs as $3, $4, …) inside
104+
// the shared tenant write lock for that tenant. The lock serializes the write
105+
// against a tenant data purge (a purge in progress yields a retryable
106+
// *huberrors.TenantWriteConflictError before any write happens), and the
107+
// tenant-scoped statement only ever touches this tenant's row, so a write can
108+
// never affect another tenant's settings.
99109
func (r *TenantSettingsRepository) writeSettings(
100-
ctx context.Context, tenantID, query string, payload []byte,
110+
ctx context.Context, tenantID, query string, payload []byte, extraArgs ...any,
101111
) (*models.TenantSettings, error) {
102112
var result *models.TenantSettings
103113

114+
// $1 = tenant_id, $2 = settings JSONB; any extraArgs follow as $3, $4, …
115+
args := append([]any{tenantID, json.RawMessage(payload)}, extraArgs...)
116+
104117
err := withTenantWritePoolTx(ctx, r.db, []string{tenantID}, func(dbTx tenantWriteTx) error {
105-
stored, scanErr := scanTenantSettings(dbTx.QueryRow(ctx, query, tenantID, json.RawMessage(payload)))
118+
stored, scanErr := scanTenantSettings(dbTx.QueryRow(ctx, query, args...))
106119
if scanErr != nil {
107120
return fmt.Errorf("write tenant settings: %w", scanErr)
108121
}

‎internal/service/tenant_settings_service.go‎

Lines changed: 49 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,18 @@ import (
1515
type TenantSettingsRepository interface {
1616
Get(ctx context.Context, tenantID string) (*models.TenantSettings, bool, error)
1717
Upsert(ctx context.Context, tenantID string, settings models.EnrichmentSettings) (*models.TenantSettings, error)
18-
Patch(ctx context.Context, tenantID string, patch *models.PatchTenantSettingsRequest) (*models.TenantSettings, error)
18+
// Patch merges set into the tenant's settings and removes removeKeys (an RFC
19+
// 7396 merge patch: set + delete); the two are disjoint.
20+
Patch(
21+
ctx context.Context, tenantID string, set models.EnrichmentSettings, removeKeys []string,
22+
) (*models.TenantSettings, error)
1923
}
2024

25+
// settingKeyTargetLanguage is the JSONB key for the target language. It must match
26+
// the json tag on models.EnrichmentSettings.TargetLanguage; it is the key removed
27+
// when a PATCH sends target_language as null.
28+
const settingKeyTargetLanguage = "target_language"
29+
2130
// TenantSettingsService reads and writes tenant-scoped enrichment settings. It is
2231
// the accessor enrichment workflows will use to resolve a tenant's configuration.
2332
type TenantSettingsService struct {
@@ -77,10 +86,12 @@ func (s *TenantSettingsService) UpdateSettings(
7786
return settings, nil
7887
}
7988

80-
// PatchSettings applies a partial update: only the fields present in req change;
81-
// omitted fields (nil pointers) are left untouched. Provided fields are validated
82-
// and normalized before the merge; an empty target_language clears it. The
83-
// tenant_id comes from the request path and scopes the write to that tenant alone.
89+
// PatchSettings applies an RFC 7396 JSON Merge Patch: a member present with a
90+
// value sets that setting (validated and normalized), a member present with JSON
91+
// null removes it, and an omitted member is left unchanged. It translates the
92+
// typed request into the keys to set and the keys to remove, which are disjoint.
93+
// The tenant_id comes from the request path and scopes the write to that tenant
94+
// alone.
8495
func (s *TenantSettingsService) PatchSettings(
8596
ctx context.Context, tenantID string, req *models.PatchTenantSettingsRequest,
8697
) (*models.TenantSettings, error) {
@@ -89,18 +100,26 @@ func (s *TenantSettingsService) PatchSettings(
89100
return nil, err
90101
}
91102

92-
var patch models.PatchTenantSettingsRequest
93-
94-
if req.TargetLanguage != nil {
95-
normalized, normErr := normalizeTargetLanguage(*req.TargetLanguage)
96-
if normErr != nil {
97-
return nil, normErr
103+
var (
104+
set models.EnrichmentSettings
105+
removeKeys []string
106+
)
107+
108+
if req.TargetLanguage.Present {
109+
if req.TargetLanguage.Value == nil {
110+
// Explicit null: remove the setting (RFC 7396).
111+
removeKeys = append(removeKeys, settingKeyTargetLanguage)
112+
} else {
113+
normalized, normErr := normalizeProvidedTargetLanguage(*req.TargetLanguage.Value)
114+
if normErr != nil {
115+
return nil, normErr
116+
}
117+
118+
set.TargetLanguage = normalized
98119
}
99-
100-
patch.TargetLanguage = &normalized
101120
}
102121

103-
settings, err := s.repo.Patch(ctx, normalizedTenantID, &patch)
122+
settings, err := s.repo.Patch(ctx, normalizedTenantID, set, removeKeys)
104123
if err != nil {
105124
return nil, fmt.Errorf("patch tenant settings: %w", err)
106125
}
@@ -110,7 +129,8 @@ func (s *TenantSettingsService) PatchSettings(
110129

111130
// normalizeTargetLanguage trims and canonicalizes a BCP-47 locale (e.g. "en-us"
112131
// -> "en-US"). An empty value is allowed and normalizes to "" (target language
113-
// not configured). A malformed locale is rejected with a validation error.
132+
// not configured) — this is the PUT full-replace semantics, where omitting the
133+
// field clears it. A malformed locale is rejected with a validation error.
114134
func normalizeTargetLanguage(raw string) (string, error) {
115135
trimmed := strings.TrimSpace(raw)
116136
if trimmed == "" {
@@ -125,3 +145,17 @@ func normalizeTargetLanguage(raw string) (string, error) {
125145

126146
return tag.String(), nil
127147
}
148+
149+
// normalizeProvidedTargetLanguage validates a non-null target_language supplied in
150+
// a PATCH. Unlike PUT, an empty value is rejected: under RFC 7396 the way to
151+
// remove a setting is JSON null, so an explicit "" is a malformed locale rather
152+
// than a clear.
153+
func normalizeProvidedTargetLanguage(raw string) (string, error) {
154+
if strings.TrimSpace(raw) == "" {
155+
return "", huberrors.NewValidationError(
156+
"target_language",
157+
"target_language must be a valid BCP-47 locale (e.g. en-US); send null to remove it")
158+
}
159+
160+
return normalizeTargetLanguage(raw)
161+
}

0 commit comments

Comments
 (0)