Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add change stream support #230

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

taka-oyama
Copy link
Collaborator

@taka-oyama taka-oyama commented Sep 20, 2024

Closes #228

Summary by CodeRabbit

  • New Features

    • Introduced methods for creating, altering, and dropping change streams, enhancing database management capabilities.
    • Added a new class for defining change stream configurations, allowing for detailed customization of data capture.
    • Implemented an enumeration to specify different types of change stream value captures.
    • Added support for snapshot queries and improved query handling with new methods.
    • Expanded documentation on Change Streams, Secondary Index Options, and Mutations for enhanced database interactions.
  • Tests

    • Added comprehensive tests to ensure the functionality of change streams, covering creation, alteration, and deletion scenarios.

@taka-oyama taka-oyama added the enhancement New feature or request label Sep 20, 2024
@taka-oyama taka-oyama requested a review from a team September 20, 2024 07:48
@taka-oyama taka-oyama self-assigned this Sep 20, 2024
Copy link

coderabbitai bot commented Sep 20, 2024

Walkthrough

The 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 Blueprint and Grammar classes. A new ChangeStreamDefinition class is defined to facilitate the configuration of change streams, along with an enumeration for value capture types. Comprehensive tests are implemented to validate these functionalities.

Changes

Files Change Summary
src/Schema/Blueprint.php Added methods: createChangeStream, alterChangeStream, dropChangeStream for managing change streams.
src/Schema/ChangeStreamDefinition.php Introduced ChangeStreamDefinition class to manage change stream configurations, including properties and methods for defining options and tables.
src/Schema/ChangeStreamValueCaptureType.php Added an enumeration ChangeStreamValueCaptureType with cases representing different types of value captures for change streams.
src/Schema/Grammar.php Introduced methods for compiling SQL statements related to change streams: compileCreateChangeStream, compileAlterChangeStream, compileDropChangeStream, along with formatting methods for options and tables.
tests/Schema/BlueprintTest.php Added test methods to validate the functionalities of change streams, including creation, alteration, and deletion scenarios.

Assessment against linked issues

Objective Addressed Explanation
Add support for CHANGE STREAM DDL (#228)

Possibly related PRs

Suggested reviewers

  • halnique
  • dkkoma

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@taka-oyama taka-oyama marked this pull request as ready for review September 24, 2024 00:41
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ChangeStreamValueCaptureType enum 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 $tables property.

The class documentation is comprehensive and follows PHPDoc standards. However, to enhance type safety, consider using a more specific type hint for the $tables property in the class-level PHPDoc.

Update the class-level PHPDoc to include:

+ * @property array<string, list<string>> $tables

This change will provide better type information for IDEs and static analysis tools.

src/Schema/Blueprint.php (2)

391-395: Consider revising the return type of dropChangeStream.

The dropChangeStream method is implemented correctly and follows the same pattern as the other change stream methods. However, returning a ChangeStreamDefinition object when dropping a change stream might not be the most appropriate approach.

Consider changing the return type to Fluent or void, 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 dropChangeStream consistent with other "drop" methods in the class, such as dropSequence.


371-395: Consider adding documentation for the new change stream methods.

The new methods for managing change streams (createChangeStream, alterChangeStream, and dropChangeStream) 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:

  1. A brief description of what the method does
  2. @param descriptions for the $name parameter
  3. @return descriptions
  4. @throws information if applicable
  5. 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 readability

In the formatChangeStreamTables method, there's an assignment within the if condition 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

Commits

Files that changed from the base of the PR and between afedc2d and 9fa6165.

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\Schema correctly 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 ChangeStreamDefinition class 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: createChangeStream method implementation.

The createChangeStream method is well-implemented. It follows the established pattern in the class, correctly creates a new ChangeStreamDefinition command, and returns the appropriate object.


381-389: LGTM: alterChangeStream method implementation.

The alterChangeStream method is correctly implemented. It follows the same pattern as createChangeStream, properly creates a new ChangeStreamDefinition command for altering an existing change stream, and returns the appropriate object.

src/Schema/Grammar.php (3)

271-283: Implementation of compileCreateChangeStream is correct

The compileCreateChangeStream method correctly constructs the SQL statement for creating a change stream, integrating the stream name, tables, and options appropriately.


310-323: Implementation of formatChangeStreamTables handles table formatting properly

The method formatChangeStreamTables effectively 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 of match expression

In the formatChangeStreamOptions method, starting at line 338, you're utilizing the match expression:

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 match expression 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 match expression to a switch statement 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.json

Length of output: 39

tests/Schema/BlueprintTest.php (6)

517-530: LGTM

The test test_createChangeStream_for_all correctly verifies the creation of a change stream for all tables. The implementation is clear and follows best practices.


532-546: LGTM

The test test_createChangeStream_for_table effectively 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: LGTM

The test test_createChangeStream_for_table_columns accurately 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: LGTM

The test test_createChangeStream_for_with_options thoroughly 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: LGTM

The test test_alterChangeStream correctly tests altering an existing change stream's options. The changes to excludeTtlDeletes and retentionPeriod are appropriately asserted.


625-646: LGTM

The test test_dropChangeStream effectively tests the dropping of a change stream. The assertions confirm that the correct SQL command is generated.

src/Schema/ChangeStreamDefinition.php Show resolved Hide resolved
src/Schema/ChangeStreamDefinition.php Show resolved Hide resolved
src/Schema/Grammar.php Show resolved Hide resolved
tests/Schema/BlueprintTest.php Show resolved Hide resolved
tests/Schema/BlueprintTest.php Outdated Show resolved Hide resolved
tests/Schema/BlueprintTest.php Outdated Show resolved Hide resolved
tests/Schema/BlueprintTest.php Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)

