googleapis / google-cloud-go

Google Cloud Client Libraries for Go.
https://cloud.google.com/go/docs/reference
Apache License 2.0
3.78k stars 1.3k forks source link

spanner: SelectAll does not correctly handle structs with spanner tag annotations that contain uppercase letters #9459

Closed yukia3e closed 8 months ago

yukia3e commented 8 months ago

Client

Spanner

Environment

Get this on all, tested on local mac PC with Docker

Go Environment

$ go version

go version go1.21.7 darwin/arm64

$ go env

GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/{masked}/.asdf/installs/golang/1.21.7/packages/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/{masked}/.asdf/installs/golang/1.21.7/packages'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/{masked}/.asdf/installs/golang/1.21.7/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/{masked}/.asdf/installs/golang/1.21.7/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.7'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/{masked}/Projects/yukia3e/go-spanner-selectall-test/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/zc/ljr5xzz11mn7dflgz3ghw_tm0000gn/T/go-build3650580967=/tmp/go-build -gno-record-gcc-switches -fno-common'

Code

I have created a sample repository where you can reproduce the issue. https://github.com/yukia3e/go-spanner-selectall-test/blob/main/internal/main.go#L115

The following fixes were incorporated in v1.57.0, but I think that the behavior of spanner:"" tag annotation still seems different from that of ToStruct.

spanner: SelectAll struct fields match should be case-insensitive (#9417) (7ff5356)

The current behavior of SelectAll for spanner tag annotations seems to be "accept only lowercase field names" instead of case-insensitive.

First, suppose I have the following test table

CREATE TABLE Singers (
    ArtistId INT64 NOT NULL, Name
    Name STRING(1024), Name
) PRIMARY KEY (ArtistId)

Also, the following data is submitted as test data.

INSERT INTO Singers (ArtistId, Name) VALUES (1, 'Alice'), (2, 'Bob'), (3, 'Charlie');

Consider the case where the *spanner.RowIterator value obtained from the above table is set to a slice of a struct containing uppercase letters in the spanner tag annotation as follows.

type Singer struct {
    ID int64 `spanner: "ArtistId"`
    Name string `spanner: "Name"`
}

Expected behavior

The expected result is that SelectAll will set the value correctly, as well as ToStruct, even if the tag annotation contains uppercase letters.

Actual behavior

An error or panic will occur if the tag annotation contains uppercase letters. If spanner.WithLenient() option is not specified, an error will occur; if spanner.WithLenient() option is specified, panic will occur.

Additional context

I have investigated the possible cause of the problem.

In row.go, initFieldTag L564

        name, keep, _, _ := spannerTagParser(fieldType.Tag)
        if !keep {
            continue
        }
        if name == "" {
            name = strings.ToLower(fieldType.Name)
        }
        (*fieldTagMap)[name] = sliceItem.Field(i)

but this is probably because strings.ToLower is not applied to the spannerTagParser result.

I have tested the above test repository with the following changes to the row.go implementation and have confirmed that it works correctly.

        name, keep, _, _ := spannerTagParser(fieldType.Tag)
        if !keep {
            continue
        }
        if name == "" {
            name = fieldType.Name
        }
        (*fieldTagMap)[strings.ToLower(name)] = sliceItem.Field(i)

As this is my first contribution to this repository, I am not fully aware of the detailed testing procedures, but I will be able to issue a PR to correct the relevant areas. The branch where we made the modifications is here.

rahul2393 commented 8 months ago

Thanks @yukia3e for the detailed investigation, feel free to create the PR for your patch. Let us know here if you want any help