fix: PATCH preserves uci options the resource does not model (PUT still replaces)

CHANGELOG.md

@@ -23,6 +23,10 @@ Both surfaces enumerate the same internal `KNOWN_PATHS` const; the accessor (rat
 
 The OpenAPI vendor-extension option from the issue thread is deferred until a code-generated client surfaces with a concrete need; premature spec annotations without consumer evidence is what 2.2.2 / 2.2.3 corrected.
 
+### Fixed
+
+- PATCH no longer deletes uci options the resource does not model. Previously every write verb shared one `diff_apply` that deleted any existing uci option not re-emitted by `toUci`, so a partial PATCH (e.g. changing `unbound` verbosity, or any field on `firewall.rules`) silently wiped hand-set or stock options the curated model omits (`dns64_prefix`, `icmp_type`, `synflood_protect`, cert `key_type`/`ec_curve`, `ttylogin`, and many more). PATCH now preserves them (RFC 7396 merge-patch: only options inside the resource's own modeled footprint are touched). PUT keeps full-replace semantics: it normalizes the section to the modeled set, so unmodeled options are intentionally dropped on a PUT. An audit of all curated resources against their stock OpenWrt configs found this affected 13 resources. The companion `reject_422` audit (validation stricter than the platform) came back clean for the optional packages; the only residual reject cases are in `wireless` (stock empty `country` / `encryption='owe'` on 6 GHz) and `network.devices` (type-less macaddr-override sections), tracked separately.
+
 ### Internal
 
 - New integration test `tests/integration/44_stock_config_test.sh` round-trips every curated CRUD resource and singleton whose package ships in the bare OpenWrt 25.12.4 image (firewall, network, dhcp, dropbear, system): GET the section, adopt if unmanaged, PUT-self (or PATCH-self for singletons), then GET again and assert the persistable shape did not drift. A 422 or a diff surfaces a regression where uapi rejects (or silently mutates) what the platform itself ships. Resources from optional packages (snmpd, lldpd, vnstat, mwan3, etc.) are deferred to a follow-up that wires their install at VM-setup time. First run forced four schema relaxations to bring API rules in line with the platform: `igmp` accepted by `firewall.rules`/`firewall.redirects`; `*`/`any` wildcards accepted by `firewall.rules` zone refs; `dhcp.servers` no longer requires the referenced network interface to exist (stock ships `dhcp.wan` against an absent `network.wan` on x86); `is_valid_cidr_any` accepts IPv6 CIDR (used by `mwan3.rules` once mwan3 coverage lands).

docs/architecture.md

@@ -176,6 +176,28 @@ inherits fromUci's string-form view of integer-typed fields, so type-checking
 the merge would falsely 422 on integer-untouched patches). The full merged
 body still goes through `resource.validate()` for cross-field logic.
 
+### Unmodeled uci options: PUT replaces, PATCH preserves
+
+A resource module curates a subset of its uci section's options; `toUci` emits
+only those. The write path treats options outside that modeled set
+differently per verb:
+
+- **PUT (replace)** normalizes the section to the modeled set. `diff_apply`
+  iterates the raw existing section and deletes every option not re-emitted by
+  `toUci`, including ones the resource does not model. PUT means "this body is
+  the whole resource," so uapi owns the section.
+- **PATCH (partial update)** preserves unmodeled options. `diff_apply_patch`
+  computes the modeled footprint as `toUci(fromUci(existing))` and only deletes
+  within it (a modeled field the patch cleared). Raw uci options outside the
+  footprint (which `toUci` cannot emit) are never enumerated, so a PATCH that
+  touches one field leaves a hand-set or stock option like firewall `icmp_type`
+  / unbound `dns64_prefix` untouched. This matches RFC 7396 merge-patch and
+  avoids the silent data loss a naive delete-everything-absent would cause.
+
+A consequence: the GET-then-PATCH-self round trip is lossless even for options
+uapi does not model, but GET-then-PUT-self is not (PUT discards them). Tooling
+that wants to preserve unmodeled state must use PATCH.
+
 Field errors are deduped by `(field, code)`. Schema errors win over
 validator errors for the same (field, code) tuple. The 422 body carries the
 full set; clients fix everything in one round trip.

src/lib/handler.uc

@@ -364,10 +364,10 @@ function translate_tx(ctx, result) {
 	                    sprintf("transaction returned unknown kind %J", result.kind));
 }
 
