Skip to content

Feature: add a new event for many-to-many relationships #498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andrestejerina97 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Feature: add a new event for many-to-many relationships #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4ecfbe
feat: add a new event for many-to-many relationships
andrestejerina97 Mar 5, 2026
3fdfd17
fix: change in strategy for adapt to adr
andrestejerina97 Mar 17, 2026
d7a2af6
feat: new workflow for cases where the collection has already started…
andrestejerina97 Mar 17, 2026
14e01c5
fix: new test and remove unused parameters
andrestejerina97 Apr 6, 2026
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
88 changes: 87 additions & 1 deletion app/Audit/AbstractAuditLogFormatter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace App\Audit;

use App\Audit\Utils\DateFormatter;
use Doctrine\ORM\PersistentCollection;

/**
* Copyright 2025 OpenStack Foundation
Expand Down Expand Up @@ -32,10 +33,73 @@ final public function setContext(AuditContext $ctx): void
$this->ctx = $ctx;
}

protected function handleManyToManyCollection(array $change_set): ?PersistentCollectionMetadata
{
if (!isset($change_set['collection'])) {
return null;
}

$collection = $change_set['collection'];
if (!($collection instanceof PersistentCollection)) {
return null;
}

$preloadedDeletedIds = $change_set['deleted_ids'] ?? [];
if (!is_array($preloadedDeletedIds)) {
$preloadedDeletedIds = [];
}

return PersistentCollectionMetadata::fromCollection($collection, $preloadedDeletedIds);
}

protected function processCollection(PersistentCollectionMetadata $metadata): ?array
{
$addedIds = [];
$removedIds = [];

if (!empty($metadata->preloadedDeletedIds)) {
$removedIds = array_values(array_unique(array_map('intval', $metadata->preloadedDeletedIds)));
sort($removedIds);
} else {
$addedIds = $this->extractCollectionEntityIds($metadata->collection->getInsertDiff());
$removedIds = $this->extractCollectionEntityIds($metadata->collection->getDeleteDiff());
}

return [
'field' => $metadata->fieldName,
'target_entity' => class_basename($metadata->targetEntity),
'is_deletion' => $this->event_type === Interfaces\IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE,
'added_ids' => $addedIds,
'removed_ids' => $removedIds,
];
}


/**
* Extract IDs from entity objects in collection
*/
protected function extractCollectionEntityIds(array $entities): array
{
$ids = [];
foreach ($entities as $entity) {
if (method_exists($entity, 'getId')) {
$id = $entity->getId();
if ($id !== null) {
$ids[] = $id;
}
}
}

$uniqueIds = array_unique($ids);
sort($uniqueIds);

return array_values($uniqueIds);
}

protected function getUserInfo(): string
{
if (app()->runningInConsole()) {
return 'Worker Job';
return 'Worker Job';
}
if (!$this->ctx) {
return 'Unknown (unknown)';
Expand Down Expand Up @@ -129,5 +193,27 @@ protected function formatFieldChange(string $prop_name, $old_value, $new_value):
return sprintf("Property \"%s\" has changed from \"%s\" to \"%s\"", $prop_name, $old_display, $new_display);
}

/**
* Format detailed message for many-to-many collection changes
*/
protected static function formatManyToManyDetailedMessage(array $details, int $addCount, int $removeCount, string $action): string
{
$field = $details['field'] ?? 'unknown';
$target = $details['target_entity'] ?? 'unknown';
$addedIds = $details['added_ids'] ?? [];
$removedIds = $details['removed_ids'] ?? [];

$parts = [];
if (!empty($addedIds)) {
$parts[] = sprintf("Added %d %s(s): %s", $addCount, $target, implode(', ', $addedIds));
}
if (!empty($removedIds)) {
$parts[] = sprintf("Removed %d %s(s): %s", $removeCount, $target, implode(', ', $removedIds));
}

$detailStr = implode(' | ', $parts);
return sprintf("Many-to-Many collection '%s' %s: %s", $field, $action, $detailStr);
}

abstract public function format(mixed $subject, array $change_set): ?string;
}
123 changes: 117 additions & 6 deletions app/Audit/AuditEventListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

use App\Audit\Interfaces\IAuditStrategy;
use Doctrine\ORM\Event\OnFlushEventArgs;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\PersistentCollection;
use Illuminate\Support\Facades\App;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Route;
Expand All @@ -25,16 +27,16 @@
class AuditEventListener
{
private const ROUTE_METHOD_SEPARATOR = '|';

private $em;
public function onFlush(OnFlushEventArgs $eventArgs): void
{
if (app()->environment('testing')) {
return;
}
$em = $eventArgs->getObjectManager();
$uow = $em->getUnitOfWork();
$this->em = $eventArgs->getObjectManager();
$uow = $this->em->getUnitOfWork();
// Strategy selection based on environment configuration
$strategy = $this->getAuditStrategy($em);
$strategy = $this->getAuditStrategy($this->em);
if (!$strategy) {
return; // No audit strategy enabled
}
Expand All @@ -52,11 +54,23 @@ public function onFlush(OnFlushEventArgs $eventArgs): void

foreach ($uow->getScheduledEntityDeletions() as $entity) {
$strategy->audit($entity, [], IAuditStrategy::EVENT_ENTITY_DELETION, $ctx);
}
foreach ($uow->getScheduledCollectionDeletions() as $col) {
[$subject, $payload, $eventType] = $this->auditCollection($col, IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE);

if (!is_null($subject)) {
$strategy->audit($subject, $payload, $eventType, $ctx);
}
}

foreach ($uow->getScheduledCollectionUpdates() as $col) {
$strategy->audit($col, [], IAuditStrategy::EVENT_COLLECTION_UPDATE, $ctx);
[$subject, $payload, $eventType] = $this->auditCollection($col, IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_UPDATE);

if (!is_null($subject)) {
$strategy->audit($subject, $payload, $eventType, $ctx);
}
}

} catch (\Exception $e) {
Log::error('Audit event listener failed', [
'error' => $e->getMessage(),
Expand Down Expand Up @@ -98,7 +112,7 @@ private function buildAuditContext(): AuditContext
$member = $memberRepo->findOneBy(["user_external_id" => $userExternalId]);
}

//$ui = app()->bound('ui.context') ? app('ui.context') : [];
$ui = [];

$req = request();
$rawRoute = null;
Expand Down Expand Up @@ -127,4 +141,101 @@ private function buildAuditContext(): AuditContext
rawRoute: $rawRoute
);
}

/**
* Audit collection changes
* Returns triple: [$subject, $payload, $eventType]
* Subject will be null if collection should not be audited
*
* @param object $subject The collection
* @param string $eventType The event type constant (EVENT_COLLECTION_MANYTOMANY_DELETE or EVENT_COLLECTION_MANYTOMANY_UPDATE)
* @return array [$subject, $payload, $eventType]
*/
private function auditCollection($subject, string $eventType): array
{
if (!$subject instanceof PersistentCollection) {
return [null, null, null];
}

$mapping = $subject->getMapping();

if (!$mapping->isManyToMany()) {
return [$subject, [], IAuditStrategy::EVENT_COLLECTION_UPDATE];
}

if (!$mapping->isOwningSide()) {
Log::debug("AuditEventListener::Skipping audit for non-owning side of many-to-many collection");
return [null, null, null];
}

$owner = $subject->getOwner();
if ($owner === null) {
return [null, null, null];
}

$payload = ['collection' => $subject];

if ($eventType === IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE
&& (
!$subject->isInitialized()
Copy link
Copy Markdown
Collaborator

@smarcet smarcet Mar 30, 2026

Choose a reason for hiding this comment

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

@andrestejerina97
The condition on lines 181-182 has two branches:

  1. Branch A (!isInitialized()) : collection was never loaded into memory. This is the primary scenario from
    https://github.com/OpenStackweb/summit-api/blob/b3d03599b463f7255ba20dc26dca1ef2326ef4d1/adr/0002-m2m-audit-logging-lightweight-join-table-query.md and the production incident in commit
    728ae67.
  2. Branch B (isInitialized() && getDeleteDiff() === 0) : collection was loaded but clear() took a new snapshot, so diffs are empty. This is the scenario from your comment here https://app.clickup.com/t/86b84w9x2?comment=90140187621105

testAuditCollectionDeleteInitializedWithoutDiffUsesJoinTableQuery covers Branch B (it calls takeSnapshot(), making the collection initialized). But Branch A , the ADR's primary scenario, has no test.

This matters because Branch A's !isInitialized() short-circuit is what prevents getDeleteDiff() from being called on an uninitialized collection, which would trigger Doctrine hydration and the memory blowup the ADR was written to prevent. A future refactor that removes the isInitialized() guard (e.g. simplifying to just count($subject->getDeleteDiff()) === 0) would reintroduce the production issue with no test to catch it.

Can you add a test similar to testAuditCollectionDeleteInitializedWithoutDiffUsesJoinTableQuery but without the $collection->takeSnapshot() call, so isInitialized() returns false? That would cover the uninitialized path end-to-end.

|| ($subject->isInitialized() && count($subject->getDeleteDiff()) === 0)
)) {
if ($this->em instanceof EntityManagerInterface) {
$payload['deleted_ids'] = $this->fetchManyToManyIds($subject, $this->em);
}
}

return [$owner, $payload, $eventType];
}


private function fetchManyToManyIds(PersistentCollection $collection, EntityManagerInterface $em): array
{
try {
$mapping = $collection->getMapping();
$joinTable = $mapping->joinTable;
$tableName = is_array($joinTable) ? ($joinTable['name'] ?? null) : ($joinTable->name ?? null);
$joinColumns = is_array($joinTable) ? ($joinTable['joinColumns'] ?? []) : ($joinTable->joinColumns ?? []);
$inverseJoinColumns = is_array($joinTable) ? ($joinTable['inverseJoinColumns'] ?? []) : ($joinTable->inverseJoinColumns ?? []);

$joinColumn = $joinColumns[0] ?? null;
$inverseJoinColumn = $inverseJoinColumns[0] ?? null;
$sourceColumn = is_array($joinColumn) ? ($joinColumn['name'] ?? null) : ($joinColumn->name ?? null);
$targetColumn = is_array($inverseJoinColumn) ? ($inverseJoinColumn['name'] ?? null) : ($inverseJoinColumn->name ?? null);

if (!$sourceColumn || !$targetColumn || !$tableName) {
return [];
}

$owner = $collection->getOwner();
if ($owner === null) {
return [];
}

$ownerId = method_exists($owner, 'getId') ? $owner->getId() : null;
if ($ownerId === null) {
$ownerMeta = $em->getClassMetadata(get_class($owner));
$ownerIds = $ownerMeta->getIdentifierValues($owner);
$ownerId = empty($ownerIds) ? null : reset($ownerIds);
}

if ($ownerId === null) {
return [];
}

$ids = $em->getConnection()->fetchFirstColumn(
"SELECT {$targetColumn} FROM {$tableName} WHERE {$sourceColumn} = ?",
[$ownerId]
);

return array_values(array_map('intval', $ids));

} catch (\Exception $e) {
Log::error("AuditEventListener::fetchManyToManyIds error: " . $e->getMessage(), [
'exception' => get_class($e),
'trace' => $e->getTraceAsString()
]);
return [];
}
}
}
17 changes: 14 additions & 3 deletions app/Audit/AuditLogFormatterFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
use App\Audit\ConcreteFormatters\EntityCollectionUpdateAuditLogFormatter;
use App\Audit\ConcreteFormatters\EntityCreationAuditLogFormatter;
use App\Audit\ConcreteFormatters\EntityDeletionAuditLogFormatter;
use App\Audit\ConcreteFormatters\DefaultEntityManyToManyCollectionUpdateAuditLogFormatter;
use App\Audit\ConcreteFormatters\DefaultEntityManyToManyCollectionDeleteAuditLogFormatter;
use App\Audit\ConcreteFormatters\EntityUpdateAuditLogFormatter;
use App\Audit\Interfaces\IAuditStrategy;
use Doctrine\ORM\PersistentCollection;
Expand Down Expand Up @@ -57,9 +59,7 @@ public function make(AuditContext $ctx, $subject, string $event_type): ?IAuditLo
);
if (method_exists($subject, 'getTypeClass')) {
$type = $subject->getTypeClass();
// Your log shows this is ClassMetadata
if ($type instanceof ClassMetadata) {
// Doctrine supports either getName() or public $name
$targetEntity = method_exists($type, 'getName') ? $type->getName() : ($type->name ?? null);
} elseif (is_string($type)) {
$targetEntity = $type;
Expand All @@ -71,7 +71,6 @@ public function make(AuditContext $ctx, $subject, string $event_type): ?IAuditLo
$targetEntity = $mapping['targetEntity'] ?? null;
Log::debug("AuditLogFormatterFactory::make getMapping targetEntity {$targetEntity}");
} else {
// last-resort: read private association metadata (still no hydration)
$ref = new \ReflectionObject($subject);
foreach (['association', 'mapping', 'associationMapping'] as $propName) {
if ($ref->hasProperty($propName)) {
Expand Down Expand Up @@ -107,6 +106,18 @@ public function make(AuditContext $ctx, $subject, string $event_type): ?IAuditLo

$formatter = new EntityCollectionUpdateAuditLogFormatter($child_entity_formatter);
break;
case IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_UPDATE:
$formatter = $this->getFormatterByContext($subject, $event_type, $ctx);
if (is_null($formatter)) {
$formatter = new DefaultEntityManyToManyCollectionUpdateAuditLogFormatter();
}
break;
case IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE:
$formatter = $this->getFormatterByContext($subject, $event_type, $ctx);
if (is_null($formatter)) {
$formatter = new DefaultEntityManyToManyCollectionDeleteAuditLogFormatter();
}
break;
case IAuditStrategy::EVENT_ENTITY_CREATION:
$formatter = $this->getFormatterByContext($subject, $event_type, $ctx);
if(is_null($formatter)) {
Expand Down
Loading
Loading