www/nginx: fix orphaned sni_hostname_upstream_map_item / ip_acl_item on edit

[muchachagrande]

Important notices

Before you submit a pull request, we ask you kindly to acknowledge the following:

• I have read the contributing guidelines at https://github.com/opnsense/plugins/blob/master/CONTRIBUTING.md
• I opened an issue first for non-trivial changes and linked it below.
• AI tools were used to create at least part of the code submitted herewith.

If AI was used, please disclose:

• Model used: Claude Sonnet 4.5/5 and ChatGPT 5.6 Luna
• Extent of AI involvement: Claude was used to read the plugin source and OPNsense core framework (BaseModel, ApiMutableModelControllerBase, Config) to trace the root cause of the bug, and to draft and iterate on the fix. ChatGPT was used for technical discussion, code review, and assistance in understanding the existing code. The changes were manually tested, and verified by the contributor before submission.

---

Describe the problem

regenerate_hostname_map() / regenerate_ipacl() in SettingsController.php deleted the previous child items sni_hostname_upstream_map_item / ip_acl_item) on every edit, but the parent's data field (a comma-separated list of child uuids) still referenced the old uuids at delete time. Because SettingsController uses $internalModelUseSafeDelete = true, the safe-delete check does a raw text search over the whole config.xml, finds this self-reference, and blocks the delete; the exception is silently caught, so the old items are left as permanent orphans. Editing the same map/ACL repeatedly accumulates orphans, and each orphan still references a real upstream server, which then can no longer be deleted ("in use").

Two related issues surfaced while fixing this:

• Saving a map/ACL with all rows removed threw an unhandled
  ValidationException (raw stack trace) instead of a normal validation
  message.
• If a save failed validation after old items had already been
  cleared/deleted, cancelling in the GUI left the configuration
  partially modified, since the failed attempt had already been
  persisted to disk.

---

Describe the proposed solution

New items are now built and fully validated in memory before anything is written to disk. If the submitted set is empty, or any row fails validation, the request returns a normal {result: "failed", validations: {...}} response without touching the existing configuration. Only once the new items are known valid does the function clear the parent's data field, persist that intermediate state to disk (required, since delBase()'s Config::lock() reloads from disk on its first call in the request and would otherwise discard an in-memory-only change), and then delete the old child items. Same fix applied to regenerate_ipacl().

---

Related issue

Fixes #5650

[kulikov-a]

Hi!

Agreed on the root cause. But I think the proposed changes can still leave orphan rows.

serializeToConfig and Config::save() write an intermediate config (parent data cleared, new child items already on disk) before setBase() runs. If setBase() then fails, those new items will be unreferenced.

delete_uuids() is still used, and it swallows all exceptions (including a failed UUID delete). After the intermediate save, safe-delete should succeed on the happy path — but what if something still blocks the delete?

regenerate_hostname_map and regenerate_ipacl are almost the same. Maybe it makes sense drop delete_uuids() and factor the shared work into something like regenerate_map_items(): build and validate the new rows in memory, remove the old ones with ArrayField::del() (not delBase()), and leave the only persist to addBase()/setBase(). The two current regenerate_* functions can then stay thin wrappers.

A model migration is also needed to purge orphans already left by #5650 (and a model version bump)?

[muchachagrande]

Hi @kulikov-a. Thanks for the detailed review. I agree that these points are valid, particularly the possibility of leaving orphan rows if the intermediate save succeeds but setBase() subsequently fails. I’ll investigate the proposed approach, including factoring the shared regeneration logic, avoiding delete_uuids(), and adding a model migration to clean up existing orphan rows. I'll push a follow-up commit once I've tested it.

[muchachagrande]

Pushed a follow-up commit addressing all three points:

• New rows are now built and validated in memory first; old rows are
  removed with ArrayField::del() instead of delBase(), so there's no
  more intermediate persisted state.
• regenerate_hostname_map()/regenerate_ipacl() are now thin wrappers
  around a shared regenerate_map_items().
• Added a model migration (version bump to 1.35.3) that purges any
  pre-existing orphaned sni_hostname_upstream_map_item / ip_acl_item rows.

Tested: repeated edits on maps/ACLs with no orphans left, ran the
migration against a config with orphans created by hand, confirmed they
get purged and referenced items are untouched.

[kulikov-a]

Hi!

Thanks, this looks good to me )

One optional note: in the migration it may be safer not to del() while iterating the live collection. Can collect the orphan uuids first, then delete after the loop (same style as in https://github.com/opnsense/core/blob/master/src/opnsense/mvc/app/models/OPNsense/IDS/Migrations/M1_0_6.php)

[muchachagrande]

I've got your point. It is safer to iterate the collection in a first step and mutate it in a second one.
Updated purgeOrphans() to collect the orphan uuids first and only call del() in a second pass after the iteration is done.
Thank you.