[go-nuts] [ANNOUNCE] sqlcommand - a wrapper lib for database/sql to reduce the amount of boilerplate

1) You have the result of test coverage run committed to the repository.

2) What's the reason of swapping arguments to the comparison operator?

    Unlike in C, in Go the assignment statement is not an expression
    so there's no point in writing

    if nil == foo ...

    as in Go the statement

    if foo = nil ...

    (with mistyped "==") won't compile anyway so this hackish "safety
    net" is not needed.

    AFAIK, gofmt does not enforce a particular style with regard to this
    issue but I think you'd have hard time trying to find something like
    this in the Go standard libraries (which are considered to be the
    reference when it comes to style).

1. Thanks, I got so excited playing with the coverage tool I forgot to add
it to .gitignore
2. It's a habit I got into when coding in languages that benefited from it.
There's no downside to Yoda Comparisons, and Go doesn't specify which
argument comes first in a comparison. Feel free to fork the repo and change
it if you prefer them the other way around ;)

Thanks for the feedback

I don't see the benefit. Sure, you'll save a couple of lines of code but at
the expense of introducing a code style that's different than the rest.
Most of the SQL boilerplate I've had in my code was in the row mapper
function which does the Scan and constructs an object. Other than that the
plain database/sql commands are simple and readable enough IMHO.

Fair enough. I found I was endlessly writing the same code to set up
transactions, prepare statements, etc. and wanted a cleaner, DRY-er way to
do it.

I don't see the code style being different? apart from the yoda
comparisons, that is. What's the difference that's sticking out?

You'll find this group is largely hostile to the chained
blah().blah().blah() syntax style of library you're using, hwich is what
Frank is probably referring to.

Not everyone feels that way though; interesting library, I'll give it a try
this week and see how it fares against real-world-problems~

~
Doug.

It's not about hostility. It's about the fact that such chaining makes
little sense in a language which handles errors explicitly and in the place
(precise context) where it (the error) happened - to handle it in the best
possible way (retry/recover/add error context info/...). The chaining also
disallows using any multiple valued function, however such functions are
common in Go.

-j

This is a valid point, but there's another angle to look at this
problem: quite a number of tasks, including (that's a coincidence)
database access is best described by "all or nothing" approach, that
is, some multi-step operation either must complete as a whole or it
should fail, and we don't care where exactly it failed. To me, simple
database access fits this model rather well: typically I don't care
which exactly step of a multi-step database operation failed: all I
need to know is *whether* something had failed, and if so, I have to
roll back the controlling transaction. That's the case this library
provides for: I might simply "chain" the steps and check for an error
at the end -- it doesn't really matter if the chain failed on its first
step, the last, or somewhere in the middle -- all I need is to roll
back the active transaction if the chain failed and then somehow
devlier the error to the user (or may be log it and/or retry the task
later if it's an unattended program).

There's still one problem with this approach -- and generally with
exceptions, as we know them from other languages -- it might be hard to
know exactly which step failed, if it's needed at the "catching" spot.
On the other hand, a typical approach to write such a "chaining" code in
go is to do

v, err = step1()
if err != nil {
   return ..., err
}
...
v, err = stepN()
if err != nil {
   return ..., err
}

which suffers from exactly the same problem anyway.
This is a very valid point.

Except that it doesn't. Afaict this library doesn't support multi-statement
transactions - and quite fundamentally at that (it's architected as a
statement factory). That effectively renders useless any proported
transactional capability of the library.

Without multi-statement transactions, its applicability for any serious
database applications is severely limited.

yeah, I looked at doing multiple sql statements, but it actually didn't fit
the pattern too well and felt clunky. You can use the Tx value from the
sqlcommand to run other sql statements against, but obviously there's no
real benefit to using the lib in that circumstance. I'd like to find a way
of chaining multiple statements together so the same 'all or nothing'
approach works.

and yes, the 'all or nothing' approach was where I came from... I found
myself endlessly doing:
if nil != err{
  return nil, err
}
and then handling the error in the parent as 'the database operation
failed'. That seemed a bit pointless.

Yeah it is, and one of Go's best features IMHO.

It is a disadvantage of the chaining style/pattern, and I could refactor to
the lib to use a nested style instead which allows this.

Valid points; I believe you forgot to mention the awkward syntax for
multi-line statements;

k, e = thing.blah().blah().
           blah().
           blah().

That's the other reason I've seen mentioned several times here before for
why it's a bad idea to use.

Opinions differ; I'm with the author on this one, I don't see why you can't
have a stateful transaction object that keeps track of it's own error state
and pending operations; and honestly, any ideas to improve working with the
database/sql library is welcome as far as I'm concerned.

I'd rather people played with ideas like this (and the various orms
floating about) to see what sticks than that no one did anything to improve
the current situation.

~
Doug.

I only took a quick glance at the code and it seems to be doing things at
the wrong level. What I mean is that, the package is only there to shorten
the syntax, and doesn't do anything else. Well mostly it just puts the
syntax on one line.

To me the better way would seem to extract the database logic from the rest
of the code and then use it as a service to do the stuff. If needed most of
the SQL / Get / Update could be generated automatically.

e.g. this is a library that would make code simpler:
err := repo.Foo.GetById(123, &foo)
if err != nil {
   // bad thing happened
}

as compared to:
sc := scf.NewCommand("SELECT bar, qux FROM foo WHERE id=?')
if
!sc.Begin().Prepare().QueryRow(idValue).Scan(&resultBar,&resultQux).CloseAll().IsValid(){
   //bad thing happened
}

Also I cannot understand why your go example is using transaction when they
weren't required? Something like this would be more realistic:

func main() {
db, err := sql.Open(driver, datasource)
if err != nil {
panic("bad thing happened")
}
  rows, err := db.Query(sqlStatement, value1, value2, value3)
if err != nil {
panic("query failed...")
}

results := make(map[int]Foo)
for rows.Next() {
foo := Foo{}
if err := rows.Scan(&foo.Id, &foo.Name); err != nil {
panic("wrong arguments while iterating...")
}
results[foo.Id] = foo
}

if err := rows.Err(); err != nil {
panic("err while iterating...")
}
}

I also would recommend against using Yoda comparisons, although you are
free to use whatever style you want, Go as a community has mostly accepted
one style that is defined by the "go fmt" and the std packages. To me, if
some package code doesn't use that style, I consider that to be of poor
quality... mainly because the maintainer doesn't take into account other
people trying to use and/or understand the library. (Of course, I can
understand that people come from different backgrounds, so their initial
style will vary.)

+ egon

After thinking how I would do something like this I came up with a different style... http://play.golang.org/p/1p12fqMWQE

It's just the gist of it because typing code on a tablet is extremely painful. Essentially make a transact function that takes a closure, which takes an transaction as an argument. The transaction would wrap all tx stuff with panicky versions of them (same result... if error yhen call the Txx.Abort) and the transact function itself would catch them. Essentially in the closure you write the happy way, ignoring all the errors. If you indeed get an error it'll just panic and abort+rollback the tx.

Also this would allow writing functions for elements e.g. func(f *Foo) Save(tx *Txx) ... and then use it like Transact(db, somefoo.Save).

Although this may have problems, because if something else panics in that code you'll get wrong stack traces. And potentially some other faults.

+egon

I currently use something like this for transactions:

func InTx(db *sql.DB, fn func(*sql.Tx) error) error {
tx, err := db.Begin()
if err != nil {
return err
}
defer func() {
if err != nil {
tx.Rollback()
}
}()
err = fn(tx)
if err != nil {
return err
}
return tx.Commit()

}

If there is an error with a query or any other processing the function fn()
just returns it and that causes the transaction to roll back. I suppose I
should handle panics in there as well, which I believe another
implementation of the same code that I left at my previous employer was
doing.

That's a nice pattern, and I looked at it as an alternative, but it got a
bit tricky with the nested function calls:
Begin(Prepare(Query(values,Commit()))) just didn't seem as easy to grok as
Begin().Prepare().Query(values).Commit()

The usage would be more like: http://play.golang.org/p/iM_-5w9M31

Of course there you have the problem, that when something is using recover
inside the transaction handler, then it won't work properly; so there
should be some additional logic inside Tx to check for "recovered aborts".

Also I'm not quite sure how well the whole example work in large
codebase... i.e. it feels a little iffy to use panic/recover to do this
sort of thing. But, if it's for some small pet-projects then it's probably
fine. Benjamin-s version is probably cleaner and safer, but would still
require the "return err"-s.

+egon

Nice, yes, that's interesting.

It gets a bit more tricky because if you've got a statement, the Exec and
Query functions work on the statement not the transaction, but a queryable
interface will solve that. I'm going to play with this and see what
happens. Thanks for the feedback!

I omitted the code for the wrapping of Stmt etc. but essentially just throw
an Abort in the same way from Stmt.Exec and Stmt.Query... and all the
other... Rows, Row, Result.

I.e. let wrapped Tx return a wrapped Stmt... and etc.

Which code patterns do you have to write that often? I'd like to understand the problem you're trying to solve.

Frank Schröder

I like keeping my code files small if I can, and I was finding that anytime
I went near db access it bloated.
I was also writing the same code over and over again, to set up
transactions, prepare statements, and scan results. Scan in particular was
getting old fast.

It just seemed that Go is this lovely, succinct, terse language that had
this big bloaty primitive data access library. Don't get me wrong, I love
bits of it, particularly its simplicity, but there had to be a better way
to do this ;)

The problem that I see (and was trying to solve) is that there's a lot of
repetitive code involved in database/sql. This comes down to two things (to
my mind, and I'm open to being wrong):
1. The error checking at each step, when 90% of the time I only cared if
the whole transaction succeeded. Every single error handling statement read
"if nil != err{ return nil, err}" which seemed pointless (although very
idiomatic).
2. The reasonably primitive methods, especially around Scan. I'm not
criticising here; I like a library that operates about this close to the
metal. But it seemed to me that every user would be rolling their own
wrapper or higher-level abstraction for this library (or using an ORM,
which I'm not a fan of for reasons that are irrelevant here).
I ended up using the same wrapper for multiple projects and figured I'd
tidy it up and offer it back to the community. Been an interesting journey
;)

I've started looking at refactoring it a bit to remove the chaining (which
doesn't support multiple function returns) and replace it with nested
function calls (which does). Some really good feedback on that from people
here (thanks!)
But the core idea; of allowing the database operations to be called as a
single operation and only error-handled at the end (and making Scan
easier/terser to use) still has merit I think. I'll keep working on it...