qustavo / sqlhooks

Attach hooks to any database/sql driver
MIT License
652 stars 44 forks source link

Return an ExecerContext if conn is an execer #14

Closed keegancsmith closed 7 years ago

keegancsmith commented 7 years ago

A driver.Conn can optionally implement driver.ExecerContext or driver.Execer. When they do, instead of creating a prepared statement and then executing that database/sql will directly call ExecContext or Exec respectively. We need to implement this since some drivers do not support all statements being prepared first. For example in postgresql you can run multiple statements in a conn.Exec, but it will fail if you try to prepare multiple statements in one call.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-13.2%) to 68.794% when pulling 4e5759f919a3b3cc9ce087478e4d993263fe01b7 on keegancsmith:execer into 240ea3c2db8117533ae62819b8f47376c800264a on gchaincl:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-16.5%) to 65.541% when pulling b4e9f9054f05abeebe9df6a3642d9458d84c8135 on keegancsmith:execer into 240ea3c2db8117533ae62819b8f47376c800264a on gchaincl:master.

qustavo commented 7 years ago

Looks like a great improvement, do you think you can add a test to cover namedValueToValue behavior?

keegancsmith commented 7 years ago

@gchaincl I can't test this via the existing unit test, since all of the tested drivers implement ExecerContext. So I pushed a commit which directly tests the function. Note the function is directly copied from the implementation used by database/sql.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-12.4%) to 69.595% when pulling 9c1df7d136ceb6a5dfbf67b31262d7998676907d on keegancsmith:execer into 240ea3c2db8117533ae62819b8f47376c800264a on gchaincl:master.

qustavo commented 7 years ago

I see what you mean, anyway, do you know any driver which implements the driver.Execer|Context interface?

keegancsmith commented 7 years ago

postgres atleast does, because I need this change otherwise I can't instrument my code. Here is the link of it being used https://github.com/lib/pq/blob/8837942c3e09574accbc5f150e2c5e057189cace/conn_go18.go#L33

We may want to add go version guards around this? ExecContext seems to be for go >= 1.8 But implementing an interface should just be ignored in older versions if I am not mistaken.

qustavo commented 7 years ago

If postgres does, can u exercise the behavior here: https://github.com/gchaincl/sqlhooks/blob/master/sqlhooks_postgres_test.go ?

qustavo commented 7 years ago

Otherwise, I'll just merge this and try to add a test in the future.

keegancsmith commented 7 years ago

It already is being exercised https://github.com/gchaincl/sqlhooks/blob/master/sqlhooks_test.go#L92

Before this change database/sql translated that call into a Prepare followed by Exec on the statement. Now that conn implement ExecContext, it will just directly call the code I added. Check the code coverage and you will see that it is being exercised.

qustavo commented 7 years ago

I see what you mean, apparently what's not being exercised is driver.Exec which calls namedValueToValue, but that's ok. Please feel free to merge!

qustavo commented 7 years ago

Wonderful, thanks!