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

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

Closed
yukia3e opened this issue Feb 23, 2024 · 1 comment · Fixed by #9460
Assignees
Labels
api: spanner Issues related to the Spanner API. triage me I really want to be triaged.

Comments

@yukia3e
Copy link
Contributor

yukia3e commented Feb 23, 2024

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.

  • Error message if spanner.WithLenient() is not specified
Go struct {ID:0 Name:}(type reflect.Value) has no or duplicate fields for Cloud Spanner STRUCT field ArtistId
  • If spanner.WithLenient() is specified.
panic: reflect.Set: value of type string is not assignable to type int64

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.

@yukia3e yukia3e added the triage me I really want to be triaged. label Feb 23, 2024
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Feb 23, 2024
@rahul2393
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. triage me I really want to be triaged.
Projects
None yet
2 participants