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

Support context based transaction for create/update only #48

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

gaurav-vmware
Copy link
Contributor

@gaurav-vmware gaurav-vmware commented Jul 12, 2023

MR details -

Changes for exposing transactions outside datastore CRUD methods.

  • Currently, transaction support is only available to create/upsert methods.
  • Update/Delete/Find still doesn't support them due to chaining of the
    where clauses

Git issue link - #49 (comment)

@gaurav-vmware gaurav-vmware changed the title CPDP-8817: expose context based transaction support for data store me… Expose context based transaction support for data store me… Jul 12, 2023
@gaurav-vmware gaurav-vmware changed the title Expose context based transaction support for data store me… https://github.com/vmware-labs/multi-tenant-persistence-for-saas/issues/49#issue-1801643594 Expose context based transaction support for data store Jul 12, 2023
@gaurav-vmware gaurav-vmware changed the title https://github.com/vmware-labs/multi-tenant-persistence-for-saas/issues/49#issue-1801643594 Expose context based transaction support for data store Expose context based transaction support for data store https://github.com/vmware-labs/multi-tenant-persistence-for-saas/issues/49#issue-1801643594 Jul 12, 2023
@jeyhun
Copy link
Contributor

jeyhun commented Jul 12, 2023

I'd like to see new/existing unit tests use the new code, in order to be able to see how it works in action and step through the code with a debugger. Right now no unit test touches the line tx = ctx.Value(authorizer.TransactionContextKey("transaction")).(*gorm.DB).Clauses(clause.Locking{Strength: "UPDATE"}) in database.go, which seems like the biggest change in this PR

@krishnamiriyala
Copy link
Contributor

@gaurav-vmware overall looks good to me, add unit-tests and remove unused code.

jeyhun
jeyhun previously requested changes Aug 14, 2023
pkg/datastore/database.go Outdated Show resolved Hide resolved
pkg/datastore/database.go Outdated Show resolved Hide resolved
pkg/datastore/database.go Outdated Show resolved Hide resolved
pkg/authorizer/transaction.go Outdated Show resolved Hide resolved
pkg/authorizer/transaction.go Outdated Show resolved Hide resolved
pkg/authorizer/transaction.go Show resolved Hide resolved
pkg/datastore/database.go Outdated Show resolved Hide resolved
pkg/datastore/database.go Outdated Show resolved Hide resolved
pkg/datastore/database.go Outdated Show resolved Hide resolved
pkg/datastore/database.go Outdated Show resolved Hide resolved
@krishnamiriyala krishnamiriyala force-pushed the expose-transaction-support branch 3 times, most recently from bdfda3e to 1c8ed04 Compare September 18, 2023 22:29
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2023

Codecov Report

Merging #48 (da43b12) into main (7b623b2) will decrease coverage by 0.10%.
The diff coverage is 48.61%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
- Coverage   70.42%   70.33%   -0.10%     
==========================================
  Files          12       13       +1     
  Lines        1454     1483      +29     
==========================================
+ Hits         1024     1043      +19     
- Misses        337      347      +10     
  Partials       93       93              
Flag Coverage Δ
unit-tests 70.33% <48.61%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
pkg/authorizer/transaction.go 0.00% <0.00%> (ø)
pkg/datastore/database.go 57.14% <52.63%> (+1.85%) ⬆️
pkg/datastore/helper.go 55.48% <100.00%> (+0.28%) ⬆️

@krishnamiriyala krishnamiriyala changed the title Expose context based transaction support for data store https://github.com/vmware-labs/multi-tenant-persistence-for-saas/issues/49#issue-1801643594 Expose context based transaction support for data store Sep 19, 2023
@krishnamiriyala krishnamiriyala changed the title Expose context based transaction support for data store Expose context based transaction support for data store #48 Sep 19, 2023
@krishnamiriyala krishnamiriyala changed the title Expose context based transaction support for data store #48 Expose context based transaction support for data store #49 Sep 19, 2023
@krishnamiriyala krishnamiriyala changed the title Expose context based transaction support for data store #49 Expose context based transaction support for data store Issue #49 Sep 19, 2023
@krishnamiriyala krishnamiriyala changed the title Expose context based transaction support for data store Issue #49 Expose context based transaction support for data store, fixes #49 Sep 19, 2023
@krishnamiriyala krishnamiriyala changed the title Expose context based transaction support for data store, fixes #49 Expose context based transaction support for data store Sep 19, 2023
…thods

- Currently transaction support is only available create/upsert methods.
- Update/Delete/Find still doesn't support them due to chaining of the
where clauses
@krishnamiriyala krishnamiriyala changed the title Expose context based transaction support for data store Support context based transaction for create/update only Sep 19, 2023
@krishnamiriyala krishnamiriyala merged commit 65e8a2f into main Sep 19, 2023
6 checks passed
@krishnamiriyala krishnamiriyala deleted the expose-transaction-support branch September 19, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants