Conversation
…BigInt method - Added abstract method getLimitForBigInt to Adapter class. - Implemented getLimitForBigInt in Mongo, Pool, and SQL classes. - Updated MariaDB and Postgres adapters to handle VAR_BIGINT type. - Enhanced error messages for unknown types to include VAR_BIGINT.
- Introduced VAR_BIGINT constant in Database class. - Updated Attribute class to include maxBigIntLength parameter and handle it in validation. - Ensured backwards compatibility for existing validator construction. - Enhanced validation logic to enforce size limits for VAR_BIGINT type.
- Introduced testCreateAttributesBigIntSizeLimit to verify exception handling for oversized BIGINT attributes. - Added testCreateDocumentWithBigIntType to ensure correct document creation and retrieval with BIGINT attributes. - Implemented testBigIntSizeTooLarge and testUnsignedBigIntSizeLimit to validate size constraints in the Attribute validator.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds VAR_BIGINT support across the database layer: new adapter accessor, SQL/MariaDB/Postgres and Mongo mappings, validator updates for bigint sizing and validation, Database constant and attribute wiring, Pool delegation, and new unit and e2e tests for bigint behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR adds Three issues remain that need attention before merge:
Confidence Score: 4/5Two P1 bugs — default value validation rejecting valid string bigints, and Mongo silently truncating large values on decode — should be fixed before merging. The core bigint plumbing (helper, validator, SQL adapters) is solid and the previous overflow concern in getLimitForBigInt() has been resolved. However, two P1 correctness bugs remain: (1) setting a bigint default > PHP_INT_MAX is impossible due to the overly-strict type check, and (2) MongoDB round-trips silently corrupt large bigint values via the (int) cast. src/Database/Adapter/Mongo.php (decode truncation) and src/Database/Validator/Attribute.php / src/Database/Database.php (default value type check)
|
| Filename | Overview |
|---|---|
| src/Database/Helpers/BigInt.php | New helper class with string-based arithmetic for safe 64-bit integer operations beyond PHP_INT_MAX; implementation looks correct. |
| src/Database/Validator/BigInt.php | New validator accepting both PHP int and numeric strings for signed/unsigned 64-bit ranges; correctly delegates to BigIntHelper. |
| src/Database/Validator/Attribute.php | Adds BIGINT attribute validation, but has two bugs: (1) duplicate VAR_BIGINT entry in supported-types error message, and (2) default value check rejects string bigints, blocking any default > PHP_INT_MAX. |
| src/Database/Adapter/Mongo.php | Maps VAR_BIGINT to MongoDB 'long' type correctly, but the decode path casts all BIGINT values with (int), silently truncating any value above PHP_INT_MAX. |
| src/Database/Validator/Query/Filter.php | Adds BigInt validator for filter queries; dead condition $attributeType === Database::VAR_BIGINT inside the VAR_INTEGER case is unreachable. |
| src/Database/Database.php | VAR_BIGINT wired through create/update/cast paths; default value type check at line 2633 has the same string-rejection bug as Attribute.php. |
| src/Database/Adapter/SQL.php | getLimitForBigInt() correctly returns Database::MAX_BIG_INT (PHP_INT_MAX); row-size accounting correctly adds 8 bytes for BIGINT columns. |
| src/Database/Adapter/MariaDB.php | Adds BIGINT SQL type mapping with optional UNSIGNED; enables getSupportForUnsignedBigInt(); looks correct. |
| src/Database/Adapter/Postgres.php | Maps VAR_BIGINT to BIGINT SQL type; Postgres doesn't support unsigned integers so unsigned flag is intentionally absent. |
| src/Database/Adapter/SQLite.php | Maps VAR_BIGINT to INTEGER for SQLite (appropriate since SQLite uses dynamic typing). |
| src/Database/Validator/Structure.php | Correctly normalises BIGINT string values within PHP int range and uses BigInt validator; looks solid. |
Reviews (3): Last reviewed commit: "updated" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/Attribute.php (1)
521-545:⚠️ Potential issue | 🟠 MajorMissing
VAR_BIGINTcase invalidateDefaultTypes()will cause default value validation to fail.The
validateDefaultTypes()method doesn't handleDatabase::VAR_BIGINT. When a bigint attribute has a default value set, it will fall through to thedefaultcase and throw an "Unknown attribute type" exception.Since bigint should support integer default values (same as
VAR_INTEGER), add a case for it.🐛 Proposed fix
case Database::VAR_INTEGER: + case Database::VAR_BIGINT: case Database::VAR_FLOAT: case Database::VAR_BOOLEAN: if ($type !== $defaultType) {Note: This assumes bigint default values should be validated as PHP integers. However, since bigint can exceed PHP's
PHP_INT_MAXon 32-bit systems, you may want to also accept string representations. Verify the intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Validator/Attribute.php` around lines 521 - 545, The switch in validateDefaultTypes() is missing Database::VAR_BIGINT so bigint defaults fall through to the default error; add Database::VAR_BIGINT to the same case group that contains Database::VAR_INTEGER (i.e., the branch that validates numeric/boolean defaults) so bigint default values are validated like integers; ensure the added case label appears alongside Database::VAR_INTEGER/VAR_FLOAT/VAR_BOOLEAN in Attribute::validateDefaultTypes().
🧹 Nitpick comments (2)
src/Database/Adapter/Postgres.php (1)
2001-2001: Keep the supported-types exception list centralized.The supported-type list in this exception is still hardcoded and easy to desync from switch cases. Consider deriving it from a shared constant/list to avoid future drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Postgres.php` at line 2001, The throw in Postgres (the DatabaseException with the long hardcoded VAR_* list) must use a single authoritative source of supported types instead of duplicating them; add or use an existing Database::SUPPORTED_TYPES (or Database::getSupportedTypes()) constant/static method that returns the array of supported VAR_* values, and change the exception construction in Postgres to build the message from that array (e.g., implode(', ', Database::SUPPORTED_TYPES)) so the list is derived from the shared constant; update the throw site that currently references DatabaseException and the VAR_* names to use that shared symbol.src/Database/Validator/Attribute.php (1)
431-451: Consider addingVAR_BIGINTto the supported types list for accurate error messages.When an unknown type is passed, the error message lists supported types but omits
VAR_BIGINT. While this won't cause functional issues (since BIGINT is handled above), adding it would make error messages more accurate.♻️ Suggested improvement
default: $supportedTypes = [ Database::VAR_STRING, Database::VAR_VARCHAR, Database::VAR_TEXT, Database::VAR_MEDIUMTEXT, Database::VAR_LONGTEXT, Database::VAR_INTEGER, + Database::VAR_BIGINT, Database::VAR_FLOAT, Database::VAR_BOOLEAN, Database::VAR_DATETIME, Database::VAR_RELATIONSHIP ];The same addition should be made in
validateDefaultTypes()around line 554.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Validator/Attribute.php` around lines 431 - 451, The supported-types array built in Attribute (the $supportedTypes list in the code path that checks supportForVectors/supportForSpatialAttributes/supportForObject) is missing Database::VAR_BIGINT, so update that array to include Database::VAR_BIGINT; also make the same addition in the validateDefaultTypes() method where a similar $supportedTypes list is constructed so error messages consistently include VAR_BIGINT. Ensure you reference Database::VAR_BIGINT when adding it to both $supportedTypes arrays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Adapter/MariaDB.php`:
- Around line 1728-1734: VAR_INTEGER was changed to map to INT causing
overflow/regression for callers expecting bigint behavior; revert or preserve
the legacy mapping by making Database::VAR_INTEGER return BIGINT semantics in
MariaDB.php (where the switch handles Database::VAR_INTEGER) or else migrate all
callers to use Database::VAR_BIGINT first. Update the case for
Database::VAR_INTEGER in the MariaDB Adapter so it produces the same BIGINT (and
UNSIGNED handling) as the Database::VAR_BIGINT case (or add a clear migration
TODO and failing assertion in callers like the tests referenced) to avoid
changing runtime column size semantics.
In `@src/Database/Adapter/Mongo.php`:
- Around line 3014-3018: The method getLimitForBigInt currently returns a
numeric literal that exceeds PHP_INT_MAX which breaks the declared return type;
update getLimitForBigInt to return the existing constant Database::MAX_BIG_INT
(or cast that constant to int if necessary) instead of the large literal
18446744073709551615 so the return value is a valid int and consistent with the
rest of the codebase.
- Line 2215: The BSON type mapping for BIGINT in the Database::VAR_BIGINT entry
is incorrect: change its value from 'int' to 'long' so that
partialFilterExpression and sparse/partial indexes use MongoDB 64-bit long type
instead of 32-bit int; locate the mapping array (the place that defines
Database::VAR_BIGINT => 'int') in the Mongo adapter (class/method that builds
type mappings or partialFilterExpression) and update that entry to 'long' so
BIGINT documents are correctly matched by $type checks.
In `@src/Database/Adapter/SQL.php`:
- Around line 904-909: The method getLimitForBigInt currently returns the
unsigned 64-bit literal 18446744073709551615 which exceeds PHP's signed 64-bit
int range and becomes a float; change the return to the signed 64-bit maximum
9223372036854775807 (as an int) so the return value matches the declared int
return type and the tests (DocumentTests.php) expectations; update the return
expression in getLimitForBigInt to use 9223372036854775807 (or cast to int if
needed) to ensure phpstan type checks pass.
In `@src/Database/Database.php`:
- Line 45: You added the new constant Database::VAR_BIGINT but the update and
default-validation code still treat 'bigint' as unknown; update the
type-handling in updateAttribute() and validateDefaultTypes() to recognize
self::VAR_BIGINT (or 'bigint') where other integer-like types are handled (e.g.,
the switch/case or type-to-validator map), and ensure the default/type coercion
paths treat bigint the same as other integer types so updates and default
validation accept bigint attributes.
---
Outside diff comments:
In `@src/Database/Validator/Attribute.php`:
- Around line 521-545: The switch in validateDefaultTypes() is missing
Database::VAR_BIGINT so bigint defaults fall through to the default error; add
Database::VAR_BIGINT to the same case group that contains Database::VAR_INTEGER
(i.e., the branch that validates numeric/boolean defaults) so bigint default
values are validated like integers; ensure the added case label appears
alongside Database::VAR_INTEGER/VAR_FLOAT/VAR_BOOLEAN in
Attribute::validateDefaultTypes().
---
Nitpick comments:
In `@src/Database/Adapter/Postgres.php`:
- Line 2001: The throw in Postgres (the DatabaseException with the long
hardcoded VAR_* list) must use a single authoritative source of supported types
instead of duplicating them; add or use an existing Database::SUPPORTED_TYPES
(or Database::getSupportedTypes()) constant/static method that returns the array
of supported VAR_* values, and change the exception construction in Postgres to
build the message from that array (e.g., implode(', ',
Database::SUPPORTED_TYPES)) so the list is derived from the shared constant;
update the throw site that currently references DatabaseException and the VAR_*
names to use that shared symbol.
In `@src/Database/Validator/Attribute.php`:
- Around line 431-451: The supported-types array built in Attribute (the
$supportedTypes list in the code path that checks
supportForVectors/supportForSpatialAttributes/supportForObject) is missing
Database::VAR_BIGINT, so update that array to include Database::VAR_BIGINT; also
make the same addition in the validateDefaultTypes() method where a similar
$supportedTypes list is constructed so error messages consistently include
VAR_BIGINT. Ensure you reference Database::VAR_BIGINT when adding it to both
$supportedTypes arrays.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87c67b39-b15b-4568-84ae-ed02167caa45
📒 Files selected for processing (11)
src/Database/Adapter.phpsrc/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Pool.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Database.phpsrc/Database/Validator/Attribute.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/e2e/Adapter/Scopes/DocumentTests.phptests/unit/Validator/AttributeTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Adapter/SQL.php`:
- Around line 899-910: Update the PHPDoc for the getLimitForBigInt method to
match its declared return type string: replace the incorrect "@return float|int"
with "@return string" in the PHPDoc block for the getLimitForBigInt() method so
static analysis (PHPStan) stops flagging the mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7545bd28-951f-49ea-b69e-8055e16fd3db
📒 Files selected for processing (6)
Dockerfilesrc/Database/Adapter.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Pool.phpsrc/Database/Adapter/SQL.phpsrc/Database/Validator/Attribute.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Database/Adapter/Pool.php
- src/Database/Validator/Attribute.php
- src/Database/Adapter/Mongo.php
…L adapters - Adjusted the getLimitForBigInt method in both Mongo and SQL classes to return 4294967295, reflecting the maximum value for a 32-bit unsigned integer. - Updated comments to clarify the handling of integer limits in PHP.
ArnabChatterjee20k
force-pushed
the
big-init
branch
from
8b93b40 to
bd14576
Compare
- Updated MariaDB adapter to return 'BIGINT' for sizes greater than or equal to 8 bytes. - Modified SQL adapter to correctly calculate total size for VAR_INTEGER based on the attribute size, distinguishing between INT and BIGINT.
src/Database/Adapter/SQL.php
Outdated
| // 2^64 - 1 | ||
| // 18446744073709551615 is the maximum value for a 64-bit unsigned integer | ||
| // 9223372036854775807 is the maximum value for a 64-bit signed integer | ||
| // in php we can't represent 64-bit integer, so greater than 4294967295 will be treated as bigint |
There was a problem hiding this comment.
@abnegate in php no option for representing a bigint in form of int so having 18446744073709551615 we need to consider string for precision but we can't store the size incoming from user inside the metadata as it will be string and we have to start taking input as string
Shall we go forward with string ? or BIGINT will be marking in the table only?
- Included VAR_BIGINT in the list of attribute types in the Database class. - Ensured consistency in handling BIGINT attributes across the database implementation.
- Added handling for VAR_BIGINT in the size limit validation logic. - Implemented exception throwing for oversized BIGINT attributes to ensure proper error handling.
- Updated validation logic to correctly determine bit size for VAR_BIGINT and VAR_INTEGER based on attribute size. - Ensured consistent handling of signed and unsigned integers across both validators.
- Updated Postgres adapter to return 'BIGINT' for sizes greater than or equal to 8 bytes in the type determination logic for VAR_INTEGER. - Ensured consistent handling of integer types across the database implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
2586-2593:⚠️ Potential issue | 🟠 Major
bigintdefaults will always fail this type check.PHP reports integer values as
integer, notbigint, soif ($type !== $defaultType)rejects every non-null bigint default. That breaks bigint defaults in attribute create/update flows.💡 Minimal fix
case self::VAR_INTEGER: case self::VAR_BIGINT: + if ($defaultType !== 'integer') { + throw new DatabaseException('Default value ' . $default . ' does not match given type ' . $type); + } + break; case self::VAR_FLOAT: case self::VAR_BOOLEAN: if ($type !== $defaultType) { throw new DatabaseException('Default value ' . $default . ' does not match given type ' . $type); } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 2586 - 2593, The type check in the block handling self::VAR_INTEGER, self::VAR_BIGINT, self::VAR_FLOAT, self::VAR_BOOLEAN incorrectly rejects bigint defaults because PHP reports them as "integer" (so $defaultType != $type); update the condition in the switch branch that throws DatabaseException to accept an integer-typed default when the target $type is self::VAR_BIGINT (i.e., treat $defaultType === 'integer' as valid for self::VAR_BIGINT) while preserving the existing strict check for other types; locate and modify the comparison around $type, $defaultType in the method where the switch uses constants VAR_INTEGER/VAR_BIGINT/VAR_FLOAT/VAR_BOOLEAN and DatabaseException is thrown.
♻️ Duplicate comments (1)
src/Database/Adapter/Mongo.php (1)
3014-3018:⚠️ Potential issue | 🟠 MajorBIGINT limit is still capped to INT-level range.
Line 3017 returns
4294967295, which matchesgetLimitForInt()and removes practical headroom for BIGINT in adapter-level size-limit checks.🔧 Proposed fix
public function getLimitForBigInt(): int { - // Mongo does not handle integers directly, so using MariaDB limit for now - return 4294967295; + return Database::MAX_BIG_INT; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Adapter/Mongo.php` around lines 3014 - 3018, getLimitForBigInt() currently returns 4294967295 (same as getLimitForInt()), removing HEADROOM for BIGINT checks; change getLimitForBigInt() to return the appropriate 64-bit BIGINT max (use 9223372036854775807 to fit the method's int return type on 64-bit builds) so adapter-level size-limit checks reflect BIGINT capacity rather than INT capacity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Adapter/SQL.php`:
- Around line 904-911: The getLimitForBigInt() currently returns 4294967295
(same as getLimitForInt()), so update it to return the correct BIGINT ceiling
(use 9223372036854775807 for signed 64-bit) and/or fall back to PHP_INT_MAX when
appropriate; modify getLimitForBigInt() to return 9223372036854775807 (or
PHP_INT_MAX on platforms where that better represents the native integer max) so
BIGINT checks differ from getLimitForInt().
In `@src/Database/Database.php`:
- Line 45: The atomic counter helpers increaseDocumentAttribute() and
decreaseDocumentAttribute() reject schema-backed bigint attributes because the
whitelist only permits VAR_INTEGER and VAR_FLOAT; update both functions to also
allow the class constant VAR_BIGINT (public const VAR_BIGINT) alongside
VAR_INTEGER and VAR_FLOAT so that schema attributes typed as bigint do not throw
the TypeException—locate the type-check/whitelist logic in
increaseDocumentAttribute() and decreaseDocumentAttribute() and add
self::VAR_BIGINT to the allowed types.
---
Outside diff comments:
In `@src/Database/Database.php`:
- Around line 2586-2593: The type check in the block handling self::VAR_INTEGER,
self::VAR_BIGINT, self::VAR_FLOAT, self::VAR_BOOLEAN incorrectly rejects bigint
defaults because PHP reports them as "integer" (so $defaultType != $type);
update the condition in the switch branch that throws DatabaseException to
accept an integer-typed default when the target $type is self::VAR_BIGINT (i.e.,
treat $defaultType === 'integer' as valid for self::VAR_BIGINT) while preserving
the existing strict check for other types; locate and modify the comparison
around $type, $defaultType in the method where the switch uses constants
VAR_INTEGER/VAR_BIGINT/VAR_FLOAT/VAR_BOOLEAN and DatabaseException is thrown.
---
Duplicate comments:
In `@src/Database/Adapter/Mongo.php`:
- Around line 3014-3018: getLimitForBigInt() currently returns 4294967295 (same
as getLimitForInt()), removing HEADROOM for BIGINT checks; change
getLimitForBigInt() to return the appropriate 64-bit BIGINT max (use
9223372036854775807 to fit the method's int return type on 64-bit builds) so
adapter-level size-limit checks reflect BIGINT capacity rather than INT
capacity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6553440-657b-4297-a48a-4c928fd82932
📒 Files selected for processing (7)
src/Database/Adapter/MariaDB.phpsrc/Database/Adapter/Mongo.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Database.phpsrc/Database/Validator/Query/Filter.phpsrc/Database/Validator/Structure.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Database/Adapter/Postgres.php
- src/Database/Adapter/MariaDB.php
| $attributes = [[ | ||
| '$id' => 'foo', | ||
| 'type' => Database::VAR_BIGINT, | ||
| 'size' => (int)$limit + 1, |
There was a problem hiding this comment.
I think we can ignore size altogether for big int, doesn't make much sense. We only need to validate it's a whole number
- Eliminated size limit checks for VAR_BIGINT in the Database and Attribute classes to allow larger values. - Updated Mongo and SQL adapters to return a constant for BIGINT limits, ensuring consistent handling across implementations. - Refactored tests to reflect the removal of size constraints and validate successful attribute creation without exceptions.
There was a problem hiding this comment.
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/Attribute.php (1)
527-534:⚠️ Potential issue | 🔴 CriticalMissing
VAR_BIGINTcase invalidateDefaultTypes()will reject valid bigint defaults.The
validateDefaultTypes()method does not handleDatabase::VAR_BIGINT. If a bigint attribute is created with a default value, validation will fall through to thedefaultcase (line 548) and throw "Unknown attribute type", even though the type is valid.🐛 Proposed fix
case Database::VAR_INTEGER: + case Database::VAR_BIGINT: case Database::VAR_FLOAT: case Database::VAR_BOOLEAN: if ($type !== $defaultType) {Also update the
$supportedTypesarray in the same method (lines 549-560):$supportedTypes = [ Database::VAR_STRING, Database::VAR_VARCHAR, Database::VAR_TEXT, Database::VAR_MEDIUMTEXT, Database::VAR_LONGTEXT, Database::VAR_INTEGER, + Database::VAR_BIGINT, Database::VAR_FLOAT, Database::VAR_BOOLEAN, Database::VAR_DATETIME, Database::VAR_RELATIONSHIP ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Validator/Attribute.php` around lines 527 - 534, validateDefaultTypes() currently omits Database::VAR_BIGINT so bigint defaults fall through and trigger the "Unknown attribute type" error; add a case for Database::VAR_BIGINT alongside Database::VAR_INTEGER/VAR_FLOAT/VAR_BOOLEAN in the switch block inside validateDefaultTypes() so bigint default type checks use the same logic, and also include Database::VAR_BIGINT in the $supportedTypes array used by validateDefaultTypes() to avoid rejecting valid bigint attributes (refer to the validateDefaultTypes() method and the $supportedTypes variable).
🧹 Nitpick comments (1)
src/Database/Validator/Attribute.php (1)
347-349: Consider addingVAR_BIGINTto$supportedTypesin the default case for error message completeness.The empty case correctly skips size validation per design. However,
VAR_BIGINTis missing from the$supportedTypesarray at lines 426-446. While the explicit case prevents reaching the default, adding it to$supportedTypesensures accurate error messages if future refactoring changes the switch structure.♻️ Suggested addition
$supportedTypes = [ Database::VAR_STRING, Database::VAR_VARCHAR, Database::VAR_TEXT, Database::VAR_MEDIUMTEXT, Database::VAR_LONGTEXT, Database::VAR_INTEGER, + Database::VAR_BIGINT, Database::VAR_FLOAT, Database::VAR_BOOLEAN, Database::VAR_DATETIME, Database::VAR_RELATIONSHIP ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Validator/Attribute.php` around lines 347 - 349, Add Database::VAR_BIGINT to the $supportedTypes array used in the default-case error message to keep the supported-types list accurate; locate the $supportedTypes definition in the Attribute validator (variable name $supportedTypes) and append Database::VAR_BIGINT so that error messages remain correct if the switch in validateSize (the case Database::VAR_BIGINT) is refactored later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Database/Validator/Attribute.php`:
- Around line 527-534: validateDefaultTypes() currently omits
Database::VAR_BIGINT so bigint defaults fall through and trigger the "Unknown
attribute type" error; add a case for Database::VAR_BIGINT alongside
Database::VAR_INTEGER/VAR_FLOAT/VAR_BOOLEAN in the switch block inside
validateDefaultTypes() so bigint default type checks use the same logic, and
also include Database::VAR_BIGINT in the $supportedTypes array used by
validateDefaultTypes() to avoid rejecting valid bigint attributes (refer to the
validateDefaultTypes() method and the $supportedTypes variable).
---
Nitpick comments:
In `@src/Database/Validator/Attribute.php`:
- Around line 347-349: Add Database::VAR_BIGINT to the $supportedTypes array
used in the default-case error message to keep the supported-types list
accurate; locate the $supportedTypes definition in the Attribute validator
(variable name $supportedTypes) and append Database::VAR_BIGINT so that error
messages remain correct if the switch in validateSize (the case
Database::VAR_BIGINT) is refactored later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 781931a2-1bf3-4b5b-a684-47ed567802ae
📒 Files selected for processing (6)
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/SQL.phpsrc/Database/Database.phpsrc/Database/Validator/Attribute.phptests/e2e/Adapter/Scopes/AttributeTests.phptests/unit/Validator/AttributeTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Database/Adapter/SQL.php
- tests/unit/Validator/AttributeTest.php
- src/Database/Adapter/Mongo.php
- src/Database/Database.php
- Included VAR_BIGINT in the whitelist for attribute types in the Database class. - Implemented a new test to validate the creation, updating, and atomic operations for BIGINT attributes in the DocumentTests. - Ensured that the new tests confirm correct behavior for incrementing and decrementing BIGINT values.
…tors - Introduced `getSupportForUnsignedBigInt` method in `Adapter`, `MariaDB`, and `SQLite` classes to indicate support for unsigned big integers. - Implemented normalization of attribute sizes for `bigint` types in the `Database` class. - Added `BigInt` helper class for managing big integer validation and formatting. - Updated `Attribute`, `Structure`, and `Filter` validators to handle signed and unsigned big integers correctly. - Enhanced tests to cover scenarios for creating and validating attributes with big integers, including edge cases for signed and unsigned values.
| return false; | ||
| } | ||
|
|
||
| public function getSupportForUnsignedBigInt(): bool |
There was a problem hiding this comment.
mongodb, postgresql, sqlite doesn't support unsigned bigint
| */ | ||
| public function getLimitForBigInt(): int | ||
| { | ||
| return Database::MAX_BIG_INT; |
There was a problem hiding this comment.
Here going other way around
It is the signed big int limit(supported by all adapter)
For adapters supporting unsigned int, in the attribute validator, we are validating via string comparison
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
| case Database::VAR_BIGINT: | ||
| if ($defaultType !== 'integer') { | ||
| $this->message = 'Default value ' . $default . ' does not match given type ' . $type; | ||
| throw new DatabaseException($this->message); | ||
| } |
There was a problem hiding this comment.
The check $defaultType !== 'integer' only accepts PHP-native int values, but any bigint default larger than PHP_INT_MAX (e.g. "9223372036854775808" for unsigned) must be passed as a string — which gettype() returns as 'string', not 'integer'. This means it's impossible to set a valid bigint default that exceeds PHP's integer range, even though the BigInt value validator (BigInt::isValid()) explicitly accepts strings.
The same issue exists in src/Database/Database.php at line 2633.
The check should accept both PHP integers and numeric strings:
| case Database::VAR_BIGINT: | |
| if ($defaultType !== 'integer') { | |
| $this->message = 'Default value ' . $default . ' does not match given type ' . $type; | |
| throw new DatabaseException($this->message); | |
| } | |
| case Database::VAR_BIGINT: | |
| if ($defaultType !== 'integer' && $defaultType !== 'string') { | |
| $this->message = 'Default value ' . $default . ' does not match given type ' . $type; | |
| throw new DatabaseException($this->message); | |
| } | |
| break; |
There was a problem hiding this comment.
Its already present(https://github.com/utopia-php/database/pull/847/changes#diff-6b7714b772da21b04d82d9ec7e003ce1155773a838e8ddf53f96f51415c5235bR538-R540), not sure why its still showing
… int and string types - Updated `createAttribute`, `updateAttribute`, and `getColumnType` methods across various database adapters to accept `int|string` for the size parameter. - Introduced a new private method `getSize` in the `Database` class to normalize size handling based on attribute type. - Enhanced tests to ensure proper functionality with the updated size handling.
- Updated the validation logic in the `Attribute` class to allow for string types as valid default values for `bigint` attributes, improving type flexibility and error handling.
…ase adapters - Updated `createAttribute`, `updateAttribute`, and `getColumnType` methods in various database adapters to require `int` for the size parameter, removing support for `string`. - Removed the `getSize` method from the `Database` class as it is no longer needed. - Adjusted related tests to validate the new integer-only size handling.
src/Database/Helpers/BigInt.php
Outdated
| class BigInt | ||
| { | ||
| public const SIGNED_MIN = '-9223372036854775808'; | ||
| public const SIGNED_MAX = '9223372036854775807'; | ||
| public const UNSIGNED_MAX = '18446744073709551615'; | ||
|
|
||
| public static function isIntegerString(string $value, bool $signed = true): bool | ||
| { | ||
| return \preg_match($signed ? '/^-?\d+$/' : '/^\d+$/', $value) === 1; | ||
| } | ||
|
|
||
| public static function fitsPhpInt(string $value, bool $signed = true): bool | ||
| { | ||
| if (!self::isIntegerString($value, $signed)) { | ||
| return false; | ||
| } | ||
|
|
||
| $phpMax = (string)\PHP_INT_MAX; |
There was a problem hiding this comment.
Let's move all of this class into the validator
| case Database::VAR_INTEGER: | ||
| $size = $attributeSchema['size'] ?? 4; | ||
| $signed = $attributeSchema['signed'] ?? true; | ||
| $bits = $size >= 8 ? 64 : 32; | ||
| $bits = ($attributeType === Database::VAR_BIGINT || $size >= 8) ? 64 : 32; |
There was a problem hiding this comment.
The check is redundant, type is always integer here
| case Database::VAR_BIGINT: | ||
| if ($defaultType !== 'integer') { | ||
| $this->message = 'Default value ' . $default . ' does not match given type ' . $type; | ||
| throw new DatabaseException($this->message); | ||
| } |
| $bits = $size >= 8 ? 64 : 32; | ||
| // BIGINT is always 64-bit in SQL adapters; VAR_INTEGER uses size to decide. | ||
| $bits = ($type === Database::VAR_BIGINT || $size >= 8) ? 64 : 32; | ||
| // For 64-bit unsigned, use signed since PHP doesn't support true 64-bit unsigned |
There was a problem hiding this comment.
Same here, type can't be bigint if we're in the int case
There was a problem hiding this comment.
yep, was a leftover from the revert , updating
src/Database/Database.php
Outdated
| } | ||
| break; | ||
| case Database::VAR_BIGINT: | ||
| if ($defaultType !== 'integer') { |
| $this->adapter->getMaxDateTime(), | ||
| $this->adapter->getSupportForAttributes() | ||
| $this->adapter->getSupportForAttributes(), | ||
| null, |
There was a problem hiding this comment.
Do we need to pass the adapter values here
There was a problem hiding this comment.
yes cause mongodb, postgres, sqlite doesn't support unsigned bigint
So for the structure validator we needed this
passing null here as current document by default is null
2 participants
Summary by CodeRabbit