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

[feature]:implement transaction #36

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion article/mocks/Repository.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions article/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

@bxcodec bxcodec Jan 15, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

@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.

Copy link

@nii236 nii236 Jan 28, 2020

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

}
27 changes: 26 additions & 1 deletion article/repository/mysql_article.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,23 @@ import (
"context"
"database/sql"
"encoding/base64"
"errors"
"fmt"
"time"

"github.com/sirupsen/logrus"

"github.com/bxcodec/go-clean-arch/article"
"github.com/bxcodec/go-clean-arch/models"
"github.com/bxcodec/go-clean-arch/types"
)

const (
timeFormat = "2006-01-02T15:04:05.999Z07:00" // reduce precision from RFC3339Nano as date format
)

type mysqlArticleRepository struct {
Conn *sql.DB
Conn types.DB
}

// NewMysqlArticleRepository will create an object that represent the article.Repository interface
Expand Down Expand Up @@ -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 {
Copy link
Owner

Choose a reason for hiding this comment

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

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)
//....
}

Copy link

@ghostiam ghostiam Jan 15, 2020

Choose a reason for hiding this comment

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

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).

Copy link
Owner

Choose a reason for hiding this comment

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

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

Or anything?

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

@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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

@henrylusjen do you have an example, a gist?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

if db, ok := m.Conn.(*sql.DB); ok {
tx, err := db.Begin()
if err != nil {
return err
}

defer tx.Rollback()

if err := f(&mysqlArticleRepository{Conn: tx}); err != nil {
return err
}

return tx.Commit()
}

if _, ok := m.Conn.(*sql.Tx); ok {
return f(m)
}

return errors.New("unknow DB type")
}
6 changes: 3 additions & 3 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ go 1.12
require (
github.com/BurntSushi/toml v0.3.1 // indirect
github.com/bxcodec/faker v1.4.2
github.com/davecgh/go-spew v1.1.0 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/go-playground/locales v0.12.1 // indirect
github.com/go-playground/universal-translator v0.16.0 // indirect
github.com/go-sql-driver/mysql v1.3.0
Expand All @@ -26,8 +26,8 @@ require (
github.com/spf13/jwalterweatherman v0.0.0-20180109140146-7c0cea34c8ec // indirect
github.com/spf13/pflag v1.0.1 // indirect
github.com/spf13/viper v1.0.2
github.com/stretchr/objx v0.1.0 // indirect
github.com/stretchr/testify v1.2.1
github.com/stretchr/objx v0.1.1 // indirect
github.com/stretchr/testify v1.2.2
github.com/valyala/bytebufferpool v1.0.0 // indirect
github.com/valyala/fasttemplate v0.0.0-20170224212429-dcecefd839c4 // indirect
golang.org/x/crypto v0.0.0-20180426230345-b49d69b5da94 // indirect
Expand Down
12 changes: 6 additions & 6 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
github.com/bxcodec/faker v1.4.2 h1:PlGLUcQ/yo/JUiwn3kUGnFkDbcv2o18oryc+ch+AkqY=
github.com/bxcodec/faker v1.4.2/go.mod h1:BNzfpVdTwnFJ6GtfYTcQu6l6rHShT+veBxNCnjCx5XM=
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/fsnotify/fsnotify v1.4.7 h1:IXs+QLmnXW2CcXuY+8Mzv/fWEsPGWxqefPtCP5CnV9I=
github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo=
github.com/go-playground/locales v0.12.1 h1:2FITxuFt/xuCNP1Acdhv62OzaCiviiE4kotfhkmOqEc=
Expand Down Expand Up @@ -51,10 +51,10 @@ github.com/spf13/pflag v1.0.1 h1:aCvUg6QPl3ibpQUxyLkrEkCHtPqYJL4x9AuhqVqFis4=
github.com/spf13/pflag v1.0.1/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4=
github.com/spf13/viper v1.0.2 h1:Ncr3ZIuJn322w2k1qmzXDnkLAdQMlJqBa9kfAH+irso=
github.com/spf13/viper v1.0.2/go.mod h1:A8kyI5cUJhb8N+3pkfONlcEcZbueH6nhAm0Fq7SrnBM=
github.com/stretchr/objx v0.1.0 h1:4G4v2dO3VZwixGIRoQ5Lfboy6nUhCyYzaqnIAPPhYs4=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.2.1 h1:52QO5WkIUcHGIR7EnGagH88x1bUzqGXTC5/1bDTUQ7U=
github.com/stretchr/testify v1.2.1/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/objx v0.1.1 h1:2vfRuCMp5sSVIDSqO8oNnWJq7mPa6KVP3iPIwFBuy8A=
github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1w=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/valyala/bytebufferpool v1.0.0 h1:GqA5TC/0021Y/b9FG4Oi9Mr3q7XYx6KllzawFIhcdPw=
github.com/valyala/bytebufferpool v1.0.0/go.mod h1:6bBcMArwyJ5K/AmCkWv1jt77kVWyCJ6HpOuEn7z0Csc=
github.com/valyala/fasttemplate v0.0.0-20170224212429-dcecefd839c4 h1:gKMu1Bf6QINDnvyZuTaACm9ofY+PRh+5vFz4oxBZeF8=
Expand Down
15 changes: 15 additions & 0 deletions types/db.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package types

import (
"context"
"database/sql"
)

type DB interface {
Query(query string, args ...interface{}) (*sql.Rows, error)
QueryContext(ctx context.Context, query string, args ...interface{}) (*sql.Rows, error)
Prepare(query string) (*sql.Stmt, error)
PrepareContext(ctx context.Context, query string) (*sql.Stmt, error)
Exec(query string, args ...interface{}) (sql.Result, error)
ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error)
}