Skip to content

fix: add unit test for check the id on event listener #565

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 1 commit into
base: main
Choose a base branch
from
+57 −3
Open

fix: add unit test for check the id on event listener #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 9 additions & 3 deletions app/Audit/AuditEventListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,15 @@ class AuditEventListener
{
private const ROUTE_METHOD_SEPARATOR = '|';
private $em;

protected function shouldSkipInTest(): bool
{
return app()->environment('testing');
}

public function onFlush(OnFlushEventArgs $eventArgs): void
{
if (app()->environment('testing')) {
if ($this->shouldSkipInTest()) {
return;
}
$this->em = $eventArgs->getObjectManager();
Expand Down Expand Up @@ -83,7 +89,7 @@ public function onFlush(OnFlushEventArgs $eventArgs): void
/**
* Get the appropriate audit strategy based on environment configuration
*/
private function getAuditStrategy($em): ?IAuditStrategy
protected function getAuditStrategy($em): ?IAuditStrategy
{
// Check if OTLP audit is enabled
if (config('opentelemetry.enabled', false)) {
Expand All @@ -102,7 +108,7 @@ private function getAuditStrategy($em): ?IAuditStrategy
return new AuditLogStrategy($em);
}

private function buildAuditContext(): AuditContext
protected function buildAuditContext(): AuditContext
{
$resourceCtx = app(\models\oauth2\IResourceServerContext::class);
$userExternalId = $resourceCtx->getCurrentUserId();
Expand Down
48 changes: 48 additions & 0 deletions tests/OpenTelemetry/Formatters/AuditEventListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,20 @@
* limitations under the License.
**/

use App\Audit\AuditContext;
use App\Audit\AuditEventListener;
use App\Audit\Interfaces\IAuditStrategy;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\DBAL\Connection;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Event\OnFlushEventArgs;
use Doctrine\ORM\Event\PostFlushEventArgs;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\DefaultNamingStrategy;
use Doctrine\ORM\Mapping\ManyToManyOwningSideMapping;
use Doctrine\ORM\Mapping\OneToManyAssociationMapping;
use Doctrine\ORM\PersistentCollection;
use Doctrine\ORM\UnitOfWork;
use Tests\TestCase;

class AuditEventListenerTest extends TestCase
Expand Down Expand Up @@ -217,4 +221,48 @@ public function testAuditCollectionDeleteUninitializedUsesJoinTableQuery(): void
$this->assertSame([10, 11], $payload['deleted_ids']);
$this->assertSame(IAuditStrategy::EVENT_COLLECTION_MANYTOMANY_DELETE, $eventType);
}

public function testCreationAuditIsBufferedInOnFlushAndDispatchedInPostFlush(): void
{
// Simulate a new entity not yet persisted: getId() returns 0 (pre-INSERT state).
$entity = $this->getMockBuilder(\models\summit\SummitEvent::class)
->disableOriginalConstructor()
->onlyMethods(['getId'])
->getMock();
$entity->method('getId')->willReturn(0);

$uow = $this->createMock(UnitOfWork::class);
$uow->method('getScheduledEntityInsertions')->willReturn([$entity]);
$uow->method('getScheduledEntityUpdates')->willReturn([]);
$uow->method('getScheduledEntityDeletions')->willReturn([]);
$uow->method('getScheduledCollectionDeletions')->willReturn([]);
$uow->method('getScheduledCollectionUpdates')->willReturn([]);

$em = $this->createMock(EntityManagerInterface::class);
$em->method('getUnitOfWork')->willReturn($uow);

$strategy = $this->createMock(IAuditStrategy::class);

$listener = $this->getMockBuilder(AuditEventListener::class)
->onlyMethods(['shouldSkipInTest', 'getAuditStrategy', 'buildAuditContext'])
->getMock();
$listener->method('shouldSkipInTest')->willReturn(false);
$listener->method('getAuditStrategy')->willReturn($strategy);
$listener->method('buildAuditContext')->willReturn(new AuditContext());

$capturedId = null;
$strategy->method('audit')->willReturnCallback(
function ($subject) use (&$capturedId) {
$capturedId = method_exists($subject, 'getId') ? $subject->getId() : null;
}
);

$listener->onFlush(new OnFlushEventArgs($em));

// Bug: audit() was called during onFlush — before the INSERT — so the captured id is 0.
$this->assertNotNull($capturedId,
'audit() must not be called during onFlush (INSERT has not run yet)');
$this->assertNotEquals(0, $capturedId,
'entity id captured during onFlush is 0 — the INSERT has not assigned the real id yet');
}
Comment on lines +225 to +267

@coderabbitai coderabbitai Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -nP '\b(postFlush|getSubscribedEvents|buffer|deferred?)\b' app/Audit/AuditEventListener.php
ast-grep outline app/Audit/AuditEventListener.php

Repository: OpenStackweb/summit-api

Length of output: 208

🏁 Script executed:

ast-grep outline app/Audit/AuditEventListener.php

Repository: OpenStackweb/summit-api

Length of output: 208

🏁 Script executed:

sed -n '/function onFlush/,/^    }$/p' app/Audit/AuditEventListener.php

Repository: OpenStackweb/summit-api

Length of output: 2213

🏁 Script executed:

#!/bin/bash
# Inspect the specific test logic to confirm the assertion contradiction
sed -n '225,267p' tests/OpenTelemetry/Formatters/AuditEventListenerTest.php

Repository: OpenStackweb/summit-api

Length of output: 2257

Production listener ignores intended "buffer then dispatch" strategy; test assertions and messages are inverted.

The onFlush implementation in app/Audit/AuditEventListener.php executes audits immediately during the onFlush event:

foreach ($uow->getScheduledEntityInsertions() as $entity) {
    $strategy->audit($entity, [], IAuditStrategy::EVENT_ENTITY_CREATION, $ctx);
}

There is no buffering logic or postFlush subscriber method to defer this until the database INSERT completes. Consequently, the test testCreationAuditIsBufferedInOnFlushAndDispatchedInPostFlush correctly captures the pre-INSERT ID 0, causing the assertNotEquals(0, $capturedId) to fail.

Additionally, the assertion messages are logically inverted:

  • assertNotNull passes if audit runs, yet the message claims "must not be called".
  • assertNotEquals fails because ID IS 0, yet the message states "INSERT has not assigned the real id yet" as if asserting a success condition.

Align the assertion logic with the current behavior or implement the deferred postFlush dispatching if that is the desired requirement.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/OpenTelemetry/Formatters/AuditEventListenerTest.php` around lines 225 -
267, The test in AuditEventListenerTest is asserting a deferred “buffer then
dispatch” flow, but AuditEventListener::onFlush currently audits entity
insertions immediately, so the captured ID will still be 0. Update the test to
match the actual behavior or, if buffering is intended, add the missing
postFlush dispatch path in AuditEventListener and move the audit call out of
onFlush. Also fix the assertion logic/messages so they reflect the real
expectation around when IAuditStrategy::audit is invoked and what ID value
should be observed.

}
Loading