Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ See [documentation](https://clickhouse.com/docs/integrations/go#the-clickhouse-g

The linter detects 2 common implementation mistakes:
- forgetting to call `rows.Err()` after calling `rows.Next()`
- forgetting to call `defer batch.Close()` when using `Batch`
- forgetting to call `defer xxx.Close()` when using `Batch` or `Rows`

These implementation mistakes are often observed in codebases using the clickhouse-go driver. They result
in incomplete data and hard-to-troubleshoot bugs.
Expand Down Expand Up @@ -55,6 +55,7 @@ Incorrect code:
```go
parsedRows := []...
rows, _ := conn.Query(ctx, "SELECT ...")
defer rows.Close()
for rows.Next() {
parsedRow, err = ....
if err {
Expand All @@ -74,6 +75,7 @@ return parsedRows, nil
```go
parsedRows := []...
rows, _ := conn.Query(ctx, "SELECT ...")
defer rows.Close()
for rows.Next() {
parsed, err = ....
if err {
Expand Down Expand Up @@ -130,8 +132,10 @@ There are some limitations:
--> in this case the linter will not be able to detect that `.Err()` may be called too late.


## 2. chbatchclose
Detect when a `github.com/ClickHouse/clickhouse-go/v2/lib/driver` `Batch` variable is used but no associated `defer batch.Close()` is called.
## 2. chclose
Detect when a `github.com/ClickHouse/clickhouse-go/v2/lib/driver`: `Batch` / `Rows` variable is used but no associated `defer xxx.Close()` is called.

*All examples below are given for `Batch` but apply the same way for `Rows`.*

Calling `defer batch.Close()` is highly recommended (see [here](https://clickhouse.com/docs/integrations/go#batch-insert)).
While it is possible to not use this statement, code that do not use it often end up with subtle leaks, as the
Expand Down Expand Up @@ -180,15 +184,21 @@ return batch.Send()

### Rule details

*All examples below are given for `Batch` but apply the same way for `Rows`.*

The linter goes through every function.
If a clickhouse driver `Batch` is instantiated and is not part of the values returned by the function, a `defer batch.Close()` must be found.
Also, assigning a `Batch` to the blank identifier `_` is flagged.
Variable re-assignments and intertwined variable are supported. See [testcases.go](passes/chbatchclose/testdata/src/testcases/testcases.go).
Variable re-assignments and intertwined variable are supported. See [testcases.go](passes/chclose/testdata/src/testcases/testcases.go).

There are some limitations:
- except for looking into defer blocks;, the linter does not cross function block boundaries. If a `Batch` variable is instantiated and `batch.Close()` is called in a
closure inside the defer call, the linter will not be able to associate the `batch.Close()` to the variable.
(note: in most cases such pattern is a bad idea). See `deferCloseIsInClosure` test case.
- linting of structs wrapping or embedding a `Batch` is not supported
- tracking across function calls is not supported:
- if a `Batch` variable is instantiated and returned by a function call `return logAndReturn(batch)`,
the linter cannot know the batch is part of the values returned.
- If a `Batch` variable is instantiated and `batch.Close()` is called in a
closure inside the defer call, the linter will not be able to associate the `batch.Close()` to the variable.
(note: in most cases such pattern is a bad idea). See `deferCloseIsInClosure` test case.
- `defer batch.Close()` must be called after checking that the `PrepareBatch` call returned no error.
incorrect:
```
Expand Down
4 changes: 2 additions & 2 deletions main.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package main

import (
"github.com/ClickHouse/clickhouse-go-linter/passes/chbatchclose"
"github.com/ClickHouse/clickhouse-go-linter/passes/chclose"
"github.com/ClickHouse/clickhouse-go-linter/passes/chrowserr"
"golang.org/x/tools/go/analysis/multichecker"
)

func main() {
multichecker.Main(chrowserr.NewAnalyzer(), chbatchclose.NewAnalyzer())
multichecker.Main(chrowserr.NewAnalyzer(), chclose.NewAnalyzer())
}
182 changes: 0 additions & 182 deletions passes/chbatchclose/chbatchclose.go

This file was deleted.

13 changes: 0 additions & 13 deletions passes/chbatchclose/chbatchclose_test.go

This file was deleted.

Loading