Constrain capone and subnetgroup, and gate the classification - #31
Merged
Conversation
The companion to fogproject #1481, which declares the three relationships. This applies them, and adds the gate that should have demanded them. WHY THEY WERE MISSED. fogproject's tests/foreign-key-map.test.php requires every id column to be classified, but it walks commons/schema-expected.php -- 70 core tables, no plugin table. So nothing has ever required a decision about a plugin's id columns, and capone.cImageID, capone.cOSID and subnetgroup.sgGroupID had never had one. That gate cannot see this repository, so the missing half belongs here: plugin-id-columns-are-classified.test.php walks every manager's createTableSql() field list and fails any id column with no recorded decision. 52 columns across 24 tables, with one bounded exception -- OIDCProviders.opClientID ends in ID and is a client credential, not a reference. CAPONE needs two preconditions before its constraints can exist, and both are measured rather than assumed. cOSID is int(11) against a mediumint(9) parent, which InnoDB refuses at errno 150. And both columns carry a `0` sentinel, because the model declares no $databaseFieldsRequired and save()'s optional-`*id` branch writes 0 for an empty value -- the same shape that made tasks.taskStateID a runtime 1452 and was fixed in core's step 389. A foreign key spells "no reference" NULL and nothing else, and the sweep deliberately skips `<> 0`, so it cannot clean a sentinel: step 2 does, and createSql() builds both columns that way for a fresh install. Once the column is nullable save() writes NULL on its own, because FOGBase::columnType() falls back to _loadPluginColumnTypes() for tables the core manifest does not describe. Making them nullable also makes the sweep non-destructive: planSweep() nulls a dangling reference where the column allows it and only deletes where it does not. No capone row is removed. SUBNETGROUP needs nothing. sgGroupID is already int(11) NOT NULL against an int(11) parent, and `groupID` IS in its model's required list, so save() refuses an empty value rather than writing 0. Its CASCADE pins what already happens -- removesubnetgroupgroup.hook.php deletes those rows when a group is destroyed -- so the constraint states in the schema what was relying on a hook being registered. The hook stays; it is what fires the plugin's own events. Both steps are appended, not folded in, because installdb() skips the pSchema steps an install has already passed. Gate mutation-verified four ways: unclassifying a known column, introducing a new unclassified one, removing a pinned column, and breaking the extraction regex. The second is the one that matters -- it is the future case, and the whole reason the gate exists. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Companion to fogproject #1481, which declares the three relationships.
This applies them, and adds the gate that should have demanded them.
Merge #1481 first. These steps call
applyConstraints('capone')/('subnetgroup'), which read core's map. The other way round they find nosuch group and silently do nothing.
Why these were missed
fogproject's
tests/foreign-key-map.test.phprequires every id column to beclassified — but it walks
commons/schema-expected.php, which is 70 coretables. No plugin table is in it, so nothing has ever required a decision
about a plugin's id columns.
That gate cannot see this repository, so the missing half belongs here.
tests/plugin-id-columns-are-classified.test.phpwalks every manager'screateTableSql()field list and fails any id column with no recordeddecision — 52 columns across 24 tables, with one bounded exception
(
OIDCProviders.opClientIDends inIDand is an OIDC client credential, nota reference). Same hand-pinned-both-sides pattern as
foreign-keys-applied-per-plugin.test.php, and for the same reason:bin/fetch-plugins.shfetches this repo on its own.capone — two preconditions, both measured
cOSIDisint(11)against amediumint(9)parent, which InnoDB refuses aterrno 150. And both columns carry a
0sentinel, because the modeldeclares no
$databaseFieldsRequiredandsave()'s optional-*idbranchwrites
0— the same shape that madetasks.taskStateIDa runtime 1452 andwas fixed in core's step 389.
A foreign key spells "no reference"
NULLand nothing else, and the sweepdeliberately skips
<> 0, so it cannot clean a sentinel. Step 2 does, andcreateSql()builds both columns that way for a fresh install. Once the columnis nullable
save()writesNULLon its own —FOGBase::columnType()fallsback to
_loadPluginColumnTypes(), which reads the server's catalog for tablesthe core manifest does not describe.
Making them nullable also makes the sweep non-destructive:
planSweep()nulls a dangling reference where the column allows it and only deletes where it
does not. No capone row is removed.
Against real MariaDB 11.8, with a legacy
0row and an orphaned imagereference present: all three constraints land and all rows survive with their
dangling references nulled. Deleting an OS a capone rule uses is then refused
1451.
subnetgroup — nothing to change
sgGroupIDis alreadyint(11) NOT NULLagainst anint(11)parent, andgroupIDis in its model's required list, sosave()refuses an emptyvalue rather than writing
0. Its CASCADE pins what already happens —removesubnetgroupgroup.hook.phpalready deletes those rows when a group isdestroyed — so the constraint states in the schema what was relying on a hook
being registered. The hook stays; it fires the plugin's own events.
Both steps are appended rather than folded in, because
installdb()skips thepSchema steps an install has already passed.
Gate mutation-verified
capone.cImageIDcapone.cNewThingIDThe second is the one that matters: it is the future case, and the whole reason
the gate exists.
Verification
sh tests/run-all.sh— 13 passed, 0 failed, includingforeign-keys-applied-per-pluginat 38 checks / 7 plugins (was 28 / 5) and thenew gate at 105 checks / 52 id columns.