feat: add change stream support #230
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request introduce functionalities for managing change streams within a database schema. This includes methods for creating, altering, and dropping change streams in the Changes
Assessment against linked issues
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: 7
Outside diff range and nitpick comments (5)
src/Schema/ChangeStreamValueCaptureType.php (1)
23-29: LGTM: Well-defined enum for change stream value capture types.The
ChangeStreamValueCaptureTypeenum is well-structured and covers the main types of value capture for change streams. The use of string backing is appropriate for this use case.Consider adding a brief PHPDoc comment above the enum to describe its purpose and potentially link to relevant documentation. For example:
/** * Represents the different types of value capture for change streams in Spanner. * * @see https://cloud.google.com/spanner/docs/change-streams */ enum ChangeStreamValueCaptureType: string { // ... (existing cases) }This addition would enhance the self-documentation of the code and provide a quick reference for developers.
src/Schema/ChangeStreamDefinition.php (1)
25-41: Consider improving type safety for the$tablesproperty.The class documentation is comprehensive and follows PHPDoc standards. However, to enhance type safety, consider using a more specific type hint for the
$tablesproperty in the class-level PHPDoc.Update the class-level PHPDoc to include:
+ * @property array<string, list<string>> $tablesThis change will provide better type information for IDEs and static analysis tools.
src/Schema/Blueprint.php (2)
391-395: Consider revising the return type ofdropChangeStream.The
dropChangeStreammethod is implemented correctly and follows the same pattern as the other change stream methods. However, returning aChangeStreamDefinitionobject when dropping a change stream might not be the most appropriate approach.Consider changing the return type to
Fluentorvoid, which would be more consistent with the action of dropping a change stream.Here's a suggested implementation:
-public function dropChangeStream(string $name): ChangeStreamDefinition +public function dropChangeStream(string $name): Fluent { - $this->commands[] = $command = new ChangeStreamDefinition(__FUNCTION__, $name); - return $command; + return $this->addCommand(__FUNCTION__, ['change_stream' => $name]); }This change would make
dropChangeStreamconsistent with other "drop" methods in the class, such asdropSequence.
371-395: Consider adding documentation for the new change stream methods.The new methods for managing change streams (
createChangeStream,alterChangeStream, anddropChangeStream) have been implemented well and are consistent with the existing code style. To improve the usability of these new features, consider adding PHPDoc comments for each method. These comments should include:
- A brief description of what the method does
- @param descriptions for the
$nameparameter- @return descriptions
- @throws information if applicable
- A link to relevant Spanner documentation about change streams
This additional documentation will help users understand how to use these new methods effectively.
src/Schema/Grammar.php (1)
315-317: Avoid assignments within conditional expressions for better readabilityIn the
formatChangeStreamTablesmethod, there's an assignment within theifcondition at line 315:if ($columnsAsString = $this->columnize($columns)) { $string .= "({$columnsAsString})"; }While valid in PHP, assignments within conditionals can reduce code readability and may lead to unintended bugs. It's clearer to separate the assignment from the condition.
Consider refactoring the code as follows:
+ $columnsAsString = $this->columnize($columns); - if ($columnsAsString = $this->columnize($columns)) { + if (!empty($columnsAsString)) { $string .= "({$columnsAsString})"; }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/Schema/Blueprint.php (1 hunks)
- src/Schema/ChangeStreamDefinition.php (1 hunks)
- src/Schema/ChangeStreamValueCaptureType.php (1 hunks)
- src/Schema/Grammar.php (1 hunks)
- tests/Schema/BlueprintTest.php (2 hunks)
Additional comments not posted (14)
src/Schema/ChangeStreamValueCaptureType.php (1)
1-21: LGTM: File structure and header are well-formed.The file structure follows best practices:
- Appropriate copyright notice and license information.
- Strict typing is enforced.
- The namespace
Colopl\Spanner\Schemacorrectly reflects the file's location in the project structure.src/Schema/ChangeStreamDefinition.php (2)
1-23: LGTM: File header and namespace declaration are correct.The file header includes appropriate copyright and license information. The namespace is correctly declared, and necessary imports are present.
1-87: Overall, the ChangeStreamDefinition class is well-implemented.The
ChangeStreamDefinitionclass provides a solid foundation for managing change stream configurations. It follows good coding practices, uses appropriate type hints, and includes comprehensive documentation. The suggested improvements, if implemented, will further enhance its type safety, flexibility, and readability.Great job on introducing this new functionality to support change streams in the Laravel Spanner package!
src/Schema/Blueprint.php (2)
371-379: LGTM:createChangeStreammethod implementation.The
createChangeStreammethod is well-implemented. It follows the established pattern in the class, correctly creates a newChangeStreamDefinitioncommand, and returns the appropriate object.
381-389: LGTM:alterChangeStreammethod implementation.The
alterChangeStreammethod is correctly implemented. It follows the same pattern ascreateChangeStream, properly creates a newChangeStreamDefinitioncommand for altering an existing change stream, and returns the appropriate object.src/Schema/Grammar.php (3)
271-283: Implementation ofcompileCreateChangeStreamis correctThe
compileCreateChangeStreammethod correctly constructs the SQL statement for creating a change stream, integrating the stream name, tables, and options appropriately.
310-323: Implementation offormatChangeStreamTableshandles table formatting properlyThe method
formatChangeStreamTableseffectively formats the tables and columns for inclusion in the SQL statement, correctly handling cases where specific columns are provided or when all tables are targeted.
271-283: Ensure compliance with minimum PHP version due to use ofmatchexpressionIn the
formatChangeStreamOptionsmethod, starting at line 338, you're utilizing thematchexpression:return Str::snake($k) . '=' . match (true) { $v === true => 'true', $v === false => 'false', is_string($v) => $this->quoteString($v), $v instanceof ChangeStreamValueCaptureType => $this->quoteString($v->value), default => throw new LogicException('Unsupported option value: ' . $v), };The
matchexpression is a feature introduced in PHP 8.0. If the project aims to support PHP versions earlier than 8.0, this code will cause compatibility issues.Please verify the project's minimum PHP version requirement. If support for PHP versions below 8.0 is necessary, consider refactoring the
matchexpression to aswitchstatement or equivalent logic.Run the following script to check the PHP version constraint in your
composer.json:Verification successful
Minimum PHP version requirement is satisfied.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the minimum PHP version requirement in composer.json # Test: Extract the PHP version constraint. Expect: Version should be >= 8.0 if using match expressions. jq -r '.require.php' composer.jsonLength of output: 39
tests/Schema/BlueprintTest.php (6)
517-530: LGTMThe test
test_createChangeStream_for_allcorrectly verifies the creation of a change stream for all tables. The implementation is clear and follows best practices.
532-546: LGTMThe test
test_createChangeStream_for_tableeffectively checks the creation of a change stream for specific tables. The use of multiple->for()calls appropriately includes each table in the change stream.
548-559: LGTMThe test
test_createChangeStream_for_table_columnsaccurately tests the creation of a change stream for specific columns within a table. The implementation ensures that only the specified columns are included.
561-595: LGTMThe test
test_createChangeStream_for_with_optionsthoroughly tests the creation of a change stream with various options, including retention period and exclusion flags. The assertions correctly validate the generated SQL commands.
597-623: LGTMThe test
test_alterChangeStreamcorrectly tests altering an existing change stream's options. The changes toexcludeTtlDeletesandretentionPeriodare appropriately asserted.
625-646: LGTMThe test
test_dropChangeStreameffectively tests the dropping of a change stream. The assertions confirm that the correct SQL command is generated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
Line range hint
3-8: LGTM with suggestions for clarityThe changelog for v8.3.0 accurately reflects the major changes, including the addition of change stream support as outlined in the PR objectives. However, I have a couple of suggestions to enhance clarity:
The entry "add support for snapshot queries (feat: Support snapshot queries #215)" is a significant feature addition. Consider providing a brief explanation of what snapshot queries are and their benefits, as this feature wasn't mentioned in the PR objectives.
For the entry "Commit options can now be set through config or through
Connection::setCommitOptions(...)", it would be helpful to briefly mention what kinds of commit options can be set and how this enhances functionality.These additions would provide more context for users reading the changelog.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- tests/Schema/BlueprintTest.php (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Schema/BlueprintTest.php
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
README.md (2)
330-330: Consider using setext-style heading for consistency.The heading "Change Streams" is using atx-style (with
#) while other headings in the document use setext-style (with===or---). Consider changing it to maintain consistency with the rest of the document.-### Change Streams +Change Streams +---------------🧰 Tools
🪛 Markdownlint
330-330: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
330-350: LGTM! Consider adding a brief explanation of Change Streams.The new Change Streams section provides clear and concise information on how to use this feature with the schema builder. The code example effectively demonstrates the creation, alteration, and dropping of change streams.
To enhance this section further, consider adding a brief explanation of what Change Streams are used for and their benefits. This would provide more context for users who might be unfamiliar with the concept.
Here's a suggested addition to the beginning of the section:
Change Streams allow you to capture and process data changes in real-time, enabling various use cases such as replication, auditing, and event-driven architectures. With Laravel Spanner, you can easily manage Change Streams using the schema builder.🧰 Tools
🪛 Markdownlint
330-330: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- README.md (1 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md
330-330: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
🔇 Additional comments (2)
README.md (2)
Line range hint
352-391: LGTM! New sections enhance documentation.The additions of "Secondary Index Options" and "Mutations" sections provide valuable information on Spanner-specific features. These sections are well-written, consistent with the document's style, and offer clear examples of usage.
The "Secondary Index Options" section effectively explains how to use null filtering and storing clauses, which are important for optimizing Spanner indexes.
The "Mutations" section clearly describes an alternative method for data modification that can improve performance, along with important caveats about its behavior within transactions.
These additions significantly enhance the documentation and provide users with important information about advanced Spanner features.
🧰 Tools
🪛 Markdownlint
330-330: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
353-353: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
Line range hint
1-391: Overall, excellent additions to the README!The changes to this README significantly enhance the documentation for the laravel-spanner project. The new sections on Change Streams, Secondary Index Options, and Mutations provide valuable information on advanced Spanner features, complete with clear examples and usage instructions.
These additions will greatly assist developers in leveraging the full capabilities of Spanner within their Laravel applications. The document maintains a consistent style and structure, making it easy for users to navigate and understand the new features.
Great job on improving the documentation and introducing these powerful features!
🧰 Tools
🪛 Markdownlint
330-330: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
353-353: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
|
test now passing |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
tests/Schema/BlueprintTest.php (6)
532-550: LGTM! Well-structured test for creating a change stream for specific tables.The test effectively covers:
- Generating a unique name for the change stream.
- Creating a change stream for multiple tables.
- Asserting the correct SQL command is generated.
- Cleaning up by dropping the change stream after the test.
Consider adding an assertion to verify the change stream's presence in the information schema, similar to the previous test. This would ensure the change stream is actually created in the database.
552-568: LGTM! Effective test for creating a change stream for specific table columns.The test covers key aspects:
- Generating a unique name for the change stream.
- Creating a change stream for specific columns of a table.
- Asserting the correct SQL command is generated.
- Cleaning up by dropping the change stream after the test.
Consider the following improvements:
- Add an assertion to verify the change stream's presence in the information schema.
- Use
beforeApplicationDestroyedfor cleanup to ensure it runs even if the test fails.
570-607: LGTM! Comprehensive test for creating a change stream with various options.The test effectively covers:
- Creating a new table and a change stream for that table.
- Setting multiple options for the change stream (retention period, value capture type, exclusions).
- Asserting the correct SQL commands for both table and change stream creation.
- Cleaning up by dropping the change stream after the test.
Consider these enhancements:
- Add assertions to verify the change stream's presence and options in the information schema.
- Use
beforeApplicationDestroyedfor cleanup to ensure it runs even if the test fails.- Consider dropping the created table as part of the cleanup process.
609-637: LGTM! Well-structured test for altering an existing change stream.The test effectively covers:
- Creating a new table and an initial change stream.
- Altering the change stream with new options.
- Asserting the correct SQL command for altering the change stream.
- Cleaning up by dropping the change stream after the test.
Consider these improvements:
- Add an assertion to verify the altered change stream options in the information schema.
- Use
beforeApplicationDestroyedfor cleanup to ensure it runs even if the test fails.- Consider dropping the created table as part of the cleanup process.
- Add a test case for altering multiple options simultaneously to ensure they're all applied correctly.
639-660: LGTM! Effective test for dropping a change stream.The test covers key aspects:
- Creating a new table and a change stream.
- Dropping the change stream.
- Asserting the correct SQL command for dropping the change stream.
Consider these enhancements:
- Add an assertion to verify that the change stream is actually dropped from the information schema.
- Add cleanup for the created table to avoid leaving test artifacts in the database.
- Consider using
try-finallyorbeforeApplicationDestroyedto ensure cleanup runs even if the test fails.
517-660: Overall, excellent test coverage for change stream operations.The new tests comprehensively cover change stream functionality, including creation for all tables, specific tables, and columns, as well as altering and dropping change streams. The tests are well-structured and use unique identifiers to avoid conflicts.
To further enhance the test suite, consider:
- Implementing a common setup and teardown method for change stream tests to reduce code duplication.
- Consistently using
beforeApplicationDestroyedfor cleanup in all tests.- Adding assertions to verify actual database state changes in addition to SQL command checks.
- Creating a helper method for generating unique names to ensure consistency across tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/Schema/BlueprintTest.php (2 hunks)
🔇 Additional comments (1)
tests/Schema/BlueprintTest.php (1)
517-530: LGTM! Comprehensive test for creating a change stream for all tables.The test is well-structured and covers important aspects:
- Generates a unique name for the change stream.
- Asserts the correct SQL command is generated.
- Verifies the stream's presence in the information schema.
- Handles cleanup appropriately.
Co-authored-by: uyan <[email protected]>
Co-authored-by: uyan <[email protected]>
Closes #228
Summary by CodeRabbit
New Features
Tests