Skip to content

Conversation

@ArnabChatterjee20k
Copy link
Contributor

@ArnabChatterjee20k ArnabChatterjee20k commented Jul 30, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation to prevent empty string values for date fields when creating and updating documents, ensuring more reliable data integrity.
    • Enforced mandatory presence of creation and update timestamps on documents.
    • Refined handling of creation and update timestamps to better preserve or auto-populate dates based on configuration.
    • Enhanced database update operations to include creation timestamps when present.
  • Tests

    • Added and enhanced tests to verify rejection of empty string date values and correct handling of valid and null dates during document creation and updates.
    • Added tests to confirm proper preservation and auto-population of creation and update dates in single and bulk document operations.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jul 30, 2025

Walkthrough

Validation logic was added to prevent empty string values for $createdAt and $updatedAt fields in document creation and update methods. Corresponding tests were introduced and enhanced to verify that empty strings for these timestamp fields trigger exceptions, while valid and null values are handled correctly. Internal logic was refined to use strict null checks and ensure required validation on these fields. No public interfaces were changed.

Changes

Cohort / File(s) Change Summary
Date Attribute Handling in Database Operations
src/Database/Database.php
Refined checks for $createdAt and $updatedAt to use strict null comparisons instead of empty(). Updated createDocument, createDocuments, updateDocument, and updateDocuments to set or preserve these dates accordingly. Added logic in encode to convert empty string date attributes to null before encoding. Introduced $attributesToCheckForRequiredValidation in batch updates to enforce required validation. No signature changes.
Enforcing Required Date Attributes in Validation
src/Database/Validator/Structure.php
Changed $createdAt and $updatedAt attribute definitions to required = true in Structure validator, enforcing their mandatory presence during validation. No signature or logic flow changes.
Enhanced Partial Structure Validation
src/Database/Validator/PartialStructure.php
Updated isValid method to include an additional required attributes presence check before other validations. Added filtering of required attributes and enforced their presence during partial validation. No signature changes.
SQL Adapter Update for CreatedAt Attribute
src/Database/Adapter/SQL.php
Modified updateDocuments method to include $createdAt in the attributes array for update when present and non-empty, ensuring _createdAt is updated in the database alongside _updatedAt. No signature changes.
New and Extended Tests for Date Validation
tests/e2e/Adapter/Scopes/DocumentTests.php, tests/e2e/Adapter/Scopes/GeneralTests.php
Added tests verifying correct handling and preservation of $createdAt and $updatedAt with null, empty string, and valid values in single and bulk document create/update scenarios. Extended existing tests to assert that empty strings for these fields trigger StructureException. Verified behavior with date preservation enabled and disabled. No changes to existing method signatures.
Test Data Updates for Required Date Attributes
tests/unit/Validator/StructureTest.php
Added $createdAt and $updatedAt fields with fixed timestamps to all document instances used in structure validation tests to reflect the new required attributes. No changes to test logic or assertions.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Database
    participant Validator
    participant Document

    Client->>Database: createDocument(collection, document)
    Database->>Document: Check if $createdAt, $updatedAt === null or preservation disabled
    alt $createdAt or $updatedAt is empty string
        Database->>Validator: Validate document (required dates)
        Validator-->>Database: throw StructureException
        Database-->>Client: throw StructureException
    else valid or null
        Database->>Document: Set or preserve $createdAt, $updatedAt
        Database->>Document: encode() converts empty string dates to null
        Database-->>Client: Return created document
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • abnegate

Poem

A rabbit hops through fields of time,
Ensuring dates are set just fine.
No empty strings in burrowed logs—
Only valid timestamps for all the docs!
With tests and checks, our warren’s neat,
Each document’s history now complete.
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dat-588

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/Database/Database.php (2)

3604-3611: Improve error messages for better clarity.

The error messages mention "Invalid type" but the actual issue is that empty strings are not allowed. Consider making the messages more descriptive:

-            throw new StructureException('Invalid type for the $createdAt');
+            throw new StructureException('Empty string is not allowed for $createdAt field');

