Primary attribute integer representation #624
Conversation
|
Warning Rate limit exceeded@fogelito has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughThis change introduces a new ID attribute type Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
participant Validator
Client->>Database: createDocument(data)
Database->>Adapter: getIdAttributeType()
Adapter-->>Database: 'int' or 'string'
Database->>Validator: Structure(attributes, indexes, idType)
Validator->>Validator: Validate attributes (including VAR_ID)
Validator-->>Database: Validation result
Database->>Adapter: Insert document
Adapter-->>Database: Document with typed sequence/id
Database-->>Client: Created Document
Possibly related PRs
Poem
✨ Finishing Touches
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. 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)
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: 3
🧹 Nitpick comments (4)
phpunit.xml (1)
10-10: Re-evaluatestopOnFailure="true"for CI runsHalting on the first failure speeds up local troubleshooting, but in CI it can mask additional regressions and shrink the failure signal. Consider:
• Keeping
stopOnFailure="false"in the committed config and passing--stop-on-failurelocally/through a CI job matrix, or
• Supplying two configs (e.g.,phpunit.ci.xmlvsphpunit.dev.xml) and selecting via-cflag.This preserves fast feedback for developers while still surfacing the full failure set in automated pipelines.
src/Database/Validator/Structure.php (1)
53-53: Remove the unclear comment.The comment
// ?????appears to be a leftover and should be removed for code clarity.- 'type' => Database::VAR_INTEGER, // ????? + 'type' => Database::VAR_INTEGER,src/Database/Database.php (2)
170-172: Verify if VAR_SEQUENCE type needs validationThe
$sequenceattribute type was changed fromVAR_STRINGtoVAR_SEQUENCE, but there's no validation case forVAR_SEQUENCEin thevalidateAttributemethod. Should this new type have validation logic?Would you like me to check if
VAR_SEQUENCEtype requires validation logic similar to other attribute types?
43-44: Consider adding comprehensive validation for new attribute typesWith the introduction of
VAR_IDandVAR_SEQUENCEtypes, ensure that:
- Both types have appropriate validation logic in
validateAttribute()- Default value validation in
validateDefaultTypes()handles these new types- Documentation is updated to explain when to use each type
Also applies to: 1792-1813
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
phpunit.xml(1 hunks)src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(3 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/Postgres.php(1 hunks)src/Database/Adapter/SQL.php(3 hunks)src/Database/Database.php(14 hunks)src/Database/Validator/Queries/Documents.php(3 hunks)src/Database/Validator/Query/Filter.php(3 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)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)
🧰 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 (7)
src/Database/Adapter.php (2)
src/Database/Adapter/SQL.php (1)
getIdAttributeType(1588-1591)src/Database/Adapter/Pool.php (1)
getIdAttributeType(493-496)
tests/unit/Validator/IndexedQueriesTest.php (1)
src/Database/Validator/Query/Filter.php (1)
Filter(15-299)
tests/unit/Validator/QueryTest.php (1)
src/Database/Validator/Queries/Documents.php (1)
Documents(16-76)
src/Database/Adapter/SQL.php (3)
src/Database/Database.php (1)
Database(35-6807)src/Database/Adapter/Pool.php (1)
getIdAttributeType(493-496)src/Database/Adapter.php (1)
getIdAttributeType(862-862)
src/Database/Adapter/MariaDB.php (1)
src/Database/Database.php (1)
Database(35-6807)
src/Database/Validator/Structure.php (2)
src/Database/Database.php (1)
Database(35-6807)src/Database/Validator/Sequence.php (1)
Sequence(10-74)
tests/unit/Validator/QueriesTest.php (1)
src/Database/Validator/Query/Filter.php (1)
Filter(15-299)
⏰ 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 (49)
src/Database/Adapter/Pool.php (1)
493-496: LGTM! Clean delegation implementation.The new
getIdAttributeType()method follows the established delegation pattern used throughout the Pool class. The implementation correctly forwards the method call to the underlying adapter instance, maintaining consistency with the proxy design.src/Database/Adapter.php (1)
857-862: LGTM! Well-documented abstract method.The new
getIdAttributeType()abstract method is properly documented and follows the established pattern for abstract methods in the Adapter class. The string return type is appropriate for type identifiers, and the method name clearly conveys its purpose.tests/unit/Validator/IndexedQueriesTest.php (3)
83-83: LGTM! Correct constructor parameter addition.The Filter validator constructor now requires an ID attribute type parameter. Adding
'int'as the second argument is correct and consistent with the new constructor signature.
146-146: LGTM! Consistent with other test method updates.The Filter constructor call correctly includes the required ID attribute type parameter, maintaining consistency with the other test methods in this file.
199-199: LGTM! Properly updated constructor call.The Filter validator instantiation correctly includes the required
'int'parameter for the ID attribute type, aligning with the updated constructor signature.tests/unit/Validator/QueriesTest.php (1)
66-66: LGTM! Consistent test update.The Filter validator constructor call correctly includes the required ID attribute type parameter
'int', matching the updated constructor signature and maintaining consistency with similar changes in other test files.src/Database/Adapter/Postgres.php (1)
1820-1821: LGTM! Correct PostgreSQL type mapping for VAR_ID.The mapping of
Database::VAR_IDto PostgreSQL'sbiginttype is appropriate and follows the established pattern in the switch statement. This provides sufficient range for ID values while being consistent with PostgreSQL's type system (which doesn't support unsigned integers).src/Database/Adapter/SQL.php (3)
1006-1009: LGTM: Correct attribute width allocation for new ID types.The addition of
VAR_SEQUENCEandVAR_IDcases with 8-byte allocation is appropriate for BIGINT storage in SQL databases. This aligns with the new attribute types introduced in the codebase.
372-372: LGTM: Proper type casting for sequence value.The explicit casting of
$sequenceto(int)ensures consistency with the integer-based ID attribute type returned bygetIdAttributeType(). This maintains type safety when mapping from the database_idfield.
1585-1591: LGTM: Implementation of required abstract method.The
getIdAttributeType()method correctly implements the abstract method from the parentAdapterclass and returns'int'as expected for SQL adapters. This is consistent with the integer-based ID handling in SQL databases.tests/unit/Validator/DocumentsQueriesTest.php (4)
79-89: LGTM: Comprehensive test coverage for new ID attribute type.The addition of the 'id' attribute document with type
Database::VAR_IDproperly extends the test collection schema to include the new attribute type. The configuration is consistent with other attribute definitions in the test setup.
126-130: LGTM: Updated validator instantiation for ID attribute type support.The addition of the third parameter
'int'to theDocumentsvalidator constructor is required for the new ID attribute type handling. This aligns with the updated constructor signature in theDocumentsvalidator class.
133-133: LGTM: Added test coverage for ID attribute queries.The new test query
Query::notEqual('id', '1000000')validates that the ID attribute can be properly queried, ensuring the newVAR_IDattribute type is fully integrated into the query validation system.
164-168: LGTM: Consistent validator instantiation in invalid queries test.The update maintains consistency with the new
Documentsvalidator constructor signature across all test methods.tests/unit/Validator/QueryTest.php (2)
111-111: LGTM: Consistent update to validator instantiation.The addition of the third parameter
'int'to theDocumentsvalidator constructor is correctly applied. This change is consistent with the new constructor signature that requires the ID attribute type parameter.
141-141: LGTM: Comprehensive update across all test methods.All instances of
Documentsvalidator instantiation have been consistently updated to include the'int'parameter. This ensures all tests remain functional with the new constructor signature and validates the integer-based ID attribute type handling throughout the query validation system.Also applies to: 157-157, 169-169, 180-180, 194-194, 208-208, 228-228, 258-258, 287-287
tests/e2e/Adapter/Scopes/CollectionTests.php (3)
110-118: LGTM - VAR_ID attribute definition follows correct pattern.The new attribute definition for
Database::VAR_IDtype is well-structured and follows the existing pattern. The configuration withsigned: falseis appropriate for ID fields, which are typically unsigned.
143-149: LGTM - Index definition correctly targets the VAR_ID attribute.The index definition properly references the new VAR_ID attribute and follows the standard index configuration pattern used in the test.
158-158: LGTM - Assertions correctly updated for the new attribute and index.The assertion counts are properly updated from 3 to 4, and the new assertions verify that:
attribute4has the correct ID andDatabase::VAR_IDtypeindex4has the correct ID andDatabase::INDEX_KEYtypeThe test coverage is comprehensive and consistent with the existing test structure.
Also applies to: 165-166, 169-169, 176-177
src/Database/Adapter/MariaDB.php (3)
907-914: LGTM! Proper type casting for sequence values.The integer cast ensures consistent type handling for sequence values returned by PDO.
1644-1647: LGTM! Consistent integer casting for document IDs.The cast ensures document sequence values are integers when retrieving documents from the database.
1909-1911: LGTM! Appropriate SQL type for ID attributes.Using
bigint UNSIGNEDprovides a suitable range for auto-incrementing ID values.tests/unit/Validator/Query/FilterTest.php (1)
21-48: LGTM! Test updated correctly for new Filter constructor.The change properly adapts to the new Filter constructor signature that requires the ID attribute type parameter.
src/Database/Validator/Structure.php (2)
34-40: LGTM! Correct type for sequence attribute.The change from
VAR_STRINGtoVAR_SEQUENCEproperly reflects the semantic type of the sequence attribute.
319-326: LGTM! Proper validation for new attribute types.The new cases correctly handle
VAR_SEQUENCEandVAR_IDtypes using the Sequence validator with appropriate primary flags.src/Database/Validator/Queries/Documents.php (1)
23-75: LGTM! Consistent implementation of ID attribute type support.The changes properly integrate the ID attribute type parameter and update the sequence attribute type to
VAR_SEQUENCE, maintaining consistency with the broader changes.tests/e2e/Adapter/Scopes/AttributeTests.php (4)
74-74: LGTM: Adding VAR_ID attribute type test coverage.This addition tests the new
VAR_IDattribute type, which is consistent with the PR objective of introducing primary attribute integer representation.
76-76: LGTM: Testing index creation on VAR_ID attribute.The addition of an index on the
idattribute provides comprehensive test coverage for the new attribute type's indexing capabilities.
82-82: LGTM: Attribute counts correctly updated.The attribute counts are properly incremented to reflect the addition of the
idattribute (from 8 to 9, 12 to 13, 17 to 18).Also applies to: 92-92, 102-102
113-113: LGTM: Proper cleanup of test attribute.The deletion of the
idattribute follows the established test pattern and ensures proper cleanup.tests/unit/Validator/StructureTest.php (4)
93-102: LGTM: Adding VAR_ID attribute to test collection.The addition of the
idattribute with typeDatabase::VAR_IDis properly structured and follows the existing attribute pattern in the test collection.
134-137: LGTM: Consistent addition of ID attribute type parameter.The second parameter
'int'(and'string'in the new test) is consistently added to all Structure validator constructor calls, which aligns with the new validator signature.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, 696-699
655-724: LGTM: Comprehensive testing of both ID attribute types.The new
testId()method provides excellent coverage for both integer and string ID types, validating the behavior differences between SQL and MongoDB-style IDs.
216-216: LGTM: Adding ID field to test document.The addition of the
idfield in the test document provides appropriate test coverage for the new attribute type.src/Database/Validator/Sequence.php (4)
10-27: LGTM: Well-structured validator class.The class design is clean with appropriate private properties and a clear constructor signature. The
getDescription()method provides a consistent error message.
29-41: LGTM: Proper type information methods.The
isArray()andgetType()methods correctly indicate that sequences are not arrays and return the appropriate type based on the ID attribute type.
49-50: LGTM: Proper MongoDB ObjectId validation.The regex pattern
/^[a-f0-9]{24}$/icorrectly validates 24-character hexadecimal strings, which is the standard format for MongoDB ObjectIds.
53-67: LGTM: Comprehensive integer validation.The integer validation logic is well-structured:
- Type check ensures the value is an integer
- Uses the Integer validator for additional validation
- Applies appropriate range validation with different start values for primary vs non-primary sequences
src/Database/Validator/Query/Filter.php (3)
9-9: LGTM: Import added for new validation logic.The import of
Sequencevalidator is correctly added to support the newVAR_SEQUENCEandVAR_IDattribute validation cases.
111-118: LGTM: New validation cases correctly implemented.The validation logic for
VAR_SEQUENCEandVAR_IDattribute types is correctly implemented:
- Both use the
Sequencevalidator with the injected$idAttributeType- The boolean parameter appropriately differentiates between sequence (true) and ID (false) types
- Follows the same pattern as other validation cases
This aligns with the broader database schema changes to support integer representation of primary attributes.
29-30: All Filter constructor calls include the new parametersVerified that every
new Filter(instantiation now supplies both the required$attributesand the new$idAttributeTypeargument. No usages were found missing the updated signature in:
- src/Database/Validator/Queries/Documents.php
- tests/unit/Validator/QueriesTest.php
- tests/unit/Validator/IndexedQueriesTest.php
- tests/unit/Validator/Query/FilterTest.php
No further changes are required.
src/Database/Database.php (1)
3654-3657: Consistent implementation of configurable ID attribute typeThe addition of
$this->adapter->getIdAttributeType()parameter to all validator instantiations is implemented consistently throughout the file. This enables adapters to configure whether ID fields should be integers or strings.Also applies to: 3743-3746, 4274-4277
tests/e2e/Adapter/Scopes/DocumentTests.php (7)
42-42: LGTM: VAR_ID attribute creation testThe test correctly creates a VAR_ID attribute, which aligns with the new functionality for integer ID representation.
70-70: LGTM: Integer value assignment for VAR_IDThe test correctly assigns an integer value (1000000) to the VAR_ID attribute, testing the core functionality.
94-95: LGTM: VAR_ID validation assertionsThe assertions correctly verify that the VAR_ID attribute maintains its integer type and value after document creation.
150-150: LGTM: Default null value testThe test correctly verifies that when no value is provided for the VAR_ID attribute, it defaults to null.
215-235: LGTM: Sequence validation testThe test appropriately validates that invalid sequence values (like '0') are rejected with proper error handling.
237-268: LGTM: Comprehensive null value testingThe test block thoroughly validates VAR_ID behavior with explicit null values:
- Document creation with null ID
- Document retrieval maintains null value
- Query functionality works with null values using
Query::isNull()This provides good coverage for the null value edge case.
269-302: LGTM: Zero value edge case testingThe test block comprehensively covers the zero value edge case for VAR_ID:
- Document creation with zero ID value
- Verifies the value is stored as integer 0 (not null)
- Tests document retrieval and querying with zero values
This is important for distinguishing between null and zero values in integer ID fields.
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 (4)
src/Database/Adapter/Postgres.php(3 hunks)src/Database/Database.php(14 hunks)src/Database/Document.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/Adapter/Scopes/DocumentTests.php
- src/Database/Adapter/Postgres.php
- src/Database/Database.php
⏰ 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 (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Unit Test
🔇 Additional comments (1)
src/Database/Document.php (1)
69-69: EnsuregetSequence()Null Safety After Removing Default
Without a default ingetAttribute('$sequence'),getSequence()can now returnnull. Since sequence values are used for:
- String concatenation in src/Database/Database.php (e.g. collection/junction naming)
- Conditional checks and parameter binding in src/Database/Adapter/{SQLite,SQL,Postgres,MariaDB}.php
- Test assertions in tests/e2e/Adapter/Scopes (e.g.
assertNotEmpty,assertNotNull)Please verify that every call site either:
- Guarantees a non-null sequence before invoking
getSequence()- Handles a
nullreturn value safely- Or update
getSequence()to provide a fallback (for example:return $this->getAttribute('$sequence') ?? '';)
src/Database/Document.php
Outdated
| * @return mixed | ||
| */ | ||
| public function getSequence(): string | ||
| public function getSequence(): mixed |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider backward compatibility implications of the return type change.
The return type change from string to mixed could break existing code that expects a string value. This change allows the method to return integers, strings, or null, which aligns with the new VAR_SEQUENCE attribute type but may require updates to consuming code.
Consider documenting the possible return types and their meanings in the method's PHPDoc comment.
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of getSequence() method to assess backward compatibility impact
# Expected: Identify code that might assume string return type
echo "Searching for getSequence() method calls..."
rg -A 3 -B 1 "getSequence\(\)" --type php
echo -e "\nSearching for type-specific operations on sequence values..."
rg -A 2 -B 2 "->getSequence\(\)->" --type php
rg -A 2 -B 2 "strlen.*getSequence" --type php
rg -A 2 -B 2 "substr.*getSequence" --type phpLength of output: 14544
Refine getSequence() return type annotation and document nullability
The change from string to mixed is safe—no direct string-only operations were found (only concatenation and PDO binding, which handle int|string|null via PHP’s implicit casts). However, the method’s PHPDoc and signature can be more precise:
- File: src/Database/Document.php (around the
getSequence()declaration)- Update the
@returntag frommixedtoint|string|null - Document what each type means (e.g. integer for auto-increment adapters, string for UUIDs, null when unset)
- Update the
Suggested diff:
/**
- * @return mixed
+ * @return int|string|null Sequence value:
+ * - int: auto-increment sequences
+ * - string: UUID or custom identifiers
+ * - null: when no sequence attribute is set
*/
- public function getSequence(): mixed
+ public function getSequence(): int|string|null🤖 Prompt for AI Agents
In src/Database/Document.php around lines 65 to 67, refine the getSequence()
method's PHPDoc and return type by changing the @return annotation from mixed to
int|string|null, and update the method signature to reflect this nullable union
type. Add documentation explaining that int represents auto-increment adapters,
string represents UUIDs, and null indicates the sequence is unset.
…to primary-attribute-asint # Conflicts: # src/Database/Validator/Sequence.php
…primary-attribute-asint
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores