fix: address broken escaping of array items for JsonbArray and preserve integer values as strings if they are outside PHP integer range
#442
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning Rate limit exceeded@martin-georgiev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 22 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 (3)
WalkthroughUpdates JsonbArray type to quote/escape JSON elements for Postgres arrays and to handle empty array literals. Parsing now delegates to PostgresArrayToPHPArrayTransformer except for {}. Adds an integration test for JSONB[] mappings and adjusts a unit test’s expected serialized Postgres value. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant ORM as Doctrine DBAL
participant Type as JsonbArray Type
participant PG as PostgreSQL
rect rgb(235, 245, 255)
note over Type: Write path (PHP -> Postgres)
App->>ORM: persist entity with PHP array of JSON-able values
ORM->>Type: convertToDatabaseValue()
Type->>Type: transformArrayItemForPostgres()<br/>- JSON encode<br/>- escape \" and \\<br/>- quote each element
Type->>PG: send JSONB[] literal (e.g., {"\"{...}\"", ...})
end
rect rgb(245, 235, 255)
note over Type: Read path (Postgres -> PHP)
PG-->>ORM: returns JSONB[] text literal (e.g., {...} or {})
ORM->>Type: convertToPHPValue()
alt Empty array literal {}
Type->>ORM: return []
else Non-empty
Type->>Type: PostgresArrayToPHPArrayTransformer::transform()
Type->>ORM: return PHP array of decoded JSON values
end
ORM->>App: hydrated values
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (4)
src/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php (3)
26-31: Escaping is correct; simplify with strtr (minor)Functionally sound. You can trim an intermediate variable and use strtr for a tiny win.
- $json = $this->transformToPostgresJson($item); - $escaped = \str_replace(['\\', '"'], ['\\\\', '\\"'], $json); - - return '"'.$escaped.'"'; + return '"'.\strtr( + $this->transformToPostgresJson($item), + ['\\' => '\\\\', '"' => '\\"'] + ).'"';
35-41: Redundant '{}' check (optional cleanup)PostgresArrayToPHPArrayTransformer already returns [] for '{}' (and trims/validates). You can delegate unconditionally.
- $trimmed = \trim($postgresArray); - if ($trimmed === '{}') { - return []; - } - - return PostgresArrayToPHPArrayTransformer::transformPostgresArrayToPHPArray($postgresArray); + return PostgresArrayToPHPArrayTransformer::transformPostgresArrayToPHPArray($postgresArray);
46-49: Consider preserving big integers when decoding JSON items (consistency)Elsewhere (PostgresArrayToPHPArrayTransformer) big ints are preserved via JSON_BIGINT_AS_STRING; here JSON decode doesn’t use it, which can coerce large numbers. If you aim for consistency across paths, enable it in PostgresJsonToPHPArrayTransformer.
Outside this file (proposal):
--- a/src/MartinGeorgiev/Utils/PostgresJsonToPHPArrayTransformer.php +++ b/src/MartinGeorgiev/Utils/PostgresJsonToPHPArrayTransformer.php @@ public static function transformPostgresJsonEncodedValueToPHPArray(string $postgresValue): array - $transformedValue = \json_decode($postgresValue, true, 512, JSON_THROW_ON_ERROR); + $transformedValue = \json_decode($postgresValue, true, 512, JSON_THROW_ON_ERROR | JSON_BIGINT_AS_STRING); @@ public static function transformPostgresJsonEncodedValueToPHPValue(string $postgresValue): array|bool|float|int|string|null - return \json_decode($postgresValue, true, 512, JSON_THROW_ON_ERROR); + return \json_decode($postgresValue, true, 512, JSON_THROW_ON_ERROR | JSON_BIGINT_AS_STRING);tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTypeTest.php (1)
32-65: Add edge-case dataset for escaping (quotes, backslashes, braces, commas, newlines)To lock in the fix and prevent regressions around quoting, add a dataset with tricky characters.
return [ @@ 'jsonb array with mixed types' => ['jsonb array with mixed types', [ [ 'string' => 'value', 'number' => 42, 'boolean' => false, 'null' => null, 'array' => [1, 2, 3], 'object' => ['a' => 1], ], [ 'different' => 'structure', 'count' => 999, 'enabled' => true, ], ]], + 'escaping edge cases' => ['escaping edge cases', [ + [ + 'text' => 'quote " and backslash \\ and braces {} and comma ,', + 'path' => 'C:\\tmp\\file.txt', + 'multiline' => "line1\nline2", + 'json' => ['nested' => ['quote' => '"', 'backslash' => '\\']], + ], + [ + 'raw' => '{}', + 'nested' => ['brace' => '{}', 'comma' => ','], + ], + ]], ];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php(2 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTypeTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#421
File: docs/AVAILABLE-TYPES.md:31-33
Timestamp: 2025-08-19T13:07:15.184Z
Learning: In martin-georgiev/postgresql-for-doctrine, the point[] array type should be documented as "point[]" not "_point" in the AVAILABLE-TYPES.md table, to be consistent with all other array types like text[], jsonb[], inet[], etc.
📚 Learning: 2025-08-24T16:52:32.488Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#0
File: :0-0
Timestamp: 2025-08-24T16:52:32.488Z
Learning: This repository uses BaseType extension pattern for custom Doctrine DBAL types with PostgreSQL platform assertions, comprehensive unit and integration testing with data providers, and dedicated exception classes for type conversion errors.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTypeTest.phpsrc/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php
📚 Learning: 2025-08-19T13:07:15.184Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#421
File: docs/AVAILABLE-TYPES.md:31-33
Timestamp: 2025-08-19T13:07:15.184Z
Learning: In martin-georgiev/postgresql-for-doctrine, the AVAILABLE-TYPES.md documentation table's second column shows DBAL type names (what getName() method returns) not PostgreSQL internal catalogue names. Array types use the format like 'text[]', 'jsonb[]', 'inet[]' - not the underscore-prefixed PostgreSQL internal names like '_text', '_jsonb', '_inet'.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTypeTest.phpsrc/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php
📚 Learning: 2025-08-19T13:07:15.184Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#421
File: docs/AVAILABLE-TYPES.md:31-33
Timestamp: 2025-08-19T13:07:15.184Z
Learning: In martin-georgiev/postgresql-for-doctrine, the AVAILABLE-TYPES.md documentation table's second column shows DBAL type names (what TYPE_NAME constants contain and getName() method returns) not PostgreSQL internal catalogue names. Array types use the format like 'text[]', 'jsonb[]', 'inet[]' - not the underscore-prefixed PostgreSQL internal names like '_text', '_jsonb', '_inet'.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTypeTest.phpsrc/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php
📚 Learning: 2025-04-11T11:23:44.192Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#340
File: tests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php:145-145
Timestamp: 2025-04-11T11:23:44.192Z
Learning: In the PostgreSQL for Doctrine test cases, methods that test database-to-PHP conversions should use `mixed` type for parameter and include non-string test cases in their data providers, following the pattern in classes like InetTest, CidrTest, and MacaddrTest.
Applied to files:
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTypeTest.phptests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTest.php
📚 Learning: 2025-08-19T13:07:15.184Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#421
File: docs/AVAILABLE-TYPES.md:31-33
Timestamp: 2025-08-19T13:07:15.184Z
Learning: In martin-georgiev/postgresql-for-doctrine, the point[] array type should be documented as "point[]" not "_point" in the AVAILABLE-TYPES.md table, to be consistent with all other array types like text[], jsonb[], inet[], etc.
Applied to files:
src/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php
📚 Learning: 2025-03-11T17:02:51.971Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#0
File: :0-0
Timestamp: 2025-03-11T17:02:51.971Z
Learning: The PostgreSQL-for-doctrine library implements the JSON array element existence check using the `RIGHT_EXISTS_ON_LEFT` DQL function, which maps to PostgreSQL's `?` operator. This is used for checking if a specific value exists in a JSONB array column.
Applied to files:
src/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php
📚 Learning: 2025-03-25T01:23:29.749Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#304
File: src/MartinGeorgiev/Utils/DataStructure.php:152-168
Timestamp: 2025-03-25T01:23:29.749Z
Learning: In PostgreSQL text arrays, backslashes must be escaped with triple backslashes. A single backslash in PHP like 'path\to\file' becomes "path\\\to\\\file" in PostgreSQL format.
Applied to files:
src/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php
🧬 Code graph analysis (2)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTypeTest.php (2)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTest.php (5)
DataProvider(37-42)DataProvider(44-49)DataProvider(90-98)Test(31-35)provideValidTransformations(57-88)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/ArrayTypeTestCase.php (2)
ArrayTypeTestCase(9-40)can_handle_array_values(33-39)
src/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php (4)
src/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformer.php (2)
PostgresArrayToPHPArrayTransformer(16-250)transformPostgresArrayToPHPArray(29-90)src/MartinGeorgiev/Utils/PostgresJsonToPHPArrayTransformer.php (2)
PostgresJsonToPHPArrayTransformer(17-65)transformPostgresArrayToPHPArray(21-34)src/MartinGeorgiev/Doctrine/DBAL/Types/JsonTransformer.php (1)
transformToPostgresJson(24-33)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseArray.php (1)
transformPostgresArrayToPHPArray(127-135)
⏰ 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). (20)
- GitHub Check: sloth
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.1
- GitHub Check: wait-for-tests-worflows-before-upload
🔇 Additional comments (5)
src/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php (2)
7-7: Right import to fix parsing pathDelegating array shell parsing to PostgresArrayToPHPArrayTransformer is the correct move and resolves the brittle split logic.
13-13: Doc link is helpful and accurateThe arrays doc reference is appropriate here.
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTest.php (1)
85-85: Expected postgres literal now matches quoted/escaped element semanticsThis aligns with the new per-element quoting. Looks good.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArrayTypeTest.php (2)
17-20: Type name casing'JSONB[]' is fine (PostgreSQL is case-insensitive). If other array tests use uppercase, keep as-is for consistency.
22-27: Round-trip hook looks goodDelegating to parent test ensures the canonical round trip for values.
martin-georgiev
changed the title
JsonbArray parsingJsonbArray and preserve integer values as strings if they are outside PHP integer range
martin-georgiev
changed the title
JsonbArray and preserve integer values as strings if they are outside PHP integer rangeJsonbArray and preserve integer values as strings if they are outside PHP integer range
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests