fix: PATCH preserves uci options the resource does not model (PUT still replaces)#9
Merged
Merged
Conversation
5abcb38 to
78668df
Compare
78668df to
c666b8f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A full-resource audit of every curated resource against its stock OpenWrt config surfaced a systemic, pre-existing data-loss bug: both PUT and PATCH shared one
diff_apply, which iterates the raw uci section and deletes every option not re-emitted bytoUci. Because each resource models only a curated subset of its section's options, a partial PATCH silently wiped any hand-set or stock option the model omits.The audit confirmed this on 13 resources, e.g.:
unbound.server: PATCH-ingverbositywipeddns64_prefix,iface_trig,iface_wan,validator,edns_size,rate_limit, +11 morefirewall.rules: any PATCH wipedicmp_type(breaks Allow-Ping / Allow-ICMPv6-*) andlimituhttpd.certs: wipedkey_type/ec_curve(cert silently regenerates EC→RSA)dhcp.dnsmasq: wipedlocalservice,rebind_localhost,localise_queries, ...system: wipedttylogin;firewall.defaults: wipedsynflood_protect;openvpn: wiped the custom-configconfigpath; etc.The existing stock-config round-trip test could not catch this: its drift check compares
fromUcioutput, which never surfaces unmodeled keys.Change
Split the write semantics by verb (RFC-hybrid):
diff_apply(raw iterate → full normalize). PUT means "this body is the whole resource," so unmodeled options are intentionally dropped.diff_apply_patchwhose delete scope is the resource's modeled footprint (toUci(fromUci(existing))). It only deletes a modeled field the patch cleared; raw options outside the footprint (whichtoUcicannot emit) are never touched. Matches RFC 7396 merge-patch.Applied to both the CRUD
make().patchand singletonmake_singleton().patchpaths.Tests
handler_test.uc): PATCH preserves an unmodeled option (icmp_type/limit); PATCH still deletes a modeled field it clears (footprint delete); PUT still normalizes away an unmodeled option.44_stock_config_test.sh): setlocalserviceon dnsmasq, PATCH a modeled field, assert it survives viauci get; seticmp_typeon a firewall rule, PUT the body back, assert it's dropped. The drift check can't see this class, so these read uci directly.Verification
make lint+make test(750 pass, +3) +make coverage(80.8% direct, 100% module) +make openapi-checkall green.merge_for_patch,create_if_missingsingletons, default-synthesizingfromUci, value-only PATCH,toUcithrowing) found no regression and reproduced the pre-fix wipe.Scope / follow-ups
reject_422audit (validation stricter than the platform) came back clean for the optional packages. Residual reject cases live only inwireless(stock emptycountry,encryption='owe'on 6 GHz) andnetwork.devices(type-less macaddr-override sections) — both in test-excluded resources; tracked separately, not in this PR.