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

add similar unique violation check as sql for spanner #287

Merged
merged 4 commits into from
May 4, 2023

Conversation

gyao852
Copy link
Contributor

@gyao852 gyao852 commented May 1, 2023

Changes:

  • Similar to what we support for mysql::MySQLUniqueViolation, added spanner::SpannerUniqueViolation that look at the error code or message to deduce if we have a duplicate error.

Note: spanner.Error will be deprecated in the future, replaced with APIError. I added tests for both responses from client.

Screenshot 2023-05-01 at 4 21 25 PM

@gyao852 gyao852 requested review from pa3ng, vxio and InfernoJJ May 1, 2023 20:22
@gyao852 gyao852 marked this pull request as ready for review May 1, 2023 20:23
@gyao852 gyao852 requested a review from adamdecaf as a code owner May 1, 2023 20:23
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07 🎉

Comparison is base (0b27896) 75.80% compared to head (847766d) 75.88%.

❗ Current head 847766d differs from pull request most recent head 7dd21d0. Consider uploading reports for the commit 7dd21d0 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
+ Coverage   75.80%   75.88%   +0.07%     
==========================================
  Files          28       28              
  Lines         926      929       +3     
==========================================
+ Hits          702      705       +3     
  Misses        178      178              
  Partials       46       46              
Impacted Files Coverage Δ
database/spanner.go 81.81% <100.00%> (+6.81%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

database/spanner.go Outdated Show resolved Hide resolved
database/spanner.go Outdated Show resolved Hide resolved
@gyao852 gyao852 requested a review from vxio May 3, 2023 15:29
adamdecaf
adamdecaf previously approved these changes May 3, 2023
@adamdecaf
Copy link
Member

Are we going to close #286 ?

This can be released as v0.41.0 since it adds to the public API. Please commit the changelog update in a standalone commit and git tag it. You can create a release from the tag with the changelog update.

@gyao852
Copy link
Contributor Author

gyao852 commented May 3, 2023

Yeah, I wasn't in the internal moov-io group before so thought I needed to fork + create that old PR . Got added to the group right after and forgot to close that one.

Sounds good @adamdecaf ; I'm guessing you're referring to something like this #266 for the changelog commit? If so just need another stamp from Vince when he starts work and I'll have it in before EOD.

@adamdecaf
Copy link
Member

Sounds good @adamdecaf ; I'm guessing you're referring to something like this #266 for the changelog commit? If so just need another stamp from Vince when he starts work and I'll have it in before EOD.

Yea but just commit the changelog to master and tag it.

vxio
vxio previously approved these changes May 3, 2023
Copy link
Member

@vxio vxio left a comment

Choose a reason for hiding this comment

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

approved w/ two minor comments

database/spanner_test.go Outdated Show resolved Hide resolved
database/spanner_test.go Show resolved Hide resolved
@gyao852 gyao852 dismissed stale reviews from vxio and adamdecaf via 083d319 May 3, 2023 22:02
@gyao852 gyao852 requested a review from vxio May 3, 2023 22:05
Comment on lines 36 to 37
match := strings.Contains(err.Error(), "Failed to insert row with primary key")
return match || spanner.ErrCode(err) == codes.AlreadyExists
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
match := strings.Contains(err.Error(), "Failed to insert row with primary key")
return match || spanner.ErrCode(err) == codes.AlreadyExists
return spanner.ErrCode(err) == codes.AlreadyExists ||
strings.Contains(err.Error(), "AlreadyExists")

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

database/spanner_test.go Show resolved Hide resolved
@gyao852 gyao852 requested a review from adamdecaf May 4, 2023 00:02
@adamdecaf adamdecaf merged commit b908e05 into master May 4, 2023
@adamdecaf adamdecaf deleted the add-spanner-duplicate-error branch May 4, 2023 13:21
@adamdecaf
Copy link
Member

I squashed the commits since there was some cleanup. Do we plan on adding more before cutting a release?

@gyao852
Copy link
Contributor Author

gyao852 commented May 4, 2023

While we could go down the rabbit hole of mapping all the errors, I think at least for now what we have is sufficient because we’re trying to replicate what we have existing for mySQL; there is the other sql verification MySQLDataTooLong which isn’t used heavily in our codebase nor does GCP Spanner docs mention any error codes mapped to this type of sql error.

Edit: There is one minor thing we can do which is update database::UniqueViolations to return MySQLUniqueViolation(err) || SpannerUniqueViolation(err). This way we can move way from making the same changes elsewhere.

cc @vxio

@vxio
Copy link
Member

vxio commented May 4, 2023

@gyao852 oh right, let's update that method (database::UniqueViolations), i didn't realize that existed.. thanks!

@adamdecaf
Copy link
Member

Sounds good. Let's get that patch in and can cut a release.

This was referenced May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants