-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: v2
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, 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 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)
//....
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @bxcodec, thanks for this architecture. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hi @ghostiam, Or anything? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It can be done without braking the "clean" rule, if you abstract a interface for repositories that has There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @henrylusjen do you have an example, a gist? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. Understood. Thanks. |
||
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") | ||
} |
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) | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use it like this, its more clear
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andpanicFunc
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.