fix: unclosed body causing resource leak

[jdockerty]

Proposed Changes

When the returned Response's body is nil, the caller of the function is expected to Close the body. If this is not done, it can lead to a resource leak.

Ref

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close.

This is also caught by a golangci-lint check of bodyclose, e.g. running from main:

git branch --show-current
main

golangci-lint run --enable-only bodyclose
influxdb3/management_cloud_decicated.go:146:31: response body must be closed (bodyclose)
        _, err = d.client.makeAPICall(ctx, param)
                                     ^
influxdb3/management_serverless.go:98:31: response body must be closed (bodyclose)
        _, err = c.client.makeAPICall(ctx, param)
                                     ^
influxdb3/write.go:168:24: response body must be closed (bodyclose)
        _, err = c.makeAPICall(ctx, httpParams{
                              ^

Other references:

• What if I don't close the response body?
• Go Response Body MUST be closed, even if you don't read it

This is mainly believed to effect a Clustered customer, but I've also addressed the lint elsewhere too for Cloud Dedicated/Serverless¹.

Checklist

• CHANGELOG.md updated
• Rebased/mergeable
• A test has been added if appropriate
• Tests pass
• Commit messages are conventional
• Sign CLA (if not already signed)

Footnotes

1. This also shows up in various tests, but I've opted to leave this for now because they are ephemeral compared to client usage. If needs be I think someone can address these in a follow-up PR. ↩

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.21%. Comparing base (e6c038d) to head (26b6c3b).
Report is 2 commits behind head on main.

Files	Patch %	Lines
influxdb3/management_serverless.go	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
- Coverage   85.16%   79.21%   -5.95%     
==========================================
  Files          14       14              
  Lines        1112     1121       +9     
==========================================
- Hits          947      888      -59     
- Misses        137      203      +66     
- Partials       28       30       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jdockerty]

I don't believe this has any behavioural change, i.e. the err is still returned when it occurs and resp.Body.Close also returns an error which upholds the return value for the write/createBucket/createDatabase functions. I'm not quite convinced the code coverage complaint here is giving us much.

I can reduce this down entirely to the Client.write if that helps. Happy to take suggestions on what to do for proceeding here though 👍

[bednar]

Thanks for your PR 👍

The failing code coverage check is due to our CI settings. Forked repositories do not run integration tests against InfluxDB Cloud to keep credentials secure. This will be addressed once InfluxDB Edge OSS is released.

Could you please update the CHANGELOG.md? After that, we'll be able to merge your awesome PR!

[bednar]

LGTM 🚀

I've prepared the integration of golangci-lint into our CI pipeline with PR #95. I'll merge this PR after we finalize #95.

[bednar]

The #95 is merged.

Please rebase your PR and add bodyclose to:

influxdb3-go/.golangci.yml

Lines 4 to 9 in 5b9dc92

	- errcheck
	- gosimple
	- govet
	- ineffassign
	- staticcheck
	- unused

[jdockerty]

@bednar Sure thing 👍

add bodyclose

As a small heads up for this. I raised this in the PR description, but this bodyclose lint is actually prevalent throughout the codebase, e.g. in many tests. If I add the bodyclose lint here, then it will fail until all instances of it are fixed.

Do we want to add those to this PR in that case or save the bodyclose CI addition for another PR?

Edit: It looks like I can add --tests=false to golangci-lint to solve that, if we're okay with it?

[bednar]

Edit: It looks like I can add --tests=false to golangci-lint to solve that, if we're okay with it?

Yes, it is good approach. Thanks @jdockerty

[jdockerty]

So I believe this is a bit of double-edged sword unfortunately.

Using --tests=false means that the *_test.go files are not being analysed, which means all of the bodyclose lints do not need to be addressed here; however, I believe this then has a knock-on effect such that it causes different lints to fail 💥

For example, by disabling the analysis of tests it looks like the setQueryClient is unused:

golangci-lint run -v --tests=false --config .golangci.yml
...snip
influxdb3/query.go:70:18  unused  func `(*Client).setQueryClient` is unused
1 issues:
* unused: 1

In actual fact this isn't the case though, it is used, but within tests 😄

rg "setQueryClient"
influxdb3/query_test.go
100:    c.setQueryClient(fc)
145:    c.setQueryClient(fc)

influxdb3/query.go
70:func (c *Client) setQueryClient(flightClient flight.Client) {

@bednar Any preference on how to proceed with this? I can either:

• Address all bodyclose lints in this PR (no need to introduce --tests=false)
• Don't introduce the bodyclose lint yet (not ideal as it is quite useful)
• Any other ideas?

[bednar]

@jdockerty, thank you for your efforts ❤️

I've disabled bodyclose from the CI checks, and we can consider re-enabling it in a subsequent PR or during the enhancement of the golangci-lint configuration as described here: Enhance golangci-lint Configuration.