-
Notifications
You must be signed in to change notification settings - Fork 123
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: foreign key on delete cascade #2340
Conversation
arpan14
commented
Mar 20, 2023
- Adding integration tests for foreign key on delete cascade feature testing both GoogleSQL + PostGres dialects.
- Adding documentation samples along with integration tests for the samples.
- Making a minor enhancement in the samples integration test setup framework to allow accepting a host URL in Spanner options to allow overriding and testing in staging env.
…od while retrying exceptions in unit tests. * For details on issue see - googleapis#2206
fix: prevent illegal negative timeout values into thread sleep() method while retrying exceptions in unit tests.
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITForeignKeyDeleteCascadeTest.java
Show resolved
Hide resolved
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITForeignKeyDeleteCascadeTest.java
Outdated
Show resolved
Hide resolved
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITForeignKeyDeleteCascadeTest.java
Outdated
Show resolved
Hide resolved
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITForeignKeyDeleteCascadeTest.java
Outdated
Show resolved
Hide resolved
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITForeignKeyDeleteCascadeTest.java
Outdated
Show resolved
Hide resolved
.../snippets/src/main/java/com/example/spanner/AlterTableWithForeignKeyDeleteCascadeSample.java
Show resolved
Hide resolved
.../snippets/src/main/java/com/example/spanner/DropForeignKeyConstraintDeleteCascadeSample.java
Outdated
Show resolved
Hide resolved
...nippets/src/test/java/com/example/spanner/AlterTableWithForeignKeyDeleteCascadeSampleIT.java
Show resolved
Hide resolved
...ippets/src/test/java/com/example/spanner/CreateTableWithForeignKeyDeleteCascadeSampleIT.java
Show resolved
Hide resolved
...nippets/src/test/java/com/example/spanner/DropForeignKeyConstraintDeleteCascadeSampleIT.java
Show resolved
Hide resolved
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITForeignKeyDeleteCascadeTest.java
Show resolved
Hide resolved
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITForeignKeyDeleteCascadeTest.java
Outdated
Show resolved
Hide resolved
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITForeignKeyDeleteCascadeTest.java
Outdated
Show resolved
Hide resolved
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITForeignKeyDeleteCascadeTest.java
Show resolved
Hide resolved
transaction -> { | ||
transaction.batchUpdate( | ||
ImmutableList.of( | ||
singerInsertStatementWithValues, concertInsertStatementWithValues)); | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip (no need to change it in this PR unless you want to): You can simplify many of these test transactions further if you just return the value that is returned by the batchUpdate
(or any other update statement in the transaction). That again would allow you to define the whole transaction as a lambda like this:
databaseClient
.readWriteTransaction()
.run(
transaction -> transaction.executeUpdate(singerDeleteStatementWithValues));
.../snippets/src/main/java/com/example/spanner/AlterTableWithForeignKeyDeleteCascadeSample.java
Show resolved
Hide resolved
.../snippets/src/main/java/com/example/spanner/DropForeignKeyConstraintDeleteCascadeSample.java
Show resolved
Hide resolved
databaseAdminClient, instanceId, databaseId)); | ||
|
||
assertTrue( | ||
"Expected to have created database " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be instead of checking the output message we should verify that the table was actually altered by the sample or not ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Yes, for all the tests in the integration test class
ITForeignKeyDeleteCascadeTest
we validate/assert the foreign key constraint. - These tests are for the samples. Samples internally use the code that is already validated through
ITForeignKeyDeleteCascadeTest
. Hence, we don't need to again make a database call. - In Java, asserting output statements is a very standard practice across all existing samples. I agree, its not the best strategy but for cases like this where we have other integration tests doing deep DDL checks, I think keeping the sample tests simple is ok.