Skip to content

Write NULL, not 0, into location's foreign-key column - #37

Merged
mastacontrola merged 1 commit into
mainfrom
claude/location-delete-hook-writes-null
Sep 2, 2026
Merged

Write NULL, not 0, into location's foreign-key column#37
mastacontrola merged 1 commit into
mainfrom
claude/location-delete-hook-writes-null

Conversation

@mastacontrola

Copy link
Copy Markdown
Member

The plugin half of FOGProject/fogproject#1655. Core's bug was that "Clear on all" on Image cleared hosts.hostImage to 0, and a foreign key refuses a 0 that names no row. Asked to sweep for the same shape across the plugins — it found one.

The bug

LocationDeleteMassItems::deletemassitems() cleaned up after a deleted storage node with

->update(['storagenodeID' => $arguments['itemIDs']], '', 0)

The third argument is the update data. FOGManagerController::update() takes an associative array of columns (or an array of them) and returns false for anything else — so the call had always been a silent no-op, and had it ever worked, 0 is precisely the value the constraint now refuses.

location.lStorageNodeID is nullable with ON DELETE SET NULL, so it writes null now. Kept rather than left to the constraint alone: a server between deploying code and running the schema updater has the column and not the foreign key, and there this is the only thing doing the work.

The storagegroup arm is removed

Same no-op, but there is nothing it could ever have done. Confirmed on a live install:

location.lStorageGroupID  int(11)  null=NO   FK: fk_location_lStorageGroupID  (RESTRICT)
location.lStorageNodeID   int(11)  null=YES  FK: fk_location_lStorageNodeID   (SET NULL)

lStorageGroupID is NOT NULL with RESTRICT, so a storage group any location still points at cannot be deleted at all — the database refusing it is the intended answer, and a location without a storage group is not a valid location. The line read as cleanup that made the delete safe; it never ran.

The gate

tests/fk-writes-are-null-not-zero.test.php covers all 23 plugin-owned columns that core's schema-constraints.php constrains — not just the three config ones where a sentinel was ever plausible. A junction column cannot hold a sentinel by construction, but a 0 in one is an orphan refused by the same constraint, and enumerating only the risky half is how the next one gets missed. (ldapUserGrant.lugTargetID and oidcUserGrant.ougTargetID are excluded: both are poly, so no constraint is expressible.)

It also refuses update($find, $op, <scalar>) anywhere in the tree — the shape that made this a no-op, and which no caller can have meant.

Two details that matter:

  • Comments are stripped with the tokenizer first. This test's own docblock quotes the broken call, and so does the comment replacing it; a raw-text scan flags both, which would make the only way to document a defect be to stop describing it.
  • The field-name mapping is asserted, not trusted. Renaming a field behind a constrained column fails here rather than quietly making every grep match nothing.

Mutations

Mutation Result
Restore update(..., '', 0) ✅ red — after the regex fix below
->set('locationID', 0) on a junction column ✅ red
Rename storagenodeID in Location.php ✅ red

The first version of the scalar-update check did not catch the real bug: \[[^\]]*\] cannot span the inner ] in $arguments['itemIDs'], so it skipped the exact line it existed for and passed green. Bounded on ; instead, it goes red. Worth stating plainly — that gate was fake until it was mutated.

Also checked, and clean

  • capone already migrated cImageID/cOSID off the 0 sentinel (CaponeManager.php:154, "stop spelling no reference as 0") and guards with isValid() before writing.
  • tasktypeedit / taskstateedit own no tables — they edit core's taskTypes/taskStates, which are parents only, so there is no FK column to write. Every reference to them is RESTRICT, and both delete through core's shared deleteModal/general-delete path, which already turns a refusal into a readable sentence via ConstraintViolation::explain().
  • No plugin writes 0 or '' to any of the 23 constrained columns.

sh tests/run-all.sh 17/17.

🤖 Generated with Claude Code

https://claude.ai/code/session_0166dqQEjAs9fhqUw5zCjvxM

The core half of this is FOGProject/fogproject#1655: "Clear on all" on
Image failed because the mass edit cleared `hosts`.`hostImage` to 0 and a
foreign key refuses a 0 that names no row. Asked to sweep for the same
shape here, and it found one.

LocationDeleteMassItems::deletemassitems() cleaned up after a deleted
storage node with

    ->update(['storagenodeID' => $arguments['itemIDs']], '', 0)

where the third argument is the UPDATE DATA. FOGManagerController::update()
takes an associative array of columns, or an array of them, and returns
false for anything else -- so the call had ALWAYS been a silent no-op, and
had it ever worked, 0 is precisely the value the constraint now refuses.
`location`.`lStorageNodeID` is nullable with ON DELETE SET NULL, so it
writes null now. Kept rather than left to the constraint alone: a server
between deploying code and running the schema updater has the column and
not the foreign key, and there this is the only thing doing the work.

The storagegroup arm above it was the same no-op and is removed, because
there is nothing it could ever have done. `lStorageGroupID` is NOT NULL
with a RESTRICT foreign key -- confirmed on a live install -- so a storage
group any location still points at cannot be deleted at all, and the
database refusing it is the intended answer. A location without a storage
group is not a valid location. The line read as cleanup that made the
delete safe; it never ran.

tests/fk-writes-are-null-not-zero.test.php gates all 23 plugin-owned
columns that core's schema-constraints.php constrains, not just the three
`config` ones where a sentinel was ever plausible -- enumerating only the
risky half is how the next one gets missed. It also refuses
update($find, $op, <scalar>) anywhere in the tree, which is the shape that
made this a no-op and which no caller can have meant. Comments are stripped
with the tokenizer first, so documenting a defect does not trip the check
that catches it.

The field-name mapping is asserted, not trusted: renaming a field behind a
constrained column fails here rather than quietly making every grep match
nothing.

Four mutations were run. The first version of the scalar-update check did
NOT catch the real bug -- `\[[^\]]*\]` cannot span the inner `]` in
`$arguments['itemIDs']`, so it skipped the exact line it existed for and
passed. Bounded on `;` instead, it goes red.

Also checked and clean: capone already migrated cImageID/cOSID off the 0
sentinel and guards with isValid() before writing; tasktypeedit and
taskstateedit own no tables and delete through core's shared path, which
already explains a refusal via ConstraintViolation::explain().

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0166dqQEjAs9fhqUw5zCjvxM
@mastacontrola
mastacontrola merged commit 981cf52 into main Sep 2, 2026
2 checks passed
@mastacontrola
mastacontrola deleted the claude/location-delete-hook-writes-null branch September 2, 2026 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant