Added new internal attributes $createdBy and$ updatedBy #680
Conversation
WalkthroughAdds createdBy/updatedBy auditing across the database layer and Authorization user-tracking; extends adapter API by adding a nullable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Auth as Authorization
participant DB as Database
participant Adapter as SQL Adapter
Note over Client,Adapter: Create / Update flow with createdBy/updatedBy
Client->>Auth: setRole(role)
Auth-->>Client: ok
Client->>DB: createDocument(document)
DB->>Auth: getUser()
Auth-->>DB: userId|null
DB->>DB: ensure $createdBy/$updatedBy (fallback to userId when missing)
DB->>Adapter: persist document (include _createdBy/_updatedBy, _createdAt/_updatedAt)
Adapter-->>DB: result
DB-->>Client: document (exposes $createdBy/$updatedBy)
sequenceDiagram
autonumber
actor Client
participant DB as Database
participant Adapter as SQL Adapter
Note over Client,Adapter: increaseDocumentAttribute with updatedBy
Client->>DB: increaseDocumentAttribute(col,id,attr,value,updatedAt,updatedBy,...)
DB->>Adapter: UPDATE ... SET attr=attr+:value, _updatedAt=:updatedAt, _updatedBy=:updatedBy
Adapter-->>DB: bool
DB-->>Client: bool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ArnabChatterjee20k
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
src/Database/Validator/Authorization.php (1)
135-135: Missing user cleanup in cleanRoles method.The
cleanRoles()method should also clear the tracked user to maintain consistency with the role cleanup.public static function cleanRoles(): void { self::$roles = []; + self::$user = null; }src/Database/Adapter/MariaDB.php (3)
1211-1217: Also update_updatedByin upsert-increment pathWhen using the “increment specific column” upsert path,
_updatedByshould be updated alongside_updatedAt.if (!empty($attribute)) { // Increment specific column by its new value in place $updateColumns = [ $getUpdateClause($attribute, increment: true), $getUpdateClause('_updatedAt'), + $getUpdateClause('_updatedBy'), ];
157-188: Add schema migration for_createdByand_updatedBy
No ALTER TABLE statements were found for the new_createdByand_updatedBycolumns. You must provide a forward-compatible migration (e.g.ALTER TABLE your_table ADD COLUMN _createdBy VARCHAR(255) DEFAULT NULL, ADD COLUMN _updatedBy VARCHAR(255) DEFAULT NULL;) so that inserts/updates don’t fail on existing tables.
1256-1265: DefaultupdatedByinincreaseDocumentAttributewhen null
In src/Database/Database.php at the adapter call (around line 5384), you forward$document->getUpdatedBy()without fallback—unlikedecreaseDocumentAttribute, which usesAuthorization::getUser()when null. Change to:$updatedBy = $document->getUpdatedBy() ?? Authorization::getUser(); $this->adapter->increaseDocumentAttribute( …, $updatedAt, $updatedBy, max: $max );src/Database/Adapter/Postgres.php (3)
1316-1319: Also update_updatedByin upsert-increment pathMirror MariaDB: include
_updatedBywhen increment upserting.if (!empty($attribute)) { // Increment specific column by its new value in place $updateColumns = [ $getUpdateClause($attribute, increment: true), $getUpdateClause('_updatedAt'), + $getUpdateClause('_updatedBy'), ];
852-857: Map$createdBy/$updatedByto internal columns in index creationOtherwise, creating indexes on these public keys will target non-existent columns.
- $attr = match ($attr) { + $attr = match ($attr) { '$id' => '_uid', '$createdAt' => '_createdAt', '$updatedAt' => '_updatedAt', + '$createdBy' => '_createdBy', + '$updatedBy' => '_updatedBy', default => $this->filter($attr), };
235-247: Implement migration scripts to add_createdByand_updatedBycolumns for all existing tables. NoALTER TABLE … ADD COLUMNstatements were found for these auditing fields, so existing databases will error on insert/update without these columns.src/Database/Adapter/SQL.php (1)
1889-1895: Always project$createdBy/$updatedByfor non- selections (parity with dates)*Without adding these, selective projections omit auditing fields even though
$createdAt/$updatedAtare auto-included.- $internalKeys = [ + $internalKeys = [ '$id', '$sequence', '$permissions', '$createdAt', '$updatedAt', + '$createdBy', + '$updatedBy', ];src/Database/Database.php (3)
5443-5453: increaseDocumentAttribute(): updatedBy may be null; returned Document misses updatedAt/updatedBy
- Inconsistent with decreaseDocumentAttribute(): no fallback to current user.
- The returned Document doesn't reflect updatedAt/updatedBy after mutation.
Apply fallback and propagate metadata to the returned document:
- $updatedBy = $document->getUpdatedBy(); + $updatedBy = $document->getUpdatedBy() ?? Authorization::getUser(); ... $this->adapter->increaseDocumentAttribute( $collection->getId(), $id, $attribute, $value, $updatedAt, $updatedBy, max: $max );Outside the selected lines, update the in-memory doc before returning:
// After adapter call, before return $document ->setAttribute('$updatedAt', $updatedAt) ->setAttribute('$updatedBy', $updatedBy);
5544-5554: decreaseDocumentAttribute(): propagate updatedAt/updatedBy on the returned DocumentAdapter is updated, but the in-memory Document returned lacks the refreshed metadata.
// After adapter call, before return $document ->setAttribute('$updatedAt', $updatedAt) ->setAttribute('$updatedBy', $updatedBy);
5446-5454: AddupdatedAt/updatedBytodecreaseDocumentAttributeacross the stack
ThedecreaseDocumentAttributemethod wasn’t updated with the newupdatedAtandupdatedByparameters. You must:
- In
src/Database/Adapter.php, declare
abstract public function decreaseDocumentAttribute(string $collection, string $id, string $attribute, int|float $value, string $updatedAt, ?string $updatedBy, int|float|null $min = null): bool;- In
src/Database/Database.phpandsrc/Database/Mirror.php, update the method signatures and delegate calls to include$updatedAtand$updatedBy(in the same order as inincreaseDocumentAttribute).- In every driver (
Pool,Postgres,MariaDB,MySQL/SQL/SQLite), implement the updated signature and adjust the SQL to set bothupdated_atandupdated_bycolumns.- Update all call sites (e.g. lines 5551–5555) to pass the new parameters.
🧹 Nitpick comments (8)
tests/unit/Validator/AuthorizationTest.php (1)
123-141: Fix typo and improve test clarity.The test logic is sound but there's a typo in the status value and the test could be more explicit about its intent.
- $roles[] = Role::user("123", 'verififed'); - $roles[] = Role::user("126", 'unverififed'); + $roles[] = Role::user("123", 'verified'); + $roles[] = Role::user("126", 'unverified');Also consider adding a comment to clarify that this test validates that
Authorization::getUser()tracks the last user role set:+ /** + * Test that Authorization::getUser() tracks the identifier from the last user role set + */ public function testSetUserFromRoles(): void- $expectedRole = Role::user("126", 'unverififed'); + $expectedRole = Role::user("126", 'unverified');src/Database/Validator/Authorization.php (2)
91-94: Fix variable name typo.There's a typo in the variable name that should be corrected for consistency.
- $userIdetifier = Role::parse($role)->getIdentifier(); - if ($userIdetifier) { - self::$user = $userIdetifier; + $userIdentifier = Role::parse($role)->getIdentifier(); + if ($userIdentifier) { + self::$user = $userIdentifier;
105-108: Fix variable name typo in unsetRole method.Same typo as in the setRole method needs to be corrected.
- $userIdetifier = Role::parse($role)->getIdentifier(); - if ($userIdetifier && self::$user == $userIdetifier) { + $userIdentifier = Role::parse($role)->getIdentifier(); + if ($userIdentifier && self::$user == $userIdentifier) {tests/e2e/Adapter/Scopes/DocumentTests.php (2)
5906-6079: Prevent auth leakage between tests.This test mutates authorization state and never resets it. Add a reset to avoid cross-test flakiness.
$finalAllDocs = $database->find($collection); $this->assertCount(8, $finalAllDocs); $database->deleteCollection($collection); + Authorization::reset(); }
5906-6079: Extend coverage to increments with auditing.Consider asserting
$updatedByafter callingincreaseDocumentAttribute/decreaseDocumentAttributeand increateOrUpdateDocumentsWithIncrease(...)to validate the new adapter parameter is honored in those paths too.I can add a minimal sub-test here that performs an increment and checks
$updatedBychanged to the current user—want me to draft it?src/Database/Adapter/MariaDB.php (1)
1250-1265: Bind:updatedBywith correct PDO type (null-safe)Bind with detected PDO type to avoid driver casting nulls to empty strings under emulated prepares.
- $stmt->bindValue(':updatedAt', $updatedAt); - $stmt->bindValue(':updatedBy', $updatedBy); + $stmt->bindValue(':updatedAt', $updatedAt, $this->getPDOType($updatedAt)); + $stmt->bindValue(':updatedBy', $updatedBy, $this->getPDOType($updatedBy));Also applies to: 1276-1278, 1290-1291
src/Database/Adapter/Postgres.php (1)
1360-1360: Bind:updatedBywith correct PDO type (null-safe)Avoid implicit string-casting of nulls.
- $stmt->bindValue(':updatedAt', $updatedAt); - $stmt->bindValue(':updatedBy', $updatedBy); + $stmt->bindValue(':updatedAt', $updatedAt, $this->getPDOType($updatedAt)); + $stmt->bindValue(':updatedBy', $updatedBy, $this->getPDOType($updatedBy));Also applies to: 1372-1374, 1386-1386
src/Database/Database.php (1)
229-250: Set an explicit VARCHAR size for $createdBy/$updatedByType is string but size is 0. Some adapters infer column sizes from these descriptors; 0 can yield suboptimal DDL or validation behavior. Align with other string internals (e.g., LENGTH_KEY).
[ '$id' => '$createdBy', 'type' => self::VAR_STRING, 'format' => '', - 'size' => 0, + 'size' => Database::LENGTH_KEY, 'signed' => false, 'required' => false, 'default' => null, 'array' => false, 'filters' => [] ], [ '$id' => '$updatedBy', 'type' => self::VAR_STRING, 'format' => '', - 'size' => 0, + 'size' => Database::LENGTH_KEY, 'signed' => false, 'required' => false, 'default' => null, 'array' => false, 'filters' => [] ],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/MariaDB.php(8 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/Postgres.php(7 hunks)src/Database/Adapter/SQL.php(7 hunks)src/Database/Adapter/SQLite.php(1 hunks)src/Database/Database.php(14 hunks)src/Database/Document.php(2 hunks)src/Database/Validator/Authorization.php(4 hunks)src/Database/Validator/Structure.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)tests/unit/Validator/AuthorizationTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Validator/Structure.php
🧬 Code graph analysis (9)
src/Database/Validator/Structure.php (1)
src/Database/Database.php (1)
Database(37-7283)
tests/unit/Validator/AuthorizationTest.php (2)
src/Database/Helpers/Role.php (4)
Role(5-178)user(115-118)any(159-162)users(126-129)src/Database/Validator/Authorization.php (3)
Authorization(8-249)setRole(89-96)getUser(117-120)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
src/Database/Database.php (6)
Database(37-7283)createDocument(3753-3848)getDocument(3361-3481)updateDocument(4297-4497)createOrUpdateDocuments(5123-5136)createDocuments(3863-3960)src/Database/Adapter.php (5)
createDocument(701-701)getDocument(691-691)updateDocument(725-725)createOrUpdateDocuments(752-756)createDocuments(713-713)src/Database/Validator/Authorization.php (2)
Authorization(8-249)setRole(89-96)src/Database/Document.php (3)
Document(12-486)getCreatedBy(170-173)getUpdatedBy(186-189)
src/Database/Validator/Authorization.php (2)
src/Database/Database.php (1)
Database(37-7283)src/Database/Helpers/Role.php (2)
Role(5-178)user(115-118)
src/Database/Adapter/SQL.php (1)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)
src/Database/Adapter/Pool.php (5)
src/Database/Database.php (1)
increaseDocumentAttribute(5384-5467)src/Database/Adapter.php (1)
increaseDocumentAttribute(1227-1236)src/Database/Adapter/MariaDB.php (1)
increaseDocumentAttribute(1256-1303)src/Database/Adapter/Postgres.php (1)
increaseDocumentAttribute(1360-1394)src/Database/Mirror.php (1)
increaseDocumentAttribute(1024-1027)
src/Database/Adapter/Postgres.php (6)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)src/Database/Database.php (1)
increaseDocumentAttribute(5384-5467)src/Database/Adapter.php (1)
increaseDocumentAttribute(1227-1236)src/Database/Adapter/MariaDB.php (1)
increaseDocumentAttribute(1256-1303)src/Database/Adapter/Pool.php (1)
increaseDocumentAttribute(484-487)src/Database/Mirror.php (1)
increaseDocumentAttribute(1024-1027)
src/Database/Database.php (4)
src/Database/Document.php (6)
getCreatedBy(170-173)getUpdatedBy(186-189)setAttribute(260-277)getAttribute(240-247)getTenant(194-203)isEmpty(412-415)src/Database/Validator/Authorization.php (2)
Authorization(8-249)getUser(117-120)src/Database/Adapter.php (2)
getSharedTables(188-191)getTenant(216-219)src/Database/DateTime.php (2)
DateTime(7-86)now(19-23)
src/Database/Adapter/MariaDB.php (1)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (27)
src/Database/Validator/Structure.php (1)
88-109: LGTM! New audit trail attributes added correctly.The
$createdByand$updatedByattributes are properly defined as optional string attributes with consistent structure and configuration. They align perfectly with the existing pattern for audit trail attributes like$createdAtand$updatedAt.src/Database/Adapter/SQLite.php (1)
171-172: LGTM! Audit trail columns added to SQLite schema.The new
_createdByand_updatedBycolumns are properly added to the SQLite table schema with appropriate VARCHAR(255) DEFAULT NULL definitions, maintaining consistency with other metadata columns.src/Database/Document.php (2)
167-173: LGTM! New getter for created by audit field.The
getCreatedBy()method follows the established pattern for audit field getters, correctly returning the$createdByattribute as a nullable string.
183-189: LGTM! New getter for updated by audit field.The
getUpdatedBy()method follows the established pattern for audit field getters, correctly returning the$updatedByattribute as a nullable string.src/Database/Validator/Authorization.php (3)
5-5: LGTM! Role helper import added.The import is necessary for the new user tracking functionality that uses
Role::parse().
27-30: LGTM! User tracking property added.The static
$userproperty is properly declared to track the current user identifier for audit trail purposes.
112-120: LGTM! User getter method implemented correctly.The
getUser()method provides the necessary interface to retrieve the current user identifier for audit trail functionality.src/Database/Adapter.php (1)
1221-1236: API surface changed: ensure all adapters are updated and null-safe.Adding
?string $updatedByto the abstract signature is a breaking change for third-party adapters. Please verify every concrete adapter (internal and external) now implements the new parameter and that backing columns_updatedByare nullable across engines to safely bindnull.src/Database/Adapter/Pool.php (1)
484-487: Signature parity with abstract method looks good.
Pool::increaseDocumentAttribute(...)matchesAdapter::increaseDocumentAttribute(...)and correctly delegates args viafunc_get_args(). LGTM.src/Database/Adapter/MariaDB.php (4)
163-165: LGTM: schema adds for auditing metadatanew columns
_createdBy/_updatedBylook correct and nullable.
833-835: LGTM: persist creator/updater on insertStoring
_createdByand_updatedByon create is correct.
961-961: LGTM: persist updater on updateSetting
_updatedByduring updates is consistent with the new auditing model.
1542-1549: LGTM: projection mapping for auditing fieldsMapping
_createdBy/_updatedByto$createdBy/$updatedByis correct.src/Database/Adapter/Postgres.php (4)
241-243: LGTM: schema adds for auditing metadataQuoted identifiers for
_createdBy/_updatedByare correct.
968-970: LGTM: persist creator/updater on insertInsert path now includes both auditing fields.
1084-1086: LGTM: persist creator/updater on updateUpdate path includes auditing fields as expected.
1629-1636: LGTM: projection mapping for auditing fieldsCorrectly exposes
$createdBy/$updatedByin query results.src/Database/Adapter/SQL.php (7)
410-417: LGTM: getDocument maps auditing fieldsCorrect mapping/unset for
_createdBy/_updatedBy.
478-484: LGTM: updateDocuments supports auditing fieldsBulk update wiring for
_createdBy/_updatedBylooks good.
1071-1077: LGTM: row-width estimate updated3109 base bytes accounting for new varchar fields is accurate.
1869-1875: LGTM: include auditing columns in star-projection with spatial attrsEnsures auditing fields are present when spatial projections force explicit selection.
1926-1928: LGTM: internal key mapping for auditing fieldsMappings for
$createdBy/$updatedBy→_createdBy/_updatedByare correct.
2022-2024: LGTM: batch create persists auditing fieldsCreate-batch path includes
_createdBy/_updatedBy.
2138-2140: LGTM: upsert-batch persists auditing fieldsCreate-or-update path includes
_createdBy/_updatedBy.src/Database/Database.php (3)
263-281: LGTM: internal keys and indexes extendedAdding _createdBy/_updatedBy to INTERNAL_ATTRIBUTE_KEYS and INTERNAL_INDEXES is consistent and necessary for querying and migrations.
4456-4458: LGTM: updatedBy set on mutationSetting $updatedBy from the current actor on updates is correct.
4594-4597: LGTM: updatedBy defaulting in bulk updateupdatedBy derives from the acting user when not provided.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Database/Database.php (3)
5228-5248: Harden audit fields in createOrUpdateDocumentsWithIncrease().Always set updatedBy to actor; createdBy from actor or old, only override in migrations.
- $updatedBy = $document->getUpdatedBy(); + $actor = Authorization::getUser(); ... - ->setAttribute('$updatedBy', $updatedBy === null ? Authorization::getUser() : $updatedBy) + ->setAttribute('$updatedBy', $actor) ... - $createdBy = $document->getCreatedBy(); - if ($createdBy === null) { - $document->setAttribute('$createdBy', $old->isEmpty() ? Authorization::getUser() : $old->getCreatedBy()); - } else { - $document->setAttribute('$createdBy', $createdBy); - } + if ($this->isMigrating() && $document->getCreatedBy() !== null) { + $document->setAttribute('$createdBy', $document->getCreatedBy()); + } else { + $document->setAttribute('$createdBy', $old->isEmpty() ? $actor : $old->getCreatedBy()); + }
5443-5453: Use actor for $updatedBy in increaseDocumentAttribute() and update returned doc.Current code forwards possibly stale/null updatedBy and doesn’t reflect new audit fields on the returned Document.
- $updatedBy = $document->getUpdatedBy(); + $updatedBy = Authorization::getUser(); ... $this->adapter->increaseDocumentAttribute( $collection->getId(), $id, $attribute, $value, $updatedAt, - $updatedBy, + $updatedBy, max: $max ); - return $document->setAttribute( - $attribute, - $document->getAttribute($attribute) + $value - ); + return $document + ->setAttribute('$updatedAt', $updatedAt) + ->setAttribute('$updatedBy', $updatedBy) + ->setAttribute($attribute, $document->getAttribute($attribute) + $value);
5544-5554: Avoid loose null check and spoofing in decreaseDocumentAttribute(); set actor and reflect audit fields.Use strict assignment to actor and update the returned Document.
- $updatedBy = $document->getUpdatedBy() == null ? Authorization::getUser() : $document->getUpdatedBy(); + $updatedBy = Authorization::getUser(); ... $this->adapter->increaseDocumentAttribute( $collection->getId(), $id, $attribute, $value * -1, $updatedAt, $updatedBy, min: $min ); - return $document->setAttribute( - $attribute, - $document->getAttribute($attribute) - $value - ); + return $document + ->setAttribute('$updatedAt', $updatedAt) + ->setAttribute('$updatedBy', $updatedBy) + ->setAttribute($attribute, $document->getAttribute($attribute) - $value);
♻️ Duplicate comments (4)
src/Database/Database.php (4)
3782-3794: Block spoofing of $createdBy/$updatedBy in createDocument().Audit fields must be server-defined. Accept overrides only in explicit migration mode.
- $createdBy = $document->getCreatedBy(); - $updatedBy = $document->getUpdatedBy(); + $actor = Authorization::getUser(); $document ->setAttribute('$id', empty($document->getId()) ? ID::unique() : $document->getId()) ->setAttribute('$collection', $collection->getId()) ->setAttribute('$createdAt', ($createdAt === null || !$this->preserveDates) ? $time : $createdAt) - ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) - ->setAttribute('$createdBy', value: $createdBy === null ? Authorization::getUser() : $createdBy) - ->setAttribute('$updatedBy', $updatedBy === null ? Authorization::getUser() : $updatedBy); + ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) + ->setAttribute('$createdBy', $this->isMigrating() && $document->getCreatedBy() !== null ? $document->getCreatedBy() : $actor) + ->setAttribute('$updatedBy', $actor);
3890-3902: Same spoofing risk in batch create; harden like create().Mirror the actor-only assignment; allow override only when migrating.
- $createdBy = $document->getCreatedBy(); - $updatedBy = $document->getUpdatedBy(); + $actor = Authorization::getUser(); $document ->setAttribute('$id', empty($document->getId()) ? ID::unique() : $document->getId()) ->setAttribute('$collection', $collection->getId()) ->setAttribute('$createdAt', ($createdAt === null || !$this->preserveDates) ? $time : $createdAt) - ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) - ->setAttribute('$createdBy', $createdBy === null ? Authorization::getUser() : $createdBy) - ->setAttribute('$updatedBy', $updatedBy === null ? Authorization::getUser() : $updatedBy); + ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) + ->setAttribute('$createdBy', $this->isMigrating() && $document->getCreatedBy() !== null ? $document->getCreatedBy() : $actor) + ->setAttribute('$updatedBy', $actor);
4323-4329: createdBy must be immutable on update (except migrations).Preserve original creator; don’t accept user-supplied value.
- $createdBy = $document->getCreatedBy(); ... - $document['$createdBy'] = $createdBy === null ? $old->getCreatedBy() : $createdBy; + $document['$createdBy'] = $this->isMigrating() && $document->getCreatedBy() !== null + ? $document->getCreatedBy() + : $old->getCreatedBy();
4583-4587: Don’t allow mass updates of $createdBy; use strict null check; gate by migration.Prevents audit forgery in bulk updates.
- if ($updates->getCreatedBy() === null) { - unset($updates['$createdBy']); - } else { - $updates['$createdBy'] = $updates->getCreatedBy(); - } + if (!$this->isMigrating()) { + unset($updates['$createdBy']); + } elseif ($updates->getCreatedBy() === null) { + unset($updates['$createdBy']); + } else { + $updates['$createdBy'] = $updates->getCreatedBy(); + }
🧹 Nitpick comments (4)
tests/e2e/Adapter/Scopes/GeneralTests.php (1)
296-299: Add inline rationale for magic number 56- $this->assertEquals(56, $this->getDatabase()->getLimitForIndexes()); + // Expect 56 = adapter limit of 64 minus 8 built-in indexes + // (_id, _uid, _createdAt, _updatedAt, _createdBy, _updatedBy, _permissions_id, _permissions) + $this->assertEquals(56, $this->getDatabase()->getLimitForIndexes());tests/e2e/Adapter/Scopes/CollectionTests.php (1)
608-612: 15500 is a magic number — add context (auditing overhead) to keep the test maintainableDocument why 15500 is the right threshold after createdBy/updatedBy widened the row, so future width changes don’t create brittle failures.
Apply this diff to explain the choice:
- $this->assertEquals(true, $database->createAttribute($collection_1->getId(), 'attr_1', Database::VAR_STRING, 15500, true)); + // 15500 chosen to stay under row-size limits given increased base width (auditing: _createdBy/_updatedBy) + $this->assertEquals(true, $database->createAttribute($collection_1->getId(), 'attr_1', Database::VAR_STRING, 15500, true));tests/e2e/Adapter/Scopes/AttributeTests.php (2)
803-807: Avoid brittle magic number for base document width (3109).The exact baseline will keep changing as internal metadata evolves (e.g., newly added auditing fields). Prefer a resilient assertion so the test doesn’t churn on every metadata tweak.
Apply this diff to relax the assertion while still guarding against regressions:
- $this->assertEquals(3109, $init); + // Baseline width can change with internal metadata (_createdBy/_updatedBy, etc.). + $this->assertGreaterThan(1000, $init);
955-967: Derive varchar size from adapter limits instead of hard-coding 15,500.The new baseline increased; this value will likely drift again. Compute the large varchar size from the adapter’s document limit and the measured baseline so the test remains stable across future metadata changes.
Apply this diff in the attribute definition:
- $attributes[] = new Document([ + $attributes[] = new Document([ '$id' => ID::custom('varchar_16000'), 'type' => Database::VAR_STRING, - 'size' => 15500, + 'size' => $varcharLarge, 'required' => true, 'default' => null, 'signed' => true, 'array' => false, 'filters' => [], ]);Add this snippet just before
$attributes = [];in the same test method to compute$varcharLargefrom runtime limits:// Compute a large varchar that fits alone but exceeds the row limit when a 200-char field is added. $tmp = $database->createCollection('width_calc_tmp'); $init = $database->getAdapter()->getAttributeWidth($tmp); $limit = $database->getAdapter()->getDocumentSizeLimit(); // Choose a size that is safely under the limit alone, but will exceed when we add 200*4+1 bytes. $varcharLarge = intdiv(($limit - $init - 801 + 4), 4); // +4 ensures we cross the limit with the 200-char field $database->deleteCollection('width_calc_tmp');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/Database/Database.php(14 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(2 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(1 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/Adapter/Scopes/CollectionTests.php (5)
src/Database/Adapter.php (1)
createAttribute(558-558)src/Database/Adapter/Pool.php (1)
createAttribute(168-171)src/Database/Adapter/Postgres.php (1)
createAttribute(449-468)src/Database/Adapter/SQL.php (1)
createAttribute(232-246)src/Database/Mirror.php (1)
createAttribute(295-357)
tests/e2e/Adapter/Scopes/GeneralTests.php (1)
tests/e2e/Adapter/Base.php (1)
getDatabase(36-36)
🔇 Additional comments (3)
src/Database/Database.php (3)
267-269: LGTM: internal keys include _createdBy/_updatedBy.Consistent with new audit attrs.
277-279: LGTM: default internal indexes for _createdBy/_updatedBy.Good for common querying.
4457-4458: LGTM: updatedBy set to acting user on update.This aligns with audit semantics.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Database/Adapter/MariaDB.php (1)
1209-1215: Also update_updatedByin upsert pathsUpserts currently refresh
_updatedAtbut not_updatedBy. Include it so auditing remains consistent for create-or-update operations and atomic increments.- $updateColumns = [ - $getUpdateClause($attribute, increment: true), - $getUpdateClause('_updatedAt'), - ]; + $updateColumns = [ + $getUpdateClause($attribute, increment: true), + $getUpdateClause('_updatedAt'), + $getUpdateClause('_updatedBy'), + ];src/Database/Adapter/Postgres.php (1)
1313-1317: Also update"_updatedBy"in upsert pathsMirror the MariaDB recommendation: include
"_updatedBy"inON CONFLICT ... DO UPDATE SETto keep auditing complete on upserts/increments.- $updateColumns = [ - $getUpdateClause($attribute, increment: true), - $getUpdateClause('_updatedAt'), - ]; + $updateColumns = [ + $getUpdateClause($attribute, increment: true), + $getUpdateClause('_updatedAt'), + $getUpdateClause('_updatedBy'), + ];src/Database/Adapter/SQL.php (1)
1891-1896: Include$createdBy/$updatedByin explicit-selection projectionsWhen callers specify selections (not
*), auditing fields won’t be fetched today, so they never map to public fields. Add them to$internalKeys.$internalKeys = [ '$id', '$sequence', '$permissions', '$createdAt', - '$updatedAt', + '$updatedAt', + '$createdBy', + '$updatedBy', ];
🧹 Nitpick comments (3)
src/Database/Adapter/MariaDB.php (2)
831-833: Clean up binding loop; remove shadowed $attributeInside createDocument(), the binding loop references
$attribute = $this->filter($attribute);but$attributeisn’t defined in that scope. It’s a harmless no-op today but confusing. Remove it.- foreach ($attributes as $value) { + foreach ($attributes as $value) { if (\is_array($value)) { $value = \json_encode($value); } $bindKey = 'key_' . $attributeIndex; - $attribute = $this->filter($attribute); $value = (\is_bool($value)) ? (int)$value : $value; $stmt->bindValue(':' . $bindKey, $value, $this->getPDOType($value)); $attributeIndex++; }Also applies to: 881-892
1288-1289: Bind NULL properly for updatedByIf
$updatedByis null, bind with the correct PDO type to avoid empty-string coercion on some drivers.- $stmt->bindValue(':updatedBy', $updatedBy); + $stmt->bindValue(':updatedBy', $updatedBy, $this->getPDOType($updatedBy));Would you like me to add an integration test that increments an attribute when
$updatedByis null to verify it persists as SQL NULL?src/Database/Adapter/Postgres.php (1)
1385-1386: Bind NULL properly for updatedByEnsure NULL is written as SQL NULL instead of an empty string.
- $stmt->bindValue(':updatedBy', $updatedBy); + $stmt->bindValue(':updatedBy', $updatedBy, $this->getPDOType($updatedBy));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Database/Adapter/MariaDB.php(7 hunks)src/Database/Adapter/Postgres.php(6 hunks)src/Database/Adapter/SQL.php(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Database/Adapter/SQL.php (1)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)
src/Database/Adapter/Postgres.php (5)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)src/Database/Adapter/MariaDB.php (1)
increaseDocumentAttribute(1254-1301)src/Database/Adapter.php (1)
increaseDocumentAttribute(1227-1236)src/Database/Adapter/Pool.php (1)
increaseDocumentAttribute(484-487)src/Database/Database.php (1)
increaseDocumentAttribute(5384-5467)
src/Database/Adapter/MariaDB.php (1)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (11)
src/Database/Adapter/MariaDB.php (2)
161-163: LGTM: added auditing columns in table DDLnew columns
_createdByand_updatedBylook correct and nullable. No index requirement here.
959-960: LGTM: update path records updaterSetting
_updatedByon updates is aligned with the auditing intent.src/Database/Adapter/Postgres.php (2)
240-242: LGTM: added auditing columns in table DDL
"_createdBy"and"_updatedBy"are added with appropriate types.
967-969: LGTM: creation path sets creator/updaterPersisting both creator and updater on insert looks good.
src/Database/Adapter/SQL.php (7)
412-419: LGTM: map internal createdBy/updatedBy to public fieldsRead path properly exposes
$createdBy/$updatedBy.
1871-1871: LGTM: include auditing columns in default projectionAdding
_createdBy/_updatedByto internal columns covers theSELECT *and spatial cases.
1928-1930: LGTM: key mapping for auditing attributesMapping
$createdBy/$updatedByto internal columns is correct.
1073-1079: LGTM: row-width accounting updatedBase width 3109 bytes reflects two additional VARCHAR(255) columns.
2024-2026: LGTM: batch create persists auditing fieldsCreate-batch path now writes
_createdBy/_updatedBy.
2140-2142: LGTM: upsert input includes auditing fieldsCreate-or-update batches now carry auditing fields into insert side.
2520-2527: LGTM: result mapping for auditing fields in find()Public getters will now see
$createdBy/$updatedBy.
| string $attribute, | ||
| int|float $value, | ||
| string $updatedAt, | ||
| ?string $updatedBy, |
There was a problem hiding this comment.
I don't see this added for any SQL adapters 🤔
There was a problem hiding this comment.
src/Database/Database.php
Outdated
| ->setAttribute('$createdAt', ($createdAt === null || !$this->preserveDates) ? $time : $createdAt) | ||
| ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt); | ||
| ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) | ||
| ->setAttribute('$createdBy', value: $createdBy === null ? Authorization::getUser() : $createdBy) |
There was a problem hiding this comment.
Let's remove the named param key (value:) here to keep consistent
src/Database/Database.php
Outdated
| $time = DateTime::now(); | ||
| $updatedAt = $document->getUpdatedAt(); | ||
| $updatedAt = (empty($updatedAt) || !$this->preserveDates) ? $time : $updatedAt; | ||
| $updatedBy = $document->getUpdatedBy(); |
There was a problem hiding this comment.
Should set the current user here
There was a problem hiding this comment.
reverting , make sense cause we are not using an external document in the increment-decrement
src/Database/Database.php
Outdated
| $time = DateTime::now(); | ||
| $updatedAt = $document->getUpdatedAt(); | ||
| $updatedAt = (empty($updatedAt) || !$this->preserveDates) ? $time : $updatedAt; | ||
| $updatedBy = $document->getUpdatedBy() == null ? Authorization::getUser() : $document->getUpdatedBy(); |
| $userIdetifier = Role::parse($role)->getIdentifier(); | ||
| if ($userIdetifier) { | ||
| self::$user = $userIdetifier; | ||
| } |
There was a problem hiding this comment.
We should only set $user if the parsed role ->getRole() is user
| '$id' => '$createdBy', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => 0, | ||
| 'signed' => false, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [] | ||
| ], | ||
| [ | ||
| '$id' => '$updatedBy', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => 0, | ||
| 'signed' => false, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [] | ||
| ], |
| $collection = 'test_created_updated_by'; | ||
| $database = static::getDatabase(); | ||
|
|
||
|
|
||
| $database->createCollection($collection); | ||
| $database->createAttribute($collection, 'string', Database::VAR_STRING, 128, false); | ||
| $database->createAttribute($collection, 'number', Database::VAR_INTEGER, 0, false); | ||
|
|
||
|
|
||
| Authorization::setRole('user:test_user_1'); |
| public function testGetIndexLimit(): void | ||
| { | ||
| $this->assertEquals(58, $this->getDatabase()->getLimitForIndexes()); | ||
| $this->assertEquals(56, $this->getDatabase()->getLimitForIndexes()); |
There was a problem hiding this comment.
Do we add indexes on them? We should, but I didn't see it in createCollection on the adapters
| $currentUserRole = Role::user("123"); | ||
| Authorization::setRole(Role::user("123")->toString()); | ||
| $this->assertEquals($currentUserRole->getIdentifier(), Authorization::getUser()); | ||
|
|
||
| $roles = []; | ||
| $roles[] = Role::user("123"); | ||
| $roles[] = Role::user("123", 'verififed'); | ||
| $roles[] = Role::user("126", 'unverififed'); | ||
| $roles[] = Role::any(); | ||
| $roles[] = Role::users(); | ||
|
|
||
| foreach ($roles as $role) { | ||
| Authorization::setRole($role->toString()); | ||
| } | ||
| $expectedRole = Role::user("126", 'unverififed'); | ||
| $this->assertEquals($expectedRole->getIdentifier(), Authorization::getUser()); |
There was a problem hiding this comment.
Let's also ensure that setting team/membership/label roles don't change the user
ArnabChatterjee20k
force-pushed
the
dat-677
branch
from
dd60369 to
67791d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Database/Adapter.php (1)
1236-1251: BC break — Adapter::increaseDocumentAttribute now requires ?string $updatedBy: update all adapters & callersFindings: Postgres (src/Database/Adapter/Postgres.php:1360) and Pool (src/Database/Adapter/Pool.php:484) were updated; MariaDB (src/Database/Adapter/MariaDB.php:1255) and Mirror (src/Database/Mirror.php:1024) still expose the older signature. Database calls adapter->increaseDocumentAttribute at src/Database/Database.php:5556 and 5657 — verify Database::increaseDocumentAttribute (src/Database/Database.php:5494) forwards both $updatedAt and $updatedBy.
Action required:
- Make all Adapter subclasses (MariaDB, MySQL, SQLite, SQL, Pool and any third‑party adapters) implement the new signature (same param order/types).
- Ensure Database::increaseDocumentAttribute forwards $updatedAt and ?$updatedBy to the adapter call sites shown above.
- Add a migration/CHANGELOG entry and bump major version for this BC change.
src/Database/Adapter/MariaDB.php (2)
1210-1216: Upsert increment path must also set _updatedByWhen $attribute is set (increment mode), getUpsertStatement only updates the target column and _updatedAt; also update _updatedBy from VALUES to keep audit fields consistent.
File: src/Database/Adapter/MariaDB.php (getUpsertStatement, ~1210-1216)
if (!empty($attribute)) { // Increment specific column by its new value in place $updateColumns = [ $getUpdateClause($attribute, increment: true), $getUpdateClause('_updatedAt'), + $getUpdateClause('_updatedBy'), ];
1249-1264: Preserve_updatedBywhen parameter is nullBoth MariaDB and Postgres adapters unconditionally set
_updatedBy = :updatedBy; if:updatedByis NULL this will wipe the stored value. Use COALESCE to keep the existing DB value when the parameter is NULL.Apply:
MariaDB (src/Database/Adapter/MariaDB.php ~lines 1274–1277):
- `_updatedBy` = :updatedBy + `_updatedBy` = COALESCE(:updatedBy, `_updatedBy`)Postgres (src/Database/Adapter/Postgres.php ~lines 1371–1374):
- "_updatedBy" = :updatedBy + "_updatedBy" = COALESCE(:updatedBy, "_updatedBy")Optionally document that passing NULL leaves
_updatedByunchanged.
♻️ Duplicate comments (9)
src/Database/Adapter/SQL.php (1)
480-486: Do not allow_createdByto change in batch updates
updateDocuments()should keep_createdByimmutable. This was flagged previously; it still needs removal.Apply:
- if (!empty($updates->getCreatedBy())) { - $attributes['_createdBy'] = $updates->getCreatedBy(); - } - if (!empty($updates->getUpdatedBy())) { $attributes['_updatedBy'] = $updates->getUpdatedBy(); }src/Database/Database.php (8)
5553-5563: Increase: set updatedBy from actor; also reflect updatedAt/updatedBy on returned Document.Prevents spoofing and keeps the in-memory Document consistent.
$updatedAt = $document->getUpdatedAt(); $updatedAt = (empty($updatedAt) || !$this->preserveDates) ? $time : $updatedAt; - $updatedBy = $document->getUpdatedBy(); + $updatedBy = Authorization::getUser(); @@ $updatedAt, - $updatedBy, + $updatedBy, max: $max ); @@ - return $document->setAttribute( - $attribute, - $document->getAttribute($attribute) + $value - ); + return $document + ->setAttribute('$updatedAt', $updatedAt) + ->setAttribute('$updatedBy', $updatedBy) + ->setAttribute($attribute, $document->getAttribute($attribute) + $value);
5654-5664: Decrease: same audit handling and state reflection as increase().Use actor, and update returned Document’s updatedAt/updatedBy.
- $updatedBy = $document->getUpdatedBy() == null ? Authorization::getUser() : $document->getUpdatedBy(); + $updatedBy = Authorization::getUser(); @@ $value * -1, $updatedAt, - $updatedBy, + $updatedBy, min: $min ); @@ - return $document->setAttribute( - $attribute, - $document->getAttribute($attribute) - $value - ); + return $document + ->setAttribute('$updatedAt', $updatedAt) + ->setAttribute('$updatedBy', $updatedBy) + ->setAttribute($attribute, $document->getAttribute($attribute) - $value);
233-254: Set VARCHAR length for $createdBy/$updatedBy (size must not be 0).Size 0 on VAR_STRING will violate validators and/or map to suboptimal types. Use Database::LENGTH_KEY (consistent with other keys).
[ '$id' => '$createdBy', 'type' => self::VAR_STRING, 'format' => '', - 'size' => 0, + 'size' => Database::LENGTH_KEY, 'signed' => false, 'required' => false, 'default' => null, 'array' => false, 'filters' => [] ], [ '$id' => '$updatedBy', 'type' => self::VAR_STRING, 'format' => '', - 'size' => 0, + 'size' => Database::LENGTH_KEY, 'signed' => false, 'required' => false, 'default' => null, 'array' => false, 'filters' => [] ],
3892-3902: Harden auditing on create: force actor; drop named param; allow override only in migrations.Avoid spoofing by ignoring client-provided createdBy/updatedBy unless explicitly migrating.
- $createdBy = $document->getCreatedBy(); - $updatedBy = $document->getUpdatedBy(); + $actor = Authorization::getUser(); $document ->setAttribute('$id', empty($document->getId()) ? ID::unique() : $document->getId()) ->setAttribute('$collection', $collection->getId()) ->setAttribute('$createdAt', ($createdAt === null || !$this->preserveDates) ? $time : $createdAt) - ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) - ->setAttribute('$createdBy', value: $createdBy === null ? Authorization::getUser() : $createdBy) - ->setAttribute('$updatedBy', $updatedBy === null ? Authorization::getUser() : $updatedBy); + ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) + ->setAttribute('$createdBy', $this->isMigrating() && $document->getCreatedBy() !== null ? $document->getCreatedBy() : $actor) + ->setAttribute('$updatedBy', $actor);
4000-4010: Batch create: same audit hardening as create().Mirror create(): set createdBy/updatedBy from actor; gate overrides behind isMigrating().
- $createdBy = $document->getCreatedBy(); - $updatedBy = $document->getUpdatedBy(); + $actor = Authorization::getUser(); $document ->setAttribute('$id', empty($document->getId()) ? ID::unique() : $document->getId()) ->setAttribute('$collection', $collection->getId()) ->setAttribute('$createdAt', ($createdAt === null || !$this->preserveDates) ? $time : $createdAt) - ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) - ->setAttribute('$createdBy', $createdBy === null ? Authorization::getUser() : $createdBy) - ->setAttribute('$updatedBy', $updatedBy === null ? Authorization::getUser() : $updatedBy); + ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) + ->setAttribute('$createdBy', $this->isMigrating() && $document->getCreatedBy() !== null ? $document->getCreatedBy() : $actor) + ->setAttribute('$updatedBy', $actor);
4434-4440: createdBy must remain immutable on updates (except migrations).Preserve the original creator; do not allow payload to change it.
- $createdBy = $document->getCreatedBy(); + // createdBy is immutable outside migrations $document = \array_merge($old->getArrayCopy(), $document->getArrayCopy()); $document['$collection'] = $old->getAttribute('$collection'); // Make sure user doesn't switch collection ID $document['$createdAt'] = ($createdAt === null || !$this->preserveDates) ? $old->getCreatedAt() : $createdAt; - $document['$createdBy'] = $createdBy === null ? $old->getCreatedBy() : $createdBy; + $document['$createdBy'] = $this->isMigrating() && $document->getCreatedBy() !== null + ? $document->getCreatedBy() + : $old->getCreatedBy();
4694-4698: Block mass-updating createdBy in updateDocuments() (allow only when migrating).Prevents audit forgery.
- if ($updates->getCreatedBy() === null) { - unset($updates['$createdBy']); - } else { - $updates['$createdBy'] = $updates->getCreatedBy(); - } + if (!$this->isMigrating()) { + unset($updates['$createdBy']); + } elseif ($updates->getCreatedBy() === null) { + unset($updates['$createdBy']); + } else { + $updates['$createdBy'] = $updates->getCreatedBy(); + }
4704-4708: updatedBy in bulk updates must come from actor (optionally allow in migrations).Avoids client-supplied spoofing.
- $updatedBy = $updates->getUpdatedBy(); + $actor = Authorization::getUser(); $updates['$updatedAt'] = ($updatedAt === null || !$this->preserveDates) ? DateTime::now() : $updatedAt; - $updates['$updatedBy'] = $updatedBy === null ? Authorization::getUser() : $updatedBy; + $updates['$updatedBy'] = $this->isMigrating() && $updates->getUpdatedBy() !== null ? $updates->getUpdatedBy() : $actor;
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
6010-6016: Prefer Role helpers for consistencyUse
Authorization::setRole(Role::user('test_user_1')->toString())instead of raw'user:test_user_1'for parity with the rest of the suite.src/Database/Adapter/SQL.php (1)
1933-1938: Always include $createdBy/$updatedBy in narrow projectionsWhen selections are specific (not “*”), internalKeys currently force include of
$createdAt/$updatedAtbut not$createdBy/$updatedBy. Add them for API consistency.$internalKeys = [ '$id', '$sequence', '$permissions', '$createdAt', '$updatedAt', + '$createdBy', + '$updatedBy', ];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/MariaDB.php(7 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/Postgres.php(6 hunks)src/Database/Adapter/SQL.php(8 hunks)src/Database/Adapter/SQLite.php(3 hunks)src/Database/Database.php(14 hunks)src/Database/Document.php(2 hunks)src/Database/Validator/Authorization.php(4 hunks)src/Database/Validator/Structure.php(1 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(2 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(1 hunks)tests/unit/Validator/AuthorizationTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/Database/Document.php
- tests/unit/Validator/AuthorizationTest.php
- src/Database/Adapter/SQLite.php
- tests/e2e/Adapter/Scopes/GeneralTests.php
- tests/e2e/Adapter/Scopes/AttributeTests.php
- src/Database/Validator/Structure.php
- src/Database/Adapter/Postgres.php
- src/Database/Validator/Authorization.php
- tests/e2e/Adapter/Scopes/CollectionTests.php
- src/Database/Adapter/Pool.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Database/Database.php (3)
src/Database/Document.php (8)
getCreatedBy(170-173)getUpdatedBy(186-189)setAttribute(260-277)getAttribute(240-247)getCreatedAt(162-165)getTenant(194-203)getUpdatedAt(178-181)isEmpty(412-415)src/Database/Adapter.php (2)
getSharedTables(188-191)getTenant(216-219)src/Database/DateTime.php (2)
DateTime(7-86)now(19-23)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
src/Database/Database.php (5)
Database(37-7386)createDocument(3861-3956)getAdapter(1223-1226)createOrUpdateDocuments(5233-5246)createDocuments(3971-4068)src/Database/Adapter.php (4)
createDocument(702-702)getSupportForUpserts(1023-1023)createOrUpdateDocuments(753-757)createDocuments(714-714)src/Database/Validator/Authorization.php (2)
Authorization(8-249)setRole(89-96)src/Database/Document.php (3)
Document(12-486)getCreatedBy(170-173)getUpdatedBy(186-189)
src/Database/Adapter/SQL.php (1)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)
src/Database/Adapter/MariaDB.php (1)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (10)
src/Database/Adapter/MariaDB.php (3)
162-164: Add auditing columns: OKDDL for
_createdBy/_updatedBylooks consistent with existing metadata columns.
832-834: Persist createdBy/updatedBy on insert: OKValues sourced from Document getters and default to NULL as desired.
960-961: Only update_updatedByon updates: OKKeeps
_createdByimmutable on updates.src/Database/Adapter/SQL.php (6)
412-419: Map internal createdBy/updatedBy on read: OK
1912-1913: *Projection includes auditing columns for “select ”: OK
1971-1973: Key mapping for $createdBy/$updatedBy: OK
1073-1079: Row width baseline updated for auditing fields: OK3109 accounts for two extra VARCHAR(255) columns.
2067-2069: Persist auditing fields in batch inserts: OK
2563-2570: Map auditing fields in find(): OKsrc/Database/Database.php (1)
271-273: LGTM: internal keys and indexes updated.Adding
_createdByand_updatedByto INTERNAL_ATTRIBUTE_KEYS and INTERNAL_INDEXES is correct.Also applies to: 281-283
| $attributes['_createdBy'] = $document->getCreatedBy(); | ||
| $attributes['_updatedBy'] = $document->getUpdatedBy(); | ||
| $attributes['_permissions'] = \json_encode($document->getPermissions()); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Upsert path sets auditing fields: verify adapter update clause
Attributes include _updatedBy, but MariaDB’s increment upsert path doesn’t update it yet. Align adapter upsert (increment) to set _updatedBy too (see related comment in MariaDB::getUpsertStatement).
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- locate SQL.php ----"
rg -n --hidden --no-ignore -S "src/Database/Adapter/SQL.php" || true
echo "---- show SQL.php 2130-2200 ----"
if [ -f src/Database/Adapter/SQL.php ]; then
sed -n '2130,2200p' src/Database/Adapter/SQL.php || true
else
echo "src/Database/Adapter/SQL.php not found"
fi
echo "---- search for getUpsertStatement definitions ----"
rg -n --hidden --no-ignore -S "function\s+getUpsertStatement" || true
echo "---- search for class MariaDB ----"
rg -n --hidden --no-ignore -S "class\s+MariaDB" || true
echo "---- show any MariaDB file contents around getUpsertStatement if found ----"
for f in $(rg -n --hidden --no-ignore -S "class\s+MariaDB" -g '!vendor' --files-with-matches || true); do
echo "---- $f ----"
sed -n '1,240p' "$f" || true
done
echo "---- search for _updatedBy/_createdBy usages ----"
rg -n --hidden --no-ignore -S "_updatedBy|_createdBy" || true
echo "---- search for ON DUPLICATE KEY UPDATE / upsert occurrences ----"
rg -n --hidden --no-ignore -S "ON DUPLICATE KEY UPDATE|ON CONFLICT|\\bupsert\\b" || trueLength of output: 9672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- show MariaDB.php 1100-1320 ----"
if [ -f src/Database/Adapter/MariaDB.php ]; then
sed -n '1100,1320p' src/Database/Adapter/MariaDB.php || true
else
echo "src/Database/Adapter/MariaDB.php not found"
fi
echo "---- show SQL.php 1520-1620 (abstract getUpsertStatement) ----"
if [ -f src/Database/Adapter/SQL.php ]; then
sed -n '1520,1620p' src/Database/Adapter/SQL.php || true
else
echo "src/Database/Adapter/SQL.php not found"
fi
echo "---- show SQL.php 1980-2260 (createOrUpdateDocuments area) ----"
if [ -f src/Database/Adapter/SQL.php ]; then
sed -n '1980,2260p' src/Database/Adapter/SQL.php || true
else
echo "src/Database/Adapter/SQL.php not found"
fi
echo "---- search for getUpsertStatement call sites ----"
rg -n --hidden --no-ignore -S "getUpsertStatement\(" || true
echo "---- search for ON DUPLICATE KEY UPDATE in MariaDB.php ----"
rg -n --hidden --no-ignore -S "ON DUPLICATE KEY UPDATE|ON DUPLICATE KEY" src/Database/Adapter/MariaDB.php || true
echo "---- search for _updatedBy occurrences in MariaDB.php ----"
rg -n --hidden --no-ignore -S "_updatedBy" src/Database/Adapter/MariaDB.php || trueLength of output: 20899
Upsert increment path must update _updatedBy (MariaDB adapter)
createOrUpdateDocuments sets _updatedBy (src/Database/Adapter/SQL.php:2183-2185), but MariaDB::getUpsertStatement’s increment branch only updates the incremented attribute and _updatedAt — add _updatedBy to the increment ON DUPLICATE KEY UPDATE clause (e.g. include $getUpdateClause('_updatedBy') so the sharedTables IF logic is preserved).
🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 2183-2185, createOrUpdateDocuments
sets _updatedBy but the MariaDB::getUpsertStatement increment branch only
updates the incremented attribute and _updatedAt; modify the increment ON
DUPLICATE KEY UPDATE clause to also update _updatedBy by adding
$getUpdateClause('_updatedBy') (ensuring you insert it in the same
IF(sharedTables) logic path so the sharedTables behavior and clause ordering
remain consistent).
|
|
||
| public function testCreatedByUpdatedBy(): void | ||
| { | ||
| $collection = 'test_created_updated_by'; | ||
| $database = static::getDatabase(); | ||
|
|
||
|
|
||
| $database->createCollection($collection); | ||
| $database->createAttribute($collection, 'string', Database::VAR_STRING, 128, false); | ||
| $database->createAttribute($collection, 'number', Database::VAR_INTEGER, 0, false); | ||
|
|
||
|
|
||
| Authorization::setRole('user:test_user_1'); | ||
| $doc1 = $database->createDocument($collection, new Document([ | ||
| '$id' => 'test1', | ||
| 'string' => 'test1', | ||
| 'number' => 100, | ||
| '$permissions' => [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())] | ||
| ])); | ||
|
|
||
|
|
||
| $this->assertEquals('test_user_1', $doc1->getCreatedBy()); | ||
| $this->assertEquals('test_user_1', $doc1->getUpdatedBy()); | ||
|
|
||
|
|
||
| $retrievedDoc1 = $database->getDocument($collection, 'test1'); | ||
| $this->assertEquals('test_user_1', $retrievedDoc1->getCreatedBy()); | ||
| $this->assertEquals('test_user_1', $retrievedDoc1->getUpdatedBy()); | ||
|
|
||
|
|
||
| $doc2 = $database->createDocument($collection, new Document([ | ||
| '$id' => 'test2', | ||
| 'string' => 'test2', | ||
| 'number' => 200, | ||
| '$createdBy' => 'explicit_creator', | ||
| '$permissions' => [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())] | ||
| ])); | ||
|
|
||
|
|
||
| $this->assertEquals('explicit_creator', $doc2->getCreatedBy()); | ||
| $this->assertEquals('test_user_1', $doc2->getUpdatedBy()); | ||
|
|
||
|
|
||
| $retrievedDoc2 = $database->getDocument($collection, 'test2'); | ||
| $this->assertEquals('explicit_creator', $retrievedDoc2->getCreatedBy()); | ||
| $this->assertEquals('test_user_1', $retrievedDoc2->getUpdatedBy()); | ||
|
|
||
|
|
||
| $doc3 = $database->createDocument($collection, new Document([ | ||
| '$id' => 'test3', | ||
| 'string' => 'test3', | ||
| 'number' => 300, | ||
| '$updatedBy' => 'explicit_updater', | ||
| '$permissions' => [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())] | ||
| ])); | ||
|
|
||
|
|
||
| $this->assertEquals('test_user_1', $doc3->getCreatedBy()); | ||
| $this->assertEquals('explicit_updater', $doc3->getUpdatedBy()); | ||
|
|
||
|
|
||
| $retrievedDoc3 = $database->getDocument($collection, 'test3'); | ||
| $this->assertEquals('test_user_1', $retrievedDoc3->getCreatedBy()); | ||
| $this->assertEquals('explicit_updater', $retrievedDoc3->getUpdatedBy()); | ||
|
|
||
|
|
||
| $doc4 = $database->createDocument($collection, new Document([ | ||
| '$id' => 'test4', | ||
| 'string' => 'test4', | ||
| 'number' => 400, | ||
| '$createdBy' => 'explicit_creator_2', | ||
| '$updatedBy' => 'explicit_updater_2', | ||
| '$permissions' => [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())] | ||
| ])); | ||
|
|
||
|
|
||
| $this->assertEquals('explicit_creator_2', $doc4->getCreatedBy()); | ||
| $this->assertEquals('explicit_updater_2', $doc4->getUpdatedBy()); | ||
|
|
||
|
|
||
| $retrievedDoc4 = $database->getDocument($collection, 'test4'); | ||
| $this->assertEquals('explicit_creator_2', $retrievedDoc4->getCreatedBy()); | ||
| $this->assertEquals('explicit_updater_2', $retrievedDoc4->getUpdatedBy()); | ||
|
|
||
|
|
||
| $allDocs = $database->find($collection); | ||
| $this->assertCount(4, $allDocs); | ||
|
|
||
|
|
||
| Authorization::setRole('user:test_user_2'); | ||
| $updateDoc = new Document([ | ||
| 'string' => 'updated_test1', | ||
| 'number' => 150 | ||
| ]); | ||
| $updatedDoc = $database->updateDocument($collection, 'test1', $updateDoc); | ||
|
|
||
| // Verify createdBy is preserved, updatedBy changes to current user | ||
| $this->assertEquals('test_user_1', $updatedDoc->getCreatedBy()); // Should preserve original creator | ||
| $this->assertEquals('test_user_2', $updatedDoc->getUpdatedBy()); // Should update to current user | ||
|
|
||
| $retrievedUpdatedDoc = $database->getDocument($collection, 'test1'); | ||
| $this->assertEquals('test_user_1', $retrievedUpdatedDoc->getCreatedBy()); | ||
| $this->assertEquals('test_user_2', $retrievedUpdatedDoc->getUpdatedBy()); | ||
|
|
||
| $upsertDoc = new Document([ | ||
| '$id' => 'test2', | ||
| 'string' => 'upserted_test2', | ||
| 'number' => 250 | ||
| ]); | ||
| if ($database->getAdapter()->getSupportForUpserts()) { | ||
| $upsertCount = $database->createOrUpdateDocuments($collection, [$upsertDoc]); | ||
| $this->assertEquals(1, $upsertCount); | ||
|
|
||
| $upsertedDoc = $database->getDocument($collection, 'test2'); | ||
| $this->assertEquals('explicit_creator', $upsertedDoc->getCreatedBy()); // Should preserve original creator | ||
| $this->assertEquals('test_user_2', $upsertedDoc->getUpdatedBy()); // Should update to current user | ||
|
|
||
| $upsertNewDoc = new Document([ | ||
| '$id' => 'test5', | ||
| 'string' => 'new_test5', | ||
| 'number' => 500, | ||
| '$createdBy' => 'new_creator', | ||
| '$permissions' => [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())] | ||
| ]); | ||
|
|
||
| $upsertNewCount = $database->createOrUpdateDocuments($collection, [$upsertNewDoc]); | ||
| $this->assertEquals(1, $upsertNewCount); | ||
|
|
||
| $newUpsertedDoc = $database->getDocument($collection, 'test5'); | ||
| $this->assertEquals('new_creator', $newUpsertedDoc->getCreatedBy()); // Should use explicit creator | ||
| $this->assertEquals('test_user_2', $newUpsertedDoc->getUpdatedBy()); // Should use current user | ||
|
|
||
|
|
||
| $bulkDocs = [ | ||
| new Document([ | ||
| '$id' => 'bulk1', | ||
| 'string' => 'bulk1', | ||
| 'number' => 600, | ||
| '$permissions' => [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())] | ||
| ]), | ||
| new Document([ | ||
| '$id' => 'bulk2', | ||
| 'string' => 'bulk2', | ||
| 'number' => 700, | ||
| '$createdBy' => 'bulk_creator', | ||
| '$permissions' => [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())] | ||
| ]), | ||
| new Document([ | ||
| '$id' => 'bulk3', | ||
| 'string' => 'bulk3', | ||
| 'number' => 800, | ||
| '$updatedBy' => 'bulk_updater', | ||
| '$permissions' => [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())] | ||
| ]) | ||
| ]; | ||
| $bulkCount = $database->createDocuments($collection, $bulkDocs); | ||
| $this->assertEquals(3, $bulkCount); | ||
|
|
||
| $bulkDoc1 = $database->getDocument($collection, 'bulk1'); | ||
| $bulkDoc2 = $database->getDocument($collection, 'bulk2'); | ||
| $bulkDoc3 = $database->getDocument($collection, 'bulk3'); | ||
|
|
||
| $this->assertEquals('test_user_2', $bulkDoc1->getCreatedBy()); | ||
| $this->assertEquals('test_user_2', $bulkDoc1->getUpdatedBy()); | ||
|
|
||
| $this->assertEquals('bulk_creator', $bulkDoc2->getCreatedBy()); | ||
| $this->assertEquals('test_user_2', $bulkDoc2->getUpdatedBy()); | ||
|
|
||
| $this->assertEquals('test_user_2', $bulkDoc3->getCreatedBy()); | ||
| $this->assertEquals('bulk_updater', $bulkDoc3->getUpdatedBy()); | ||
|
|
||
| $finalAllDocs = $database->find($collection); | ||
| $this->assertCount(8, $finalAllDocs); | ||
|
|
||
| $database->deleteCollection($collection); | ||
| } | ||
| } |
There was a problem hiding this comment.
Test leaves global Authorization state and collection behind when upserts unsupported
- Authorization is never reset; residual role can leak into later tests.
- Collection cleanup is gated inside the upserts support branch; on adapters without upserts the collection persists.
Wrap the test body in try/finally to always reset roles and drop the collection.
Apply this diff:
@@
- $collection = 'test_created_updated_by';
- $database = static::getDatabase();
+ $collection = 'test_created_updated_by';
+ $database = static::getDatabase();
+ try {
@@
- if ($database->getAdapter()->getSupportForUpserts()) {
+ if ($database->getAdapter()->getSupportForUpserts()) {
@@
- $finalAllDocs = $database->find($collection);
+ $finalAllDocs = $database->find($collection);
$this->assertCount(8, $finalAllDocs);
-
- $database->deleteCollection($collection);
}
+ } finally {
+ \Utopia\Database\Validator\Authorization::reset();
+ $database->deleteCollection($collection);
+ }🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/DocumentTests.php around lines 5998 to 6174, the
test leaves global Authorization state and the collection behind when upserts
are not supported; wrap the main test body in a try/finally: save the current
role (e.g. $previousRole = Authorization::getRole() if available), run the
existing test logic in the try, and in finally restore the role with
Authorization::setRole($previousRole) (or clear it with
Authorization::setRole(null) if getRole is not available) and always attempt to
drop the collection with $database->deleteCollection($collection) inside a
try/catch to ignore delete errors so the collection is removed even when the
upsert branch is skipped.
…res adapters * update size for createdBy and updatedBy attributes in Database and Structure validators * enhance Authorization tests with additional roles
ArnabChatterjee20k
changed the title
| { | ||
| /** @var Database $database */ | ||
| $database = static::getDatabase(); | ||
| $collectionName = 'tesstAutoIncr'; |
There was a problem hiding this comment.
@abnegate changed the collection name here because of postgres issue with relation names
-
Postgres store indexes in a separate relation and relation name is truncated to max 63 chars
-
Issue with created_at and created_by
If we use FUNCTION as the collection name then while creating index relation
the relation name truncated to myapp_68c7c6ce86d87_testCreateDocumentsWithAutoIncrement_cre
As a result -> collection already exists error
So changed the name to accomodate this
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/Adapter/Scopes/AttributeTests.php (1)
771-778: Test not executed: method name typo.Method is named “textRenameAttributeMissing”; PHPUnit won’t run it. Rename to “testRenameAttributeMissing”.
- public function textRenameAttributeMissing(): void + public function testRenameAttributeMissing(): void
♻️ Duplicate comments (10)
src/Database/Adapter/Postgres.php (1)
1086-1090: Don’t modify "_createdBy" on update (immutable).Align with MariaDB and preserve provenance: only set "_updatedBy" during updates.
$attributes['_createdAt'] = $document->getCreatedAt(); $attributes['_updatedAt'] = $document->getUpdatedAt(); - $attributes['_createdBy'] = $document->getCreatedBy(); $attributes['_updatedBy'] = $document->getUpdatedBy();src/Database/Database.php (8)
3997-4010: Same hardening for batch create.- $createdBy = $document->getCreatedBy(); - $updatedBy = $document->getUpdatedBy(); + $actor = Authorization::getUser(); @@ - ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) - ->setAttribute('$createdBy', $createdBy === null ? Authorization::getUser() : $createdBy) - ->setAttribute('$updatedBy', $updatedBy === null ? Authorization::getUser() : $updatedBy); + ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) + ->setAttribute('$createdBy', $this->isMigrating() && $document->getCreatedBy() !== null ? $document->getCreatedBy() : $actor) + ->setAttribute('$updatedBy', $actor);
5354-5359: Upsert path: preserve existing createdBy; set to actor only on insert (or allow override when migrating).- $createdBy = $document->getCreatedBy(); - if ($createdBy === null) { - $document->setAttribute('$createdBy', $old->isEmpty() ? Authorization::getUser() : $old->getCreatedBy()); - } else { - $document->setAttribute('$createdBy', $createdBy); - } + $document->setAttribute( + '$createdBy', + $this->isMigrating() && $document->getCreatedBy() !== null + ? $document->getCreatedBy() + : ($old->isEmpty() ? Authorization::getUser() : $old->getCreatedBy()) + );
4435-4441: createdBy must be immutable on update (unless migrating).- $createdBy = $document->getCreatedBy(); @@ - $document['$createdBy'] = $createdBy === null ? $old->getCreatedBy() : $createdBy; + $document['$createdBy'] = $this->isMigrating() && $document->getCreatedBy() !== null + ? $document->getCreatedBy() + : $old->getCreatedBy();
5338-5345: Upsert path: same updatedBy hardening.- ->setAttribute('$updatedBy', $updatedBy === null ? Authorization::getUser() : $updatedBy) + ->setAttribute('$updatedBy', $this->isMigrating() && $updatedBy !== null ? $updatedBy : Authorization::getUser())
4695-4699: Bulk update: don’t allow mass-editing createdBy outside migrations.- if ($updates->getCreatedBy() === null) { - unset($updates['$createdBy']); - } else { - $updates['$createdBy'] = $updates->getCreatedBy(); - } + if (!$this->isMigrating()) { + unset($updates['$createdBy']); + } elseif ($updates->getCreatedBy() === null) { + unset($updates['$createdBy']); + } else { + $updates['$createdBy'] = $updates->getCreatedBy(); + }
4705-4709: Bulk update: force updatedBy to actor (gate override by migration).- $updates['$updatedBy'] = $updatedBy === null ? Authorization::getUser() : $updatedBy; + $updates['$updatedBy'] = $this->isMigrating() && $updatedBy !== null ? $updatedBy : Authorization::getUser();
3892-3902: Block audit spoofing at create(): source createdBy/updatedBy from the acting user (allow override only when migrating).Currently client payload can forge
$createdBy/$updatedBy. Set both from the actor; allow$createdByoverride only under$this->isMigrating().- $createdBy = $document->getCreatedBy(); - $updatedBy = $document->getUpdatedBy(); + $actor = Authorization::getUser(); @@ - ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) - ->setAttribute('$createdBy', $createdBy === null ? Authorization::getUser() : $createdBy) - ->setAttribute('$updatedBy', $updatedBy === null ? Authorization::getUser() : $updatedBy); + ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt) + ->setAttribute('$createdBy', $this->isMigrating() && $document->getCreatedBy() !== null ? $document->getCreatedBy() : $actor) + ->setAttribute('$updatedBy', $actor);
4564-4566: updatedBy on update should reflect the actor (optionally allow in migrations).- $document->setAttribute('$updatedBy', $newUpdatedBy === null ? Authorization::getUser() : $newUpdatedBy); + $document->setAttribute('$updatedBy', $this->isMigrating() && $newUpdatedBy !== null ? $newUpdatedBy : Authorization::getUser());tests/e2e/Adapter/Scopes/DocumentTests.php (1)
6005-6218: Fix global Authorization/collection leakage; avoid collection name collision; tighten assertions
- Authorization state is never reset; roles leak into later tests.
- Deleting test_created_updated_by happens only when upserts are supported, leaving the collection behind otherwise.
- Reusing the collection name "increase_decrease" risks a duplicate with other tests in this file.
- Minor: asserts after a fresh getDocument() read from $doc instead of $document.
Wrap the body in try/finally to always reset Authorization and delete created collections, use a unique name for the inc/dec collection, and fix the variable used in assertions.
Apply this diff:
public function testCreatedByUpdatedBy(): void { - $collection = 'test_created_updated_by'; - $database = static::getDatabase(); + $collection = 'test_created_updated_by'; + $database = static::getDatabase(); + $collections = [$collection]; + try { $database->createCollection($collection); $database->createAttribute($collection, 'string', Database::VAR_STRING, 128, false); $database->createAttribute($collection, 'number', Database::VAR_INTEGER, 0, false); @@ - $finalAllDocs = $database->find($collection); + $finalAllDocs = $database->find($collection); $this->assertCount(8, $finalAllDocs); - - $database->deleteCollection($collection); + // cleanup moved to finally } - - // testing with increment decrement + // testing with increment decrement Authorization::setRole('user:test_user_3'); - $collection = 'increase_decrease'; - $database->createCollection($collection); + $incCollection = __FUNCTION__ . '_incdec'; + $database->createCollection($incCollection); + $collections[] = $incCollection; - $this->assertEquals(true, $database->createAttribute($collection, 'increase', Database::VAR_INTEGER, 0, true)); + $this->assertEquals(true, $database->createAttribute($incCollection, 'increase', Database::VAR_INTEGER, 0, true)); - $document = $database->createDocument($collection, new Document([ + $document = $database->createDocument($incCollection, new Document([ 'increase' => 100, '$permissions' => [ Permission::read(Role::any()), Permission::create(Role::any()), Permission::update(Role::any()), Permission::delete(Role::any()), ] ])); - $doc = $database->increaseDocumentAttribute($collection, $document->getId(), 'increase', 1, 301); + $doc = $database->increaseDocumentAttribute($incCollection, $document->getId(), 'increase', 1, 301); $this->assertEquals(101, $doc->getAttribute('increase')); $this->assertEquals('test_user_3', $doc->getCreatedBy()); $this->assertEquals('test_user_3', $doc->getUpdatedBy()); - $document = $database->getDocument($collection, $document->getId()); - $this->assertEquals(101, $document->getAttribute('increase')); - $this->assertEquals('test_user_3', $doc->getCreatedBy()); - $this->assertEquals('test_user_3', $doc->getUpdatedBy()); + $document = $database->getDocument($incCollection, $document->getId()); + $this->assertEquals(101, $document->getAttribute('increase')); + $this->assertEquals('test_user_3', $document->getCreatedBy()); + $this->assertEquals('test_user_3', $document->getUpdatedBy()); Authorization::setRole('user:test_user_4'); - $doc = $database->increaseDocumentAttribute($collection, $document->getId(), 'increase', 1, 301); + $doc = $database->increaseDocumentAttribute($incCollection, $document->getId(), 'increase', 1, 301); $this->assertEquals('test_user_3', $doc->getCreatedBy()); $this->assertEquals('test_user_4', $doc->getUpdatedBy()); - $doc = $database->getDocument($collection, $document->getId()); + $doc = $database->getDocument($incCollection, $document->getId()); $this->assertEquals('test_user_3', $doc->getCreatedBy()); $this->assertEquals('test_user_4', $doc->getUpdatedBy()); - $doc = $database->decreaseDocumentAttribute($collection, $document->getId(), 'increase', 1, -100); + $doc = $database->decreaseDocumentAttribute($incCollection, $document->getId(), 'increase', 1, -100); $this->assertEquals('test_user_3', $doc->getCreatedBy()); $this->assertEquals('test_user_4', $doc->getUpdatedBy()); - $doc = $database->getDocument($collection, $document->getId()); + $doc = $database->getDocument($incCollection, $document->getId()); $this->assertEquals('test_user_3', $doc->getCreatedBy()); $this->assertEquals('test_user_4', $doc->getUpdatedBy()); Authorization::setRole('user:test_user_5'); - $doc = $database->decreaseDocumentAttribute($collection, $document->getId(), 'increase', 1, -100); + $doc = $database->decreaseDocumentAttribute($incCollection, $document->getId(), 'increase', 1, -100); $this->assertEquals('test_user_3', $doc->getCreatedBy()); $this->assertEquals('test_user_5', $doc->getUpdatedBy()); - $doc = $database->getDocument($collection, $document->getId()); + $doc = $database->getDocument($incCollection, $document->getId()); $this->assertEquals('test_user_3', $doc->getCreatedBy()); $this->assertEquals('test_user_5', $doc->getUpdatedBy()); } + } finally { + Authorization::reset(); + foreach ($collections as $c) { + try { $database->deleteCollection($c); } catch (\Throwable $e) {} + } + } }
🧹 Nitpick comments (10)
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (1)
2385-2386: Prefer ID::unique() over uniqid() to avoid rare cross‑process collisions.uniqid() is time‑based and can collide under heavy parallelism. Using ID::unique() keeps names ASCII and collision‑resistant without dots.
Apply:
- $parentCollection = uniqid('parent_rel'); - $childCollection = uniqid('child_rel'); + $parentCollection = 'parent_rel_' . ID::unique(); + $childCollection = 'child_rel_' . ID::unique();tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php (1)
1773-1775: Use stronger unique IDs for dynamic collection names.Swap uniqid() for ID::unique() to eliminate collision risk in parallel test runs.
- $parentCollection = uniqid('parent_rel'); - $childCollection = uniqid('child_rel'); + $parentCollection = 'parent_rel_' . ID::unique(); + $childCollection = 'child_rel_' . ID::unique();tests/e2e/Adapter/Scopes/SpatialTests.php (1)
725-725: Harden all dynamic names: prefer ID::unique() over uniqid().Consistently use ID::unique() to avoid time‑based collisions and keep names dot‑free.
- $collectionName = uniqid('sp_idx'); + $collectionName = 'sp_idx_' . ID::unique(); - $collOrderCreate = uniqid('sp_idx_'); + $collOrderCreate = 'sp_idx_' . ID::unique(); - $collOrderIndex = uniqid('sp_idx_'); + $collOrderIndex = 'sp_idx_' . ID::unique(); - $collNullCreate = uniqid('sp_idx'); + $collNullCreate = 'sp_idx_' . ID::unique(); - $collNullIndex = uniqid('sp_idx_'); + $collNullIndex = 'sp_idx_' . ID::unique(); - $collUpdateNull = uniqid('sp_idx'); + $collUpdateNull = 'sp_idx_' . ID::unique(); - $collUpdateNull = uniqid('sp_idx'); + $collUpdateNull = 'sp_idx_' . ID::unique(); - $collectionName = uniqid("sp_idx"); + $collectionName = 'sp_idx_' . ID::unique();Also applies to: 749-749, 786-786, 809-809, 847-847, 865-865, 889-889, 2490-2490
tests/e2e/Adapter/Scopes/AttributeTests.php (4)
805-807: Avoid brittle baseline width assertion (3109).This hardcoded baseline will keep breaking as internal attrs evolve per adapter. Keep the delta checks (they’re stable) and drop or gate this baseline assert by adapter.
Apply this diff:
- $init = $database->getAdapter()->getAttributeWidth($collection); - $this->assertEquals(3109, $init); + $init = $database->getAdapter()->getAttributeWidth($collection); + $this->assertGreaterThan(0, $init);
957-961: ID/size mismatch nit: rename attribute id to match new size.Keep test intent clear by aligning the id with its size.
- $attributes[] = new Document([ - '$id' => ID::custom('varchar_16000'), + $attributes[] = new Document([ + '$id' => ID::custom('varchar_15500'), 'type' => Database::VAR_STRING, - 'size' => 15500, + 'size' => 15500,
905-907: Make limit message assertion adapter-driven.The expected message embeds a fixed “1017” that may diverge per adapter. Assert using the adapter value to avoid false negatives across engines.
- $this->assertEquals('Attribute limit of 1017 exceeded. Cannot create collection.', $e->getMessage()); + $this->assertEquals( + 'Attribute limit of ' . $database->getAdapter()->getLimitForAttributes() . ' exceeded. Cannot create collection.', + $e->getMessage() + );
1843-1863: Typo in variable name ($collecitonName).Use a consistent, correctly spelled variable; reduces confusion and IDE misses.
- $collecitonName = uniqid('test_attr_dup'); + $collectionName = uniqid('test_attr_dup'); @@ - $database->createCollection($collecitonName); - $database->createAttribute($collecitonName, 'dup', Database::VAR_STRING, 10, false); + $database->createCollection($collectionName); + $database->createAttribute($collectionName, 'dup', Database::VAR_STRING, 10, false); @@ - $database->createAttributes($collecitonName, $attributes); + $database->createAttributes($collectionName, $attributes);src/Database/Adapter/MariaDB.php (1)
1775-1779: Update internal index keys to include created_by/updated_by.These new indexes are internal; add them so tooling doesn’t surface/drop them accidentally.
public function getInternalIndexesKeys(): array { - return ['primary', '_created_at', '_updated_at', '_tenant_id']; + return ['primary', '_created_at', '_updated_at', '_created_by', '_updated_by', '_tenant_id']; }src/Database/Adapter/Postgres.php (1)
856-862: Map $createdBy/$updatedBy to internal columns in index DDL.Enable creating indexes via public names, consistent with $createdAt/$updatedAt mapping.
- $attr = match ($attr) { + $attr = match ($attr) { '$id' => '_uid', '$createdAt' => '_createdAt', '$updatedAt' => '_updatedAt', + '$createdBy' => '_createdBy', + '$updatedBy' => '_updatedBy', default => $this->filter($attr), };tests/e2e/Adapter/Scopes/DocumentTests.php (1)
376-416: LGTM: per-test collection with deterministic cleanupSwitching to a dedicated collection name and using a finally block to delete the collection is solid. One nit: the loop creates $count + 1 docs due to the inclusive upper bound.
Apply this small fix to avoid the off‑by‑one:
- $count = 10; + $count = 10; $sequence = 1_000_000; - for ($i = $sequence; $i <= ($sequence + $count); $i++) { + for ($i = $sequence; $i < ($sequence + $count); $i++) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Database/Adapter/MariaDB.php(8 hunks)src/Database/Adapter/Postgres.php(7 hunks)src/Database/Database.php(15 hunks)src/Database/Validator/Authorization.php(4 hunks)src/Database/Validator/Structure.php(1 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(13 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(9 hunks)tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php(1 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php(1 hunks)tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php(1 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(8 hunks)tests/unit/Validator/AuthorizationTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/Validator/AuthorizationTest.php
- src/Database/Validator/Authorization.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Validator/Structure.php
🧬 Code graph analysis (6)
src/Database/Database.php (3)
src/Database/Document.php (9)
getCreatedBy(170-173)getUpdatedBy(186-189)setAttribute(260-277)getAttribute(240-247)getCreatedAt(162-165)getTenant(194-203)getUpdatedAt(178-181)isEmpty(412-415)setAttributes(285-292)src/Database/Adapter.php (3)
withTransaction(377-425)getSharedTables(188-191)getTenant(216-219)src/Database/DateTime.php (2)
DateTime(7-86)now(19-23)
tests/e2e/Adapter/Scopes/AttributeTests.php (4)
src/Database/Database.php (6)
getAdapter(1223-1226)Database(37-7385)createAttributes(1787-1879)createAttribute(1716-1771)getCollection(1540-1556)createDocument(3861-3956)src/Database/Adapter/SQLite.php (2)
getSupportForBatchCreateAttributes(972-975)createDocument(524-634)src/Database/Adapter.php (4)
getSupportForBatchCreateAttributes(1051-1051)createAttributes(569-569)createAttribute(558-558)createDocument(702-702)src/Database/Adapter/Pool.php (4)
getSupportForBatchCreateAttributes(423-426)createAttributes(173-176)createAttribute(168-171)createDocument(228-231)
src/Database/Adapter/Postgres.php (5)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)src/Database/Database.php (1)
increaseDocumentAttribute(5495-5577)src/Database/Adapter/MariaDB.php (1)
increaseDocumentAttribute(1259-1306)src/Database/Adapter.php (1)
increaseDocumentAttribute(1242-1251)src/Database/Adapter/Pool.php (1)
increaseDocumentAttribute(484-487)
src/Database/Adapter/MariaDB.php (1)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)
src/Database/Database.php (12)
createCollection(1352-1486)createAttribute(1716-1771)Database(37-7385)create(1265-1284)delete(1323-1337)createDocuments(3971-4068)deleteCollection(1647-1690)createDocument(3861-3956)createOrUpdateDocuments(5234-5247)getDocument(3469-3589)increaseDocumentAttribute(5495-5577)decreaseDocumentAttribute(5593-5677)src/Database/Adapter.php (11)
createCollection(526-526)createAttribute(558-558)create(489-489)delete(516-516)createDocuments(714-714)deleteCollection(535-535)getSupportForUpserts(1023-1023)createDocument(702-702)createOrUpdateDocuments(753-757)getDocument(692-692)increaseDocumentAttribute(1242-1251)src/Database/Validator/Authorization.php (2)
Authorization(8-250)setRole(89-97)
src/Database/Validator/Structure.php (1)
src/Database/Database.php (1)
Database(37-7385)
🔇 Additional comments (8)
src/Database/Validator/Structure.php (1)
88-109: Verify $createdBy/$updatedBy propagation & sanitization
- Add '$createdBy' and '$updatedBy' to PartialStructure/internal-attribute lists and ensure PartialStructure uses $this->attributes (not the merged $attributes) when filtering required internal attributes. (src/Database/Validator/PartialStructure.php, src/Database/Database.php)
- Ensure adapters override/ignore user-supplied values and set these fields from Authorization; confirm adapter method signatures/implementations in src/Database/Adapter/* handle/accept ?string $createdBy/$updatedBy or obtain them from Authorization.
- Confirm Authorization exposes current user (e.g., getUser()) and adapters call it (src/Database/Validator/Authorization or equivalent).
- Add/verify tests for createdBy/updatedBy propagation and sanitization (search/add tests for createdBy/updatedBy or testCreatedByUpdatedBy).
Run to confirm:
rg -n --hidden -C2 '\$createdBy|\$updatedBy' src || true rg -n --hidden -C2 'INTERNAL_ATTRIBUTES' src/Database || true rg -nP --hidden -C2 'increaseDocumentAttribute\s*\([^)]*\?\s*string\s*\$updatedBy' src/Database/Adapter || true rg -n --hidden -C2 'class\s+Authorization|function\s+getUser' src/Database || true rg -n 'createdBy|updatedBy|testCreatedByUpdatedBy' tests || truesrc/Database/Adapter/MariaDB.php (1)
1214-1221: Confirm upsert-increment also updates “_updatedBy”.In the increment path, ON DUPLICATE UPDATE sets only _updatedAt. If your upsert callers provide _updatedBy in insert columns, consider adding it to $updateColumns as well for parity with non-increment updates. Otherwise, document that upsert-increment won’t touch _updatedBy.
Would you like a patch that conditionally adds $getUpdateClause('_updatedBy') when provided?
src/Database/Adapter/Postgres.php (1)
1318-1323: Confirm upsert-increment sets “_updatedBy” too.Like MariaDB, the increment branch only updates _updatedAt. If callers pass _updatedBy in INSERT columns, consider adding it to the SET list for consistency.
Do you want a patch adding $getUpdateClause('_updatedBy') when present?
Also applies to: 1357-1391
src/Database/Database.php (4)
271-272: Keys list updated — good.
5654-5664: Decrement path sets updatedBy from actor — good.Also applies to: 5667-5669
233-254: Auditing attributes added: schema looks correct; confirm adapter wiring.Using LENGTH_KEY for both string fields is correct. DDL and encode/decode mapping for _createdBy/_updatedBy exist across SQLite/Postgres/MariaDB/SQL adapters, but adapter->getInternalIndexesKeys is inconsistent — src/Database/Adapter/SQL.php returns [] and src/Database/Adapter/MariaDB.php returns ['primary','_created_at','_updated_at','_tenant_id'] (both omit _createdBy/_updatedBy). Confirm whether omission is intentional; if not, add those keys to adapters so DDL/indices and encode/decode/casting remain aligned.
5554-5564: Approved — adapters accept updatedByAdapter.php's abstract signature includes string $updatedAt, ?string $updatedBy and implementations in src/Database/Adapter/Postgres.php, src/Database/Adapter/MariaDB.php and src/Database/Adapter/Pool.php all accept the parameter. Mirror delegates to Database; Database builds $updatedAt and $updatedBy and passes them to the adapter.
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
839-953: LGTM: unique per-test collection and attribute-mismatch coverageUsing uniqid() for the collection avoids cross-test collisions, and the assertions exercise optional/missing attributes well. No issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
896-965: Fix: undefined method createOrUpdateDocuments() — replace with upsertDocuments().The pipeline fails on these calls. Database exposes upsertDocuments(...), not createOrUpdateDocuments(...). Replace all occurrences in this test hunk.
Apply:
- $docs = $database->createOrUpdateDocuments($collectionName, [ + $docs = $database->upsertDocuments($collectionName, [ $existingDocument->setAttribute('first', 'updated'), $newDocument, ]); @@ - $database->createOrUpdateDocuments($collectionName, [ + $database->upsertDocuments($collectionName, [ $existingDocument->removeAttribute('first'), $newDocument ]); @@ - $docs = $database->createOrUpdateDocuments($collectionName, [ + $docs = $database->upsertDocuments($collectionName, [ $existingDocument ->setAttribute('first', 'first') ->removeAttribute('last'), $newDocument ->setAttribute('last', 'last') ]); @@ - $docs = $database->createOrUpdateDocuments($collectionName, [ + $docs = $database->upsertDocuments($collectionName, [ $existingDocument ->setAttribute('first', 'first') ->setAttribute('last', null), $newDocument ->setAttribute('last', 'last') ]); @@ - $docs = $database->createOrUpdateDocuments($collectionName, [ + $docs = $database->upsertDocuments($collectionName, [ $doc3, $doc4 ]);
♻️ Duplicate comments (2)
src/Database/Adapter/Postgres.php (1)
1087-1088: Do not mutate_createdByon updates (keep immutable).Updating
_createdByon edits clobbers provenance and diverges from MariaDB behavior. Only_updatedByshould change.$attributes['_createdAt'] = $document->getCreatedAt(); $attributes['_updatedAt'] = $document->getUpdatedAt(); - $attributes['_createdBy'] = $document->getCreatedBy(); $attributes['_updatedBy'] = $document->getUpdatedBy();tests/e2e/Adapter/Scopes/DocumentTests.php (1)
6030-6243: Stabilize the new createdBy/updatedBy test: undefined method calls, state/collection leaks, and a couple assertion glitches.
- Replace the two createOrUpdateDocuments(...) calls with upsertDocuments(...). They currently break the pipeline.
- Avoid reusing the "increase_decrease" collection name: it already exists earlier in this suite and will collide. Use a uniqid‑based name.
- Wrap the whole test in try/finally to always reset Authorization and drop collections, even when upserts aren’t supported. This addresses prior feedback about global state/leftovers.
- Fix assertions using $doc instead of $document after a fresh getDocument().
Apply:
@@ - $collection = 'test_created_updated_by'; + $collection = 'test_created_updated_by'; $database = static::getDatabase(); - - $database->createCollection($collection); + try { + $database->createCollection($collection); @@ - if ($database->getAdapter()->getSupportForUpserts()) { - $upsertCount = $database->createOrUpdateDocuments($collection, [$upsertDoc]); + if ($database->getAdapter()->getSupportForUpserts()) { + $upsertCount = $database->upsertDocuments($collection, [$upsertDoc]); @@ - $upsertNewCount = $database->createOrUpdateDocuments($collection, [$upsertNewDoc]); + $upsertNewCount = $database->upsertDocuments($collection, [$upsertNewDoc]); @@ - $database->deleteCollection($collection); + $database->deleteCollection($collection); @@ - Authorization::setRole('user:test_user_3'); - $collection = 'increase_decrease'; - $database->createCollection($collection); + Authorization::setRole('user:test_user_3'); + $collection = uniqid('increase_decrease_'); + $database->createCollection($collection); @@ - $document = $database->getDocument($collection, $document->getId()); - $this->assertEquals(101, $document->getAttribute('increase')); - $this->assertEquals('test_user_3', $doc->getCreatedBy()); - $this->assertEquals('test_user_3', $doc->getUpdatedBy()); + $document = $database->getDocument($collection, $document->getId()); + $this->assertEquals(101, $document->getAttribute('increase')); + $this->assertEquals('test_user_3', $document->getCreatedBy()); + $this->assertEquals('test_user_3', $document->getUpdatedBy()); @@ - } + } finally { + \Utopia\Database\Validator\Authorization::reset(); + // Best-effort cleanup for both phases of the test + try { $database->deleteCollection('test_created_updated_by'); } catch (\Throwable $e) {} + // $collection may have been reassigned to the inc/dec temp collection + try { $database->deleteCollection($collection); } catch (\Throwable $e) {} + }
🧹 Nitpick comments (5)
src/Database/Adapter/Postgres.php (3)
196-201: Redundant variable initialization.
$attributeStringsis initialized twice with identical docblocks.- /** @var array<string> $attributeStrings */ - $attributeStrings = []; - /** @var array<string> $attributeStrings */ $attributeStrings = [];
855-861: Map$createdBy/$updatedByto internal columns in createIndex.Without these, user-defined indexes on
$createdBy/$updatedBywon’t target the correct columns.$attr = match ($attr) { '$id' => '_uid', '$createdAt' => '_createdAt', '$updatedAt' => '_updatedAt', + '$createdBy' => '_createdBy', + '$updatedBy' => '_updatedBy', default => $this->filter($attr), };
1318-1322: Upsert flow should also update_updatedBy.When
$attributeis set (increment path), upsert updates_updatedAtbut not_updatedBy. Add it for parity with other update paths.if ($increment) { $new = "target.{$attribute} + EXCLUDED.{$attribute}"; } else { $new = "EXCLUDED.{$attribute}"; } @@ if (!empty($attribute)) { // Increment specific column by its new value in place $updateColumns = [ $getUpdateClause($attribute, increment: true), $getUpdateClause('_updatedAt'), + $getUpdateClause('_updatedBy'), ];Note: Ensure callers include
_updatedByin the insert/EXCLUDED column list and bind values when using this upsert.tests/e2e/Adapter/Scopes/DocumentTests.php (2)
401-441: Nice: added try/finally cleanup for the collection in this test.Two tiny nits:
- Collection name looks like a typo: "tesstAutoIncr". Consider a shorter deterministic name or uniqid to avoid future collisions.
- Reusing $count (first as intended count=10, then as returned created count=11 due to <=) is a bit confusing; using distinct vars would read clearer.
6213-6214: Fix assertion target: using $doc instead of freshly fetched $document.Use the just-fetched $document to assert createdBy/updatedBy.
Apply:
- $this->assertEquals('test_user_3', $doc->getCreatedBy()); - $this->assertEquals('test_user_3', $doc->getUpdatedBy()); + $this->assertEquals('test_user_3', $document->getCreatedBy()); + $this->assertEquals('test_user_3', $document->getUpdatedBy());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/MariaDB.php(8 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/Postgres.php(7 hunks)src/Database/Adapter/SQL.php(8 hunks)src/Database/Adapter/SQLite.php(4 hunks)src/Database/Database.php(15 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(9 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(1 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Database/Database.php
- src/Database/Adapter/Pool.php
- tests/e2e/Adapter/Scopes/GeneralTests.php
- src/Database/Adapter/SQLite.php
- src/Database/Adapter/MariaDB.php
- tests/e2e/Adapter/Scopes/SpatialTests.php
- src/Database/Adapter/SQL.php
- src/Database/Adapter.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (7)
src/Database/Database.php (12)
createCollection(1354-1488)createAttribute(1718-1773)Database(37-7384)create(1267-1286)delete(1325-1339)createDocuments(3974-4078)deleteCollection(1649-1692)createDocument(3863-3958)getDocument(3471-3591)updateDocument(4415-4619)increaseDocumentAttribute(5546-5628)decreaseDocumentAttribute(5644-5728)src/Database/Adapter.php (11)
createCollection(526-526)createAttribute(558-558)create(489-489)delete(516-516)createDocuments(714-714)deleteCollection(535-535)getSupportForUpserts(1023-1023)createDocument(702-702)getDocument(692-692)updateDocument(726-726)increaseDocumentAttribute(1242-1251)src/Database/Adapter/SQLite.php (7)
createCollection(141-229)create(113-116)delete(126-129)deleteCollection(290-309)getSupportForUpserts(952-955)createDocument(524-634)updateDocument(648-858)src/Database/Adapter/Postgres.php (9)
createCollection(191-340)createAttribute(451-470)create(135-162)delete(172-180)deleteCollection(417-425)getSupportForUpserts(1901-1904)createDocument(965-1067)updateDocument(1081-1283)increaseDocumentAttribute(1363-1397)src/Database/Adapter/MariaDB.php (8)
createCollection(76-231)create(28-43)delete(53-64)deleteCollection(326-341)getSupportForUpserts(1731-1734)createDocument(828-941)updateDocument(956-1179)increaseDocumentAttribute(1259-1306)src/Database/Document.php (5)
Document(12-486)getId(63-66)getAttribute(240-247)getCreatedBy(170-173)getUpdatedBy(186-189)src/Database/Validator/Authorization.php (2)
Authorization(8-250)setRole(89-97)
src/Database/Adapter/Postgres.php (5)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)src/Database/Database.php (1)
increaseDocumentAttribute(5546-5628)src/Database/Adapter.php (1)
increaseDocumentAttribute(1242-1251)src/Database/Adapter/Pool.php (1)
increaseDocumentAttribute(478-481)src/Database/Adapter/MariaDB.php (1)
increaseDocumentAttribute(1259-1306)
🪛 GitHub Actions: CodeQL
tests/e2e/Adapter/Scopes/DocumentTests.php
[error] 896-896: Call to an undefined method Utopia\Database\Database::createOrUpdateDocuments().
[error] 908-908: Call to an undefined method Utopia\Database\Database::createOrUpdateDocuments().
[error] 918-918: Call to an undefined method Utopia\Database\Database::createOrUpdateDocuments().
[error] 933-933: Call to an undefined method Utopia\Database\Database::createOrUpdateDocuments().
[error] 960-960: Call to an undefined method Utopia\Database\Database::createOrUpdateDocuments().
[error] 6124-6124: Call to an undefined method Utopia\Database\Database::createOrUpdateDocuments().
[error] 6139-6139: Call to an undefined method Utopia\Database\Database::createOrUpdateDocuments().
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (6)
src/Database/Adapter/Postgres.php (5)
253-255: LGTM: indexes for_createdBy/_updatedByon shared tables.Adds tenant-prefixed indexes; naming consistent with existing pattern.
262-264: LGTM: indexes for_createdBy/_updatedByon non-shared tables.Consistent with other internal indexes.
971-973: LGTM: set_createdBy/_updatedByon insert.Correctly persists audit fields during creation.
1357-1364: Signature aligned with Adapter/Pool; confirm all call sites updated.
increaseDocumentAttribute(..., string $updatedAt, ?string $updatedBy, ...)matches the abstract + Pool. Verify no stale invocations remain.
1375-1390: LGTM: write_updatedAtand_updatedByon atomic increments.Matches MariaDB; binds look correct, including tenant handling.
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
868-883: Good call switching to a uniqid-based collection.This prevents Postgres 63‑char relation name truncation issues from FUNCTION and avoids accidental collisions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
6191-6196: Prevent cross‑test collection collision and variable shadowing for increment/decrement block.Don’t reuse the global “increase_decrease” name and don’t overwrite
$collectionfrom the first part of the test. Use a unique$incCollection.- Authorization::setRole('user:test_user_3'); - $collection = 'increase_decrease'; - $database->createCollection($collection); + Authorization::setRole('user:test_user_3'); + $incCollection = uniqid('increase_decrease_'); + $database->createCollection($incCollection); - $this->assertEquals(true, $database->createAttribute($collection, 'increase', Database::VAR_INTEGER, 0, true)); + $this->assertEquals(true, $database->createAttribute($incCollection, 'increase', Database::VAR_INTEGER, 0, true)); - $document = $database->createDocument($collection, new Document([ + $document = $database->createDocument($incCollection, new Document([And replace subsequent uses of
$collectionin this block with$incCollection. Thefinallyblock should drop$incCollection(see prior comment).
6030-6188: Leak of global Authorization state and collection when upserts unsupported; wrap in try/finally.Authorization is never reset, and
$collectionis deleted only in the upsert branch. Always reset and always drop collections.- public function testCreatedByUpdatedBy(): void - { - $collection = 'test_created_updated_by'; - $database = static::getDatabase(); - - $database->createCollection($collection); + public function testCreatedByUpdatedBy(): void + { + $collection = 'test_created_updated_by'; + $database = static::getDatabase(); + try { + $database->createCollection($collection); $database->createAttribute($collection, 'string', Database::VAR_STRING, 128, false); $database->createAttribute($collection, 'number', Database::VAR_INTEGER, 0, false); Authorization::setRole('user:test_user_1'); ... - $finalAllDocs = $database->find($collection); - $this->assertCount(8, $finalAllDocs); - - $database->deleteCollection($collection); + $finalAllDocs = $database->find($collection); + $this->assertCount(8, $finalAllDocs); ... - } + } finally { + \Utopia\Database\Validator\Authorization::reset(); + try { $database->deleteCollection($collection); } catch (\Throwable) {} + if (isset($incCollection)) { + try { $database->deleteCollection($incCollection); } catch (\Throwable) {} + } + }
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
401-441: Good isolation with try/finally; tiny readability nit on counters.Cleanup is solid. Minor: the loop creates 11 docs while
$countis 10 and then$countis reused for the return value. Consider distinct names and non‑inclusive upper bound to avoid off‑by‑one confusion.- $documents = []; - $count = 10; + $documents = []; + $numDocs = 10; $sequence = 1_000_000; - for ($i = $sequence; $i <= ($sequence + $count); $i++) { + for ($i = $sequence; $i < $sequence + $numDocs; $i++) { $documents[] = new Document([ '$sequence' => (string)$i, ... ]); } - - $count = $database->createDocuments($collectionName, $documents, 6); - $this->assertEquals($count, \count($documents)); + $created = $database->createDocuments($collectionName, $documents, 6); + $this->assertEquals($created, \count($documents));
6211-6215: Assert on the freshly retrieved document, not the prior returned instance.You fetch
$documentthen assert on$doc. Assert on$documentto validate persisted values.- $document = $database->getDocument($collection, $document->getId()); + $document = $database->getDocument($incCollection, $document->getId()); $this->assertEquals(101, $document->getAttribute('increase')); - $this->assertEquals('test_user_3', $doc->getCreatedBy()); - $this->assertEquals('test_user_3', $doc->getUpdatedBy()); + $this->assertEquals('test_user_3', $document->getCreatedBy()); + $this->assertEquals('test_user_3', $document->getUpdatedBy());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Adapter/Scopes/DocumentTests.php(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
src/Database/Adapter.php (9)
create(489-489)delete(516-516)createDocuments(714-714)deleteCollection(535-535)createDocument(702-702)upsertDocuments(753-757)getDocument(692-692)updateDocument(726-726)increaseDocumentAttribute(1242-1251)src/Database/Database.php (11)
Database(37-7384)create(1267-1286)delete(1325-1339)createDocuments(3974-4078)deleteCollection(1649-1692)createDocument(3863-3958)upsertDocuments(5275-5290)getDocument(3471-3591)updateDocument(4415-4619)increaseDocumentAttribute(5546-5628)decreaseDocumentAttribute(5644-5728)src/Database/Document.php (6)
Document(12-486)getSequence(71-80)getId(63-66)getAttribute(240-247)getCreatedBy(170-173)getUpdatedBy(186-189)src/Database/Validator/Authorization.php (2)
Authorization(8-250)setRole(89-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
868-883: Nice: unique per‑test collection name prevents collisions.Using
uniqid("test_attr_")and explicit setup looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1177-1219: Do not drop 'increase_decrease' here; breaks all @Depends testsDeleting the collection in testIncreaseDecrease invalidates downstream tests that depend on this fixture (they reference the same collection/id). Remove deletion here and clean up after the last dependent test instead.
- $database->deleteCollection($collection); return $document;Add cleanup at the end of testIncreaseArrayAttribute (last dependent):
// at end of testIncreaseArrayAttribute(...) $database->deleteCollection('increase_decrease');
♻️ Duplicate comments (3)
src/Database/Adapter/Postgres.php (1)
1086-1088: Don’t mutate "_createdBy" on update (keep immutable)Updating
_createdByon edits clobbers provenance and diverges from MariaDB behavior. Only_updatedByshould change.Apply this diff:
- $attributes['_createdBy'] = $document->getCreatedBy(); $attributes['_updatedBy'] = $document->getUpdatedBy();tests/e2e/Adapter/Scopes/DocumentTests.php (2)
6191-6196: Use a unique name to avoid cross‑test collision with prior 'increase_decrease'This hard‑coded name collides with other tests in this file. Generate a unique name and use it consistently in this block.
- $collection = 'increase_decrease'; + $incCollection = uniqid('increase_decrease_'); - $database->createCollection($collection); + $database->createCollection($incCollection);Then replace subsequent $collection uses in this block with $incCollection.
6031-6245: Wrap test in try/finally; always reset Authorization and clean collectionsThis test mutates global Authorization roles multiple times and only deletes the collection inside the upsert branch. On adapters without upserts, state and data leak to later tests.
Refactor skeleton:
- public function testCreatedByUpdatedBy(): void + public function testCreatedByUpdatedBy(): void { - $collection = 'test_created_updated_by'; - $database = static::getDatabase(); + $collection = 'test_created_updated_by'; + $database = static::getDatabase(); + try { // ... current test body ... - $database->deleteCollection($collection); - // increment/decrement block... + } finally { + \Utopia\Database\Validator\Authorization::reset(); + try { $database->deleteCollection($collection); } catch (\Throwable) {} + if (isset($incCollection)) { + try { $database->deleteCollection($incCollection); } catch (\Throwable) {} + } + } }When switching users (test_user_1 → test_user_2 → ...), call Authorization::cleanRoles() before each setRole to prevent role accumulation.
🧹 Nitpick comments (7)
src/Database/Adapter/Postgres.php (3)
855-869: Map "$createdBy"/"$updatedBy" in createIndex()If callers pass
$createdBy/$updatedBy, the current match falls through and tries to index a non-existent column literally named$createdBy. Add mappings to internal names.Suggested change:
$attr = match ($attr) { '$id' => '_uid', '$createdAt' => '_createdAt', '$updatedAt' => '_updatedAt', '$createdBy' => '_createdBy', '$updatedBy' => '_updatedBy', default => $this->filter($attr), };
1356-1395: Bind :updatedBy with correct PDO type (handles NULL safely)Ensure NULL is bound as PDO::PARAM_NULL to avoid driver quirks.
- $stmt->bindValue(':updatedBy', $updatedBy); + $stmt->bindValue(':updatedBy', $updatedBy, $this->getPDOType($updatedBy));
1316-1321: Parity: increment upserts update _updatedAt but not _updatedByFor consistency with increaseDocumentAttribute(), consider also updating
_updatedByin the increment branch when available.Example:
$updateColumns = [ $getUpdateClause($attribute, increment: true), $getUpdateClause('_updatedAt'), $getUpdateClause('_updatedBy'), ];tests/e2e/Adapter/Scopes/DocumentTests.php (4)
401-441: Off‑by‑one insertion and shadowed $count variableLoop uses <=, producing 11 docs when $count=10, then $count is reassigned to the inserted count. If unintentional, tighten the loop and avoid shadowing:
- $count = 10; + $total = 10; $sequence = 1_000_000; - for ($i = $sequence; $i <= ($sequence + $count); $i++) { + for ($i = $sequence; $i < $sequence + $total; $i++) { $documents[] = new Document([ '$sequence' => (string)$i, ... ]); } - $count = $database->createDocuments($collectionName, $documents, 6); - $this->assertEquals($count, \count($documents)); + $inserted = $database->createDocuments($collectionName, $documents, 6); + $this->assertEquals($total, $inserted);Also, consider renaming 'tesstAutoIncr' to 'testAutoIncr'.
6188-6189: Move cleanup out of the upsert branchDeleting the collection only when upserts are supported leaves data behind otherwise. Delete in finally (see previous comment) and remove these lines.
6213-6215: Wrong variable in assertions (uses $doc instead of $document)After fetching $document, assertions should verify persisted values on $document.
- $this->assertEquals('test_user_3', $doc->getCreatedBy()); - $this->assertEquals('test_user_3', $doc->getUpdatedBy()); + $this->assertEquals('test_user_3', $document->getCreatedBy()); + $this->assertEquals('test_user_3', $document->getUpdatedBy());
6040-6046: Optional: normalize permissions helpers for consistencyYou mix Permission::write(...) with explicit create/update/delete in other tests. Consider standardizing on one approach within this new test for readability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/Postgres.php(7 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
src/Database/Adapter.php (12)
createCollection(526-526)createAttribute(558-558)count(826-826)create(489-489)delete(516-516)createDocuments(714-714)deleteCollection(535-535)getSupportForUpserts(1023-1023)createDocument(702-702)upsertDocuments(753-757)getDocument(692-692)increaseDocumentAttribute(1242-1251)src/Database/Database.php (13)
createCollection(1354-1488)createAttribute(1718-1773)Database(37-7384)count(6624-6660)create(1267-1286)delete(1325-1339)createDocuments(3974-4078)deleteCollection(1649-1692)createDocument(3863-3958)upsertDocuments(5275-5290)getDocument(3471-3591)increaseDocumentAttribute(5546-5628)decreaseDocumentAttribute(5644-5728)src/Database/Document.php (5)
Document(12-486)getSequence(71-80)getId(63-66)getCreatedBy(170-173)getUpdatedBy(186-189)src/Database/Validator/Authorization.php (2)
Authorization(8-250)setRole(89-97)
src/Database/Adapter/Postgres.php (5)
src/Database/Document.php (2)
getCreatedBy(170-173)getUpdatedBy(186-189)src/Database/Adapter.php (1)
increaseDocumentAttribute(1242-1251)src/Database/Adapter/Pool.php (1)
increaseDocumentAttribute(478-481)src/Database/Adapter/MariaDB.php (1)
increaseDocumentAttribute(1259-1306)src/Database/Database.php (1)
increaseDocumentAttribute(5546-5628)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Unit Test
🔇 Additional comments (6)
src/Database/Adapter/Postgres.php (4)
240-244: Schema additions for _createdBy/_updatedBy look goodColumns added with NULL defaults and placed before
_permissions. Duplicate_permissionsissue appears resolved here.
252-254: Indexes on (_tenant, "_createdBy"/"_updatedBy") — LGTMAppropriate composite indexes for shared tables.
261-263: Indexes on "_createdBy"/"_updatedBy" — LGTMNon-shared variant matches the shared-table intent.
970-972: Propagating _createdBy/_updatedBy on insert — LGTMCreate path sets both fields alongside timestamps and permissions.
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
864-883: LGTM: per‑test isolation with unique collectionUsing uniqid("test_attr_") + explicit permissions + documentSecurity=false improves isolation and clarity.
6031-6245: Duplicate collection 'increase_decrease' detected — potential test state collision
- Occurrences: tests/e2e/Adapter/Scopes/DocumentTests.php lines 1168–1293 and 6031–6245.
- Action: ensure isolation — use unique collection names per test or add explicit cleanup/reset (deleteCollection or DB reset in setUp/tearDown) to avoid flaky tests.
Added new internal attributes$createdBy and$ updatedBy
Summary by CodeRabbit
New Features
Storage
API
Tests