-            throw new StructureException('Invalid type for the $updatedAt');
+            throw new StructureException('Empty string is not allowed for $updatedAt field');

3710-3716: Improve error messages and consider extracting validation logic.

Same issue with error messages as in the createDocument method. Additionally, consider extracting this validation into a private helper method to reduce code duplication:

-            if(is_string($createdAt) && empty($createdAt)){
-                throw new StructureException('Invalid type for the $createdAt');
-            }
-            
-            if(is_string($updatedAt) && empty($updatedAt)){
-                throw new StructureException('Invalid type for the $updatedAt');
-            }
+            $this->validateTimestampFields($createdAt, $updatedAt);

Then add a helper method:

private function validateTimestampFields($createdAt, $updatedAt): void
{
    if (is_string($createdAt) && empty($createdAt)) {
        throw new StructureException('Empty string is not allowed for $createdAt field');
    }
    
    if (is_string($updatedAt) && empty($updatedAt)) {
        throw new StructureException('Empty string is not allowed for $updatedAt field');
    }
}
tests/e2e/Adapter/Scopes/GeneralTests.php (1)

181-182: Consider uncommenting or removing the commented assertion lines.

The commented assertion lines suggest uncertainty about the exact error message format. Either uncomment them with the correct expected message or remove them entirely to keep the code clean.

Apply this diff to clean up the commented assertions:

-            // $this->assertEquals('Invalid document structure: Missing required attribute "$createdAt"', $e->getMessage());
-            // $this->assertEquals('Invalid document structure: Missing required attribute "$createdAt"', $e->getMessage());

Also applies to: 202-203

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 68545df and f8c2589.

📒 Files selected for processing (3)
  • src/Database/Database.php (2 hunks)
  • tests/e2e/Adapter/Scopes/DocumentTests.php (1 hunks)
  • tests/e2e/Adapter/Scopes/GeneralTests.php (2 hunks)
🔇 Additional comments (4)
tests/e2e/Adapter/Scopes/GeneralTests.php (4)

169-183: Well-structured validation test for empty string $createdAt.

The test correctly validates that empty strings for $createdAt throw a StructureException. The exception handling and assertion are properly implemented.


184-203: Comprehensive batch document creation validation.

Good test coverage for batch document creation with empty string $createdAt values. The test properly validates that the exception is thrown during batch operations.


227-233: Excellent addition of null value test case.

Adding a document with $createdAt set to null provides comprehensive test coverage for the auto-generation behavior when no explicit date is provided.


238-243: Robust assertions for null $createdAt handling.

The assertions correctly verify that:

  1. A $createdAt value is auto-generated when null is provided
  2. The auto-generated date is different from the explicitly set date

This ensures the date preservation logic works correctly for both explicit and auto-generated scenarios.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)

4654-4654: Empty string for VAR_DATETIME attribute violates validation rules

This line uses an empty string for the datetime attribute, which conflicts with the recent validation hardening that treats empty strings as invalid for datetime fields. This will cause a StructureException.

Apply this fix:

-            'datetime' => ''
+            'datetime' => null
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)

4735-4735: Remove unused variable

The variable $resultDocs is assigned but never used.

Apply this fix:

-        $resultDocs = $database->createDocuments($collection, $documents, batchSize: 2);
+        $database->createDocuments($collection, $documents, batchSize: 2);
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f8c2589 and b81f27a.

