Fix panic on format nil *fmt.Stringer type value #1200

zaneli · 2024-02-24T13:41:17Z

Summary

Currently, if the pointer to the struct that implements fmt.Stringer is nil, the bind method will panic.
This PR fixes that.

Checklist

Delete items not relevant to your PR:

Unit and integration tests covering the common scenarios were added
A human-readable description of the changes was provided to include in CHANGELOG
- Is this what I should do in this PR?
~~[ ] For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials~~

CLAassistant · 2024-02-24T13:41:22Z

All committers have signed the CLA.

zaneli · 2024-02-25T12:47:30Z

bind.go

+ if v := reflect.ValueOf(v); v.Kind() == reflect.Pointer &&
+ v.IsNil() &&
+ v.Type().Elem().Implements(reflect.TypeOf((*fmt.Stringer)(nil)).Elem()) {
+ return "NULL", nil


I made a similar check using database/sql as a reference.
This implementation is also copied go-sql-driver/mysql.
Should I write a source code comment mentioning the reference source?

https://cs.opensource.google/go/go/+/6ba6e72e39ac5be92f604832640c3131ad3d0ade:src/database/sql/driver/types.go;l=217-235

https://github.com/go-sql-driver/mysql/blob/097fe6e3ad83bbd7c84debe810aec4c4a533bcaa/statement.go#L199-L217

zaneli · 2024-02-26T00:46:14Z

tests/issues/1200_pr_test.go

+ },
+ {
+ name: "fmt.Stringer implemented struct typed-nil value",
+ value: (*Test1200NullStr)(nil),


Currently, panic this case.

--- FAIL: Test1200 (0.03s) --- FAIL: Test1200/fmt.Stringer_implemented_struct_typed-nil_value (0.00s) panic: value method github.com/ClickHouse/clickhouse-go/v2/tests/issues.Test1200NullStr.String called using nil *Test1200NullStr pointer [recovered] panic: value method github.com/ClickHouse/clickhouse-go/v2/tests/issues.Test1200NullStr.String called using nil *Test1200NullStr pointer goroutine 188 [running]: testing.tRunner.func1.2({0x10084d7a0, 0xc0001ae200}) /Users/zaneli/sdk/go1.21.1/src/testing/testing.go:1545 +0x238 testing.tRunner.func1() /Users/zaneli/sdk/go1.21.1/src/testing/testing.go:1548 +0x397 panic({0x10084d7a0?, 0xc0001ae200?}) /Users/zaneli/sdk/go1.21.1/src/runtime/panic.go:914 +0x21f github.com/ClickHouse/clickhouse-go/v2/tests/issues.(*Test1200NullStr).String(0xc00008a958?) <autogenerated>:1 +0x25 github.com/ClickHouse/clickhouse-go/v2.format(0x1010c8680?, 0x60?, {0x100839d60?, 0x0?}) /Users/zaneli/go/src/github.com/zaneli/clickhouse-go/bind.go:309 +0x62e github.com/ClickHouse/clickhouse-go/v2.bindPositional(0x0?, {0x1009644b4, 0x32}, {0xc0000d6160, 0x2, 0x0?}) /Users/zaneli/go/src/github.com/zaneli/clickhouse-go/bind.go:144 +0x2d4 github.com/ClickHouse/clickhouse-go/v2.bind(0x0?, {0x1009644b4, 0x32}, {0xc0000d6160?, 0x2, 0x2}) /Users/zaneli/go/src/github.com/zaneli/clickhouse-go/bind.go:93 +0x235 github.com/ClickHouse/clickhouse-go/v2.bindQueryOrAppendParameters(0x0?, 0xc00008ad58?, {0x1009644b4?, 0x32?}, 0x0?, {0xc0000d6160?, 0x2?, 0x2?}) /Users/zaneli/go/src/github.com/zaneli/clickhouse-go/query_parameters.go:59 +0x16e github.com/ClickHouse/clickhouse-go/v2.(*connect).exec(0xc000512000, {0x100ac6c00, 0x1010c8680}, {0x1009644b4, 0x32}, {0xc0000d6160, 0x2, 0x2}) /Users/zaneli/go/src/github.com/zaneli/clickhouse-go/conn_exec.go:30 +0x105 github.com/ClickHouse/clickhouse-go/v2.(*clickhouse).Exec(0x5ad?, {0x100ac6c00, 0x1010c8680}, {0x1009644b4, 0x32}, {0xc0000d6160, 0x2, 0x2}) /Users/zaneli/go/src/github.com/zaneli/clickhouse-go/clickhouse.go:149 +0x99 github.com/ClickHouse/clickhouse-go/v2/tests/issues.Test1200.func2(0x1010c8680?) /Users/zaneli/go/src/github.com/zaneli/clickhouse-go/tests/issues/1200_pr_test.go:61 +0x126 testing.tRunner(0xc000622680, 0xc0003ee4c0) /Users/zaneli/sdk/go1.21.1/src/testing/testing.go:1595 +0xff created by testing.(*T).Run in goroutine 185 /Users/zaneli/sdk/go1.21.1/src/testing/testing.go:1648 +0x3ad

….21.1 (#31065) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [github.com/ClickHouse/clickhouse-go/v2](https://togithub.com/ClickHouse/clickhouse-go) | `v2.17.1` -> `v2.21.1` | [![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.21.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.21.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.17.1/v2.21.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fClickHouse%2fclickhouse-go%2fv2/v2.17.1/v2.21.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>ClickHouse/clickhouse-go (github.com/ClickHouse/clickhouse-go/v2)</summary> ### [`v2.21.1`](https://togithub.com/ClickHouse/clickhouse-go/releases/tag/v2.21.1) [Compare Source](https://togithub.com/ClickHouse/clickhouse-go/compare/v2.21.0...v2.21.1)  #### What's Changed ##### Fixes 🐛 - Adds the ability to Append a zero valued time.Time by [@KevinJoiner](https://togithub.com/KevinJoiner) in [ClickHouse/clickhouse-go#1228 #### New Contributors - [@KevinJoiner](https://togithub.com/KevinJoiner) made their first contribution in [ClickHouse/clickhouse-go#1228 **Full Changelog**: ClickHouse/clickhouse-go@v2.21.0...v2.21.1 ### [`v2.21.0`](https://togithub.com/ClickHouse/clickhouse-go/releases/tag/v2.21.0) [Compare Source](https://togithub.com/ClickHouse/clickhouse-go/compare/v2.20.0...v2.21.0)  #### What's Changed ##### Enhancements 🎉 - Stream HTTP response body read for decompression by [@rogeryk](https://togithub.com/rogeryk) in [ClickHouse/clickhouse-go#1213 ##### Fixes 🐛 - Fix Date32 extremes values handling by [@jkaflik](https://togithub.com/jkaflik) in [ClickHouse/clickhouse-go#1218 - Fix column with double quotes PrepareBatch failed.([#1216](https://togithub.com/ClickHouse/clickhouse-go/issues/1216)) by [@YenchangChan](https://togithub.com/YenchangChan) in [ClickHouse/clickhouse-go#1217 - Fix an obscure error in HTTP protocol with LZ4 compression by [@YenchangChan](https://togithub.com/YenchangChan) in [ClickHouse/clickhouse-go#1230 ##### Other Changes 🛠 - Replace deprecated `io/ioutil` functions with equivalents by [@zaneli](https://togithub.com/zaneli) in [ClickHouse/clickhouse-go#1211 - Run cloud workflow on main branch only by [@jkaflik](https://togithub.com/jkaflik) in [ClickHouse/clickhouse-go#1219 - ClickHouse 24.2 by [@jkaflik](https://togithub.com/jkaflik) in [ClickHouse/clickhouse-go#1220 **Full Changelog**: ClickHouse/clickhouse-go@v2.20.0...v2.21.0 ### [`v2.20.0`](https://togithub.com/ClickHouse/clickhouse-go/blob/HEAD/CHANGELOG.md#v2200-2024-02-28----Release-notes-generated-using-configuration-in-githubreleaseyml-at-main---) [Compare Source](https://togithub.com/ClickHouse/clickhouse-go/compare/v2.19.0...v2.20.0) #### What's Changed ##### Enhancements 🎉 - Support \[n]byte/\[]byte type Scan/Append to FixedString column by [@rogeryk](https://togithub.com/rogeryk) in [ClickHouse/clickhouse-go#1205 ##### Other Changes 🛠 - Enable cloud tests by [@jkaflik](https://togithub.com/jkaflik) in [ClickHouse/clickhouse-go#1202 - Removed LowCardinality(UInt64) tests that caused allow_suspicious_low_cardinality_types related error by [@jkaflik](https://togithub.com/jkaflik) in [ClickHouse/clickhouse-go#1206 **Full Changelog**: ClickHouse/clickhouse-go@v2.19.0...v2.20.0 ### [`v2.19.0`](https://togithub.com/ClickHouse/clickhouse-go/blob/HEAD/CHANGELOG.md#v2190-2024-02-26----Release-notes-generated-using-configuration-in-githubreleaseyml-at-main---) [Compare Source](https://togithub.com/ClickHouse/clickhouse-go/compare/v2.18.0...v2.19.0) #### What's Changed ##### Enhancements 🎉 - handle ctx.Done() in acquire by [@threadedstream](https://togithub.com/threadedstream) in [ClickHouse/clickhouse-go#1199 ##### Fixes 🐛 - Fix panic on format nil \*fmt.Stringer type value by [@zaneli](https://togithub.com/zaneli) in [ClickHouse/clickhouse-go#1200 ##### Other Changes 🛠 - Update Go/ClickHouse versions by [@jkaflik](https://togithub.com/jkaflik) in [ClickHouse/clickhouse-go#1201 #### New Contributors - [@threadedstream](https://togithub.com/threadedstream) made their first contribution in [ClickHouse/clickhouse-go#1199 - [@zaneli](https://togithub.com/zaneli) made their first contribution in [ClickHouse/clickhouse-go#1200 **Full Changelog**: ClickHouse/clickhouse-go@v2.18.0...v2.19.0 ### [`v2.18.0`](https://togithub.com/ClickHouse/clickhouse-go/blob/HEAD/CHANGELOG.md#v2180-2024-02-01----Release-notes-generated-using-configuration-in-githubreleaseyml-at-main---) [Compare Source](https://togithub.com/ClickHouse/clickhouse-go/compare/v2.17.1...v2.18.0) #### What's Changed ##### Enhancements 🎉 - Add WithAllocBufferColStrProvider string column allocator for batch insert performance boost by [@hongker](https://togithub.com/hongker) in [ClickHouse/clickhouse-go#1181 ##### Fixes 🐛 - Fix bind for seconds scale DateTime by [@jkaflik](https://togithub.com/jkaflik) in [ClickHouse/clickhouse-go#1184 ##### Other Changes 🛠 - resolves [#1163](https://togithub.com/ClickHouse/clickhouse-go/issues/1163) debugF function is not respected by [@omurbekjk](https://togithub.com/omurbekjk) in [ClickHouse/clickhouse-go#1166 #### New Contributors - [@omurbekjk](https://togithub.com/omurbekjk) made their first contribution in [ClickHouse/clickhouse-go#1166 - [@hongker](https://togithub.com/hongker) made their first contribution in [ClickHouse/clickhouse-go#1181 **Full Changelog**: ClickHouse/clickhouse-go@v2.17.1...v2.18.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: opentelemetrybot <[email protected]>

zaneli marked this pull request as draft February 24, 2024 13:42

zaneli marked this pull request as ready for review February 24, 2024 13:50

This comment was marked as outdated.

Sign in to view

zaneli closed this Feb 24, 2024

zaneli deleted the fix/nil-stringer branch February 24, 2024 23:03

zaneli restored the fix/nil-stringer branch February 25, 2024 12:38

zaneli reopened this Feb 25, 2024

zaneli changed the title ~~fix to bind typed nil value~~ fix to format typed nil *fmt.Stringer value Feb 25, 2024

zaneli commented Feb 25, 2024

View reviewed changes

fix to format typed nil *fmt.Stringer value

1277bd2

zaneli commented Feb 26, 2024

View reviewed changes

jkaflik self-requested a review February 26, 2024 08:57

jkaflik approved these changes Feb 26, 2024

View reviewed changes

jkaflik changed the title ~~fix to format typed nil *fmt.Stringer value~~ Fix panic on format nil *fmt.Stringer type value Feb 26, 2024

jkaflik merged commit f96b394 into ClickHouse:main Feb 26, 2024
13 checks passed

jkaflik added bug fix labels Feb 26, 2024

zaneli deleted the fix/nil-stringer branch February 26, 2024 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic on format nil *fmt.Stringer type value #1200

Fix panic on format nil *fmt.Stringer type value #1200

zaneli commented Feb 24, 2024 •

edited

Loading

CLAassistant commented Feb 24, 2024 •

edited

Loading

This comment was marked as outdated.

zaneli Feb 25, 2024

zaneli Feb 26, 2024

Fix panic on format nil *fmt.Stringer type value #1200

Fix panic on format nil *fmt.Stringer type value #1200

Conversation

zaneli commented Feb 24, 2024 • edited Loading

Summary

Checklist

CLAassistant commented Feb 24, 2024 • edited Loading

This comment was marked as outdated.

zaneli Feb 25, 2024

Choose a reason for hiding this comment

zaneli Feb 26, 2024

Choose a reason for hiding this comment

zaneli commented Feb 24, 2024 •

edited

Loading

CLAassistant commented Feb 24, 2024 •

edited

Loading