feat: add PostgreSQL driver support #4
Conversation
-Adds native PostgreSQL support to Laravel Audit Logger via a new `PostgreSQLDriver The driver mirrors the MySQL implementation while using PostgreSQL’s jsonb columns and indexing for better performance and schema compatibility. Key Changes - Added PostgreSQLDriver under src/Drivers/ - Updated AuditLoggerServiceProvider to resolve drivers dynamically - Added postgresql config block to config/audit-logger.php - Added PostgreSQLDriverTest with full CRUD, schema, and migration coverage Usage AUDIT_DRIVER=postgresql AUDIT_PGSQL_CONNECTION=pgsql Testing composer test --filter=PostgreSQLDriverTest
Summary of ChangesHello @kalebalebachew, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Laravel Audit Logger by integrating native PostgreSQL support. It introduces a new driver that intelligently utilizes PostgreSQL's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces PostgreSQL support by adding a new PostgreSQLDriver, updating the service provider for dynamic driver resolution, and including necessary configurations and tests. The approach of mirroring the MySQLDriver is sound for consistency. The new driver correctly utilizes jsonb columns for better performance with PostgreSQL.
However, I've identified a critical issue where the EloquentAuditLog model factory is hardcoded with MySQL configurations, which could break the PostgreSQL driver under certain setups. Additionally, there's a significant performance concern with the storeBatch method, and some minor improvements could be made to the new driver and its tests for clarity and correctness. I've detailed these points in the specific comments below.
| $this->ensureStorageExists($log->getEntityType()); | ||
|
|
||
| try { | ||
| $model = EloquentAuditLog::forEntity(entityClass: $log->getEntityType()); |
There was a problem hiding this comment.
The EloquentAuditLog::forEntity() method appears to be hardcoded to use the mysql driver's configuration for determining table names (i.e., table_prefix and table_suffix). This will cause the PostgreSQLDriver to generate incorrect table names if its prefix/suffix configuration differs from the mysql driver's settings, breaking the driver's functionality. This issue also affects the call on line 122.
To resolve this, EloquentAuditLog::forEntity() should be updated to accept the driver name (e.g., 'postgresql') and use it to retrieve the correct configuration. The MySQLDriver would also need to be updated to pass its driver name.
| public function storeBatch(array $logs): void | ||
| { | ||
| if (empty($logs)) { | ||
| return; | ||
| } | ||
|
|
||
| // Group logs by entity type (and thus by table) | ||
| $groupedLogs = []; | ||
| foreach ($logs as $log) { | ||
| $this->validateEntityType($log->getEntityType()); | ||
| $entityType = $log->getEntityType(); | ||
| $groupedLogs[$entityType][] = $log; | ||
| } | ||
|
|
||
| // Process each entity type separately using Eloquent models to leverage casting | ||
| foreach ($groupedLogs as $entityType => $entityLogs) { | ||
| $this->ensureStorageExists($entityType); | ||
|
|
||
| // Use Eloquent models to leverage automatic JSON casting | ||
| foreach ($entityLogs as $log) { | ||
| $model = EloquentAuditLog::forEntity(entityClass: $entityType); | ||
| $model->setConnection($this->connection); | ||
| $model->fill([ | ||
| 'entity_id' => $log->getEntityId(), | ||
| 'action' => $log->getAction(), | ||
| 'old_values' => $log->getOldValues(), // Eloquent casting handles JSON encoding | ||
| 'new_values' => $log->getNewValues(), // Eloquent casting handles JSON encoding | ||
| 'causer_type' => $log->getCauserType(), | ||
| 'causer_id' => $log->getCauserId(), | ||
| 'metadata' => $log->getMetadata(), // Eloquent casting handles JSON encoding | ||
| 'created_at' => $log->getCreatedAt(), | ||
| 'source' => $log->getSource(), | ||
| ]); | ||
| $model->save(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The storeBatch method currently iterates through logs and saves them individually, resulting in multiple database queries. This is inefficient for a batch operation and the method name is misleading. For large numbers of logs, this could cause performance problems.
For a true batch operation, you should consider collecting all log data into an array and using a single insert statement. This would be much more performant. Note that this would require manually encoding JSON fields, as a mass-insert operation bypasses Eloquent's automatic casting.
| try { | ||
| $model = EloquentAuditLog::forEntity(entityClass: $log->getEntityType()); | ||
| $model->setConnection($this->connection); | ||
| $model->fill([ | ||
| 'entity_id' => $log->getEntityId(), | ||
| 'action' => $log->getAction(), | ||
| 'old_values' => $log->getOldValues(), // Remove manual json_encode - let Eloquent handle it | ||
| 'new_values' => $log->getNewValues(), // Remove manual json_encode - let Eloquent handle it | ||
| 'causer_type' => $log->getCauserType(), | ||
| 'causer_id' => $log->getCauserId(), | ||
| 'metadata' => $log->getMetadata(), // Remove manual json_encode - let Eloquent handle it | ||
| 'created_at' => $log->getCreatedAt(), | ||
| 'source' => $log->getSource(), | ||
| ]); | ||
| $model->save(); | ||
| } catch (\Exception $e) { | ||
| throw $e; | ||
| } |
There was a problem hiding this comment.
The try-catch block here is redundant because it only catches the exception to re-throw it. Removing it will make the code simpler and easier to read without changing the functionality.
$model = EloquentAuditLog::forEntity(entityClass: $log->getEntityType());
$model->setConnection($this->connection);
$model->fill([
'entity_id' => $log->getEntityId(),
'action' => $log->getAction(),
'old_values' => $log->getOldValues(), // Remove manual json_encode - let Eloquent handle it
'new_values' => $log->getNewValues(), // Remove manual json_encode - let Eloquent handle it
'causer_type' => $log->getCauserType(),
'causer_id' => $log->getCauserId(),
'metadata' => $log->getMetadata(), // Remove manual json_encode - let Eloquent handle it
'created_at' => $log->getCreatedAt(),
'source' => $log->getSource(),
]);
$model->save();| // Directly call the createStorageForEntity method since ensureStorageExists might not work in tests | ||
| $this->driver->createStorageForEntity('App\\Models\\Order'); |
There was a problem hiding this comment.
This test is named test_ensure_storage_exists_creates_table_if_needed, but it directly calls createStorageForEntity instead of ensureStorageExists. As a result, it's not actually testing the logic within ensureStorageExists. The test should call $this->driver->ensureStorageExists(...) to correctly verify its behavior.
// Call ensureStorageExists to test the auto-migration logic
$this->driver->ensureStorageExists('App\\Models\\Order');|
|
||
| protected function tearDown(): void | ||
| { | ||
| Mockery::close(); |
There was a problem hiding this comment.
The PostgreSQLDriver uses static properties for caching ($existingTables, $configCache), which persist between tests. This can lead to test inter-dependency and unexpected behavior, especially when tests modify configuration values. To ensure proper test isolation, you should clear these caches in the tearDown method.
PostgreSQLDriver::clearCache();
Mockery::close();
3 participants
-Adds native PostgreSQL support to Laravel Audit Logger via a new
PostgreSQLDriverThe driver mirrors the MySQL implementation while using PostgreSQL’s jsonb columns and indexing for better performance and schema compatibility.Key Changes
Added PostgreSQLDriver under src/Drivers/
Updated AuditLoggerServiceProvider to resolve drivers dynamically
Added postgresql config block to config/audit-logger.php
Added PostgreSQLDriverTest with full CRUD, schema, and migration coverage
Usage
Testing