Fix io.Closer support for connectors #199
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
=======================================
+ Coverage 80.5% 80.7% +0.1%
=======================================
Files 13 13
Lines 716 723 +7
=======================================
+ Hits 577 584 +7
Misses 115 115
Partials 24 24 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @begelundmuller, thanks for raising this PR. It looks good.
Please also update the CHANGELOG.md to reflect this fix.
| func (c *otConnector) Close() error { | ||
| // database/sql uses a type assertion to check if connectors implement io.Closer. | ||
| // The type assertion does not pass through to otConnector.Connector, so we explicitly implement it here. | ||
| if closer, ok := c.Connector.(io.Closer); ok { |
There was a problem hiding this comment.
Could you please add an implement assertion in line 26 to show otConnector implements io.Closer?
https://github.com/XSAM/otelsql/pull/199/files#diff-c1caa8de39e277339469495d65efd1a363c54065c3cc02e7422b097774e56c86R25
It should be
var _ io.Closer = (*otConnector)(nil)
|
@XSAM thanks for the rapid review! I have addressed both of your comments. |
|
@begelundmuller Thanks for your contribution. Please update this branch with the base branch. |
Done! |
It appears that closing a DB using
XSAM/otelsqldoes not always propagate to the underlying driver. That's apparently due toio.Closerbeing optional for connectors anddatabase/sqlusing a type assertion to check whether the connector implements it. That type assertion does not pass through tootConnector.Connector, so we need to explicitly implementClose() erroronotConnectorand replicate the type assertion there.