Conversation
WalkthroughThe changes enforce a strict prohibition against creating indexes on array attributes in the database layer. The validator now immediately rejects such attempts, and all related tests have been updated to expect a unified exception message. Test cases were adjusted to handle and assert this new behavior, ensuring consistency across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Validator
participant Database
Tester->>Validator: Attempt to create index on array attribute
Validator-->>Tester: Throw IndexException ("Index on array attributes is forbidden")
Tester->>Tester: Catch and assert exception message
Poem
✨ Finishing Touches
🪧 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: 1
🔭 Outside diff range comments (1)
src/Database/Validator/Index.php (1)
140-161:⚠️ Potential issueRemove unreachable code after early return.
The code following the early return statement is unreachable and should be removed to fix the PHPStan error in the pipeline.
Apply this diff to remove the unreachable code:
if ($attribute->getAttribute('array', false)) { $this->message = 'Index on array attributes is forbidden'; return false; - - // Database::INDEX_UNIQUE Is not allowed! since mariaDB VS MySQL makes the unique Different on values - if ($index->getAttribute('type') != Database::INDEX_KEY) { - $this->message = '"' . ucfirst($index->getAttribute('type')) . '" index is forbidden on array attributes'; - return false; - } - - if (empty($lengths[$attributePosition])) { - $this->message = 'Index length for array not specified'; - return false; - } - - $arrayAttributes[] = $attribute->getAttribute('key', ''); - if (count($arrayAttributes) > 1) { - $this->message = 'An index may only contain one array attribute'; - return false; - } - - $direction = $orders[$attributePosition] ?? ''; - if (!empty($direction)) { - $this->message = 'Invalid index order "' . $direction . '" on array attribute "'. $attribute->getAttribute('key', '') .'"'; - return false; - } } elseif ($attribute->getAttribute('type') !== Database::VAR_STRING && !empty($lengths[$attributePosition])) {🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 141-141: PHPStan: Unreachable statement - code above always terminates.
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/IndexTests.php (1)
373-373: Remove debug statement from production test.The
var_dumpstatement appears to be leftover debug code and should be removed from the production test suite.Apply this diff to remove the debug statement:
- var_dump(static::getDatabase()->getCollection('documents')); -
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
src/Database/Validator/Index.php(1 hunks)tests/e2e/Adapter/Base.php(0 hunks)tests/e2e/Adapter/Scopes/AttributeTests.php(3 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(2 hunks)tests/e2e/Adapter/Scopes/IndexTests.php(4 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/Adapter/Base.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/e2e/Adapter/Scopes/IndexTests.php (4)
src/Database/Database.php (1)
Database(33-6732)src/Database/Exception.php (1)
Exception(7-21)src/Database/Validator/Index.php (1)
Index(10-312)tests/e2e/Adapter/Base.php (1)
getDatabase(33-33)
tests/e2e/Adapter/Scopes/AttributeTests.php (5)
src/Database/Database.php (2)
Database(33-6732)createIndex(3002-3119)src/Database/Exception.php (1)
Exception(7-21)src/Database/Validator/Index.php (1)
Index(10-312)src/Database/Adapter.php (1)
createIndex(649-649)src/Database/Adapter/Pool.php (1)
createIndex(213-216)
🪛 GitHub Actions: CodeQL
src/Database/Validator/Index.php
[error] 141-141: PHPStan: Unreachable statement - code above always terminates.
🪛 GitHub Actions: Linter
tests/e2e/Adapter/Scopes/IndexTests.php
[error] 1-1: PSR 12 linting failed: 'ordered_imports' rule violation.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (10)
src/Database/Validator/Index.php (1)
137-139: LGTM! Clean implementation of array index prohibition.The early return with the clear error message effectively enforces the new policy against indexing array attributes.
tests/e2e/Adapter/Scopes/IndexTests.php (2)
32-38: Excellent test update for array index prohibition.The try-catch block correctly verifies that
IndexExceptionis thrown with the expected message when attempting to create an index on an array attribute.
47-47: Good consistency improvement.Updating the duplicate attribute tests to expect
IndexExceptioninstead of the genericDatabaseExceptionprovides better test specificity and consistency with the new validation behavior.Also applies to: 54-54
tests/e2e/Adapter/Scopes/CollectionTests.php (2)
11-11: LGTM! Proper import addition.The
IndexExceptionimport is correctly placed in alphabetical order with other exception imports.
650-663: Excellent test coverage for collection creation with array indexes.The test properly validates that attempting to create a collection with schema indexes on array attributes throws an
IndexExceptionwith the correct message "Index on array attributes is forbidden". This ensures the prohibition is enforced at the collection creation level as well.tests/e2e/Adapter/Scopes/AttributeTests.php (5)
15-15: LGTM: Necessary import for IndexException testing.The import addition is required for the updated test cases that expect
IndexExceptionto be thrown when attempting to create indexes on array attributes.
1353-1359: LGTM: Correct test for array attribute index prohibition.The test correctly verifies that attempting to create an index on an array attribute (
cards) throws anIndexExceptionwith the expected message. This aligns with the PR objective to enforce strict prohibition against indexing array attributes.
1405-1405: LGTM: Updated exception message for fulltext index on array attributes.The assertion correctly expects the unified error message "Index on array attributes is forbidden" when attempting to create a fulltext index on an array attribute, consistent with the new validation behavior.
1407-1407: LGTM: Consistent error message for unsupported fulltext indexes.The assertion ensures that regardless of adapter fulltext support capabilities, the unified error message "Index on array attributes is forbidden" is used when attempting to create indexes on array attributes.
1415-1415: LGTM: Correct error message for multi-attribute index with array attributes.The assertion properly expects the unified error message when attempting to create an index containing multiple attributes where at least one is an array attribute, ensuring comprehensive coverage of the array indexing prohibition.
| use Utopia\Database\Exception\Duplicate as DuplicateException; | ||
| use Utopia\Database\Exception\Limit as LimitException; | ||
| use Utopia\Database\Exception\Query as QueryException; | ||
| use Utopia\Database\Exception\Index as IndexException; |
There was a problem hiding this comment.
Fix import ordering to resolve PSR 12 linting error.
The new IndexException import should be ordered alphabetically with other exception imports.
Apply this diff to fix the import ordering:
use Utopia\Database\Exception\Duplicate as DuplicateException;
+use Utopia\Database\Exception\Index as IndexException;
use Utopia\Database\Exception\Limit as LimitException;
use Utopia\Database\Exception\Query as QueryException;
-use Utopia\Database\Exception\Index as IndexException;🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/IndexTests.php at line 12, the import statement for
IndexException is not ordered alphabetically according to PSR 12 standards.
Reorder the use statements so that the import for
Utopia\Database\Exception\Index as IndexException is placed in the correct
alphabetical position among other exception imports to fix the linting error.
2 participants
Summary by CodeRabbit
Bug Fixes
Tests