Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement CheckNamedValue for statsColumnConverter #3

Merged

Conversation

chihkaiyu
Copy link

@chihkaiyu chihkaiyu commented Oct 14, 2020

Description

實作 NamedValueChecker 這個 interface。

Motivation and Context

起因

試著再 migrate 一次到 go modules 時,發現 unit test fail:

err=“sql: converting argument $4 type: driver ColumnConverter error converted uint64 to unsupported type uint64”

追查後發現是升級 mongo-go-driver 時導致其他 dependencies 也跟著升級:
升級 mongo-go-driver (v1.3.1 -> v1.4.0)
-> mongo-go-driver@v1.4.0 多了 github.com/aws/aws-sdk-go@v1.29.15
-> aws-sdk-go 依賴 go-sql-driver/mysql@v1.5.0
-> 我們原本的 go-sql-driver/mysql 是 7ebe0a500653eeb1859664bed5e48dec1e164e73 (v1.2 ~ v1.3 中間的 commit),根據 minimal version selection,go module 會選擇 v1.5.0。

根源

在原本的 go-sql-driver/mysql 版本裡,呼叫 ConvertValue() 時,遇到 type 是 uint 的都會轉成 int64。[1]
而 golang 內建的 sql package 會再檢查 converted 後的值,若是 uint 會回傳 false。[2]
但到了 go-sql-driver/mysql@v1.5.0 時,ConvertValue() 不再把 uint 轉型成 int64 了 [3],driver.IsValue() 會是 false [4]。

[1] ConvertValue()@7ebe0a500653eeb1859664bed5e48dec1e164e73
[2] IsValue()
[3] ConvertValue()@v1.5.0
[4] CheckNamedValue()

解法

  1. 參照底下 related Issue and PR 所提及的,實作 CheckNamedValue() 而躲開使用 ccChecker [5]。
  2. aws-sdk-go 在 v1.35.5 就把 go-sql-driver/mysql 移除了 [6],但目前 mongo-go-driver 最新也才用到 v1.34.28 [7],或許等他慢慢升級上去,就可以不被強迫升級 go-sql-driver/mysql 的版本。但總有一天還是要升級的,所以此方法不建議。

[5] https://github.com/golang/go/blob/7c58ef732efd9bf0d0882bb95371ce1909924a75/src/database/sql/convert.go#L127
[6] https://github.com/aws/aws-sdk-go/blob/v1.35.5/go.mod
[7] https://github.com/mongodb/mongo-go-driver/blob/master/go.mod

詳情

ColumnConverter 裡的 idx 參數實際上並沒有用到,所以給什麼都無所謂,他會回傳一個空的 converter object [8]。

[8] https://github.com/go-sql-driver/mysql/blob/v1.5.0/statement.go#L42

Related Issue and PR

@houjunchen houjunchen merged commit 72259dd into 17media:master Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants