[feature]:implement transaction #36

philchia · 2020-01-14T09:30:10Z

No description provided.

bxcodec · 2020-01-14T12:19:24Z

article/repository/mysql_article.go

@@ -213,3 +215,26 @@ func EncodeCursor(t time.Time) string {

 	return base64.StdEncoding.EncodeToString([]byte(timeString))
 }
+
+func (m *mysqlArticleRepository) WithTransaction(f func(repo article.Repository) error) error {


Hi @philchia , thanks for the PR.

I have a question, have you tried with context?

I have another approachment,
I'm using context, then insert the tx to Context.

So generally, my function will look like this,

func (m * mysqlArticleRepository) WithTransaction(ctx context.Context, fn func(c context.Context) error) (err error) { tx, err := m.DB.BeginTx(ctx, nil) if err != nil { return } _, err = tx.ExecContext(ctx, "SET TRANSACTION ISOLATION LEVEL READ COMMITTED") if err != nil { return } c := context.WithValue(ctx, "TransactionContextKey", tx) err = fn(c) if err != nil { if errTX := tx.Rollback(); errTX != nil { logrus.Error("failed to rollback transaction", errTX) } return } if errTX := tx.Commit(); errTX != nil { logrus.Error("failed to commmit transaction", errTX) } return }

Usage in usecase/service:

err = s.repo.WithTransaction(ctx, func(c context.Context) (err error) { err = s.repo.Create(c, article) if err != nil { return } err = s.authorRepo.Create(c, author) return })

But, the cons, maybe a bit redundant jobs, we need to check the tx in ctx.
Like this.

func getTransactionFromCtx(ctx context.Context) (*sql.Tx, bool) { tx, ok := ctx.Value("TransactionContextKey").(*sql.Tx) return tx, ok } func (m *mysqlArticleRepository) Insert(ctx context.Context, article Article)error{ var tx *sql.Tx tx, isTxFromCtx := getTransactionFromCtx(ctx) if !isTxFromCtx { tx, err = r.DB.BeginTx(ctx, nil) if err != nil { return } } tx.ExecContext(ctx, "INSERT article SET title=?", article.Titlle) //.... }

Hi @bxcodec, thanks for this architecture.
But I would like to object to the use of context to convey something not explicitly.
I think it’s worthwhile either to wrap the transaction (which is suggested in this PR), or to transfer the transaction explicitly.
Also, as an option, it may just be worth creating a method in which there will be a transaction inside (for example CreateArticleWithAuthor).

hi @ghostiam,
Have you tried with the submitted PR, is there any issue so far?
Maybe related to memory and performance?

Or anything?

Transferring transaction explicitly is something I don't want to do ideally.

Since, the use case layer (who will use the repository), shouldn't know anything about the transaction. It's only know about calling repository.

What if, the repository using NoSQL, which is doesn't support transactions?
Let's say like MongoDB in the early stage, that still doesn't support transaction?

There are plenty of possible business reasons why you would need to batch multiple repositories into one transaction though. By not passing the tx in explicitly, you will prevent the developer from being able to do this.

NoSQL and SQL databases are pretty different, if you want to have both persistence layers be supported by the repository pattern, you'll only be able to use the features that are available to both types of databases, meaning you won't be able to support transactions.

@nii236 I also agree here. But this "violates" the "clean" architecture since the use case layer knows about the implementation details.

It can be done without braking the "clean" rule, if you abstract a interface for repositories that has Begin(), Commit(), Rollback().
Doing this, the use case layer won't know any details about how repositories implement, and still has the power to control transaction.

My 2cents. I fully agree with @bxcodec. Usecases should not know about transactions at all. Tx are a repo thing.

@henrylusjen do you have an example, a gist?

My 2cents. I fully agree with @bxcodec. Usecases should not know about transactions at all. Tx are a repo thing.

I want to point out that we are talking about two different kinks of the transaction. One is the "DB" transaction, another one is business logic transaction (multiple work should do together).

Yes, usecase should not know about "DB" transaction at all. But like @nii236 said, it should have the power to raise "Business" transaction, and it's much complicated because it usually involve multiple repos and sometimes MQ or external service.

There's a solution called "UnitOfWork", but golang doesn't have any ORM support it yet. You might have to build it yourself.

OK. Understood. Thanks.
What do you think about frederikhors@2ec0f2d?

bxcodec · 2020-01-14T12:20:03Z

article/repository.go

@@ -14,4 +14,5 @@ type Repository interface {
 	Update(ctx context.Context, ar *models.Article) error
 	Store(ctx context.Context, a *models.Article) error
 	Delete(ctx context.Context, id int64) error
+	WithTransaction(func(Repository)error) error


I wonder, how we use this in usecase/service layer?

err = s.repo.WithTransaction( func(repo aritcle.Repository) error { if err := repo.Create(c, article); err != nil { return err } return nil })

use it like this, its more clear

Yeah... I see this more simple.
But how do you handle like transaction across repositories.

Right now, it's okay since it's still the same article (in one repository)
How about across to another repository, like to author repository, or maybe publisher repository, category repository?

Let's say, I want to store article, including the category, the author, then the publisher?

I think one transaction should only change one aggregate root, and we should have only one repo for one aggregate root.
if your operation across other aggregate root, try eventual consistency instead of strong consistency

By saying that, so will you use a pub/sub pattern for that from the app level. Or from infra levels like using 3rd party message broker, and pubsub?

@r3code
This is new. This will decouple everything. Thanks for the idea.

What do you think @philchia?
*anyway, I'll keep this PR open. Since there's a lot of options to do the transactions here. If someone in the future has another idea, just keep posting on this PR. Just let's make this as a knowledge base.

How would you use the same transaction across multiple repositories?

The way the okFunc and panicFunc works in the linked test is that it runs the repository func with the transaction then commits or rollbacks based on a panic after only one query.

An ideal implementation would allow the transaction to be committed or rolled back after multiple uses across multiple repositories, at the discretion of the developer.

I also got some inspiration from https://medium.com/@jfeng45/go-microservice-with-clean-architecture-transaction-support-61eb0f886a36,

I really liked it where it states that transaction logic should be applied to the use case layer, and not the persistence layer.

Basically, what he did is to create another wrapper around sql.DB and sql.Tx

https://gist.github.com/adamfdl/49da2b850991c59939da2426fa85c1ab

Thanks for the idea @adamfdl

I'm thinking of a general-purpose of architecture here. Luckily you use SQL which RDBMS here that supports the transaction features so that we can wrap the sql.DB and sql.Tx

But for RDBMS, I think this a good solution.

@adamfdl can you post a more complete example, please? How to use Wrap() in a usecase for example? Thanks, really.

frederikhors · 2020-11-15T14:08:39Z

@bxcodec I love Golang and your project. Is there any news on this? No pressure. 😄

bxcodec · 2020-11-15T15:26:34Z

Hi @frederikhors ,

So yeah, I keep this PR open, so everyone can see all the opinions by the other.

I want to make this PR just like a reference and any new golang engineer can choose which evil side they prefer. Every opinion is valid from anyone's perspective. So since this a public PR, I don't wanna choose one and merge it, unless if it's for my private project I'll choose one and convince my team about it.

frederikhors · 2020-11-15T15:52:10Z

@bxcodec thanks for your answer.

Since I don't know how to decide (because I'm newbie and because they all seem not 100% correct to me) I can ask you which method do you prefer?

I need to use a transaction:

in a one usecase method that uses multiple repositories methods
in a single method of a single repository (but here I can also do it manually)

frederikhors · 2020-11-16T00:56:39Z

@bxcodec what do you think about this?

frederikhors@2ec0f2d

I don't know how to do this:

//I can also create an helper like this:
//dbConn := db.GetTxFromCtxOrUseDBConn(ctx, m.Conn)
//instead of the below:
var dbConn interface{} // I think I need an interface for *sql.DB & *sql.Tx here
tx, ok := repository.GetTransactionFromCtx(ctx)
if ok {
    dbConn = tx
} else {
    dbConn, _ = m.Conn.BeginTx(ctx, nil)
}

Is that what you thought? To use context like this to handle transactions?

nii236 · 2020-11-23T04:50:42Z

If you have to use transactions across multiple repositories, it doesn't make sense to put it behind an interface anyway. Unless you'll only be dealing with different flavours of SQL databases.

frederikhors · 2020-11-23T10:45:20Z

@nii236 I don't understand. Can you please explain it better? Thanks.

masajip · 2023-01-26T09:33:55Z

@nii236 I don't understand. Can you please explain it better? Thanks.

I would try to explain what is meant by @nii236 , the idea of defining a repository as an interface is because we're trying to make it pluggable/changeable by following the principle of Dependency Inversion, but if we use a transaction moreover if it's across multiple repositories, it would be impossible for our system to change our dependency from a SQL database into another kind of repository (NoSQL, Rest API, GRPC, or any other kind) because the concept of atomic transaction is only exist on SQL Database, then if we define those repository as an interface it would be useless because we're preparing for something that would never happen

nii236 · 2023-01-30T04:14:08Z

This blog post explains my thinking: https://threedots.tech/post/common-anti-patterns-in-go-web-applications/

Scroll down to "Starting with the database schema".

[feature]:implement transaction

62ac34b

bxcodec reviewed Jan 14, 2020

View reviewed changes

bxcodec changed the base branch from master to v2 April 18, 2020 05:51

bxcodec mentioned this pull request May 15, 2020

How to use transactions in the Service layer to wrap different Repository methods? #45

Closed

bxcodec mentioned this pull request Aug 4, 2022

How to handle multiple Repository transactions #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature]:implement transaction #36

[feature]:implement transaction #36

philchia commented Jan 14, 2020

bxcodec Jan 14, 2020

ghostiam Jan 15, 2020 •

edited

Loading

bxcodec Jan 15, 2020

bxcodec Jan 15, 2020

nii236 Jan 28, 2020

henrylusjen Sep 29, 2020 •

edited

Loading

frederikhors Oct 30, 2020

frederikhors Nov 15, 2020

henrylusjen Nov 19, 2020

frederikhors Nov 21, 2020

bxcodec Jan 14, 2020

philchia Jan 15, 2020

bxcodec Jan 15, 2020 •

edited

Loading

philchia Jan 16, 2020

bxcodec Jan 18, 2020

bxcodec Jan 28, 2020

nii236 Jan 28, 2020 •

edited

Loading

adamfdl Jan 28, 2020

bxcodec Jan 28, 2020

frederikhors Nov 15, 2020

frederikhors commented Nov 15, 2020

bxcodec commented Nov 15, 2020

frederikhors commented Nov 15, 2020 •

edited

Loading

frederikhors commented Nov 16, 2020

nii236 commented Nov 23, 2020

frederikhors commented Nov 23, 2020

masajip commented Jan 26, 2023

nii236 commented Jan 30, 2023 •

edited

Loading

[feature]:implement transaction #36

Are you sure you want to change the base?

[feature]:implement transaction #36

Conversation

philchia commented Jan 14, 2020

Choose a reason for hiding this comment

ghostiam Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrylusjen Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bxcodec Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nii236 Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikhors commented Nov 15, 2020

bxcodec commented Nov 15, 2020

frederikhors commented Nov 15, 2020 • edited Loading

frederikhors commented Nov 16, 2020

nii236 commented Nov 23, 2020

frederikhors commented Nov 23, 2020

masajip commented Jan 26, 2023

nii236 commented Jan 30, 2023 • edited Loading

ghostiam Jan 15, 2020 •

edited

Loading

henrylusjen Sep 29, 2020 •

edited

Loading

bxcodec Jan 15, 2020 •

edited

Loading

nii236 Jan 28, 2020 •

edited

Loading

frederikhors commented Nov 15, 2020 •

edited

Loading

nii236 commented Jan 30, 2023 •

edited

Loading