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(spanner): add support of checking row not found errors from ReadRow and ReadRowUsingIndex #10405

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

rahul2393
Copy link
Contributor

@rahul2393 rahul2393 commented Jun 21, 2024

Fixes: #10031

Customers can check row not found cases with below condition

if errors.Is(err, spanner.ErrRecordNotFound) {
  • Add test case for checking row not found with errors.Is
  • Ensure backward compatibility

@rahul2393 rahul2393 requested review from a team as code owners June 21, 2024 12:22
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Jun 21, 2024
@rahul2393 rahul2393 requested a review from olavloite June 21, 2024 12:23
@rahul2393 rahul2393 force-pushed the support_errors_is branch 2 times, most recently from b550e3b to a8e5581 Compare June 21, 2024 13:58
@rahul2393 rahul2393 changed the title fix(spanner): add support of errors.Is when custom row not found error is returned fix(spanner): add support of checking row not found errors from ReadRow and ReadRowUsingIndex Jun 21, 2024
@rahul2393 rahul2393 changed the title fix(spanner): add support of checking row not found errors from ReadRow and ReadRowUsingIndex feat(spanner): add support of checking row not found errors from ReadRow and ReadRowUsingIndex Jun 21, 2024
@rahul2393 rahul2393 force-pushed the support_errors_is branch from a8e5581 to bacee62 Compare June 21, 2024 14:03
spanner/errors.go Show resolved Hide resolved
@@ -26,6 +27,11 @@ import (
"google.golang.org/grpc/status"
)

var (
// ErrRecordNotFound record not found error
ErrRecordNotFound = errors.New("record not found")
Copy link
Contributor

Choose a reason for hiding this comment

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

ErrRowNotFound corresponds better with the original error message row not found...

Suggested change
ErrRecordNotFound = errors.New("record not found")
ErrRowNotFound = errors.New("record not found")

spanner/transaction.go Show resolved Hide resolved
@@ -373,7 +377,7 @@ func (t *txReadOnly) ReadRowWithOptions(ctx context.Context, table string, key K
// ReadRowUsingIndex reads a single row from the database using an index.
//
// If no row is present with the given index, then ReadRowUsingIndex returns an
// error where spanner.ErrCode(err) is codes.NotFound.
// error(spanner.ErrRecordNotFound) where spanner.ErrCode(err) is codes.NotFound.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here: Maybe add an example in the comments for how users should check for this specific error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@rahul2393 rahul2393 force-pushed the support_errors_is branch from c04025c to bda801f Compare June 21, 2024 14:29
@rahul2393 rahul2393 requested a review from olavloite June 21, 2024 14:29
//
// To check if the error is spanner.ErrRecordNotFound:
//
// if errors.Is(err, ErrRecordNotFound) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// if errors.Is(err, ErrRecordNotFound) {
// if errors.Is(err, ErrRowNotFound) {

//
// To check if the error is spanner.ErrRecordNotFound:
//
// if errors.Is(err, ErrRecordNotFound) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// if errors.Is(err, ErrRecordNotFound) {
// if errors.Is(err, ErrRowNotFound) {

// error where spanner.ErrCode(err) is codes.NotFound.
// error(spanner.ErrRowNotFound) where spanner.ErrCode(err) is codes.NotFound.
//
// To check if the error is spanner.ErrRecordNotFound:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// To check if the error is spanner.ErrRecordNotFound:
// To check if the error is spanner.ErrRowNotFound:

//
// To check if the error is spanner.ErrRecordNotFound:
//
// if errors.Is(err, ErrRecordNotFound) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// if errors.Is(err, ErrRecordNotFound) {
// if errors.Is(err, ErrRowNotFound) {

@rahul2393 rahul2393 force-pushed the support_errors_is branch from 1838660 to 02d99fd Compare June 21, 2024 14:37
@rahul2393 rahul2393 requested a review from olavloite June 21, 2024 14:37
@rahul2393 rahul2393 enabled auto-merge (squash) June 21, 2024 14:38
@rahul2393 rahul2393 merged commit 5cb0c26 into main Jun 21, 2024
12 checks passed
@rahul2393 rahul2393 deleted the support_errors_is branch June 21, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner: add better support for reading a row that may not exist
2 participants