Line range hint 3-8: LGTM with suggestions for clarity

The 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:

  1. 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.

  2. 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

📥 Commits

Files that changed from the base of the PR and between e762105 and 30897c2.

📒 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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Files that changed from the base of the PR and between 6d1a980 and 1bc8559.

📒 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)

@taka-oyama
Copy link
Collaborator Author

test now passing

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Generating a unique name for the change stream.
  2. Creating a change stream for multiple tables.
  3. Asserting the correct SQL command is generated.
  4. 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:

  1. Generating a unique name for the change stream.
  2. Creating a change stream for specific columns of a table.
  3. Asserting the correct SQL command is generated.
  4. Cleaning up by dropping the change stream after the test.

Consider the following improvements:

  1. Add an assertion to verify the change stream's presence in the information schema.
  2. Use beforeApplicationDestroyed for 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:

  1. Creating a new table and a change stream for that table.
  2. Setting multiple options for the change stream (retention period, value capture type, exclusions).
  3. Asserting the correct SQL commands for both table and change stream creation.
  4. Cleaning up by dropping the change stream after the test.

Consider these enhancements:

  1. Add assertions to verify the change stream's presence and options in the information schema.
  2. Use beforeApplicationDestroyed for cleanup to ensure it runs even if the test fails.
  3. 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:

  1. Creating a new table and an initial change stream.
  2. Altering the change stream with new options.
  3. Asserting the correct SQL command for altering the change stream.
  4. Cleaning up by dropping the change stream after the test.

Consider these improvements:

  1. Add an assertion to verify the altered change stream options in the information schema.
  2. Use beforeApplicationDestroyed for cleanup to ensure it runs even if the test fails.
  3. Consider dropping the created table as part of the cleanup process.
  4. 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:

  1. Creating a new table and a change stream.
  2. Dropping the change stream.
  3. Asserting the correct SQL command for dropping the change stream.

Consider these enhancements:

  1. Add an assertion to verify that the change stream is actually dropped from the information schema.
  2. Add cleanup for the created table to avoid leaving test artifacts in the database.
  3. Consider using try-finally or beforeApplicationDestroyed to 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:

  1. Implementing a common setup and teardown method for change stream tests to reduce code duplication.
  2. Consistently using beforeApplicationDestroyed for cleanup in all tests.
  3. Adding assertions to verify actual database state changes in addition to SQL command checks.
  4. Creating a helper method for generating unique names to ensure consistency across tests.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1bc8559 and d60c239.

📒 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:

  1. Generates a unique name for the change stream.
  2. Asserts the correct SQL command is generated.
  3. Verifies the stream's presence in the information schema.
  4. Handles cleanup appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for CHANGE STREAM DDL
1 participant