Passthru max UID #743
Passthru max UID #743
Conversation
WalkthroughPropagates adapter-derived max UID length into document query validators and constructors; introduces public Database::MAX_UID_DEFAULT_LENGTH = 36; updates UID/Key/Label/Cursor defaults to use the constant and refactors Key to use PHP 8 promoted readonly constructor properties. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DB as Database
participant Adapter as Adapter
participant DV as DocumentsValidator
participant CV as CursorValidator
participant UV as UIDValidator
DB->>Adapter: getMaxUIDLength()
Adapter-->>DB: maxUIDLength
DB->>DV: new DocumentsValidator(..., maxUIDLength)
DV->>CV: new Cursor(maxUIDLength)
CV->>UV: new UID(maxLength)
CV->>UV: validate(cursorId)
note right of UV #DFF2E1: UID validation uses maxLength (36 by default)
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 (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Validator/UID.php (1)
24-24: Fix the property access violation flagged by PHPStan.The
$maxLengthproperty in the parentKeyclass is nowprivate readonly, which prevents access from child classes. Line 24 attempts to access$this->maxLength, causing the PHPStan error reported in the pipeline.Solution: Change the property visibility in
Key.phpfromprivatetoprotected:In
src/Database/Validator/Key.php, apply this diff:public function __construct( private readonly bool $allowInternal = false, - private readonly int $maxLength = 36, + protected readonly int $maxLength = 36, ) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Database.php(5 hunks)src/Database/Validator/Key.php(3 hunks)src/Database/Validator/Queries/Documents.php(2 hunks)src/Database/Validator/Query/Cursor.php(2 hunks)src/Database/Validator/UID.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Database/Validator/UID.php (3)
src/Database/Validator/Key.php (1)
__construct(26-31)src/Database/Validator/Queries/Documents.php (1)
__construct(27-79)src/Database/Validator/Query/Cursor.php (1)
__construct(11-13)
src/Database/Validator/Queries/Documents.php (4)
src/Database/Validator/Query/Cursor.php (2)
__construct(11-13)Cursor(9-55)src/Database/Validator/UID.php (1)
__construct(10-13)src/Database/Validator/Index.php (1)
__construct(33-54)src/Database/Validator/Query/Filter.php (1)
__construct(28-39)
src/Database/Database.php (4)
src/Database/Adapter/SQL.php (1)
getMaxUIDLength(1888-1891)src/Database/Adapter.php (1)
getMaxUIDLength(883-883)src/Database/Adapter/Pool.php (1)
getMaxUIDLength(313-316)src/Database/Adapter/Mongo.php (1)
getMaxUIDLength(3100-3103)
src/Database/Validator/Query/Cursor.php (3)
src/Database/Validator/Key.php (1)
__construct(26-31)src/Database/Validator/Queries/Documents.php (1)
__construct(27-79)src/Database/Validator/UID.php (2)
__construct(10-13)UID(5-26)
src/Database/Validator/Key.php (4)
src/Database/Validator/Queries/Documents.php (1)
__construct(27-79)src/Database/Validator/Query/Cursor.php (1)
__construct(11-13)src/Database/Validator/UID.php (1)
__construct(10-13)src/Database/Validator/Index.php (1)
__construct(33-54)
🪛 GitHub Actions: CodeQL
src/Database/Validator/UID.php
[error] 24-24: PHPStan: Access to private property $maxLength of parent class Utopia\Database\Validator\Key.
⏰ 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 (11)
src/Database/Validator/Query/Cursor.php (2)
11-13: LGTM! Clean use of promoted constructor properties.The constructor properly uses PHP 8.0+ promoted properties with readonly modifier, ensuring immutability and reducing boilerplate. The default maxLength of 36 aligns with the other validators in this PR.
40-40: LGTM! Properly propagates maxLength to UID validator.The change correctly passes the configured
maxLengthto the UID validator, enabling dynamic UID length constraints as intended by this PR.src/Database/Validator/UID.php (1)
10-10: No breaking change impact detected in codebase.The only UID instantiation (Cursor.php:40) explicitly passes
$this->maxLength, so the default parameter change has no actual impact on existing code. The Cursor validator and parent Key class both default to 36, indicating the change is already aligned with the codebase design. Database and client code concerns cannot be verified within the repository.Likely an incorrect or invalid review comment.
src/Database/Database.php (5)
5149-5158: Propagating adapter max UID length in bulk updates is spot on.Thanks for threading the adapter-specific max UID limit through to
DocumentsValidator; it lines up with the updated signature and keeps update validation consistent.
6708-6717: Max UID length now reachesdeleteDocumentsas expected.Good call ensuring bulk deletes share the same adapter-driven UID limit, keeping validation in sync with the validator’s new contract.
6911-6919:findpipelines the new max UID limit correctly.This keeps read-query validation aligned with adapter capabilities and the revised validator expectations.
7141-7149: Count validation now respects the adapter’s UID length.Passing the adapter-derived limit here matches the validator update and avoids divergent constraints between read paths.
7204-7212: Sum queries also benefit from the adapter-aware UID cap.Nice to see the validator parameter updated here too—ensures aggregation runs under the same guardrails.
src/Database/Validator/Queries/Documents.php (3)
21-25: Documentation looks good.The docblock properly documents the new
$maxUIDLengthparameter, and the exception type is more specific than before.
32-32: Parameter addition is well-designed.The
$maxUIDLengthparameter with a default of 36 maintains backward compatibility while enabling configurable max UID length validation. The default aligns with UUID format.
65-65: Correct propagation of max UID length.The Cursor validator instantiation properly passes the configured
$maxUIDLength, ensuring consistent UID validation across the query pipeline.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/Validator/PermissionsTest.php (1)
265-269: Consider using 37 characters for the boundary test.While the current test with 256 characters is functionally correct, using 37 characters (just one over the limit) would make the boundary test clearer and more targeted.
Apply this diff to improve the boundary test:
- $this->assertFalse($object->isValid([Permission::read(Role::user(ID::custom(str_repeat('a', 256))))])); + $this->assertFalse($object->isValid([Permission::read(Role::user(ID::custom(str_repeat('a', 37))))]));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/Validator/PermissionsTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/Validator/PermissionsTest.php (1)
src/Database/Validator/Label.php (1)
isValid(26-38)
⏰ 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 (Pool)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
🔇 Additional comments (3)
tests/unit/Validator/PermissionsTest.php (3)
251-263: LGTM! Error messages consistently updated.The error messages have been correctly updated to reflect the new 36-character limit for role identifiers across user, team, and member roles.
281-293: LGTM! Team permission error messages correctly updated.The error messages for team identifier and dimension validation have been properly updated to reflect the 36-character limit.
265-269: This review comment is incorrect and should be disregarded.The breaking change (255 → 36 characters) already exists in the codebase. This PR does not introduce that change; it only updates test assertions to match the current implementation. The constant
MAX_UID_DEFAULT_LENGTH = 36is already set insrc/Database/Database.php, and this PR makes no modifications to it.The test changes simply correct outdated assertions that were still referencing the old 255-character limit. Migration guidance and breaking change documentation, if needed, would apply to whichever earlier commit actually introduced the
MAX_UID_DEFAULT_LENGTH = 36change—not this one.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/Validator/KeyTest.php(1 hunks)tests/unit/Validator/LabelTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/Validator/LabelTest.php (1)
src/Database/Validator/Label.php (1)
isValid(26-38)
⏰ 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 (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
Summary by CodeRabbit
Bug Fixes
Chores
Tests