Skip to content

fix(bigtable): Allow nil condition in conditional mutation#11457

Merged
bhshkh merged 2 commits intogoogleapis:mainfrom
bhshkh:fix/cbt-TestCheckAndMutateRow_NoRetry_TrueMutations
Jan 22, 2025
Merged

fix(bigtable): Allow nil condition in conditional mutation#11457
bhshkh merged 2 commits intogoogleapis:mainfrom
bhshkh:fix/cbt-TestCheckAndMutateRow_NoRetry_TrueMutations

Conversation

@bhshkh
Copy link
Copy Markdown
Contributor

@bhshkh bhshkh commented Jan 16, 2025

When the test proxy receives a conditional mutation request with nil condition, it uses the PassAllFilter to call Apply method of Go library. Thus, the request sent by Go library to Cloud Bigtable service contains the field pass all filter.
TestCheckAndMutateRow_NoRetry_TrueMutations conformance test fails with error

=== RUN   TestCheckAndMutateRow_NoRetry_TrueMutations
    checkandmutaterow_test.go:138: diff found (-want +got):
          (*bigtablepb.CheckAndMutateRowRequest)(Inverse(protocmp.Transform, protocmp.Message{
          	"@type":            s"google.bigtable.v2.CheckAndMutateRowRequest",
        + 	"predicate_filter": s"{pass_all_filter:true}",
          	"row_key":          []uint8("row-01"),
          	"table_name":       string("projects/project/instances/instance/tables/table"),
          	"true_mutations":   []protocmp.Message{{"@type": s"google.bigtable.v2.Mutation", ...}, {"@type": s"google.bigtable.v2.Mutation", ...}},
          }))
--- FAIL: TestCheckAndMutateRow_NoRetry_TrueMutations (0.00s)

The Go library assumes that the field cond in Mutation being non-nil denotes conditional mutation but as per the above test we want to allow conditional mutation with no condition. So, a new variable isConditional is being added to Mutation to distinguish between conditional and non-conditional mutations.

After fix:

=== RUN   TestCheckAndMutateRow_NoRetry_TrueMutations
--- PASS: TestCheckAndMutateRow_NoRetry_TrueMutations (0.00s)

@bhshkh bhshkh requested review from a team and gkevinzheng January 16, 2025 01:55
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jan 16, 2025
@bhshkh bhshkh requested a review from andre-sampaio January 16, 2025 01:55
@bhshkh bhshkh enabled auto-merge (squash) January 16, 2025 01:58
@bhshkh bhshkh force-pushed the fix/cbt-TestCheckAndMutateRow_NoRetry_TrueMutations branch from af826d7 to 0397be0 Compare January 16, 2025 02:00
gkevinzheng
gkevinzheng reviewed Jan 16, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@gkevinzheng gkevinzheng left a comment

Choose a reason for hiding this comment

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

Where is m.isConditional ever set to false?

@bhshkh
Copy link
Copy Markdown
Contributor Author

bhshkh commented Jan 16, 2025

Where is m.isConditional ever set to false?

If it is not set, that means it is false. Default value of bool types is false

@bhshkh bhshkh merged commit d83bc05 into googleapis:main Jan 22, 2025
@bhshkh bhshkh deleted the fix/cbt-TestCheckAndMutateRow_NoRetry_TrueMutations branch January 22, 2025 20:20
@release-please release-please Bot mentioned this pull request Jan 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gkevinzheng gkevinzheng gkevinzheng approved these changes

+1 more reviewer

@andre-sampaio andre-sampaio andre-sampaio approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bhshkh @andre-sampaio @gkevinzheng