fix(client): decode BSON int64 to native PHP integers - #49
Conversation
MongoDB\BSON\Document::toPHP() returns a MongoDB\BSON\Int64 instance for every 64-bit BSON integer, on all platforms, not just 32-bit ones. Any stored value outside the int32 range therefore came back to callers as an object where a PHP int was expected. The wrapper is not inert. Consumers that array-access a decoded value fatal with "Cannot use object of type MongoDB\BSON\Int64 as array", and consumers that apply an (int) cast get 1 plus a warning — silent corruption of the value on read. utopia-php/database hits both: a single out-of-int32-range element inside an array column makes every subsequent read of the table fail with a 500 (appwrite/appwrite#13175). Unwrap at the decode boundary so the whole client returns plain PHP values, rather than asking each consumer to recognise the wrapper. On 64-bit PHP the unwrap is lossless; on 32-bit builds the wrapper is the only lossless representation, so it is left alone there. $clusterTime is excluded: it is echoed back to the server verbatim on later commands, and its signature.keyId is an int64 that must not be re-encoded as an int32.
📝 WalkthroughWalkthroughThe client now converts BSON ChangesBSON Int64 normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟡 Moderate · up to The PR correctly targets BSON int64 decoding, but its new round-trip test uses large integer literals that may become floats on 32-bit PHP, leaving that supported behavior inadequately verified and potentially causing incorrect test results. Merge should wait for platform-independent fixtures or explicit acceptance of the 32-bit test limitation. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 SummaryThe client now recursively converts decoded BSON Int64 wrappers to native PHP integers on 64-bit platforms while preserving
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the prior 32-bit test incompatibility is resolved by skipping before out-of-range fixtures and native-integer assertions are reached. Important Files Changed
Reviews (2): Last reviewed commit: "test: skip the int64 round-trip on 32-bi..." | 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/MongoTest.php`:
- Around line 327-364: Update the int64 fixture setup in the MongoDB test around
the $negative, $positive, and $extreme values to construct BSON Int64 instances
from string literals, avoiding platform-dependent float conversion. Adjust the
assertions for result and toArray values to expect native integers with matching
values on 64-bit PHP, while expecting MongoDB\BSON\Int64 instances with matching
string representations on 32-bit PHP, including nested and list values.
🪄 Autofix
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 Plus
Run ID: c892cc42-6689-477c-aaf7-22f685015cfe
📒 Files selected for processing (2)
src/Client.phptests/MongoTest.php
The test asserts the 64-bit contract: native integers out of the client. On a 32-bit build normalizeInt64() keeps the wrapper on purpose, because there it is the only lossless representation, and the literals in the fixture would already have been coerced to float before reaching the driver. Declare that requirement instead of letting the assertions contradict the implementation.
|
@greptileai review Pushed 4875ed6 addressing the one finding from both reviewers: the int64 round-trip test now declares a 64-bit requirement instead of asserting native integers unconditionally, which contradicted Also worth a look:
|
Reading a row with an integer outside the int32 range returned a 500 on MongoDB. The driver decodes 64-bit BSON integers as MongoDB\BSON\Int64 wrappers, and array-accessing one fatals with "Cannot use object of type MongoDB\BSON\Int64 as array", so any table holding such a value became unlistable from the Console. 1.5.3 unwraps at the decode boundary (utopia-php/mongo#49). Fixes appwrite#13175
1 participant
Fixes the root cause of appwrite/appwrite#13175 — BigInt[] Breaks MongoDb.
The bug
MongoDB\BSON\Document::toPHP()returns aMongoDB\BSON\Int64instance for every 64-bit BSON integer, on all platforms — php.net documents this explicitly, and since PECL mongodb 1.16.0 it is no longer 32-bit-only behaviour. Verified on both ext-mongodb 2.1.1 and 2.3.3.Client::receive()passed that tree straight through, so any stored value outside the int32 range reached callers as an object where a PHPintwas expected.Two consequences downstream:
Cannot use object of type MongoDB\BSON\Int64 as array— the error in the linked issue. Store[-3408048000]in aninteger/bigintarray column and every subsequent read of the table 500s; the table is unusable from the Console until deleted.json_encode()on anInt64emits{"$numberLong":"-3408048000"}instead of a number, because the class implementsJsonSerializablein extended-JSON form. Any value that reaches a response without passing through an explicit cast is served to clients in the wrong shape.[-42]works because a small value is encoded as int32 and decodes to a plainint— which is why this arrived as a "big numbers break my table" report rather than a type bug.The fix
Unwrap at the decode boundary in
receive(), so the whole client returns plain PHP values. On 64-bit PHP the unwrap is lossless. On 32-bit builds the wrapper is the only lossless representation, so it is left in place there.Why here, and not in each consumer
The alternative is teaching every consumer to recognise
Int64. That leaves the same trap set for the next caller, and it does not help paths with no schema to consult — utopia-php/database normalises typedinteger/bigintattributes in itscastingAfter()step, but anything outside that (schemaless mode, and integers nested insideobjectcolumns) has no attribute list to drive a cast. The client's contract is "PHP values in, PHP values out";toArray()already normalises nestedstdClassfor the same reason. This puts int64 on the same footing.Cost is one recursive walk of a freshly-decoded (refcount-1) response, so array writes mutate in place rather than separating.
$clusterTimeis deliberately excluded$clusterTimeis stored verbatim byupdateCausalConsistency()and echoed back to the server on every later command. Itssignature.keyIdis an int64; unwrapping it would letDocument::fromPHP()re-encode a small value as an int32 and change the signed payload the server validates. Skipping that one top-level key keeps the wire representation byte-identical. This was the one non-obvious hazard in the change and is called out in a comment at the call site.Verification
This repo
MongoTest::testInt64ValuesDecodeToNativeIntegers— a real round-trip through Mongo covering a scalar int64, an int64 inside a list, a mixed int32/int64 list,PHP_INT_MAX, and a nested document. Seen red before the fix (Failed asserting that MongoDB\BSON\Int64 Object ('integer' => '-3408048000') is of type "int") and green after.TransactionTestpasses, which exercises the$clusterTimeround-trip.pint --test: PASS.phpstan: no errors.Downstream, against utopia-php/database
mainwith this client patched incastingAfter()does not cover: anobjectcolumn holding{"count": -3408048000}reads back asMongoDB\BSON\Int64and serialises to{"count":{"$numberLong":"-3408048000"}}. With this fix:int, and{"count":-3408048000}.MongoDBTeste2e suite: 661 tests, 4962 assertions, 2 failures. Both aretestObjectAttribute*EmptyObject*({}vs[]) and are unrelated to this change — they reproduce identically with an unmodified client, and are caused by my local image havingutopia-php/cache4.0.1 baked in where that repo's lock requires 4.0.2.Review notes
markTestSkippedrather than adding per-architecture expectations that this repo's CI can never execute. Reasoning is on the threads.(int)cast onInt64yields1with a warning. That is wrong for ext-mongodb 2.x, which supplies a numeric cast handler —(int)$int64returns the correct value. The real failure modes are the two listed above.Not verified
PHP_INT_SIZEbranch is reasoned, not exercised — CI has no 32-bit target, and the new test now skips there rather than pretending otherwise.signature.keyId. The$clusterTimeexclusion is verified only in that transactions still pass with real (large) keyIds; I did not synthesise a keyId small enough to re-encode as int32 and confirm the server would have rejected it. The exclusion is a type-preservation guarantee, not a fix for an observed failure.utopia-php/mongorelease, thencomposer updatein utopia-php/database and appwrite. No code change is needed in either — both take this via the existing1.*constraint.integercolumn created without explicitmin/maxgetssize = 8, i.e. a 64-bit column, so-3408048000is a legal value.Structuredoes range-validate array elements when a narrowermaxis set. Only the read path was broken.