-
Notifications
You must be signed in to change notification settings - Fork 11
Conversation
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.
team, what's your opinion?
adapter.go
Outdated
"net/http" | ||
"net/url" | ||
) | ||
|
||
//Adapter interface | ||
type Adapter interface { | ||
GetTx() (tx *sql.Tx, err 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 prefer explicit name, example GetTransaction
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.
+1 to GetTransaction
Personally I don't see any advantage to run single SQL commands inside a explicit transaction block... in PostgreSQL when you execute, for example an INSERT, it will be executed inside a implicit transaction. Actually all statements runs inside transactions in PostgreSQL. Implement it in this way just add unnecessary complexity and more round trips on code. |
The implementation is to perform multiple actions on a transaction.
|
Rename from GetTX to GetTransaction. |
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.
followed pattern names (get like):
- InsertTx > InsertTransaction
- UpdateTx > UpdateTransaction
- DeleteTx > DeleteTransaction
@avelino |
My opinion Team help pls |
I also prefer InsertWithTransaction. |
IMHO I didn't like the idea of create other DML methods for transaction... is more simple create new methods just for BEGIN, COMMIT and ROLLBACK and then internally perform the properly checks to make sure we're inside a transaction... or maybe I missed something. |
@fabriziomello |
@fabriziomello
|
You don’t need break signatures. Maybe using some static variable to store
sql.Tx should be enough to control if you’re inside a transaction or not.
Em ter, 4 de set de 2018 às 17:46, Joel M. de Souza <
notifications@github.com> escreveu:
@fabriziomello <https://github.com/fabriziomello>
Example:
func (adapter *Postgres) Delete(SQL string, params ...interface{}) (sc adapters.Scanner) {
db, err := connection.Get()
if err != nil {
log.Println(err)
sc = &scanner.PrestScanner{Error: err}
return
}
sc = adapter.delete(db, nil, SQL, params...)
return
}
func (adapter *Postgres) DeleteTx(tx *sql.Tx, SQL string, params ...interface{}) (sc adapters.Scanner) {
sc = adapter.delete(nil, tx, SQL, params...)
return
}
func (adapter *Postgres) delete(db *sqlx.DB, tx *sql.Tx, SQL string, params ...interface{}) (sc adapters.Scanner) {
... implementation ...
}
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAlYgv9V5SGlLsDaKFi4hBJmx-uYvcnpks5uXuawgaJpZM4WT4FJ>
.
--
Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
|
Passing the transaction explicitly is more versatile and gives you more control because you have control over which transaction you are currently executing. What is the opinion of others on this issue? |
Create a method called SetTransaction then, or maintain a internal list of
opened transactions. But seems we’re complicating things.
My main concern is because in SQL we don’t have different DML for implicit
or explicit transactions, we have just one Insert, Delete and Update. So
seems natural we implement it in that way IMHO.
Em ter, 4 de set de 2018 às 19:10, Joel M. de Souza <
notifications@github.com> escreveu:
Passing the transaction explicitly is more versatile and gives you more
control because you have control over which transaction you are currently
executing.
I do not like the idea of using a global transaction.
Using a local transaction makes the scope of actions better delimited.
What is the opinion of others on this issue?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAlYghr-hL9B-U9j8dHTeEaT86Yxtf0cks5uXvpLgaJpZM4WT4FJ>
.
--
Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
|
I renamed the methods in the Postgres adapter and added the methods in the Mock adapter. |
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.
the code is fine, but I will document why I do not like this approach because we would have to create new endpoints for transaction actions.
What I would do is create 3 new endpoints open transaction that replies with UUID only for the transaction and another to commit and another to abort, if we pass? _TxID = "123" it would use the existing transaction otherwise it would continue as it is today
@felipeweb
|
@@ -54,7 +54,7 @@ type Stmt struct { | |||
} | |||
|
|||
// Prepare statement | |||
func (s *Stmt) Prepare(db *sqlx.DB, SQL string) (statement *sql.Stmt, err error) { | |||
func (s *Stmt) Prepare(db *sqlx.DB, tx *sql.Tx, SQL string) (statement *sql.Stmt, err 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.
Changed function signature, you need open PR (another repo) upgrading where you use the adapter
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.
This function is only used inside the adapter package.
The other packages in the pRest project do not access it.
Implements transaction support, at the packet level.
Implemented only for Delete yet so we can discuss the way forward.
@felipeweb @crgimenes @avelino