fix(audit-log) : address code review findings

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).
2026-04-20 21:10:46 +02:00
parent de39fe6a3e
commit 37eafd276c
11 changed files with 153 additions and 37 deletions
--- 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
--- 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}
--- 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()
--- 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.
--- 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,
--- 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 ?? []
 }
--- 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,
        ),
--- 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,
        ]);
--- 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)]
--- 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');
--- 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],