📒 Files selected for processing (4)
  • src/Database/Database.php (4 hunks)
  • src/Database/Validator/Structure.php (2 hunks)
  • tests/e2e/Adapter/Scopes/DocumentTests.php (1 hunks)
  • tests/e2e/Adapter/Scopes/GeneralTests.php (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/e2e/Adapter/Scopes/GeneralTests.php
  • src/Database/Database.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/DocumentTests.php

4735-4735: Avoid unused local variables such as '$resultDocs'. (Unused Code Rules)

(UnusedLocalVariable)

⏰ 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 (MySQL)
  • GitHub Check: Adapter Tests (SharedTables/MySQL)
  • GitHub Check: Adapter Tests (SQLite)
  • GitHub Check: Adapter Tests (SharedTables/Postgres)
  • GitHub Check: Adapter Tests (SharedTables/SQLite)
  • GitHub Check: Adapter Tests (Mirror)
  • GitHub Check: Adapter Tests (SharedTables/MariaDB)
  • GitHub Check: Adapter Tests (Postgres)
  • GitHub Check: Adapter Tests (Pool)
  • GitHub Check: Adapter Tests (MariaDB)
  • GitHub Check: Unit Test
🔇 Additional comments (4)
src/Database/Validator/Structure.php (2)

71-78: Database layer correctly auto-populates $createdAt and $updatedAt

The validator now requires both $createdAt and $updatedAt, and the database layer guarantees those attributes are always set:

  • In src/Database/Database.php
    ->setAttribute('$createdAt', ($createdAt === null || !$this->preserveDates) ? $time : $createdAt)
    ->setAttribute('$updatedAt', ($updatedAt === null || !$this->preserveDates) ? $time : $updatedAt)
  • E2E tests in tests/e2e/Adapter/Scopes/GeneralTests.php confirm:
    • Empty string for $createdAt/$updatedAt throws a StructureException
    • Non‐empty ISO dates are preserved when preserveDates is enabled
    • Null or missing dates auto‐populate with the current timestamp
  • Batch create/modify flows and query operations (ordering, filtering by these attributes) are covered across multiple adapter tests.

No further changes required.


79-87: Database layer auto-populates $updatedAt, no action needed

The Database class always sets $updatedAt during create and update flows—using the current timestamp or preserving user-supplied dates—so the validator’s required: true constraint will never fail.

tests/e2e/Adapter/Scopes/DocumentTests.php (2)

4665-4683: LGTM! Date preservation logic is well tested.

The test correctly verifies that when setPreserveDates(true) is enabled, explicitly provided $createdAt values are preserved while $updatedAt is still auto-generated with a different timestamp.


4737-4751: Excellent test coverage for bulk document date preservation

The test comprehensively validates different scenarios:

  • Documents with explicit $createdAt preserve the specified date
  • Documents with explicit $updatedAt preserve the specified date
  • Documents with null or omitted dates get auto-generated timestamps
  • Proper verification for each document ID scenario

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/Database/Validator/PartialStructure.php (1)

16-16: Improve parameter documentation.

The parameter documentation could be more descriptive about what the array contains and how it's used.

-     * @param array<string, string> $requiredAttributes optional list of required attributes to check
+     * @param array<string> $requiredAttributes optional list of attribute IDs to validate as required
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b81f27a and d63dcbd.

📒 Files selected for processing (4)
  • src/Database/Database.php (7 hunks)
  • src/Database/Validator/PartialStructure.php (2 hunks)
  • tests/e2e/Adapter/Scopes/DocumentTests.php (1 hunks)
  • tests/e2e/Adapter/Scopes/GeneralTests.php (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • tests/e2e/Adapter/Scopes/DocumentTests.php
  • src/Database/Database.php
  • tests/e2e/Adapter/Scopes/GeneralTests.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Database/Validator/PartialStructure.php (1)
src/Database/Validator/Structure.php (2)
  • isValid (206-240)
  • checkForAllRequiredValues (251-266)
🔇 Additional comments (2)
src/Database/Validator/PartialStructure.php (2)

20-20: Method signature enhancement looks good.

The addition of the optional $requiredAttributes parameter maintains backward compatibility while enabling selective required attribute validation.


47-49: Validation logic implementation is correct.

The call to checkForAllRequiredValues with the filtered required attributes map properly enforces selective validation while maintaining the existing validation flow.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)

4638-4685: Enhance test documentation and assertions for better clarity.

The test logic is sound, but could be improved for better maintainability and clarity:

  1. Document intentional behavior testing: Add comments explaining that the empty string test for datetime attribute is intentional to document current system behavior.

  2. Add missing assertion: Verify what happens to the empty datetime field to fully document the behavior.

  3. Consider test organization: This test covers multiple scenarios - consider splitting into focused test methods.

 public function testDateTimeDocument(): void
 {
     /**
      * @var Database $database
      */
     $database = static::getDatabase();
     $collection = 'create_modify_dates';
     $database->createCollection($collection);
     $this->assertEquals(true, $database->createAttribute($collection, 'string', Database::VAR_STRING, 128, false));
     $this->assertEquals(true, $database->createAttribute($collection, 'datetime', Database::VAR_DATETIME, 0, false, null, true, false, null, [], ['datetime']));

     $date = '2000-01-01T10:00:00.000+00:00';
-    // test - default behaviour of external datetime attribute not changed
+    // Test: Document current behavior when datetime attribute is set to empty string
+    // This is intentional to verify existing system behavior with empty datetime values
     $doc = $database->createDocument($collection, new Document([
         '$id' => 'doc1',
         '$permissions' => [Permission::read(Role::any()),Permission::write(Role::any()),Permission::update(Role::any())],
-        'datetime' => ''
+        'datetime' => '' // Intentionally testing empty string behavior
     ]));
     $this->assertNotEmpty($doc->getAttribute('datetime'));
+    $this->assertNotEmpty($doc->getAttribute('datetime'), 'Empty datetime should be populated with current timestamp');
     $this->assertNotEmpty($doc->getAttribute('$createdAt'));
     $this->assertNotEmpty($doc->getAttribute('$updatedAt'));

4687-4935: Consider refactoring for better maintainability while preserving excellent test coverage.

This is a comprehensive and well-structured test that thoroughly covers date operations scenarios. The test logic is sound and covers important edge cases.

However, the function is quite long (247 lines) which could impact maintainability:

Consider splitting into focused test methods:

  • testCreateDocumentWithCustomDates()
  • testUpdateDocumentWithCustomDates()
  • testUpsertDocumentWithCustomDates()
  • testDateOperationsWithPreserveDatesDisabled()

You could also extract common setup into helper methods:

private function createTestDocument(string $id, array $attributes = []): Document
{
    return new Document(array_merge([
        '$id' => $id,
        '$permissions' => [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())],
        'string' => 'test_value'
    ], $attributes));
}

This would make individual test failures easier to debug while maintaining the comprehensive coverage.


4937-5191: Excellent bulk operations test coverage - consider refactoring for maintainability.

This is a thorough and well-organized test that comprehensively covers bulk document operations with date handling. The test scenarios are well thought out and cover important edge cases.

Similar to the previous function, the main concern is the length (254 lines) which could impact maintainability:

Consider splitting into focused test methods:

  • testBulkCreateDocumentsWithCustomDates()
  • testBulkUpdateDocumentsWithCustomDates()
  • testBulkUpsertDocumentsWithCustomDates()
  • testBulkOperationsWithPreserveDatesDisabled()

Helper methods could reduce code duplication:

private function createBulkTestDocuments(array $configs): array
{
    $permissions = [Permission::read(Role::any()), Permission::write(Role::any()), Permission::update(Role::any())];
    return array_map(function ($config) use ($permissions) {
        return new Document(array_merge([
            '$permissions' => $permissions,
        ], $config));
    }, $configs);
}

private function assertDateValues(array $expectedDates, string $collection): void
{
    foreach ($expectedDates as $id => $dates) {
        $doc = $this->getDatabase()->getDocument($collection, $id);
        if (isset($dates['$createdAt'])) {
            $this->assertEquals($dates['$createdAt'], $doc->getAttribute('$createdAt'));
        }
        if (isset($dates['$updatedAt'])) {
            $this->assertEquals($dates['$updatedAt'], $doc->getAttribute('$updatedAt'));
        }
    }
}

This would make the tests more modular and easier to debug while maintaining the excellent coverage.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5164991 and 5b5e2c5.

📒 Files selected for processing (3)
  • src/Database/Adapter/SQL.php (1 hunks)
  • src/Database/Database.php (8 hunks)
  • tests/e2e/Adapter/Scopes/DocumentTests.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Database/Database.php
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.598Z
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.
src/Database/Adapter/SQL.php (1)

Learnt from: ArnabChatterjee20k
PR: #642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.598Z
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.

tests/e2e/Adapter/Scopes/DocumentTests.php (1)

Learnt from: ArnabChatterjee20k
PR: #642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.598Z
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.

🧬 Code Graph Analysis (1)
src/Database/Adapter/SQL.php (1)
src/Database/Document.php (1)
  • getCreatedAt (156-159)
⏰ 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). (8)
  • GitHub Check: Adapter Tests (SharedTables/Postgres)
  • GitHub Check: Adapter Tests (SharedTables/MySQL)
  • GitHub Check: Adapter Tests (SharedTables/MariaDB)
  • GitHub Check: Adapter Tests (Mirror)
  • GitHub Check: Adapter Tests (MySQL)
  • GitHub Check: Adapter Tests (Pool)
  • GitHub Check: Adapter Tests (MariaDB)
  • GitHub Check: Adapter Tests (Postgres)
🔇 Additional comments (1)
src/Database/Adapter/SQL.php (1)

425-427: LGTM! Consistent implementation following established patterns.

The addition of $createdAt attribute handling follows the same pattern as the existing $updatedAt logic. The use of !empty() is appropriate here to filter out both null values and empty strings, ensuring only valid timestamps are included in the SQL update.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)

4972-5236: Remove unused variables to improve code clarity.

The test provides excellent coverage of upsert operations with date preservation. However, there are unused variables that should be removed:

  • Line 4989: $date1 is defined but never used
  • Line 4990: $date2 is defined but never used

These appear to be duplicates of $createDate and $updateDate. Consider removing them or using them consistently throughout the test.

-        $createDate = '2000-01-01T10:00:00.000+00:00';
-        $updateDate = '2000-02-01T15:30:00.000+00:00';
-        $date1 = '2000-01-01T10:00:00.000+00:00';
-        $date2 = '2000-02-01T15:30:00.000+00:00';
-        $date3 = '2000-03-01T20:45:00.000+00:00';
+        $date1 = '2000-01-01T10:00:00.000+00:00';
+        $date2 = '2000-02-01T15:30:00.000+00:00';
+        $date3 = '2000-03-01T20:45:00.000+00:00';

Then update the test to use $date1 instead of $createDate and $date2 instead of $updateDate throughout.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb43ebd and f02b0e8.

📒 Files selected for processing (1)
  • tests/e2e/Adapter/Scopes/DocumentTests.php (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.598Z
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.
tests/e2e/Adapter/Scopes/DocumentTests.php (1)

Learnt from: ArnabChatterjee20k
PR: #642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.598Z
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.

🧬 Code Graph Analysis (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (8)
tests/e2e/Adapter/Base.php (1)
  • getDatabase (34-34)
src/Database/Database.php (12)
  • getDatabase (673-676)
  • createCollection (1206-1324)
  • createAttribute (1554-1605)
  • Database (36-6858)
  • createDocument (3572-3658)
  • getDocument (3187-3308)
  • setPreserveDates (984-989)
  • deleteCollection (1485-1528)
  • updateDocument (4097-4294)
  • createDocuments (3673-3760)
  • updateDocuments (4317-4480)
  • createOrUpdateDocuments (4885-4898)
src/Database/Adapter.php (11)
  • getDatabase (155-158)
  • createCollection (505-505)
  • createAttribute (537-537)
  • createDocument (680-680)
  • getDocument (670-670)
  • deleteCollection (514-514)
  • updateDocument (704-704)
  • createDocuments (692-692)
  • updateDocuments (719-719)
  • getSupportForUpserts (995-995)
  • createOrUpdateDocuments (731-735)
src/Database/Adapter/Pool.php (10)
  • createCollection (153-156)
  • createAttribute (168-171)
  • createDocument (228-231)
  • getDocument (223-226)
  • deleteCollection (158-161)
  • updateDocument (238-241)
  • createDocuments (233-236)
  • updateDocuments (243-246)
  • getSupportForUpserts (403-406)
  • createOrUpdateDocuments (248-251)
src/Database/Document.php (1)
  • Document (12-464)
src/Database/Helpers/Permission.php (4)
  • Permission (9-264)
  • read (186-195)
  • write (254-263)
  • update (220-229)
src/Database/Helpers/Role.php (2)
  • Role (5-178)
  • any (159-162)
src/Database/Query.php (2)
  • Query (8-730)
  • equal (355-358)
🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/DocumentTests.php

4989-4989: Avoid unused local variables such as '$date1'. (Unused Code Rules)

(UnusedLocalVariable)


4990-4990: Avoid unused local variables such as '$date2'. (Unused Code Rules)

(UnusedLocalVariable)

⏰ 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 (SharedTables/SQLite)
  • GitHub Check: Adapter Tests (SharedTables/MariaDB)
  • GitHub Check: Adapter Tests (Mirror)
  • GitHub Check: Adapter Tests (Postgres)
  • GitHub Check: Adapter Tests (SharedTables/Postgres)
  • GitHub Check: Adapter Tests (SharedTables/MySQL)
  • GitHub Check: Adapter Tests (MariaDB)
  • GitHub Check: Adapter Tests (SQLite)
  • GitHub Check: Adapter Tests (Pool)
  • GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (3)
tests/e2e/Adapter/Scopes/DocumentTests.php (3)

4638-4685: LGTM! Test covers datetime attribute behavior comprehensively.

The test properly validates:

  • Default behavior when empty string is passed to datetime attributes
  • Preservation of custom $createdAt values when preserveDates is enabled
  • Automatic population of $updatedAt when not provided

4687-4839: Excellent test coverage for single document date operations.

The test thoroughly validates all date preservation scenarios including:

  • Custom date preservation during create and update operations
  • Null value handling for date fields
  • Behavior differences when preserveDates is enabled vs disabled
  • Edge cases like updating only one date field

4841-4970: Comprehensive bulk operations test with date preservation.

The test effectively validates:

  • Bulk document creation with various date configurations
  • Bulk updates respecting custom date values
  • Proper behavior when toggling preserveDates flag
  • Null value handling in bulk operations

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)

4989-4990: Remove unused variables $date1 and $date2

These variables are defined but never used in the test method.

-        $date1 = '2000-01-01T10:00:00.000+00:00';
-        $date2 = '2000-02-01T15:30:00.000+00:00';
         $date3 = '2000-03-01T20:45:00.000+00:00';
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f02b0e8 and e15bf27.

📒 Files selected for processing (2)
  • src/Database/Database.php (8 hunks)
  • tests/e2e/Adapter/Scopes/DocumentTests.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Database/Database.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.598Z
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.
tests/e2e/Adapter/Scopes/DocumentTests.php (1)

Learnt from: ArnabChatterjee20k
PR: #642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.598Z
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.

🪛 PHPMD (2.15.0)
tests/e2e/Adapter/Scopes/DocumentTests.php

4989-4989: Avoid unused local variables such as '$date1'. (Unused Code Rules)

(UnusedLocalVariable)


4990-4990: Avoid unused local variables such as '$date2'. (Unused Code Rules)

(UnusedLocalVariable)

⏰ 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 (SharedTables/Postgres)
  • GitHub Check: Adapter Tests (SharedTables/MariaDB)
  • GitHub Check: Adapter Tests (SharedTables/MySQL)
  • GitHub Check: Adapter Tests (Postgres)
  • GitHub Check: Adapter Tests (Mirror)
  • GitHub Check: Adapter Tests (Pool)
  • GitHub Check: Adapter Tests (SharedTables/SQLite)
  • GitHub Check: Adapter Tests (SQLite)
  • GitHub Check: Adapter Tests (MariaDB)
  • GitHub Check: Adapter Tests (MySQL)

@abnegate abnegate merged commit 45cf27f into main Jul 31, 2025
15 checks passed
@abnegate abnegate deleted the dat-588 branch July 31, 2025 10:53
@coderabbitai coderabbitai bot mentioned this pull request Sep 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants