DamienHarper / auditor

auditor, the missing audit log library
MIT License
164 stars 53 forks source link

Symfony uuid type support #230

Open CalinBolea opened 3 weeks ago

CalinBolea commented 3 weeks ago

Hello, not sure if this is a bug or it's intended so I'm adding a support question.

First of all, thank you to all the contributors and to @DamienHarper especially for all the work done here! A lot of people appreciate what you are doing!

A bit of context: A relatively simple symfony7.1 app, using php8.3, doctrine as ORM and MySql8.0. Relatively standard entities. We are using uuids, they're binary(16) in the db. Example config for ids:

#[ORM\Id]
#[ORM\Column(type: 'uuid', unique: true)]
private Uuid $id;

From what I can see, up until version 2.4.8, this type was cast into string in the AuditTrait https://github.com/DamienHarper/auditor/blob/2.4.8/src/Provider/Doctrine/Auditing/Transaction/AuditTrait.php#L106-L111

From version 3 onward, this type is not being cast anymore, only ulid: https://github.com/DamienHarper/auditor/blob/3.0.0/src/Provider/Doctrine/Auditing/Transaction/AuditTrait.php#L109

I see this change was made in a PR that refers to setting up docker for testing so I figure it might not be intended. https://github.com/DamienHarper/auditor/pull/206

The issue is that uuid binary representations are not valid strings to be persisted in the audit tables as object_ids (which are varchar(255)). The exceptions also fail silently https://github.com/DamienHarper/auditor/blob/master/src/EventSubscriber/AuditEventSubscriber.php#L35 So nothing gets stored anymore and you don't get any feedback.

The "naive" fix I came up with is to listen to the event just before the AuditEventSubscriber kicks in and just transform the object_id representation from binary to rfc4122.

    public static function getSubscribedEvents(): array
    {
        return [
            LifecycleEvent::class => [
                ['fixUuidRepresentation', -999_999],  // should be fired just before the audit event subscriber
            ],
        ];
    }

    public function fixUuidRepresentation(LifecycleEvent $event): LifecycleEvent
    {
        $payload = $event->getPayload();

        try {
            $uuid = Uuid::fromString((string) $payload['object_id']);
        } catch (InvalidArgumentException) {
            return $event;
        }

        $payload['object_id'] = $uuid->toRfc4122();
        $event->setPayload($payload);

        return $event;
    }

My questions in the end are: Is this intended? Am I missing something? Would it make sense to revert the change for the AuditorTrait and cast the uuid type as string again (I would gladly contribute if this is the case) or is there a better solution anyone could recommend?

Thank you again for the tool you've built and all the time invested!