この記事は、Wano Group Advent Calendar 2024の22日目の記事です。
21日目は、@johnnytfhs
さんの、2024年よく使用されるGoLandの新機能の紹介でした。
グループ会社のEDOCODEもAdvent Calendarをやっていますので、そちらもどうぞ。
Pull Requestを送った話
直近のプロジェクトで、go-mysql-serverをテストで使うDBサーバとして使っていたのですが、STR_TO_DATEという関数にバグがあることがわかりました。
最終的に、Pull Requestを送り、無事マージされて解決されました。
今回は、どういうふうにバグを見つけて、報告し、修正されたかというのを書いてみようかと思います。OSSはただ使うだけじゃなくて、貢献したほうがいいと思いますので、一例として参考になれば良いなと思って書いてみました。
発見の経緯
同僚からの相談で、STR_TO_DATE が期待通り動かないということを言われました。
このタイミングではとりあえず、issueだけ書こうかなと思っており、特にPull Requestを送る気はありませんでした。
issue報告
とりあえず、
- この場合の期待通りとは何なのか
- 期待通り動く場合と、動かない場合を確認
- バージョンが最新かどうか(最新でも起きるのか)
というくらいを確認しました。
go-mysql-serverは、MySQL互換のGo製のDBサーバなので、
- 期待される動作とは、MySQLと同じ挙動
-
%Y-%m-%dは期待どおりにうごくが、%Y%m%dは期待通りに動かない - 最新のコードで同じことが起きることを確認
そして、以下のissueを書きました。
期待される結果として、MySQLの結果を貼り付けました。
あとは、テストコードがあったので、失敗するケースのパッチと、テスト結果を貼り付けました。
追加情報で、STR_TO_DATE の挙動が確認できるSQLと結果を貼り付けました。
自明な感じだったので、そんなに丁寧には書きませんでした。
調査
最初、Pull Requestを書く気はなかったんですが、issueを書く段階である程度調べてしまっていたので、そんなに大変じゃないかなぁと思って書いてしまいました。調査をどうやったですかですが、
まず、テストコードを見つけました。
find -name '*test.go'
これで、ざっと眺めると、下記のようなものが見つかったので
./sql/expression/function/concat_test.go
./sql/expression/function/version_test.go
./sql/expression/function/elt_test.go
./sql/expression/function/space_test.go
...
これは、関数ごとにテスト書いてるんだなというのがさくっとわかりますので、
find -name '*test.go'|grep -i str_to_date
これで簡単に見つかりました。
./sql/expression/function/str_to_date_test.go
テストコードを見ていると、
for _, tt := range testCases {
f, err := NewStrToDate(
expression.NewGetField(0, types.Text, "", true),
expression.NewGetField(1, types.Text, "", true),
)
if err != nil {
t.Fatal(err)
}
t.Run(tt.name, func(t *testing.T) {
dtime := eval(t, f, sql.NewRow(tt.dateStr, tt.fmtStr))
require.Equal(t, nil, dtime)
})
req := require.New(t)
req.True(f.IsNullable())
}
NewStrToDate で、作られたfをevalに渡しています。
func eval(t *testing.T, e sql.Expression, row sql.Row) interface{} {
ctx := sql.NewEmptyContext()
t.Helper()
v, err := e.Eval(ctx, row)
require.NoError(t, err)
return v
}
eval の中では、Eval というメソッドを呼んでいますね。
StrToDateのEvalは以下のようになっています。
// Eval evaluates the given row and returns a result.
func (s StrToDate) Eval(ctx *sql.Context, row sql.Row) (interface{}, error) {
date, err := s.Date.Eval(ctx, row)
if err != nil {
return nil, err
}
format, err := s.Format.Eval(ctx, row)
if err != nil {
return nil, err
}
dateStr, ok := date.(string)
if !ok {
// TODO: improve this error
return nil, sql.ErrInvalidType.New(fmt.Sprintf("%T", date))
}
formatStr, ok := format.(string)
if !ok {
// TODO: improve this error
return nil, sql.ErrInvalidType.New(fmt.Sprintf("%T", formatStr))
}
goTime, err := dateparse.ParseDateWithFormat(dateStr, formatStr)
if err != nil {
ctx.Warn(1411, fmt.Sprintf("Incorrect value: '%s' for function %s", dateStr, s.FunctionName()))
return nil, nil
}
// zero dates '0000-00-00' and '2010-00-13' are allowed,
// but depends on strict sql_mode with NO_ZERO_DATE or NO_ZERO_IN_DATE modes enabled.
return goTime, nil
}
ParseDateWithFormatてのがありました。このあたりっぽいなぁ。
goTime, err := dateparse.ParseDateWithFormat(dateStr, formatStr)
if err != nil {
ctx.Warn(1411, fmt.Sprintf("Incorrect value: '%s' for function %s", dateStr, s.FunctionName()))
return nil, nil
}
すぐに、parsersFromFormatStringを呼んでますね。
func ParseDateWithFormat(date, format string) (interface{}, error) {
parsers, specifiers, err := parsersFromFormatString(format)
if err != nil {
parser, ok := formatSpecifiers[specifier]から、parserを取得している。
func parsersFromFormatString(format string) ([]parser, map[uint8]bool, error) {
parsers := make([]parser, 0, len(format))
var specifiersInFormat = make(map[uint8]bool)
for i := 0; i < len(format); i++ {
char := format[i]
if char == '%' {
if len(format) <= i+1 {
return nil, nil, fmt.Errorf("\"%%\" found at end of format string")
}
specifier := format[i+1]
specifiersInFormat[specifier] = true
parser, ok := formatSpecifiers[specifier]
formatSpecifiersを調べると...%dとかの、dに対するparserの関数を指定しているのですね。
var formatSpecifiers = map[byte]parser{
// %a Abbreviated weekday name (Sun..Sat)
'a': parseWeekdayAbbreviation,
// %b Abbreviated month name (Jan..Dec)
'b': parseMonthAbbreviation,
// %c Month, numeric (0..12)
'c': parseMonthNumeric,
// %D Day of the month with English suffix (0th, 1st, 2nd, 3rd, …)
'D': parseDayNumericWithEnglishSuffix,
// %d Day of the month, numeric (00..31)
'd': parseDayOfMonth2DigitNumeric,
// %e Day of the month, numeric (0..31)
'e': parseDayOfMonthNumeric,
// %f Microseconds (000000..999999)
'f': parseMicrosecondsNumeric,
// %H Hour (00..23)
'H': parse24HourNumeric,
// %h Hour (01..12)
'h': parse12HourNumeric,
// %I Hour (01..12)
'I': parse12HourNumeric,
// %i Minutes, numeric (00..59)
'i': parseMinuteNumeric,
// %j Day of year (001..366)
'j': parseDayOfYearNumeric,
// %k Hour (0..23)
'k': parse24HourNumeric,
// %l Hour (1..12)
'l': parse12HourNumeric,
// %M Month name (January..December)
'M': parseMonthName,
// %m Month, numeric (00..12)
'm': parseMonth2DigitNumeric,
// %p AM or PM
'p': parseAmPm,
// %r Time, 12-hour (hh:mm:ss followed by AM or PM)
'r': parse12HourTimestamp,
// %S Seconds (00..59)
'S': parseSecondsNumeric,
// %s Seconds (00..59)
's': parseSecondsNumeric,
// %T Time, 24-hour (hh:mm:ss)
'T': parse24HourTimestamp,
'U': nil,
'u': nil,
'V': nil,
'v': nil,
'W': nil,
'w': nil,
'X': nil,
'x': nil,
// %Y Year, numeric, four digits
'Y': parseYear4DigitNumeric,
// %y Year, numeric (two digits)
'y': parseYear2DigitNumeric,
'%': literalParser('%'),
}
ここがバグの本丸っぽいですね。%Y%m%dでバグるので、そのあたりをチェックします。
// %d Day of the month, numeric (00..31)
'd': parseDayOfMonthNumeric,
// %m Month, numeric (00..12)
'm': parseMonthNumeric,
// %Y Year, numeric, four digits
'Y': parseYear4DigitNumeric,
名前的に%Yは大丈夫そうですが、一応チェックします。
func parseYear4DigitNumeric(result *datetime, chars string) (rest string, _ error) {
if len(chars) < 4 {
return "", fmt.Errorf("expected at least 4 chars, got %d", len(chars))
}
year, rest, err := takeNumber(chars)
if err != nil {
return "", err
}
result.year = &year
return rest, nil
}
takeNumberに、範囲をしているする引数ないですね。ちょっと怪しい。
func takeNumber(chars string) (num uint, rest string, err error) {
numChars, rest := takeAll(chars, isNumeral)
parsedNum, err := strconv.ParseUint(numChars, 10, 32)
if err != nil {
return 0, "", err
}
return uint(parsedNum), rest, nil
}
takeNumberで使われているtakeAllという名前からしてわかりますが(追って確認もしましたが)、連続した数字を全部取ることになってます。
つまり、20241222 でしたら、20241222 が%Yの部分となってしまいます。これは、よろしくない。
parseMonthNumericはどうでしょうか。
func parseMonthNumeric(result *datetime, chars string) (rest string, _ error) {
num, rest, err := takeNumber(chars)
if err != nil {
return "", err
}
month := time.Month(num)
result.month = &month
return rest, nil
}
これも、同じで、takeNubmer使っています。まずい。
これで、もう原因はわかりましたので、あとは修正していくだけですね。
修正
takeNumber で全部とってるのが問題です。ですが、他のparseYear2DigitNumericとかを見ると、takeNumberAtMostNChars(2, chars)のように、ちゃんと文字数指定で取れる仕組みは用意されていました。じゃぁ、簡単ですね。
%m, %d用に新しい関数を用意します。
- 'd': parseDayOfMonthNumeric,
+ 'd': parseDayOfMonth2DigitNumeric,
- 'm': parseMonthNumeric,
+ 'm': parseMonth2DigitNumeric,
parseYear4DigitNumeric は、名前自体は問題ないので、takeNumberの代わりに、takeNumberAtMostNCharsを使えば良いだけですね。
- year, rest, err := takeNumber(chars)
+ year, rest, err := takeNumberAtMostNChars(4, chars)
parseDayOfMonth2DigitNumeric と parseMonth2DigitNumeric は新しく作りました。
Pull Requestは、下記になりますので、修正を全部確認したい方は、ご参照ください。
https://github.com/dolthub/go-mysql-server/pull/2667
Pull Requestを出した結果、コメントがついたので、それに従って修正をして、無事mergeされました。
終わり
というわけで、Pull Requestを送った経緯とどうやってバグを修正したのかというのを書いてみました。
今回、修正が簡単だったということはありますが、特段難しいことはしていないというのはわかるかと思います。知らないコードではありますが、普段のバグ修正とやってることはかわらないのではないかとおもいます。
みなさんも、なんかバグってるなーと思ったら修正して、Pull Requestを投げてみるのも良いと思いますよ。
明日は、@tutti618さんのGithubのPRコメント数を集計→Slack通知で賞賛の文化を作るです。お楽しみに!