Primary attributes #623
Primary attributes #623
Conversation
WalkthroughThese changes introduce a new attribute type, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
participant Validator
Client->>Database: createDocument(collection, document)
Database->>Adapter: getIdAttributeType()
Adapter-->>Database: VAR_ID_INT or VAR_ID_MONGO
Database->>Validator: new Structure(attributes, idAttributeType)
Validator->>Validator: Validate 'id' attribute with Sequence(idAttributeType)
Validator-->>Database: Validation result
Database-->>Client: Document created or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (12)
⏰ 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)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🔭 Outside diff range comments (2)
src/Database/Database.php (2)
1812-1812: Update error message to include new attribute typesThe error message should be updated to include the new
VAR_IDandVAR_SEQUENCEtypes in the list of valid attribute types.-throw new DatabaseException('Unknown attribute type: ' . $type . '. Must be one of ' . self::VAR_STRING . ', ' . self::VAR_INTEGER . ', ' . self::VAR_FLOAT . ', ' . self::VAR_BOOLEAN . ', ' . self::VAR_DATETIME . ', ' . self::VAR_RELATIONSHIP); +throw new DatabaseException('Unknown attribute type: ' . $type . '. Must be one of ' . self::VAR_STRING . ', ' . self::VAR_INTEGER . ', ' . self::VAR_FLOAT . ', ' . self::VAR_BOOLEAN . ', ' . self::VAR_DATETIME . ', ' . self::VAR_ID . ', ' . self::VAR_SEQUENCE . ', ' . self::VAR_RELATIONSHIP);
1867-1883: Add validation for new attribute types in validateDefaultTypesThe
validateDefaultTypesmethod should handle the newVAR_IDandVAR_SEQUENCEtypes. Currently, these types will fall through to the default case and throw an exception when setting default values.Consider adding cases for these new types or updating the error message to include them in the list of valid types.
🧹 Nitpick comments (2)
src/Database/Adapter/SQL.php (1)
1585-1591: Add documentation for the new method.The implementation correctly fulfills the abstract method contract from the parent
Adapterclass. However, consider adding a PHPDoc comment for consistency with other methods in the class.+ /** + * Get the primitive type used for ID attributes in this SQL adapter + * + * @return string + */ public function getIdAttributeType(): string { return 'int'; }src/Database/Validator/Structure.php (1)
53-53: Address the uncertainty comment for $tenant attribute type.The comment
// ?????suggests uncertainty about the $tenant attribute type. This should be clarified or removed before merging.Would you like me to help determine the correct type for the $tenant attribute or open an issue to track this?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
phpunit.xml(1 hunks)src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Postgres.php(1 hunks)src/Database/Adapter/SQL.php(2 hunks)src/Database/Database.php(8 hunks)src/Database/Validator/Queries/Documents.php(1 hunks)src/Database/Validator/Query/Filter.php(2 hunks)src/Database/Validator/Sequence.php(1 hunks)src/Database/Validator/Structure.php(4 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(4 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(3 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/Database/Adapter/Postgres.php (1)
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#613
File: src/Database/Adapter/Postgres.php:1254-1319
Timestamp: 2025-07-01T11:31:37.438Z
Learning: In PostgreSQL adapter methods like getUpsertStatement, complexity for database-specific SQL generation is acceptable when the main business logic is properly separated in the parent SQL adapter class, following the adapter pattern where each database adapter handles its own SQL syntax requirements.
🧬 Code Graph Analysis (4)
src/Database/Adapter.php (1)
src/Database/Adapter/SQL.php (1)
getIdAttributeType(1588-1591)
src/Database/Adapter/Postgres.php (1)
src/Database/Database.php (1)
Database(35-6800)
src/Database/Adapter/SQL.php (2)
src/Database/Database.php (1)
Database(35-6800)src/Database/Adapter.php (1)
getIdAttributeType(862-862)
src/Database/Database.php (2)
src/Database/Adapter/SQL.php (1)
getIdAttributeType(1588-1591)src/Database/Adapter.php (1)
getIdAttributeType(862-862)
🪛 GitHub Actions: Linter
src/Database/Validator/Sequence.php
[error] 1-1: PHP CS Fixer (Pint) PSR-12 style check failed: 'elseif' construct must have single space around it.
⏰ 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 (20)
phpunit.xml (1)
10-10: LGTM! Good practice for catching new feature issues early.The change to stop test execution on first failure is appropriate when introducing new features like VAR_ID and VAR_SEQUENCE types, ensuring immediate feedback on any issues.
src/Database/Validator/Queries/Documents.php (1)
41-41: Excellent semantic improvement.Changing the
$sequenceattribute type fromVAR_STRINGtoVAR_SEQUENCEprovides better type safety and aligns with the introduction of dedicated sequence handling in the system.src/Database/Adapter.php (1)
857-862: Well-designed abstraction for adapter-specific ID handling.The new
getIdAttributeType()abstract method provides a clean way for different database adapters to specify their ID attribute primitive type, enabling proper validation and type handling across the system.src/Database/Adapter/MariaDB.php (1)
1907-1908: Appropriate SQL type mapping for MariaDB.The
VAR_IDcase returning'bigint UNSIGNED'is well-suited for MariaDB ID columns, providing sufficient range while maintaining unsigned constraint for positive ID values.src/Database/Adapter/SQL.php (1)
1006-1009: LGTM: Correct attribute width calculation for new types.The addition of
VAR_SEQUENCEandVAR_IDattribute types with 8-byte width allocation is correct for BIGINT storage. The fall-through case structure is properly implemented with the shared logic and break statement.src/Database/Adapter/Postgres.php (1)
1820-1822: LGTM! Appropriate SQL type mapping for the new VAR_ID attribute type.The implementation correctly maps
Database::VAR_IDto'bigint', which is suitable for ID fields in PostgreSQL. This change aligns with the broader enhancement introducing explicit support for ID and sequence attribute types across the database system.tests/e2e/Adapter/Scopes/CollectionTests.php (2)
110-118: LGTM! Appropriate attribute definition for VAR_ID type.The new attribute definition correctly uses:
- Size 0 (appropriate for ID types)
- Not signed (false, correct for IDs)
- Not required (reasonable default)
- Not an array
158-158: LGTM! Assertions properly updated for the new attribute and index.The test correctly:
- Updates counts from 3 to 4 for both attributes and indexes
- Verifies the new attribute4 has the correct ID and VAR_ID type
- Verifies the new index4 has the correct ID and INDEX_KEY type
Also applies to: 165-166, 169-169, 176-177
tests/e2e/Adapter/Scopes/AttributeTests.php (1)
74-116: Test coverage for VAR_ID attribute looks comprehensive.The test properly covers creation, indexing, and deletion of the new VAR_ID attribute type. The assertion counts have been correctly adjusted to account for the additional attribute.
src/Database/Validator/Structure.php (1)
35-36: Sequence validation implementation looks correct.The changes properly integrate the new VAR_SEQUENCE and VAR_ID types with the Sequence validator, correctly using the injected idAttributeType parameter.
Also applies to: 319-326
tests/e2e/Adapter/Scopes/DocumentTests.php (6)
42-42: LGTM: VAR_ID attribute creation is correctly implemented.The test properly creates a VAR_ID attribute with appropriate parameters (size=0, required=false), which aligns with the expected behavior for ID-type attributes.
70-70: Excellent test coverage for VAR_ID attribute value handling.The test correctly sets an ID value as a string and validates both the data type (string) and the exact value. This provides good coverage for the VAR_ID functionality.
Also applies to: 94-95
150-150: Good edge case testing for unset ID attribute.This assertion correctly verifies that when creating a document with manual sequence but no explicit
idattribute, the attribute value is null. This tests the expected default behavior.
215-235: Comprehensive validation testing for invalid sequence values.The test correctly validates that setting
$sequenceto '0' triggers appropriate validation errors. The exception type and message verification ensure the VAR_SEQUENCE validation is working as expected.
237-268: Thorough testing of null ID values across operations.This test segment excellently covers:
- Document creation with null
idattribute- Retrieval verification after creation
- Database fetch verification
- Query operations using
isNullThe test ensures null values are properly handled throughout the document lifecycle.
269-302: Excellent edge case coverage for '0' string values.This test properly validates that the string '0' is treated as a valid ID value (not confused with falsy values). The comprehensive testing includes:
- Creation with '0' string value
- Type verification (string)
- Value verification
- Retrieval operations
- Query operations with equal condition
This is particularly important for edge case handling in ID validation.
src/Database/Database.php (4)
43-44: LGTM!The new attribute type constants follow the existing naming convention and are appropriately placed.
171-172: LGTM!The change to use
VAR_SEQUENCEtype with size 0 is appropriate for the$sequenceattribute as sequences don't require size constraints.
3652-3657: LGTM!The Structure validator instantiation correctly includes the adapter's ID attribute type, enabling adapter-specific ID validation. This pattern is consistently applied across all Structure validator instantiations in the file.
1792-1794: Skip validation for VAR_ID/VAR_SEQUENCE invalidateAttributeThe empty
case self::VAR_ID:and absence ofVAR_SEQUENCEare intentional—this method only enforces size/format rules on user‐defined types (string, integer, etc.). Bothidandsequenceattributes are managed by the adapter and other validators (e.g. inStructure.php) rather than here. No changes are needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/Validator/StructureTest.php (1)
655-724: Remove redundant test case for MongoDB ID validation.The
testId()method correctly tests ID validation for different database types, but contains a redundant test case for MongoDB validation.Apply this diff to remove the duplicate test:
- $this->assertEquals(true, $validator->isValid(new Document([ - '$collection' => ID::custom('posts'), - 'title' => 'My Title', - 'description' => null, - 'rating' => 5, - 'price' => 1.99, - 'published' => true, - 'tags' => ['dog', 'cat', 'mouse'], - 'feedback' => 'team@appwrite.io', - 'id' => $mongoid, - ])));The first MongoDB validation test (lines 701-711) already covers this scenario adequately.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Database/Adapter/SQL.php(2 hunks)src/Database/Database.php(13 hunks)src/Database/Validator/Sequence.php(1 hunks)tests/unit/Validator/DocumentsQueriesTest.php(4 hunks)tests/unit/Validator/IndexedQueriesTest.php(3 hunks)tests/unit/Validator/QueriesTest.php(1 hunks)tests/unit/Validator/Query/FilterTest.php(1 hunks)tests/unit/Validator/QueryTest.php(10 hunks)tests/unit/Validator/StructureTest.php(23 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/unit/Validator/QueriesTest.php
- tests/unit/Validator/DocumentsQueriesTest.php
- src/Database/Adapter/SQL.php
- tests/unit/Validator/QueryTest.php
- tests/unit/Validator/IndexedQueriesTest.php
- tests/unit/Validator/Query/FilterTest.php
- src/Database/Validator/Sequence.php
- src/Database/Database.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit/Validator/StructureTest.php (4)
src/Database/Database.php (1)
Database(35-6809)src/Database/Validator/Structure.php (2)
Structure(18-429)isValid(207-241)src/Database/Document.php (1)
Document(12-460)src/Database/Validator/Sequence.php (1)
isValid(39-68)
⏰ 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/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Unit Test
🔇 Additional comments (3)
tests/unit/Validator/StructureTest.php (3)
93-102: LGTM: ID attribute definition is properly configured.The new
idattribute definition correctly usesDatabase::VAR_IDtype and appropriate default values for a non-required ID field.
134-137: LGTM: Structure constructor calls updated consistently.All Structure validator instantiations have been systematically updated to include the
Database::VAR_ID_INTparameter, ensuring consistent behavior across all test methods.Also applies to: 149-152, 161-164, 182-185, 202-205, 233-236, 255-258, 276-279, 295-298, 316-319, 377-380, 396-399, 415-418, 449-452, 507-510, 541-544, 575-578, 596-599, 617-620, 638-641, 657-660
216-216: LGTM: ID field added to test document.The addition of
'id' => '1000'to the test document is appropriate for testing ID validation with the new attribute type.
…primary-attribute
…tribute # Conflicts: # src/Database/Adapter/Pool.php
…tribute # Conflicts: # tests/unit/Validator/StructureTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Postgres.php(1 hunks)src/Database/Adapter/SQL.php(2 hunks)src/Database/Database.php(15 hunks)src/Database/Validator/Structure.php(4 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(5 hunks)tests/unit/Validator/StructureTest.php(23 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Database/Adapter/SQL.php
- src/Database/Adapter/Postgres.php
- src/Database/Validator/Structure.php
- src/Database/Adapter/MariaDB.php
- src/Database/Database.php
- tests/e2e/Adapter/Scopes/DocumentTests.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in partialstructure validator, when filtering for required attributes validation using the $required...
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:
tests/unit/Validator/StructureTest.php
🧬 Code Graph Analysis (1)
tests/unit/Validator/StructureTest.php (3)
src/Database/Database.php (1)
Database(36-6880)src/Database/Validator/Structure.php (2)
Structure(18-425)isValid(207-241)tests/unit/DocumentTest.php (1)
testId(89-93)
⏰ 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 (3)
tests/unit/Validator/StructureTest.php (3)
93-102: LGTM! New VAR_ID attribute definition added correctly.The new
idattribute with typeDatabase::VAR_IDis properly configured for testing the new ID attribute type functionality. The attribute settings (non-required, unsigned, non-array) are appropriate for ID fields.
134-137: LGTM! Structure constructor calls updated correctly.All test methods have been consistently updated to include the new
Database::VAR_ID_INTparameter in the Structure constructor, reflecting the updated constructor signature that now requires an ID attribute type parameter.Also applies to: 149-152, 161-164, 184-187, 206-209, 241-244, 265-268, 288-291, 309-312, 332-335, 401-404, 422-425, 443-446, 481-484, 547-550, 585-588, 623-626, 646-649, 669-672, 692-695
221-223: LGTM! ID field test case added appropriately.The addition of the
idfield with value'1000'in the test document properly exercises the new VAR_ID attribute validation functionality.
Previous PR
#243
Summary by CodeRabbit
New Features
idattribute type with explicit validation for integer and string (MongoDB ObjectId) formats.idattribute type.idattribute type.idattribute type with appropriate SQL type mappings.Bug Fixes
Tests
idattribute type, including creation, querying, edge cases (null, zero), and validation behavior across different ID types.idattribute type.Documentation