diff --git a/capone/class/caponemanager.class.php b/capone/class/caponemanager.class.php index 923964f..836aaea 100644 --- a/capone/class/caponemanager.class.php +++ b/capone/class/caponemanager.class.php @@ -47,14 +47,23 @@ public function createSql() ], [ 'INTEGER', + // MEDIUMINT(9) matches os.osID. InnoDB requires an EXACT type + // match on both sides of a foreign key and answers errno 150 + // otherwise -- int(11) against mediumint(9) is refused, which + // is what this column was until fogproject ADR 0031. 'INTEGER', - 'INTEGER', + 'MEDIUMINT(9)', 'VARCHAR(255)' ], [ false, - false, - false, + // Nullable, because a foreign key spells "no reference" NULL + // and nothing else. These carried 0, which is a value: a + // constraint over them would demand an image with imageID 0 + // and an os with osID 0 and refuse every row without one. + // Step 2 converts the rows an existing install already has. + true, + true, false ], [ @@ -139,6 +148,98 @@ public function schema() function () { return $this->seedSettings(); }, + // 2 - cImageID and cOSID stop spelling "no reference" as 0. + // + // fogproject ADR 0031. A foreign key accepts NULL for "no + // reference" and nothing else, so a constraint over these columns + // would demand an image with imageID 0 and an OS with osID 0 and + // refuse every row that had neither. cOSID also has to become + // MEDIUMINT(9): os.osID is mediumint(9) and InnoDB refuses a + // foreign key whose sides are not the same type, at errno 150. + // + // Capone declares no $databaseFieldsRequired, so save()'s + // optional-`*id` branch is what wrote the 0 -- the same shape + // that made tasks.taskStateID a runtime 1452 and was fixed in + // core's schema step 389. Once the column is nullable the branch + // writes NULL on its own: FOGBase::columnType() falls back to + // _loadPluginColumnTypes(), which reads the server's own catalog + // for tables the core manifest does not describe. + // + // Appended rather than folded into step 0 because installdb() + // SKIPS the pSchema steps an install has already passed instead + // of replaying them. createSql() above now builds both columns + // this way for a fresh install; this is what an existing one + // gets. + function () { + $sql = sprintf( + 'ALTER TABLE `%s` MODIFY COLUMN `cImageID` INTEGER NULL' + . ' DEFAULT NULL, MODIFY COLUMN `cOSID` MEDIUMINT(9) NULL' + . ' DEFAULT NULL', + $this->tablename + ); + if (false !== self::$DB->query($sql)->error) { + return self::$DB->error; + } + $converted = 0; + foreach (['cImageID', 'cOSID'] as $column) { + $sql = sprintf( + 'UPDATE `%s` SET `%s` = NULL WHERE `%s` = 0', + $this->tablename, + $column, + $column + ); + if (false !== self::$DB->query($sql)->error) { + return self::$DB->error; + } + $converted += (int)self::$DB->affectedRows(); + } + // Logged rather than silent: this rewrites rows, and the + // count is the only evidence it did. + error_log( + sprintf( + '%s: %d %s', + _('Capone schema'), + $converted, + _('row(s) converted from 0 to NULL') + ) + ); + return true; + }, + // 3 - the plugin's foreign keys. + // + // fogproject ADR 0031 decision 8: sweep, then add. ADD CONSTRAINT + // validates the rows already in the table and answers 1452 if any + // of them point at a parent that is gone -- and applyConstraints() + // REPORTS a refusal rather than returning it, so an install that + // skipped the sweep would succeed while silently not creating the + // constraint. Both columns are nullable by step 2, so the sweep + // nulls the dangling references rather than deleting the rows. + // + // Both calls are filtered to this plugin's own group, so neither + // can reach another plugin's tables or core's. The relationships + // are declared in fogproject's commons/schema-constraints.php: + // capone.cImageID RESTRICT to `images`, capone.cOSID RESTRICT to + // `os`. + // + // RESTRICT, not CASCADE, for both. Nothing in FOG deletes a + // capone row today when its image or OS goes -- there is no hook + // and deletemass() has no case -- so the reference simply + // dangles, and the map's rule is RESTRICT exactly where PHP does + // nothing and the reference dangles. CASCADE would silently + // delete an administrator's deployment rule as a side effect of + // deleting an image, which is the argument core used to reject + // CASCADE for storage nodes. + // + // Idempotent, and re-run by the unfiltered reconcile after every + // core schema update: planConstraints() skips a constraint whose + // declaration already matches the map. + function () { + $res = \FOG\Db\SchemaReconciler::sweepOrphans('capone'); + if (is_string($res)) { + return $res; + } + return \FOG\Db\SchemaReconciler::applyConstraints('capone'); + }, ]; } /** diff --git a/subnetgroup/class/subnetgroupmanager.class.php b/subnetgroup/class/subnetgroupmanager.class.php index d633df4..ef2c50a 100644 --- a/subnetgroup/class/subnetgroupmanager.class.php +++ b/subnetgroup/class/subnetgroupmanager.class.php @@ -89,6 +89,50 @@ public function schema() return [ // 0 $this->createSql(), + // 1 - the plugin's foreign key. + // + // fogproject ADR 0031 decision 8: sweep, then add. ADD CONSTRAINT + // validates the rows already in the table and answers 1452 if any + // of them point at a parent that is gone -- and applyConstraints() + // REPORTS a refusal rather than returning it, so an install that + // skipped the sweep would succeed while silently not creating the + // constraint. The sweep is the precondition for the statement, + // not a policy choice. + // + // Both calls are filtered to this plugin's own group, so neither + // can reach another plugin's tables or core's. The relationship + // is declared in fogproject's commons/schema-constraints.php: + // subnetgroup.sgGroupID CASCADE to `groups`. + // + // CASCADE PINS WHAT ALREADY HAPPENS, so nothing observable + // changes. removesubnetgroupgroup.hook.php already calls + // Route::deletemass('subnetgroup', ['groupID' => ...]) when a + // group is destroyed; the constraint states that in the schema + // instead of relying on a hook being registered. The hook stays + // -- it is what fires the plugin's own events -- and the two + // agree rather than compete. + // + // No column change was needed: sgGroupID is already int(11) NOT + // NULL against an int(11) parent, and it carries no sentinel + // because `groupID` IS in the model's $databaseFieldsRequired, + // so save() refuses an empty value rather than writing 0. + // + // Appended rather than folded into step 0 because installdb() + // SKIPS the pSchema steps an install has already passed instead + // of replaying them. + // + // Idempotent, and re-run by the unfiltered reconcile after every + // core schema update: planConstraints() skips a constraint whose + // declaration already matches the map. + function () { + $res = \FOG\Db\SchemaReconciler::sweepOrphans('subnetgroup'); + if (is_string($res)) { + return $res; + } + return \FOG\Db\SchemaReconciler::applyConstraints( + 'subnetgroup' + ); + }, ]; } /** diff --git a/tests/foreign-keys-applied-per-plugin.test.php b/tests/foreign-keys-applied-per-plugin.test.php index 71390e8..b046628 100644 --- a/tests/foreign-keys-applied-per-plugin.test.php +++ b/tests/foreign-keys-applied-per-plugin.test.php @@ -57,6 +57,8 @@ 'windowskey' => 'windowskey/class/windowskeymanager.class.php', 'ldap' => 'ldap/class/ldapmanager.class.php', 'oidc' => 'oidc/class/oidcmanager.class.php', + 'capone' => 'capone/class/caponemanager.class.php', + 'subnetgroup' => 'subnetgroup/class/subnetgroupmanager.class.php', ]; /** diff --git a/tests/plugin-id-columns-are-classified.test.php b/tests/plugin-id-columns-are-classified.test.php new file mode 100644 index 0000000..522198f --- /dev/null +++ b/tests/plugin-id-columns-are-classified.test.php @@ -0,0 +1,193 @@ + + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ + +$root = dirname(__DIR__); +$failures = []; +$checks = 0; + +/* + * Each table's OWN primary key. Not a reference, so nothing to classify. + */ +$primary = [ + 'LDAPGroups.lgID', 'LDAPServers.lsID', 'OIDCGroups.ogID', + 'OIDCProviders.opID', 'capone.cID', 'helloWorld.hwID', + 'ldapGroupRoleAssoc.lgraID', 'ldapGroupUserGroupAssoc.lgugID', + 'ldapUserGrant.lugID', 'location.lID', 'locationAssoc.laID', 'ntfy.nID', + 'oidcGroupRoleAssoc.ograID', 'oidcGroupUserGroupAssoc.ogugID', + 'oidcIdentity.oiID', 'oidcUserGrant.ougID', 'ou.ouID', 'ouAssoc.oaID', + 'pushbullet.pID', 'slack.sID', 'subnetgroup.sgID', 'windowsKeys.wkID', + 'windowsKeysAssoc.wkaID', 'wolbroadcast.wbID', +]; + +/* + * Classified in fogproject's commons/schema-constraints.php. A constraint is + * declared for each of these; the group name is the plugin's directory. + */ +$classified = [ + 'locationAssoc.laLocationID', 'locationAssoc.laHostID', + 'location.lStorageGroupID', 'location.lStorageNodeID', + 'ouAssoc.oaOUID', 'ouAssoc.oaHostID', + 'windowsKeysAssoc.wkaImageID', 'windowsKeysAssoc.wkaKeyID', + 'LDAPGroups.lgServerID', 'ldapGroupRoleAssoc.lgraGroupID', + 'ldapGroupRoleAssoc.lgraRoleID', 'ldapGroupUserGroupAssoc.lgugGroupID', + 'ldapGroupUserGroupAssoc.lgugUserGroupID', 'ldapUserGrant.lugUserID', + 'OIDCGroups.ogProviderID', 'oidcIdentity.oiProviderID', + 'oidcIdentity.oiUserID', 'oidcGroupRoleAssoc.ograGroupID', + 'oidcGroupRoleAssoc.ograRoleID', 'oidcGroupUserGroupAssoc.ogugGroupID', + 'oidcGroupUserGroupAssoc.ogugUserGroupID', 'oidcUserGrant.ougUserID', + 'capone.cImageID', 'capone.cOSID', 'subnetgroup.sgGroupID', +]; + +/* + * Classified `poly` in that map: the target table is named by a sibling + * column, so no foreign key is expressible. A decision, not an omission. + */ +$poly = ['ldapUserGrant.lugTargetID', 'oidcUserGrant.ougTargetID']; + +/* + * Ends in ID and is not an id. The single bounded exception; see the header. + */ +$notAReference = ['OIDCProviders.opClientID']; + +$decided = array_flip( + array_merge($primary, $classified, $poly, $notAReference) +); + +/** + * Source with comments removed, so a commented-out column cannot satisfy a + * check or a commented-out table escape one. + * + * @param string $file the file to read + * + * @return string + */ +function idStrip($file) +{ + $clean = ''; + foreach (token_get_all((string)file_get_contents($file)) as $token) { + if (is_array($token) + && ($token[0] === T_COMMENT || $token[0] === T_DOC_COMMENT) + ) { + continue; + } + $clean .= is_array($token) ? $token[1] : $token; + } + + return $clean; +} + +$seen = []; +$iterator = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($root) +); +foreach ($iterator as $file) { + if (!preg_match('/manager\.class\.php$/', $file->getFilename())) { + continue; + } + $path = $file->getPathname(); + if (false !== strpos($path, '/tests/')) { + continue; + } + $src = idStrip($path); + if (false === strpos($src, 'createTableSql(')) { + continue; + } + if (!preg_match('/\$tablename\s*=\s*.([A-Za-z_]+)./', $src, $tm)) { + continue; + } + $table = $tm[1]; + // Only the createTableSql() argument list. A column name mentioned in a + // query elsewhere in the manager is not this table declaring it. + $chunk = substr($src, (int)strpos($src, 'createTableSql('), 2000); + preg_match_all('/.([A-Za-z_]+(?:ID|Id)).\s*[,\]]/', $chunk, $cm); + foreach (array_unique($cm[1]) as $column) { + $key = $table . '.' . $column; + $seen[$key] = true; + $checks++; + if (!isset($decided[$key])) { + $failures[] = "$key has never been classified." + . ' Add it to fogproject commons/schema-constraints.php --' + . ' as a constraint, or as `poly` if a sibling column names' + . ' its table -- and pin it in this test. An id column with' + . ' no decision is how a dangling reference ships'; + } + } +} + +// The inverse: a pinned entry whose column is gone is a stale pin, and a +// stale pin is how this test quietly stops covering the thing it names. +foreach (array_keys($decided) as $key) { + $checks++; + if (!isset($seen[$key])) { + $failures[] = "$key is pinned in this test but no plugin table" + . ' declares it. Remove the pin, or restore the column'; + } +} + +// A regex that matches nothing passes every check above. Anchor the sweep on +// a count that a broken extraction cannot reach. +$checks++; +if (count($seen) < 50) { + $failures[] = sprintf( + 'only %d id column(s) were found across the plugin managers, so the' + . ' checks above proved almost nothing -- the extraction is broken', + count($seen) + ); +} + +if (count($failures)) { + echo 'FAIL: ' . count($failures) . " problem(s).\n\n"; + foreach ($failures as $f) { + echo " $f\n"; + } + exit(1); +} + +printf( + "plugin-id-columns-are-classified: %d checks passed, %d id column(s)\n", + $checks, + count($seen) +); +exit(0);