Skip to content

[NO TICKET] Fix_database_cmd_warnings #331

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 137 additions & 11 deletions app/src/Lib/Util/SchemaManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@

use Cake\Console\ConsoleIo;

use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Schema\Comparator;
use Doctrine\DBAL\Schema\Schema;
use Doctrine\DBAL\Schema\SchemaDiff;
Expand Down Expand Up @@ -327,15 +329,17 @@ protected function processSchema(
}

// This is the SQL that represents the desired state of the database
$toSql = $schema->toSql($this->conn->getDatabasePlatform());
// XXX This never used keeping as a reference
// $toSql = $schema->toSql($this->conn->getDatabasePlatform());

// SchemaManager provides info about the database
$sm = $this->conn->createSchemaManager();

// The is the current database representation
$curSchema = $sm->createSchema();

$fromSql = $curSchema->toSql($this->conn->getDatabasePlatform());
// This is the current database representation
$curSchema = $sm->introspectSchema();

// XXX This is never used keeping as a reference
// $fromSql = $curSchema->toSql($this->conn->getDatabasePlatform());

try {
// We manually call compare so we can get the SchemaDiff object. We need
Expand All @@ -344,8 +348,11 @@ protected function processSchema(
// schema file).
$comparator = new Comparator();
$schemaDiff = $comparator->compareSchemas($curSchema, $schema);

$diffSql = $schemaDiff->toSaveSql($this->conn->getDatabasePlatform());

$diffSql = $this->getAlterSchemaSqlSaveMode(
$this->conn->getDatabasePlatform(),
$schemaDiff
);

// We don't start a transaction since in general we always want to move to
// the desired state, and if we fail in flight it's probably a bug that
Expand All @@ -359,12 +366,43 @@ protected function processSchema(
// Remove the DROP SEQUENCE statements in $fromSql because they're Postgres automagic
// being misinterpreted. (Note toSaveSql might mask this now.)
// XXX Maybe debug and file a PR to not emit DROP SEQUENCE on PG for autoincrementesque fields?
if($this->io) $io->out("Skipping sequence drop");
} else {
if($this->io) $this->io->out("Skipping sequence drop");
continue;
}

// PostgreSQL: translate DROP INDEX for names that are actually unique constraints
if($this->conn->driver == 'Cake\Database\Driver\Postgres'
&& preg_match('/^DROP INDEX\s+"?([a-zA-Z0-9_\.]+)"?/i', $sql, $m)) {
$idxOrConstraint = $m[1];

// Resolve if this name is a constraint and on which table
$qualifiedTable = $this->resolvePgConstraintTable($idxOrConstraint);

if($qualifiedTable !== null) {
// We will skip this index if it's bound to a constraint
if($this->io) $this->io->out('Skipping index:' . $m[1] . ' drop since it is bound to a constraint');
continue;
}
}

// PostgreSQL: make index renames idempotent and tolerant using IF EXISTS
Comment on lines +373 to +388
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we try getting these issues addressed upstream rather than having to patch around DBAL behavior?

Copy link
Contributor Author

@Ioannis Ioannis Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benno Before we can begin implementing changes, I believe it is essential to first upgrade to the latest version, which is currently 4.3. Once the system is up to date, we can start evaluating and suggesting modifications. For instance, introducing changes such as ALTER INDEX IF EXISTS should be considered early in the process. However, our initial priority should be to complete the version upgrade.

After upgrading, I would like to reassess these changes. This will allow us to determine whether the latest upstream code addresses our concerns or if any issues remain. The goal of this pull request is twofold: first, to resolve any issues or warnings reported at runtime via the command line, which can be confusing for deployers; and second, to address deprecations in preparation for the upgrade.

As part of the 5.2.7 upgrade, I have updated DBAL to the latest 3.x release for stability. Once we are running the most recent version of CakePHP, we can safely move to the latest DBAL and begin making further improvements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benno i created the CFM-467 ticket to track the reassessment

if($this->conn->driver == 'Cake\Database\Driver\Postgres'
&& preg_match('/^ALTER INDEX\s+([^\s]+)\s+RENAME\s+TO\s+([^\s]+)$/i', trim($sql), $m)) {
$oldName = $m[1];
$newName = $m[2];

// Use IF EXISTS to avoid error if the old index name doesn't exist in this environment
$translated = 'ALTER INDEX IF EXISTS ' . $oldName . ' RENAME TO ' . $newName;
if($this->io) $this->io->out($translated . ' -- added IF EXISTS for PostgreSQL resilience');
if(!$diffOnly) {
$stmt = $this->conn->executeQuery($sql);
// $stmt just returns the query string so we don't bother examining it
$this->conn->executeQuery($translated);
}
continue;
}

if(!$diffOnly) {
$stmt = $this->conn->executeQuery($sql);
// $stmt just returns the query string so we don't bother examining it
}
}

Expand All @@ -380,4 +418,92 @@ protected function processSchema(
// but so far we don't have an example indicating it's needed.
}

/**
* Resolve a PostgreSQL constraint's owning table (schema-qualified) by constraint name.
* Returns null if the given name is not found as a constraint.
*
* @param string $constraintName
* @return ?string Schema-qualified table name or null if constraint not found
* @since COmanage Registry v5.2.0
*/

protected function resolvePgConstraintTable(string $constraintName): ?string {
if($this->conn->driver !== 'Cake\\Database\\Driver\\Postgres') {
return null;
}

// Query pg catalogs to find the table for this constraint name
// We return schema-qualified name: schema.table
$sql = <<<CATALOG
SELECT n.nspname AS schema, c.relname AS table
FROM pg_constraint co
JOIN pg_class c ON c.oid = co.conrelid
JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE co.conname = :name
LIMIT 1
CATALOG;

try {
$stmt = $this->conn->executeQuery($sql, ['name' => $constraintName]);
$row = $stmt->fetchAssociative();
if(!$row || empty($row['schema']) || empty($row['table'])) {
return null;
}
// Quote/qualify as schema.table (matching how other statements look)
return $row['schema'] . '.' . $row['table'];
} catch(\Throwable $e) {
// On any error, fail open by returning null so caller uses original SQL
return null;
}
}


/**
* Generate non-destructive (save-mode) schema SQL.
*
* Equivalent to SchemaDiff::_toSql($platform, true).
*
* - Does NOT drop tables
* - Does NOT drop sequences
* - Does NOT drop orphaned FKs
*
* @param AbstractPlatform $platform Database platform abstraction
* @param SchemaDiff $diff Schema differences to generate SQL for
* @return array Array of SQL statements to execute
* @throws Exception
* @since COmanage Registry v5.2.0
*/
protected function getAlterSchemaSqlSaveMode(
AbstractPlatform $platform,
SchemaDiff $diff
): array {
$sql = [];

// 1) Schemas to create
if ($platform->supportsSchemas()) {
foreach ($diff->getCreatedSchemas() as $schema) {
$sql[] = $platform->getCreateSchemaSQL($schema);
}
}

// 2) Sequences (alter + create; no drops in save mode)
if ($platform->supportsSequences()) {
foreach ($diff->getAlteredSequences() as $sequence) {
$sql[] = $platform->getAlterSequenceSQL($sequence);
}
foreach ($diff->getCreatedSequences() as $sequence) {
$sql[] = $platform->getCreateSequenceSQL($sequence);
}
}

// 3) Tables (create only; no drops in save mode)
$sql = array_merge($sql, $platform->getCreateTablesSQL($diff->getCreatedTables()));

// 4) Alter existing tables (may emit FK drops/adds as required for alterations)
foreach ($diff->getAlteredTables() as $tableDiff) {
$sql = array_merge($sql, $platform->getAlterTableSQL($tableDiff));
}

return $sql;
}
}