Merge pull request #1915 from OpenConext/feature/1906-fix-mapping-issues

Fix and clarify doctrine mapping issues

Author: johanib

CHANGELOG.md

@@ -18,6 +18,12 @@ Upgrade to `doctrine/dbal` 4
 Bugfixes:
 * Metadata push will now reject all metadata if any service contains invalid PHP syntax in its attribute manipulations (#1778)
 
+Changes:
+* The `consent.deleted_at` should be not nullable, and have a default value of `0000-00-00 00:00:00`.
+  * Because `deleted_at` is part of the PK, no migration is provided. The database engine should not allow this to be null in the first place, so it is probably not nullable already on your db.
+  * The `0000-00-00 00:00:00` is added for clarity/consistency, as this is probably the default behaviour of your database already.
+* Removed unused index `consent.deleted_at`. Delete this from your production database if it's there.
+
 ## 7.1.0
 SRAM integration
 

README.md

@@ -136,6 +136,9 @@ EngineBlock requires database settings, without it doctrine migrate will not fun
 the application must use the production settings (`--env=prod`), this could be replaced with `dev` should you run a
 development version._
 
+_**Note**:
+The doctrine migrations shipped with engineblock are built against MariaDB. They should work on MySQL, if the version matches._
+
 ### Configure HTTP server
 
 Configure a single virtual host, this should point to the `public` directory:

ci/qa/docheader.sh

@@ -4,4 +4,4 @@ set -e
 cd $(dirname $0)/../../
 
 echo -e "\nDoc header check\n"
-./vendor/bin/docheader check src/ tests/ library/ --exclude-dir resources --exclude-dir languages
+./vendor/bin/docheader check src/ tests/ library/ migrations/ --exclude-dir resources --exclude-dir languages

migrations/DoctrineMigrations/AbstractEngineBlockMigration.php

@@ -0,0 +1,74 @@
+<?php
+
+/**
+ * Copyright 2026 SURFnet B.V.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+declare(strict_types=1);
+
+namespace OpenConext\EngineBlock\Doctrine\Migrations;
+
+use Doctrine\DBAL\Platforms\MariaDBPlatform;
+use Doctrine\DBAL\Platforms\MySQLPlatform;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Base class for all EngineBlock Doctrine migrations.
+ *
+ * All migrations in this project target MariaDB exclusively. The generated DDL SQL is platform-specific
+ * and is not guaranteed to be compatible with MySQL or any other database engine.
+ *
+ */
+abstract class AbstractEngineBlockMigration extends AbstractMigration
+{
+    public function preUp(Schema $schema): void
+    {
+        $this->checkPlatform();
+    }
+
+    public function preDown(Schema $schema): void
+    {
+        $this->checkPlatform();
+    }
+
+    private function checkPlatform(): void
+    {
+        if ($this->platform instanceof MariaDBPlatform) {
+            return;
+        }
+
+        if ($this->platform instanceof MySQLPlatform) {
+            $this->warnIf(
+                true,
+                sprintf(
+                    'This migration is built for MariaDB. The current database platform is MySQL ("%s"). '
+                    . 'EngineBlock migrations may contain MariaDB-specific DDL that may fail. '
+                    . 'Check manually to ensure the migrations run as expected.',
+                    get_class($this->platform),
+                ),
+            );
+            return;
+        }
+
+        $this->abortIf(
+            true,
+            sprintf(
+                'This migration is built for MariaDB only. The current database platform "%s" is not supported.',
+                get_class($this->platform),
+            ),
+        );
+    }
+}

migrations/DoctrineMigrations/Version20260210000000.php

@@ -1,11 +1,26 @@
 <?php
 
+/**
+ * Copyright 2026 SURFnet B.V.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
 declare(strict_types=1);
 
 namespace OpenConext\EngineBlock\Doctrine\Migrations;
 
 use Doctrine\DBAL\Schema\Schema;
-use Doctrine\Migrations\AbstractMigration;
 
 /**
  * Baseline migration - Creates initial database schema based on production state (6.18) as of feb 2026
@@ -14,7 +29,7 @@
  * It creates all required tables from scratch for new installations, and gracefully skips
  * execution on existing databases where tables are already present.
  */
-final class Version20260210000000 extends AbstractMigration
+final class Version20260210000000 extends AbstractEngineBlockMigration
 {
     public function getDescription(): string
     {
@@ -23,6 +38,8 @@ public function getDescription(): string
 
     public function preUp(Schema $schema): void
     {
+        parent::preUp($schema);
+
         $tables = $this->sm->listTableNames();
         $this->skipIf(
             in_array('sso_provider_roles_eb5', $tables, true),

migrations/DoctrineMigrations/Version20260224000000.php

@@ -0,0 +1,54 @@
+<?php
+
+/**
+ * Copyright 2026 SURFnet B.V.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+declare(strict_types=1);
+
+namespace OpenConext\EngineBlock\Doctrine\Migrations;
+
+use Doctrine\DBAL\Schema\Schema;
+
+/**
+ * Patch/repair migration - Removes the deleted_at index from the consent table if present.
+ *
+ * On existing databases where the index is already absent this migration is marked as done
+ * without executing any SQL. On databases where the index still exists it will be dropped.
+ */
+final class Version20260224000000 extends AbstractEngineBlockMigration
+{
+    public function getDescription(): string
+    {
+        return 'Patch migration: Removes the deleted_at index from the consent table. Skips if the index does not exist.';
+    }
+
+    public function preUp(Schema $schema): void
+    {
+        parent::preUp($schema);
+
+        $indexes = $this->connection->createSchemaManager()->listTableIndexes('consent');
+        $this->skipIf(
+            !isset($indexes['deleted_at']),
+            'Index deleted_at on consent table does not exist. Skipping.'
+        );
+    }
+
+    public function up(Schema $schema): void
+    {
+        $this->addSql('ALTER TABLE `consent` DROP INDEX `deleted_at`');
+    }
+
+}

src/OpenConext/EngineBlock/Metadata/Entity/IdentityProvider.php

@@ -78,6 +78,17 @@ class IdentityProvider extends AbstractRole
 
     /**
      * @var ConsentSettings
+     *
+     * NOTE: The consent_settings and idp_discoveries columns are physically stored as LONGTEXT in the database.
+     * This predates native JSON column support and is intentional. Running `doctrine:schema:update` will suggest:
+     *
+     *   ALTER TABLE sso_provider_roles_eb5
+     *     CHANGE consent_settings consent_settings JSON DEFAULT NULL,
+     *     CHANGE idp_discoveries idp_discoveries JSON DEFAULT NULL;
+     *
+     * Do not apply this migration. Switching to native MySQL/MariaDB JSON columns requires a careful migration to work
+     * with green/blue deployment strategies.
+     *
      */
     #[ORM\Column(name: 'consent_settings', type: Types::JSON, length: 16777215)]
     private $consentSettings;

src/OpenConext/EngineBlockBundle/Authentication/Entity/Consent.php

@@ -31,7 +31,6 @@
 #[ORM\Entity]
 #[ORM\Index(name: 'hashed_user_id', columns: ['hashed_user_id'])]
 #[ORM\Index(name: 'service_id', columns: ['service_id'])]
-#[ORM\Index(name: 'deleted_at', columns: ['deleted_at'])]
 class Consent
 {
     /**
@@ -68,8 +67,20 @@ class Consent
 
     /**
      * @var DateTime
+     *
+     * Soft-delete sentinel using MariaDB's special zero-date ('0000-00-00 00:00:00') as the "not deleted" value.
+     *
+     * Active (non-deleted) records have deleted_at = '0000-00-00 00:00:00'.
+     * Soft-deleted records have deleted_at = NOW()
+     *
+     * Queries use `deleted_at IS NULL` to select active records. This works because MariaDB defines that
+     * expressions involving a zero-date evaluate to NULL at the database level (see MariaDB DATETIME docs).
+     *
+     * IMPORTANT deleted_at cannot be made nullable because it is part of the composite primary key (hashed_user_id,
+     * service_id, deleted_at). Primary key columns cannot be nullable in MySQL/MariaDB.
+     *
      */
     #[ORM\Id]
-    #[ORM\Column(name: 'deleted_at', type: Types::DATETIME_MUTABLE, nullable: true, options: ['default' => null])]
+    #[ORM\Column(name: 'deleted_at', type: Types::DATETIME_MUTABLE, nullable: false, options: ['default' => '0000-00-00 00:00:00'])]
     public ?DateTimeInterface $deletedAt = null;
 }

src/OpenConext/EngineBlockBundle/Authentication/Repository/DbalConsentRepository.php

@@ -62,6 +62,8 @@ public function __construct(ManagerRegistry $registry, LoggerInterface $logger)
      */
     public function findAllFor($userId)
     {
+        // deleted_at IS NULL matches active records whose deleted_at is '0000-00-00 00:00:00'.
+        // See Consent::$deletedAt for full context.
         $sql       = '
             SELECT
                 service_id
@@ -126,6 +128,7 @@ public function deleteAllFor($userId)
      */
     public function deleteOneFor(string $userId, string $serviceProviderEntityId): bool
     {
+        // deleted_at IS NULL matches active records. See Consent::$deletedAt for full context.
         $sql = '
             UPDATE
                 consent