yangjj-iso opened a new pull request, #1144: URL: https://github.com/apache/incubator-seata-go/pull/1144
- [x] I have registered the PR [changes](https://github.com/apache/incubator-seata-go/tree/master/changes). **What this PR does**: `MysqlXAConn.Recover` read the xid out of the `XA RECOVER` result set with `dest[3].(string)`. The driver never puts a `string` there: `go-sql-driver/mysql` v1.6.0 reads a text-protocol row with `readLengthEncodedString`, which returns `[]byte`, and assigns it straight into `dest[i]` (`packets.go:770`) — its own datetime branch eleven lines down asserts `dest[i].([]byte)` on the same slice. The assertion therefore failed on the first row, and `Recover` answered `the protocol of XA RECOVER statement is error` for any real connection, whatever the server had returned. It now accepts both shapes, which is what `PostgresXAConn.Recover` in the same package already does. Two more things in the same function: - The result set was never closed. The Postgres sibling defers `rows.Close()`; without it a recovery scan strands the connection it borrowed. `sqlclosecheck` is enabled in `.golangci.yml` but cannot see this one, because it is a `driver.Rows` and not a `*sql.Rows`. - A leftover `fmt.Printf("gtr: %v", ...)` printed every recovered xid to stdout, with no newline. It goes, along with the `for true` loop and the return it made unreachable. **Which issue(s) this PR fixes**: Fixes #1143 **Special notes for your reviewer**: Why this went unnoticed: the existing `TestMysqlXAConn_Recover` builds its rows out of Go strings (`{1, 3, 0, "xid"}`), so the assertion succeeds under the mock and only fails under a driver. That test is left exactly as it was — a `string` is still accepted — and a second one feeds the `[]byte` rows a driver produces. On master it fails with the error above: ``` Recover() error = the protocol of XA RECOVER statement is error, want nil --- FAIL: TestMysqlXAConn_RecoverDriverValues ``` Being straight about the impact: `Recover` has no production callers yet, only tests, so nobody is hitting this today. It seems worth fixing now anyway, because a recovery scan is the first thing that will call it, and because the current test reads as if the path already worked. The two `mysqlMockRows` methods that were `panic("implement me")` are implemented, since `Close()` is now called on it. Verified locally, Go 1.26.1 on Windows: | command | result | | --- | --- | | `go build ./...` | ok | | `go vet ./pkg/datasource/sql/xa/...` | ok | | `go test ./pkg/datasource/sql/...` | ok, 22 packages | | `gofmt` | clean | **Does this PR introduce a user-facing change?**: ```release-note Fix MySQL XA recovery: the xids of `XA RECOVER` are read as the []byte the driver returns, rather than failing with "the protocol of XA RECOVER statement is error", and the result set is released before returning. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