-// Replace any uci options not in new_opts; set the rest. The two-loop shape
-// is required because uci_set on an existing key updates; we have to
-// explicitly delete the keys the new body omitted. The leading-dot keys
-// (.name/.type/.anonymous) are pseudo and must not be touched.
+// PUT/replace semantics: the body is the whole resource, so any uci option
+// not in new_opts is removed. Iterates the RAW existing section, so options
+// the resource does not model are normalized away too (uapi owns the section
+// on replace). The leading-dot keys (.name/.type/.anonymous) are pseudo.
 function diff_apply(c, p, id, existing, new_opts) {
 	for (let k in existing) {
 		if (substr(k, 0, 1) == ".") continue;
@@ -377,6 +377,21 @@ function diff_apply(c, p, id, existing, new_opts) {
 	for (let k in new_opts) c.uci_set(p, id, k, new_opts[k]);
 }
 
+// PATCH semantics: a partial update must not wipe options the resource does
+// not model. old_opts is toUci(fromUci(existing)) -- exactly the modeled keys
+// the section currently carries. We only delete within that footprint (a key
+// the patch cleared); raw uci options outside it (toUci cannot emit them) are
+// left untouched. Without this, a PATCH that touches one field would delete
+// every stock/operator option the resource happens not to model.
+function diff_apply_patch(c, p, id, old_opts, new_opts) {
+	for (let k in old_opts) {
+		if (substr(k, 0, 1) == ".") continue;
+		if (exists(new_opts, k)) continue;
+		c.uci_delete(p, id, k);
+	}
+	for (let k in new_opts) c.uci_set(p, id, k, new_opts[k]);
+}
+
 // Reduce a PATCH body against existing state to a post-image plus a schema-
 // validation target. JSON Patch (RFC 6902) synthesises the full post-image,
 // so we schema-check that; merge-patch (RFC 7396) is partial, so we schema-
@@ -587,7 +602,7 @@ function make(resource, opts) {
 					return { ok: false, kind: "validation", errors: uf_errs };
 
 				let new_opts = resource.toUci(r.merged);
-				diff_apply(c, p, id, existing, new_opts);
+				diff_apply_patch(c, p, id, resource.toUci(existing_view), new_opts);
 				let view = { ...new_opts };
 				view['.name'] = id;
 				view['.anonymous'] = false;
@@ -748,7 +763,7 @@ function make_singleton(resource, opts) {
 					return { ok: false, kind: "validation", errors: errs };
 
 				let new_opts = resource.toUci(r.merged);
-				diff_apply(c, p, id, existing, new_opts);
+				diff_apply_patch(c, p, id, resource.toUci(existing_view), new_opts);
 				let view = { ...new_opts };
 				view['.name'] = id;
 				view['.anonymous'] = !!existing['.anonymous'];

tests/integration/44_stock_config_test.sh

@@ -147,4 +147,25 @@ for s in $SINGLETONS; do
 	echo "  ok: $s"
 done
 
+echo "=== PATCH preserves uci options uapi does not model (RFC-hybrid) ==="
+# Regression guard for the diff_apply silent-drop class: a partial PATCH must
+# not delete options the resource doesn't model. `localservice` is a real
+# dnsmasq option uapi does not curate; set it, PATCH a modeled field, and
+# confirm it survives. The drift check above can't catch this (fromUci never
+# surfaces unmodeled keys), so it needs a direct uci read.
+$SSH "uci set dhcp.@dnsmasq[0].localservice='1'; uci commit dhcp"
+patch_code=$(curl -sS -H "$ADMIN" -H 'Content-Type: application/json' \
+	-o /dev/null -w '%{http_code}' -X PATCH "$URL/dhcp/dnsmasq" -d '{"domain":"uapitest"}')
+[ "$patch_code" = "200" ] || fail "PATCH /dhcp/dnsmasq returned $patch_code"
+# `uci -q get` exits non-zero when the key is absent; `|| true` keeps the
+# assignment from tripping `set -e` so the explicit check below reports clearly.
+preserved=$($SSH "uci -q get dhcp.@dnsmasq[0].localservice" 2>/dev/null || true)
+[ "$preserved" = "1" ] || fail "PATCH wiped unmodeled uci option localservice (silent_drop regression): got '$preserved'"
+echo "  ok: unmodeled option survived PATCH"
+
+# Note: the PUT-side counterpart (full replace DOES drop unmodeled options) is
+# covered by the unit test handler_test.uc "normalizes away unmodeled uci
+# options" and is already exercised by the round-trip loop above; no separate
+# integration assertion here.
+
 echo "stock-config round-trip ok"

tests/unit/handler_test.uc

@@ -303,6 +303,18 @@ t.describe('handler.replace', () => {
 		t.assert_deep_equal(r.body.match.dest_port, []);
 		t.assert_deep_equal(r.body.match.proto, []);
 	});
+
+	t.it('normalizes away unmodeled uci options (PUT = full replace, uapi owns the section)', () => {
+		// Counterpart to the PATCH-preserve test: PUT is a whole-resource
+		// replace, so an option uapi does not model is intentionally dropped.
+		let c = with_existing();
+		c._state.uci.firewall.r_existing.icmp_type = ['echo-request'];
+		let r = rules.replace(c, ctx(), 'r_existing', {
+			target: 'DROP', match: { src_zone: 'wan' },
+		});
+		t.assert_equal(r.status, 200);
+		t.assert_equal(c._state.uci.firewall.r_existing.icmp_type, null);
+	});
 });
 
 t.describe('handler.patch', () => {
@@ -349,6 +361,26 @@ t.describe('handler.patch', () => {
 		t.assert_equal(r.body.id, 'r_existing');
 		t.assert_equal(r.body.target, 'REJECT');
 	});
+
+	t.it('preserves uci options the resource does not model (PATCH is partial)', () => {
+		// icmp_type and limit are real stock firewall-rule options uapi does
+		// not model. A partial PATCH must not delete them (RFC-7396 merge).
+		let c = with_existing();
+		c._state.uci.firewall.r_existing.icmp_type = ['echo-request'];
+		c._state.uci.firewall.r_existing.limit = '1000/sec';
+		let r = rules.patch(c, ctx(), 'r_existing', { target: 'REJECT' });
+		t.assert_equal(r.status, 200);
+		t.assert_deep_equal(c._state.uci.firewall.r_existing.icmp_type, ['echo-request']);
+		t.assert_equal(c._state.uci.firewall.r_existing.limit, '1000/sec');
+	});
+
+	t.it('still deletes a modeled field the patch clears (footprint delete)', () => {
+		let c = with_existing();
+		let r = rules.patch(c, ctx(), 'r_existing', { match: { dest_port: [] } });
+		t.assert_equal(r.status, 200);
+		t.assert_deep_equal(r.body.match.dest_port, []);
+		t.assert_equal(c._state.uci.firewall.r_existing.dest_port, null);
+	});
 });
 
 t.describe('handler.adopt', () => {