Add Cast for JSON #265
Add Cast for JSON #265
Conversation
WalkthroughThe pull request introduces a new "JSON Type" section in the README to document handling JSON columns, including a code snippet that demonstrates casting JSON attributes using the Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Casts/SpannerJson.php (1)
22-28: Add class and method documentation.Consider adding PHPDoc blocks to explain the purpose of the class and method.
+/** + * Custom JSON type for Google Cloud Spanner. + * Extends PgJsonb to provide JSON support with unspecified type annotation. + */ class SpannerJsonType extends PgJsonb { + /** + * Returns the type annotation code for JSON values. + * + * @return int + */ public function typeAnnotation() { return TypeAnnotationCode::TYPE_ANNOTATION_CODE_UNSPECIFIED; } }README.md (1)
237-247: Enhance JSON Type documentation.Consider adding more context about JSON type support:
- Purpose and benefits of using JSON columns
- Any limitations or performance considerations
- Example of reading/writing JSON data
### JSON Type -In order to use JSON columns, use the provided Cast on your model as below +Google Cloud Spanner supports storing and querying JSON data. To work with JSON columns in your Laravel models, use the provided `SpannerJson` cast as shown below: ```php use Colopl\Spanner\Casts\SpannerJson; protected $casts = [ 'json_column_name' => SpannerJson::class, ]
+#### Example Usage
+
+php +// Writing JSON data +$model->json_column_name = ['key' => 'value']; + +// Reading JSON data +$data = $model->json_column_name; // Returns array +
+
+#### Limitations and Considerations
+
+- JSON columns are stored as strings in the database
+- Consider indexing specific JSON fields if you need to query them frequently
+- Large JSON objects may impact performance<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [style] ~238-~238: Consider a shorter alternative to avoid wordiness. Context: ...have these conversions. ### JSON Type In order to use JSON columns, use the provided Cast... (IN_ORDER_TO_PREMIUM) </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 237-237: Heading style Expected: setext; Actual: atx (MD003, heading-style) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 03dbf66b58566989e1eb77c1546dc615eeb32e1d and f671a3db1e2610b7ec454878daaabdc128d8369b. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `README.md` (1 hunks) * `src/Casts/SpannerJson.php` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>README.md</summary> [style] ~238-~238: Consider a shorter alternative to avoid wordiness. Context: ...have these conversions. ### JSON Type In order to use JSON columns, use the provided Cast... (IN_ORDER_TO_PREMIUM) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>README.md</summary> 237-237: Heading style Expected: setext; Actual: atx (MD003, heading-style) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (1)</summary> * GitHub Check: Run-PHPUnit </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
src/Casts/SpannerJson.php
Outdated
| public function get($model, $key, $value, $attributes) | ||
| { | ||
| return json_decode((string) $value, true); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON decoding.
The get method should handle potential JSON decoding errors and null values explicitly.
public function get($model, $key, $value, $attributes)
{
- return json_decode((string) $value, true);
+ if ($value === null) {
+ return null;
+ }
+ $decoded = json_decode((string) $value, true);
+ if (json_last_error() !== JSON_ERROR_NONE) {
+ throw new \JsonException(json_last_error_msg());
+ }
+ return $decoded;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function get($model, $key, $value, $attributes) | |
| { | |
| return json_decode((string) $value, true); | |
| } | |
| public function get($model, $key, $value, $attributes) | |
| { | |
| if ($value === null) { | |
| return null; | |
| } | |
| $decoded = json_decode((string) $value, true); | |
| if (json_last_error() !== JSON_ERROR_NONE) { | |
| throw new \JsonException(json_last_error_msg()); | |
| } | |
| return $decoded; | |
| } |
src/Casts/SpannerJson.php
Outdated
| public function set($model, $key, $value, $attributes) | ||
| { | ||
| return [$key => new SpannerJsonType($value)]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for JSON values.
The set method should validate the input value to ensure it can be encoded as JSON.
public function set($model, $key, $value, $attributes)
{
+ if ($value !== null && !is_array($value) && !is_object($value)) {
+ throw new \InvalidArgumentException('The given value must be array, object or null.');
+ }
return [$key => new SpannerJsonType($value)];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function set($model, $key, $value, $attributes) | |
| { | |
| return [$key => new SpannerJsonType($value)]; | |
| } | |
| public function set($model, $key, $value, $attributes) | |
| { | |
| if ($value !== null && !is_array($value) && !is_object($value)) { | |
| throw new \InvalidArgumentException('The given value must be array, object or null.'); | |
| } | |
| return [$key => new SpannerJsonType($value)]; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Casts/SpannerJson.php (1)
27-27: 🛠️ Refactor suggestionAdd error handling for JSON decoding.
The
getmethod should handle potential JSON decoding errors explicitly.- return json_decode($value, true); + $decoded = json_decode($value, true); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \JsonException(json_last_error_msg()); + } + return $decoded;
🧹 Nitpick comments (3)
src/Casts/SpannerJson.php (3)
19-21: Inconsistent coding style in conditional statements.There's inconsistent spacing in the
ifconditions. Laravel's coding standards recommend a space afterifkeyword.- if(is_array($value)) { + if (is_array($value)) { return $value; } - if(!is_string($value)) { + if (!is_string($value)) { throw new \InvalidArgumentException('The given value must be an array, string or null.'); }Also applies to: 23-25
13-28: Consider adding validation for non-UTF-8 characters in the JSON string.JSON strings must be UTF-8 encoded, and non-UTF-8 characters can cause decoding errors. Consider adding validation for this case.
public function get($model, $key, $value, $attributes): array|null { if ($value === null) { return null; } if (is_array($value)) { return $value; } if (!is_string($value)) { throw new \InvalidArgumentException('The given value must be an array, string or null.'); } + // Ensure the string is valid UTF-8 + if (!mb_check_encoding($value, 'UTF-8')) { + throw new \InvalidArgumentException('The given JSON string contains invalid UTF-8 characters.'); + } + $decoded = json_decode($value, true); if (json_last_error() !== JSON_ERROR_NONE) { throw new \JsonException(json_last_error_msg()); } return $decoded; }
30-37: Consider validating JSON serializability in the set method.For JsonSerializable objects or arrays, it would be good to validate that they can actually be encoded to JSON without errors.
public function set($model, $key, $value, $attributes): array { if (!is_array($value) && !$value instanceof JsonSerializable && $value !== null && !is_string($value)) { throw new \InvalidArgumentException('The given value must be an array, JsonSerializable, string or null.'); } + // Validate that the value can be encoded to JSON + if ($value !== null && !is_string($value)) { + try { + json_encode($value); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \JsonException(json_last_error_msg()); + } + } catch (\Throwable $e) { + throw new \InvalidArgumentException('The given value cannot be encoded to JSON: ' . $e->getMessage()); + } + } + return [$key => new SpannerJsonType($value)]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Casts/SpannerJson.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
| throw new \InvalidArgumentException('The given value must be an array, JsonSerializable, string or null.'); | ||
| } | ||
|
|
||
| return [$key => new SpannerJsonType($value)]; |
There was a problem hiding this comment.
❓ Verification inconclusive
Missing import for SpannerJsonType class.
The SpannerJsonType class is used but not imported. This will cause a runtime error.
+use Colopl\Spanner\Query\Grammar\SpannerJsonType;
namespace Colopl\Spanner\Casts;Please verify the correct namespace and path for the SpannerJsonType class in your codebase.
🏁 Script executed:
#!/bin/bash
# Search for the SpannerJsonType class in the codebase
echo "Searching for SpannerJsonType class..."
rg -l "class SpannerJsonType" --type phpLength of output: 129
Verify Missing Import for SpannerJsonType
The code instantiates a new SpannerJsonType($value) on line 36 of src/Casts/SpannerJson.php without an import. We attempted to verify its existence using a search for a class declaration but did not find a matching definition. Please manually verify whether the SpannerJsonType class exists in the codebase:
- Confirm if the class is defined (possibly with a different name or in a different namespace).
- If present, ascertain its correct namespace and update the import statement accordingly (the suggested
use Colopl\Spanner\Query\Grammar\SpannerJsonType;is unconfirmed). - If missing entirely, consider adding the required class definition or adjusting the logic to prevent runtime errors.
Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Casts/SpannerJson.php (1)
30-30: 🛠️ Refactor suggestionAdd error handling for JSON decoding.
The
json_decodefunction doesn't throw exceptions on invalid JSON, so errors should be explicitly checked to prevent unexpected behavior with malformed input.- return json_decode($value, true); + $decoded = json_decode($value, true); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \JsonException(json_last_error_msg()); + } + return $decoded;
🧹 Nitpick comments (1)
src/Casts/SpannerJson.php (1)
18-30: Improve error handling for null or already processed values.The method correctly handles null and array inputs, but the implementation could be more concise.
public function get(Model $model, string $key, mixed $value, array $attributes) { if ($value === null) { return null; } if (is_array($value)) { return $value; } if (!is_string($value)) { throw new \InvalidArgumentException('The given value must be an array, string or null.'); } - return json_decode($value, true); + $decoded = json_decode($value, true); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \JsonException(json_last_error_msg()); + } + return $decoded; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Casts/SpannerJson.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
🔇 Additional comments (2)
src/Casts/SpannerJson.php (2)
44-44: Missing import for SpannerJsonType.The
SpannerJsonTypeclass is defined within the same file (lines 49-55), but it's being directly used without any special reference. While this works in PHP, it's unconventional and could cause confusion.There are two approaches to fix this:
- Move the
SpannerJsonTypeclass to its own file (preferred for maintainability)- If keeping both classes in the same file, ensure proper documentation indicates this relationship
#!/bin/bash # Check if SpannerJsonType is referenced elsewhere in the codebase echo "Checking for other references to SpannerJsonType..." rg "SpannerJsonType" --type php
1-56: Overall implementation looks good with minor improvements needed.The
SpannerJsoncast implementation follows Laravel conventions and properly handles JSON conversion between database and model. The validation in both thegetandsetmethods appropriately enforces type constraints.A few minor improvements would make this implementation more robust:
- Add proper error handling for JSON decode operations
- Consider moving the
SpannerJsonTypeclass to its own file for better maintainability- Add
use JsonException;to the imports if you implement JSON error handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/Casts/SpannerJson.php (4)
17-19: Fix code style: add space after control structures.The code style is inconsistent with PSR-12 standards. There should be a space between the
ifkeyword and the opening parenthesis.- if($value === null) { + if ($value === null) { return null; }
21-23: Fix code style: add space after control structures.The code style is inconsistent with PSR-12 standards. There should be a space between the
ifkeyword and the opening parenthesis.- if($value instanceof SpannerJsonType) { + if ($value instanceof SpannerJsonType) { $value = (string) $value; }
25-27: Fix code style: add space after control structures.The code style is inconsistent with PSR-12 standards. There should be a space between the
ifkeyword and the opening parenthesis.- if(!is_string($value)) { + if (!is_string($value)) { throw new \InvalidArgumentException('The given value must be a string, SpannerJsonType or null'); }
47-54: Consider adding more comprehensive docblocks for the SpannerJsonType class.The
SpannerJsonTypeclass would benefit from documentation explaining its purpose, especially since it extends a Google Cloud class and overrides type annotation behavior.+/** + * Extended PgJsonb type for Spanner JSON columns with custom type annotation. + * + * @extends PgJsonb + */ class SpannerJsonType extends PgJsonb { + /** + * Override the typeAnnotation to return an unspecified annotation code. + * + * @return int + */ public function typeAnnotation(): int { return TypeAnnotationCode::TYPE_ANNOTATION_CODE_UNSPECIFIED; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Casts/SpannerJson.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
🔇 Additional comments (3)
src/Casts/SpannerJson.php (3)
10-12: Good use of PHPDoc for generics!The PHPDoc annotation properly implements the generic types for the CastsAttributes interface, which enhances type safety when using static analysis tools.
15-30: Add error handling for JSON decoding operations.The current implementation doesn't handle potential JSON decoding errors that could occur when processing malformed JSON data from the database.
public function get(Model $model, string $key, mixed $value, array $attributes) { if($value === null) { return null; } if($value instanceof SpannerJsonType) { $value = (string) $value; } if(!is_string($value)) { throw new \InvalidArgumentException('The given value must be a string, SpannerJsonType or null'); } - return json_decode($value, true); + $decoded = json_decode($value, true); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \JsonException(json_last_error_msg(), json_last_error()); + } + return $decoded; }
32-44: Add validation for JSON encoding in the set method.The current implementation validates the type of input but doesn't ensure that objects or arrays can be properly encoded as JSON.
public function set(Model $model, string $key, mixed $value, array $attributes) { if ( !is_array($value) && !$value instanceof \JsonSerializable && $value !== null && !is_string($value) ) { throw new \InvalidArgumentException('The given value must be an array, JsonSerializable, string or null.'); } + // If the value is a string, validate it's valid JSON or accept it as-is + if (is_string($value) && $value !== '' && $value[0] !== '{' && $value[0] !== '[') { + // If it's not empty and doesn't look like JSON, encode it as a JSON string + $value = json_encode($value); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \JsonException(json_last_error_msg(), json_last_error()); + } + } elseif ($value !== null && !is_string($value)) { + // For arrays and objects, ensure they can be encoded + $encoded = json_encode($value); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \JsonException(json_last_error_msg(), json_last_error()); + } + } + return [$key => new SpannerJsonType($value)]; }
| <?php | ||
|
|
||
| namespace Colopl\Spanner\Casts; | ||
|
|
||
| use Google\Cloud\Spanner\PgJsonb; | ||
| use Google\Cloud\Spanner\V1\TypeAnnotationCode; | ||
| use Illuminate\Contracts\Database\Eloquent\CastsAttributes; | ||
| use Illuminate\Database\Eloquent\Model; | ||
|
|
||
| /** | ||
| * @implements CastsAttributes<mixed, mixed> | ||
| * */ | ||
| class SpannerJson implements CastsAttributes | ||
| { | ||
| public function get(Model $model, string $key, mixed $value, array $attributes) | ||
| { | ||
| if($value === null) { | ||
| return null; | ||
| } | ||
|
|
||
| if($value instanceof SpannerJsonType) { | ||
| $value = (string) $value; | ||
| } | ||
|
|
||
| if(!is_string($value)) { | ||
| throw new \InvalidArgumentException('The given value must be a string, SpannerJsonType or null'); | ||
| } | ||
|
|
||
| return json_decode($value, true); | ||
| } | ||
|
|
||
| public function set(Model $model, string $key, mixed $value, array $attributes) | ||
| { | ||
| if ( | ||
| !is_array($value) && | ||
| !$value instanceof \JsonSerializable && | ||
| $value !== null && | ||
| !is_string($value) | ||
| ) { | ||
| throw new \InvalidArgumentException('The given value must be an array, JsonSerializable, string or null.'); | ||
| } | ||
|
|
||
| return [$key => new SpannerJsonType($value)]; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| class SpannerJsonType extends PgJsonb | ||
| { | ||
| public function typeAnnotation(): int | ||
| { | ||
| return TypeAnnotationCode::TYPE_ANNOTATION_CODE_UNSPECIFIED; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document expected behavior for numeric JSON keys.
JSON objects can have numeric keys, but PHP arrays convert these to integers which can cause unexpected behavior when encoding/decoding between PHP and JSON formats.
Please document or consider handling the case where JSON data contains numeric strings as keys, which might be transformed during the encode/decode process.
🏁 Script executed:
#!/bin/bash
# Create a sample PHP script to test numeric key behavior
cat > test_json_keys.php << 'EOF'
<?php
// Test case with numeric string keys
$jsonWithNumericKeys = '{"1": "value1", "2": "value2"}';
$decoded = json_decode($jsonWithNumericKeys, true);
echo "Original JSON: $jsonWithNumericKeys\n";
echo "Decoded array: " . print_r($decoded, true) . "\n";
$reencoded = json_encode($decoded);
echo "Re-encoded JSON: $reencoded\n";
echo "Are they identical? " . ($jsonWithNumericKeys === $reencoded ? "Yes" : "No") . "\n";
EOF
# Run the test script
php test_json_keys.phpLength of output: 206
Update Documentation on Numeric JSON Key Conversion Behavior
The test confirms that when decoding JSON with numeric string keys, PHP converts these keys to integers. When re-encoding, the keys are output as strings—which causes the re-encoded JSON not to be byte-identical to the original (e.g., extra whitespace differences aside, the numeric keys’ types have shifted). Please update the documentation to clearly explain this behavior in the context of the casting logic in SpannerJson. Consider whether the current handling is acceptable or if additional conversion logic is needed to preserve key types consistently.
- File:
src/Casts/SpannerJson.php - Areas to Address:
- Document the type conversion that occurs during JSON decode/encode.
- Explain that numeric keys are converted to integers upon decoding and back to strings upon encoding.
- Review if any special handling is required for numeric keys in this context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
238-240: Refactor Introductory Text for ConcisenessThe introductory sentence for the JSON cast section could be more concise. Consider revising:
-In order to use JSON columns, use the provided Cast on your model as below +To use JSON columns, apply the `SpannerJson` cast to your model as shown:This makes the instruction clearer and more direct.
🧰 Tools
🪛 LanguageTool
[style] ~238-~238: Consider a shorter alternative to avoid wordiness.
Context: ...have these conversions. ### JSON Type In order to use JSON columns, use the provided Cast...(IN_ORDER_TO_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~238-~238: Consider a shorter alternative to avoid wordiness.
Context: ...have these conversions. ### JSON Type In order to use JSON columns, use the provided Cast...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.17.2)
README.md
237-237: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
🔇 Additional comments (2)
README.md (2)
237-237: Verify Heading Style ConsistencyThe new section header "### JSON Type" appears in ATX style. Please verify that this style is consistent with the project's overall markdown guidelines. If the project intentionally uses ATX for secondary headings—as seen with "## Requirements" and "## Installation"—then this is fine. Otherwise, consider adjusting to setext style if that is preferred.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
237-237: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
241-247: Code Snippet ValidationThe provided code snippet imports
SpannerJsonand demonstrates its use in the model casts array correctly. This clear demonstration aligns well with the PR’s objective of enabling JSON column support.
Added a custom Cast class to support handling JSON columns, updated readme to remove JSON from unsupported
I am not sure if this is still necessary but this is the approach I use to get JSON columns working smoothly
Summary by CodeRabbit
New Features
Documentation