feat: invisble columns support #240
Conversation
WalkthroughThis pull request introduces significant updates to the changelog and the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (4)
CHANGELOG.md (2)
3-3: Enhance the changelog entry for invisible columns.The entry for invisible columns support could be more descriptive to help users understand the feature and its implications. Consider expanding it to:
-add support for invisible columns (#240) +add support for invisible columns in table schemas (#240) + - Allows creating columns that are hidden from SELECT * queries + - Can be explicitly selected by column name + - Useful for gradual schema changes and A/B testing
Line range hint
1-9: Consider categorizing changes in version 8.3.0.To improve readability and help users understand the impact of changes, consider organizing the entries into categories:
# v8.3.0 (2024-09-02) -add support for invisible columns (#240) -add support for change streams using Blueprint (#230) -add support for snapshot queries (#215) -deprecate Connection::getDatabaseContext() and move logic to UseMutations::getMutationExecutor() (#227) -add support for `Query\Builder::whereNotInUnnest(...)` (#225) -`Query\Builder::whereIn` will now wrap values in `UNNEST` if the number of values exceeds the limit (950). (#226) -Commit options can now be set through config or through `Connection::setCommitOptions(...)` (#229) +Added: +- Support for invisible columns in table schemas (#240) +- Support for change streams using Blueprint (#230) +- Support for snapshot queries (#215) +- Support for `Query\Builder::whereNotInUnnest(...)` (#225) + +Changed: +- `Query\Builder::whereIn` will now wrap values in `UNNEST` if the number of values exceeds the limit (950) (#226) +- Commit options can now be set through config or through `Connection::setCommitOptions(...)` (#229) + +Deprecated: +- `Connection::getDatabaseContext()` - use `UseMutations::getMutationExecutor()` instead (#227)src/Schema/Grammar.php (1)
740-752: Consider making the invisible column check more explicit.The implementation is correct, but could be more explicit about the boolean nature of the invisible property.
Consider this alternative implementation for better type safety and readability:
protected function modifyInvisible(Blueprint $blueprint, Fluent $column) { - return $column->invisible !== null + return $column->invisible === true ? ' hidden' : null; }tests/Schema/BlueprintTest.php (1)
258-280: LGTM! Consider adding more test cases.The test effectively verifies the basic functionality of invisible columns. The implementation follows good testing practices with clear setup, execution, and assertions.
Consider adding these additional test cases to improve coverage:
- Verify behavior when explicitly selecting the invisible column
- Test multiple invisible columns
- Test updating invisible column values
- Test behavior with NULL values in invisible columns
Example addition:
$row = $conn->table($tableName)->first(); $this->assertArrayHasKey('id', $row); $this->assertArrayNotHasKey('name', $row); + +// Test explicit selection of invisible column +$row = $conn->table($tableName)->select(['id', 'name'])->first(); +$this->assertArrayHasKey('name', $row); +$this->assertSame('test', $row['name']);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)src/Schema/Grammar.php(3 hunks)tests/Schema/BlueprintTest.php(1 hunks)
🔇 Additional comments (1)
src/Schema/Grammar.php (1)
42-42: LGTM! The modifier list is updated correctly.
The addition of 'Invisible' to the $modifiers array is consistent with the new invisible column feature.
taka-oyama
force-pushed
the
feature/invisible-columns
branch
from
40a0dfe to
fbe37a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.md (1)
Line range hint
3-9: Consider adding usage details for the invisible columns feature.While the changelog entry correctly documents the addition of invisible columns support, it would be helpful to include:
- Basic usage example of invisible columns
- Common use cases (e.g., TOKENLIST)
- Any configuration requirements
Would you like me to help draft detailed documentation for the invisible columns feature?
src/Schema/Grammar.php (1)
737-739: Consider adding implementation note in PHPDoc.Add a note explaining that this method translates Laravel's 'invisible' modifier to Google Cloud Spanner's 'hidden' keyword for better maintainability.
/** * Get the SQL for an invisible column modifier. + * Note: This translates Laravel's 'invisible' modifier to Google Cloud Spanner's 'hidden' keyword. *tests/Schema/BlueprintTest.php (1)
258-280: LGTM! Well-structured test for invisible columns feature.The test comprehensively covers the invisible columns functionality:
- SQL generation for table creation
- Data insertion behavior
- Data retrieval verification
Consider adding these additional test cases to make it more robust:
- Test multiple invisible columns
- Test making visible columns invisible after table creation
- Test making invisible columns visible
public function test_invisible_columns(): void { $conn = $this->getDefaultConnection(); $grammar = new Grammar(); $tableName = $this->generateTableName('Invisible'); $blueprint = new Blueprint($tableName, function (Blueprint $table) { $table->create(); $table->integer('id')->primary(); $table->string('name')->nullable()->invisible(); + // Add more test cases + $table->string('description')->invisible(); }); $this->assertSame([ - "create table `{$tableName}` (`id` int64 not null, `name` string(255) hidden) primary key (`id`)", + "create table `{$tableName}` (`id` int64 not null, `name` string(255) hidden, `description` string(255) hidden) primary key (`id`)", ], $blueprint->toSql($conn, $grammar)); $blueprint->build($conn, $grammar); - $conn->table($tableName)->insert(['id' => 1, 'name' => 'test']); + $conn->table($tableName)->insert(['id' => 1, 'name' => 'test', 'description' => 'test desc']); $row = $conn->table($tableName)->first(); $this->assertArrayHasKey('id', $row); $this->assertArrayNotHasKey('name', $row); + $this->assertArrayNotHasKey('description', $row); + + // Test altering column visibility + $blueprint = new Blueprint($tableName, function (Blueprint $table) { + $table->string('name')->invisible(false)->change(); + }); + $blueprint->build($conn, $grammar); + + $row = $conn->table($tableName)->first(); + $this->assertArrayHasKey('name', $row); + $this->assertArrayNotHasKey('description', $row); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)src/Schema/Grammar.php(3 hunks)tests/Schema/BlueprintTest.php(1 hunks)
🔇 Additional comments (4)
CHANGELOG.md (1)
Line range hint 1-999: LGTM! The changelog maintains excellent structure and clarity.
The changelog follows best practices with:
- Consistent semantic versioning
- Clear PR references
- Proper categorization of changes
- Explicit marking of breaking changes
src/Schema/Grammar.php (3)
42-42: LGTM! Modifier array updated correctly.
The addition of 'Invisible' to the modifiers array is consistent with the existing pattern and correctly enables the invisible column support.
737-749: LGTM! Invisible column modifier implemented correctly.
The implementation correctly translates Laravel's 'invisible' modifier to Google Cloud Spanner's 'hidden' keyword.
737-749: Verify test coverage for invisible columns.
The implementation looks good. Let's verify the test coverage:
✅ Verification successful
Test coverage for invisible columns is confirmed ✅
The implementation is properly tested in tests/Schema/BlueprintTest.php with a dedicated test case that verifies the invisible column functionality, including:
- Table creation with an invisible column
- Proper column definition using the
invisible()modifier - Integration with other column modifiers (nullable)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are tests for invisible columns
rg -l "test.*invisible.*column" "tests/"
Length of output: 71
Script:
#!/bin/bash
# Check the actual test implementation for invisible columns
rg -A 10 "test.*invisible.*column" "tests/Schema/BlueprintTest.php"
Length of output: 492
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Schema/Grammar.php (1)
736-748: LGTM! Implementation of invisible column modifier.The implementation is clean and follows Laravel's modifier method patterns. The SQL syntax generation is correct with proper spacing.
Consider enhancing the PHPDoc with:
- A brief description of invisible columns' purpose
- A link to Cloud Spanner's documentation about invisible columns
- Example usage
/** * Get the SQL for an invisible column modifier. + * + * Invisible columns are hidden from SELECT * queries and must be explicitly + * specified in the SELECT clause to be retrieved. + * + * @see https://cloud.google.com/spanner/docs/invisible-columns * * @param Blueprint $blueprint * @param ColumnDefinition&object{ invisible: bool } $column * @return string|null + * + * @example + * $table->string('token')->invisible(); */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Schema/Grammar.php(2 hunks)
🔇 Additional comments (2)
src/Schema/Grammar.php (2)
41-41: LGTM! Addition of 'Invisible' modifier.
The addition of 'Invisible' to the modifiers array is consistent with Laravel's schema grammar pattern and enables the invisible column feature.
736-748: Verify test coverage and documentation.
Let's ensure the invisible column feature is properly tested and documented.
✅ Verification successful
Invisible column feature is properly implemented and tested
The verification shows:
- Test coverage exists in
tests/Schema/BlueprintTest.phpwith a dedicated test casetest_invisible_columns() - The test verifies the correct SQL generation including the
hiddenkeyword - Implementation is consistent across
Grammar.phpandColumnDefinition.php
While there's no dedicated documentation found, this is a low-level database feature that is typically documented in the database's own documentation rather than requiring extensive application-level documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test coverage of invisible columns
echo "Checking for test coverage..."
rg -l "invisible|hidden" "tests/"
# Check for documentation updates
echo -e "\nChecking for documentation updates..."
rg -l "invisible|hidden" "docs/"
Length of output: 303
Script:
#!/bin/bash
# Let's check the actual test implementation
echo "Checking test implementation for invisible columns..."
rg "invisible|hidden" "tests/Schema/BlueprintTest.php" -B 2 -A 2
# Let's also check if there are any schema-related documentation files in other locations
echo -e "\nChecking for schema documentation in other locations..."
fd -t f -e md . | rg -l "schema|column"
# Let's verify the Blueprint class implementation
echo -e "\nChecking Blueprint implementation..."
rg "invisible" "src/" -B 2 -A 2
Length of output: 1918
Its not documented, but it's supported.
Probably because its used to define TOKENLIST.
Summary by CodeRabbit