From 37eafd276c60bc47228850cca43f6c0342ef27f2 Mon Sep 17 00:00:00 2001
From: matthieu <matthieu@yuno.malio.fr>
Date: Mon, 20 Apr 2026 21:10:46 +0200
Subject: [PATCH] fix(audit-log) : address code review findings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Blocker
- Frontend attendait `hydra:member` / `hydra:totalItems` / `hydra:view` mais
  API Platform 4 sert `member` / `totalItems` / `view` (sans prefixe) sous
  ld+json, et un tableau plat sous json. Consequence : tableau admin et
  timeline silencieusement vides.
  Fix : `useAuditLog` force `Accept: application/ld+json` (necessaire pour
  obtenir l'objet Hydra avec pagination), types `HydraCollection`/`HydraView`
  renommes, composants accedent aux proprietes sans prefixe. Nouveau test
  fonctionnel verrouille le format.

Should-fix
- `AuditLogWriter` : ajout de `'id' => Types::GUID` pour expliciter le type
  natif PG `uuid` (fonctionnait par cast implicite mais l'intention etait
  floue).
- `AuditListener` docblock : documente que le DQL bulk DELETE/UPDATE et
  `Connection::executeStatement()` bypassent le listener (onFlush non
  appele). Piege pour les futures commandes de purge.
- `AuditLogResource` : ajout d'une regex UUID dans `requirements` de
  l'operation Get — un `GET /api/audit-logs/not-a-uuid` produisait un 500
  (cast PG rejete) au lieu d'un 404.
- `audit-log.vue` : le watcher des filtres faisait `filters.page = 1` ce
  qui declenchait le watcher de `page`, causant deux `loadEntries()` en
  parallele. Fusionne : la navigation page appelle `loadEntries()`
  directement depuis `goPrevious`/`goNext`, plus de watcher dedie.
- `useAuditLog.fetchEntityLogs` : bypass du cache `lastCollection` pour ne
  pas polluer la reference page-level quand la timeline est ouverte.
- `AuditTimeline.vue` : remplacement du `<div v-if="!canView"/>` vide par
  un `v-if` sur le wrapper — aucun DOM quand l'utilisateur n'a pas le droit.
- `AuditListenerTest` tag : retire le `_` (wildcard LIKE SQL) du prefix
  pour eviter un faux negatif de match cross-test.
- `AuditLogApiTest` : proprietes `auditConnection` / `runTag` nullable et
  tearDown guarde, sinon un echec setUp provoquait un fatal typed-property
  au lieu de propager l'exception d'origine.

Stabilite suite de tests
- `doctrine.yaml when@test` : `idle_connection_ttl: 1` sur les deux
  connexions pour eviter l'accumulation de connexions orphelines.
- tearDown des tests audit : `close()` explicite sur la connexion audit
  apres chaque test.
- `docker-compose.yml` : `max_connections=300` sur la DB dev (defaut PG=100
  insuffisant pour 220+ tests * 2 connexions/test).
---
 config/packages/doctrine.yaml                 |  8 +++
 docker-compose.yml                            |  5 +-
 .../modules/core/pages/admin/audit-log.vue    | 21 ++++---
 .../shared/components/audit/AuditTimeline.vue | 15 +++--
 frontend/shared/composables/useAuditLog.ts    | 28 +++++++--
 frontend/shared/utils/api.ts                  | 31 +++++++---
 .../ApiPlatform/Resource/AuditLogResource.php |  1 +
 .../Infrastructure/Audit/AuditLogWriter.php   |  5 +-
 .../Infrastructure/Doctrine/AuditListener.php |  6 ++
 tests/Module/Core/Api/AuditLogApiTest.php     | 61 ++++++++++++++++---
 .../Doctrine/AuditListenerTest.php            |  9 ++-
 11 files changed, 153 insertions(+), 37 deletions(-)

diff --git a/config/packages/doctrine.yaml b/config/packages/doctrine.yaml
index 2864ce8..fddef1e 100644
--- a/config/packages/doctrine.yaml
+++ b/config/packages/doctrine.yaml
@@ -44,11 +44,19 @@ when@test:
             # la connexion `audit` ecrirait dans la base dev pendant que l'ORM
             # ecrit dans la base test — divergence invisible en apparence mais
             # fatale pour les tests du journal d'audit.
+            #
+            # `idle_connection_ttl: 1` (au lieu du defaut 600s) : en test on
+            # reboote le kernel a chaque test. Sans TTL court, les connexions
+            # orphelines s'accumulent dans PG et on finit par saturer le pool
+            # (max_connections=100) sur une suite de 200+ tests qui utilisent
+            # 2 connexions chacun (default + audit).
             connections:
                 default:
                     dbname_suffix: '_test%env(default::TEST_TOKEN)%'
+                    idle_connection_ttl: 1
                 audit:
                     dbname_suffix: '_test%env(default::TEST_TOKEN)%'
+                    idle_connection_ttl: 1
         orm:
             mappings:
                 # Entite fictive SiteAware utilisee uniquement en tests du
diff --git a/docker-compose.yml b/docker-compose.yml
index 9815dd2..451caf0 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -45,7 +45,10 @@ services:
     restart: unless-stopped
   db:
     image: postgres:16-alpine
-    command: -p ${POSTGRES_PORT:-5436}
+    # max_connections eleve (defaut PG=100) pour absorber la suite de tests :
+    # ~220 tests * kernel reboot par test * 2 connexions (default + audit)
+    # peut saturer le pool, meme avec idle_connection_ttl court cote Doctrine.
+    command: -p ${POSTGRES_PORT:-5436} -c max_connections=300
     environment:
       POSTGRES_DB: ${POSTGRES_DB}
       POSTGRES_USER: ${POSTGRES_USER}
diff --git a/frontend/modules/core/pages/admin/audit-log.vue b/frontend/modules/core/pages/admin/audit-log.vue
index f4b6494..bbc6afe 100644
--- a/frontend/modules/core/pages/admin/audit-log.vue
+++ b/frontend/modules/core/pages/admin/audit-log.vue
@@ -159,7 +159,7 @@
             </table>
         </section>
 
-        <!-- Pagination via hydra:view -->
+        <!-- Pagination via hydra (view.next / view.previous) -->
         <nav class="mt-3 flex items-center justify-between text-sm">
             <span class="text-gray-600">
                 {{ totalItems }} entrée{{ totalItems > 1 ? 's' : '' }}
@@ -270,11 +270,11 @@ async function loadEntries(): Promise<void> {
             performedAtAfter:  filters.performedAtAfter ? toIso(filters.performedAtAfter) : undefined,
             performedAtBefore: filters.performedAtBefore ? toIso(filters.performedAtBefore) : undefined,
         })
-        entries.value = data['hydra:member'] ?? []
-        totalItems.value = data['hydra:totalItems'] ?? 0
-        const view = data['hydra:view']
-        hasPrevious.value = Boolean(view?.['hydra:previous'])
-        hasNext.value = Boolean(view?.['hydra:next'])
+        entries.value = data.member ?? []
+        totalItems.value = data.totalItems ?? 0
+        const view = data.view
+        hasPrevious.value = Boolean(view?.previous)
+        hasNext.value = Boolean(view?.next)
     } finally {
         loading.value = false
     }
@@ -317,12 +317,14 @@ function goPrevious(): void {
     if (!hasPrevious.value || !filters.page) return
     filters.page = Math.max(1, filters.page - 1)
     syncQuery()
+    loadEntries()
 }
 
 function goNext(): void {
     if (!hasNext.value) return
     filters.page = (filters.page ?? 1) + 1
     syncQuery()
+    loadEntries()
 }
 
 // Persiste les filtres dans les query params URL pour que le reload ou le
@@ -339,7 +341,11 @@ function syncQuery(): void {
 }
 
 // Synchronisation reactive : tout changement de filtre declenche un fetch
-// + reset de la pagination a la page 1 (sauf si seul `page` a change).
+// + reset de la pagination a la page 1. La navigation page (prev/next) ne
+// passe PAS par un watcher : elle appelle `loadEntries()` directement dans
+// `goPrevious`/`goNext`. Cette separation evite un double-fetch concurrent
+// quand une filtre reset la page a 1 (sinon le watch de `filters.page`
+// serait declenche une seconde fois en parallele).
 watch(
     () => [filters.performedAtAfter, filters.performedAtBefore, filters.entityType, filters.performedBy, filters.action],
     () => {
@@ -348,7 +354,6 @@ watch(
         loadEntries()
     },
 )
-watch(() => filters.page, () => { loadEntries() })
 
 onMounted(() => {
     loadEntries()
diff --git a/frontend/shared/components/audit/AuditTimeline.vue b/frontend/shared/components/audit/AuditTimeline.vue
index 61212d3..9f360f9 100644
--- a/frontend/shared/components/audit/AuditTimeline.vue
+++ b/frontend/shared/components/audit/AuditTimeline.vue
@@ -1,8 +1,11 @@
 <template>
-    <!-- Garde permission : aucun rendu ni appel API si l'utilisateur n'a pas le droit. -->
-    <div v-if="!canView" />
-
-    <div v-else class="audit-timeline">
+    <!--
+        Garde permission : aucun rendu DOM ni appel API si l'utilisateur n'a
+        pas le droit. On wrappe le contenu dans un bloc v-if plutot qu'un div
+        vide pour eviter de polluer la layout quand le composant est embarque
+        dans une page qui rend deja sa propre structure.
+    -->
+    <div v-if="canView" class="audit-timeline">
         <!-- Skeleton loader initial -->
         <ul v-if="loading && entries.length === 0" class="space-y-3">
             <li v-for="i in 3" :key="i" class="flex gap-3">
@@ -115,9 +118,9 @@ async function loadPage(targetPage: number, append: boolean): Promise<void> {
     loading.value = true
     try {
         const data = await fetchEntityLogs(entityType.value, entityId.value, targetPage)
-        const slice = (data['hydra:member'] ?? []).slice(0, append ? undefined : INITIAL_LIMIT)
+        const slice = (data.member ?? []).slice(0, append ? undefined : INITIAL_LIMIT)
         entries.value = append ? [...entries.value, ...slice] : slice
-        totalItems.value = data['hydra:totalItems'] ?? entries.value.length
+        totalItems.value = data.totalItems ?? entries.value.length
         page.value = targetPage
     } catch {
         // Erreur silencieuse (timeline secondaire) — useApi n'affiche pas de toast avec toast: false.
diff --git a/frontend/shared/composables/useAuditLog.ts b/frontend/shared/composables/useAuditLog.ts
index 778cc51..ba695e7 100644
--- a/frontend/shared/composables/useAuditLog.ts
+++ b/frontend/shared/composables/useAuditLog.ts
@@ -50,21 +50,38 @@ function buildQuery(filters: AuditLogFilters | undefined): Record<string, string
  * pour purger le cache (conforme a la regle CLAUDE.md sur les composables
  * singletons, cf. `useSidebar.resetSidebar`).
  */
+// Accept explicitement JSON-LD : API Platform 4 retourne un tableau PLAT (liste
+// d'items, sans envelope de pagination) sous `application/json`, et un objet
+// Hydra complet avec `member`, `totalItems` et `view` (first/last/next/previous)
+// sous `application/ld+json`. Pour obtenir `view` cote front — indispensable
+// a la pagination prev/next — on force donc ld+json.
+const JSONLD_HEADERS = { Accept: 'application/ld+json' } as const
+
 export function useAuditLog() {
     const api = useApi()
 
     async function fetchLogs(filters?: AuditLogFilters): Promise<HydraCollection<AuditLogEntry>> {
-        const data = await api.get<HydraCollection<AuditLogEntry>>(
+        return api.get<HydraCollection<AuditLogEntry>>(
             '/audit-logs',
             buildQuery(filters),
-            { toast: false },
+            { toast: false, headers: JSONLD_HEADERS },
         )
+    }
+
+    /**
+     * Variante de `fetchLogs` qui met a jour le cache `lastCollection`.
+     * N'est utilisee que par la page admin — le composant Timeline appelle
+     * `fetchEntityLogs` qui bypass le cache pour ne pas polluer la reference
+     * page-level quand plusieurs timelines sont ouvertes.
+     */
+    async function fetchLogsCached(filters?: AuditLogFilters): Promise<HydraCollection<AuditLogEntry>> {
+        const data = await fetchLogs(filters)
         lastCollection.value = data
         return data
     }
 
     async function fetchLogById(id: string): Promise<AuditLogEntry> {
-        return api.get<AuditLogEntry>(`/audit-logs/${id}`, {}, { toast: false })
+        return api.get<AuditLogEntry>(`/audit-logs/${id}`, {}, { toast: false, headers: JSONLD_HEADERS })
     }
 
     async function fetchEntityLogs(
@@ -72,6 +89,9 @@ export function useAuditLog() {
         entityId: string | number,
         page: number = 1,
     ): Promise<HydraCollection<AuditLogEntry>> {
+        // Volontairement via `fetchLogs` (sans cache) pour ne pas ecraser
+        // `lastCollection` — la timeline peut etre rendue simultanement a
+        // la page globale et doit rester independante.
         return fetchLogs({
             entityType,
             entityId: String(entityId),
@@ -81,7 +101,7 @@ export function useAuditLog() {
 
     return {
         lastCollection,
-        fetchLogs,
+        fetchLogs: fetchLogsCached,
         fetchLogById,
         fetchEntityLogs,
         resetAuditLog,
diff --git a/frontend/shared/utils/api.ts b/frontend/shared/utils/api.ts
index fecbb8d..8e57001 100644
--- a/frontend/shared/utils/api.ts
+++ b/frontend/shared/utils/api.ts
@@ -1,18 +1,33 @@
+/**
+ * Schemas Hydra / API Platform 4.
+ *
+ * Important : API Platform 4 abandonne le prefixe `hydra:` dans les noms de
+ * proprietes (compare a la version 3). Un GET /api/audit-logs renvoie :
+ *   { "@context": ..., "@id": ..., "@type": "...",
+ *     "member": [...],
+ *     "totalItems": 30,
+ *     "view": { "@id": ..., "@type": "...", "first": ..., "next": ..., ... } }
+ *
+ * En `application/json` (sans ld), API Platform retourne un simple tableau
+ * plat sans ces metadonnees — on doit donc explicitement demander
+ * `application/ld+json` (via l'option `headers: { Accept: ... }` de useApi)
+ * pour avoir acces a la pagination.
+ */
 export interface HydraView {
     '@id'?: string
     '@type'?: string
-    'hydra:first'?: string
-    'hydra:last'?: string
-    'hydra:next'?: string
-    'hydra:previous'?: string
+    first?: string
+    last?: string
+    next?: string
+    previous?: string
 }
 
 export interface HydraCollection<T> {
-    'hydra:member': T[]
-    'hydra:totalItems': number
-    'hydra:view'?: HydraView
+    member: T[]
+    totalItems: number
+    view?: HydraView
 }
 
 export function extractHydraMembers<T>(collection: HydraCollection<T>): T[] {
-    return collection['hydra:member'] ?? []
+    return collection.member ?? []
 }
diff --git a/src/Module/Core/Infrastructure/ApiPlatform/Resource/AuditLogResource.php b/src/Module/Core/Infrastructure/ApiPlatform/Resource/AuditLogResource.php
index 94ee273..387fb08 100644
--- a/src/Module/Core/Infrastructure/ApiPlatform/Resource/AuditLogResource.php
+++ b/src/Module/Core/Infrastructure/ApiPlatform/Resource/AuditLogResource.php
@@ -44,6 +44,7 @@ use App\Module\Core\Infrastructure\ApiPlatform\State\Provider\AuditLogProvider;
         ),
         new Get(
             uriTemplate: '/audit-logs/{id}',
+            requirements: ['id' => '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}'],
             security: "is_granted('core.audit_log.view')",
             provider: AuditLogProvider::class,
         ),
diff --git a/src/Module/Core/Infrastructure/Audit/AuditLogWriter.php b/src/Module/Core/Infrastructure/Audit/AuditLogWriter.php
index d467abb..012e1de 100644
--- a/src/Module/Core/Infrastructure/Audit/AuditLogWriter.php
+++ b/src/Module/Core/Infrastructure/Audit/AuditLogWriter.php
@@ -69,7 +69,10 @@ final class AuditLogWriter
             'ip_address'   => $this->requestStack->getCurrentRequest()?->getClientIp(),
             'request_id'   => $this->requestIdProvider->getRequestId(),
         ], [
-            // Types de conversion DBAL : JSON encode jsonb + datetimetz.
+            // Types de conversion DBAL : UUID natif PG + jsonb + datetimetz.
+            // Sans 'id' => GUID, DBAL passerait un varchar et Postgres ferait
+            // un cast implicite — ca marche mais l'intention est floue.
+            'id'           => Types::GUID,
             'changes'      => Types::JSON,
             'performed_at' => Types::DATETIMETZ_IMMUTABLE,
         ]);
diff --git a/src/Module/Core/Infrastructure/Doctrine/AuditListener.php b/src/Module/Core/Infrastructure/Doctrine/AuditListener.php
index aac32c6..3011e49 100644
--- a/src/Module/Core/Infrastructure/Doctrine/AuditListener.php
+++ b/src/Module/Core/Infrastructure/Doctrine/AuditListener.php
@@ -47,6 +47,12 @@ use Throwable;
  *    (`getEntityChangeSet()` ne les couvre pas). Extension future via
  *    `getScheduledCollectionUpdates()`.
  *  - Les ManyToOne sont tracees par ID (null-safe via `?->getId()`).
+ *  - Les DELETE / UPDATE bulk DQL et les `Connection::executeStatement()`
+ *    bruts BYPASSENT le listener : onFlush n'est jamais appele. Toute
+ *    operation de purge/nettoyage qui doit etre auditee doit passer par
+ *    `EntityManager::remove()` + `flush()`. Si un futur batch (ex: commande
+ *    "purger users inactifs") utilise du DQL bulk, les suppressions ne
+ *    seront pas dans `audit_log` — choix d'architecture explicite a faire.
  */
 #[AsDoctrineListener(event: Events::onFlush)]
 #[AsDoctrineListener(event: Events::postFlush)]
diff --git a/tests/Module/Core/Api/AuditLogApiTest.php b/tests/Module/Core/Api/AuditLogApiTest.php
index 4c81c5c..609ea8b 100644
--- a/tests/Module/Core/Api/AuditLogApiTest.php
+++ b/tests/Module/Core/Api/AuditLogApiTest.php
@@ -28,9 +28,13 @@ use Symfony\Component\Uid\Uuid;
  */
 final class AuditLogApiTest extends AbstractApiTestCase
 {
-    private Connection $auditConnection;
+    // Proprietes nullable : si `bootKernel()` ou l'acces container echoue,
+    // `tearDown` se declenche quand meme et doit survivre a un setUp incomplet
+    // (sinon on masque l'exception d'origine avec un "typed property must not
+    // be accessed before initialization").
+    private ?Connection $auditConnection = null;
 
-    private string $runTag;
+    private ?string $runTag = null;
 
     protected function setUp(): void
     {
@@ -41,16 +45,22 @@ final class AuditLogApiTest extends AbstractApiTestCase
         $conn                  = self::getContainer()->get('doctrine.dbal.audit_connection');
         $this->auditConnection = $conn;
 
-        $this->runTag = 'api_audit_'.bin2hex(random_bytes(4));
+        $this->runTag = 'apiaudit'.bin2hex(random_bytes(4));
         $this->seedAuditLog();
     }
 
     protected function tearDown(): void
     {
-        $this->auditConnection->executeStatement(
-            'DELETE FROM audit_log WHERE request_id = :tag',
-            ['tag' => $this->runTag],
-        );
+        if (null !== $this->auditConnection && null !== $this->runTag) {
+            $this->auditConnection->executeStatement(
+                'DELETE FROM audit_log WHERE request_id = :tag',
+                ['tag' => $this->runTag],
+            );
+            // Close explicite pour liberer la connexion PG : en test, le
+            // kernel reboote et les connexions pendantes saturent le pool
+            // sur une suite de 200+ tests qui ouvrent 2 connexions chacun.
+            $this->auditConnection->close();
+        }
         parent::tearDown();
     }
 
@@ -85,6 +95,43 @@ final class AuditLogApiTest extends AbstractApiTestCase
         self::assertArrayHasKey('totalItems', $data);
     }
 
+    /**
+     * Le frontend force `Accept: application/ld+json` dans `useAuditLog` pour
+     * recuperer les cles prefixees `hydra:*` (et `hydra:view` pour la
+     * pagination). Ce test verrouille ce contrat : sans lui, un changement
+     * de configuration API Platform cassant le JSON-LD passerait inaperçu
+     * et le tableau admin apparaitrait silencieusement vide en production.
+     */
+    /**
+     * Le frontend demande explicitement `application/ld+json` dans `useAuditLog`
+     * pour obtenir l'objet Hydra complet (`member`, `totalItems`, `view`). Sous
+     * `application/json`, API Platform 4 renvoie un tableau plat sans ces
+     * metadonnees, ce qui casserait la pagination prev/next. Ce test verrouille
+     * le contrat : un changement de format par defaut ou une desactivation de
+     * JSON-LD produirait un 200 trompeur mais un tableau admin vide.
+     */
+    public function testJsonLdFormatExposesHydraEnvelope(): void
+    {
+        $client   = $this->authenticatedClient('admin', 'admin');
+        $response = $client->request('GET', '/api/audit-logs', [
+            'headers' => ['Accept' => 'application/ld+json'],
+        ]);
+
+        self::assertSame(200, $response->getStatusCode());
+        self::assertStringContainsString('application/ld+json', $response->getHeaders()['content-type'][0]);
+
+        $data = $response->toArray();
+        self::assertArrayHasKey('member', $data);
+        self::assertArrayHasKey('totalItems', $data);
+        // `view` n'est presente que si une pagination est active (plus d'items
+        // que la limite par page). Avec paginationItemsPerPage=30 et les 3
+        // lignes seedees (+ d'autres lignes de tests precedents), la collection
+        // peut excelder 30. Si presente, elle doit porter au moins @id.
+        if (isset($data['view'])) {
+            self::assertArrayHasKey('@id', $data['view']);
+        }
+    }
+
     public function testAdminGets200(): void
     {
         $client   = $this->authenticatedClient('admin', 'admin');
diff --git a/tests/Module/Core/Infrastructure/Doctrine/AuditListenerTest.php b/tests/Module/Core/Infrastructure/Doctrine/AuditListenerTest.php
index 523500b..24b6841 100644
--- a/tests/Module/Core/Infrastructure/Doctrine/AuditListenerTest.php
+++ b/tests/Module/Core/Infrastructure/Doctrine/AuditListenerTest.php
@@ -48,7 +48,7 @@ final class AuditListenerTest extends KernelTestCase
 
         // Tag unique par run pour filtrer les lignes audit_log produites
         // exclusivement par ce test (la table n'a ni truncate ni rollback).
-        $this->testRunTag = 'audit_test_'.bin2hex(random_bytes(4));
+        $this->testRunTag = 'audittest'.bin2hex(random_bytes(4));
     }
 
     protected function tearDown(): void
@@ -71,6 +71,11 @@ final class AuditListenerTest extends KernelTestCase
             ['tag' => $this->testRunTag.'%'],
         );
 
+        // Libere la connexion PG : en test, le kernel reboote par test et
+        // sans close explicite, la connexion `audit` reste ouverte jusqu'au
+        // TTL Doctrine, saturant le pool sur une suite de 200+ tests.
+        $this->auditConnection->close();
+
         parent::tearDown();
     }
 
@@ -154,7 +159,7 @@ final class AuditListenerTest extends KernelTestCase
      */
     private function fetchAuditRows(int $userId): array
     {
-        /** @var list<array{id: string, entity_type: string, entity_id: string, action: string, changes: string}> $rows */
+        // @var list<array{id: string, entity_type: string, entity_id: string, action: string, changes: string}> $rows
         return $this->auditConnection->fetchAllAssociative(
             'SELECT id, entity_type, entity_id, action, changes FROM audit_log WHERE entity_type = :type AND entity_id = :id ORDER BY performed_at ASC',
             ['type' => 'core.User', 'id' => (string) $userId],