datastore: query filter value cannot be of defined/custom type (breaking change from appengine/datastore)

[Hexcles]

Client

Cloud Datastore

Environment

anywhere

Go Environment

$ go version go version go1.15 linux/amd64 $ go env [I believe this is unrelated.]

Code

e.g.

package main

import (
    "cloud.google.com/go/datastore"
)

type Score int

const Passed Score = 60

func main() {
    q := datastore.NewQuery("People")
    q = q.Filter("Score >=", Passed)
    // Then Use the query.
}

Expected behavior

Get People with Score greater than or equal to 60. This used to work with google.golang.org/appengine/datastore.

Actual behavior

datastore: bad query filter value type: invalid Value type shared.PendingTestRunStage

Additional context

This is a breaking change from the legacy google.golang.org/appengine/datastore library from which I'm migrating. The difference boils down to how the two libraries use reflection on query filters.

google.golang.org/appengine/datastore uses reflect.Value.Kind to get the kind of the concrete type of the interface: https://github.com/golang/appengine/blob/5d1c1d03f8703c2e81478d9a30e9afa2d3e4bd8a/datastore/save.go#L43

whereas this library simply uses a type switch on the concrete type: https://github.com/googleapis/google-cloud-go/blob/28a9062d1dd4272371932472056f6fc8c1780372/datastore/save.go#L376

[Hexcles]

I know I could cast my filter value to the primitive type, but that's really not ideal given the scale.

[tbpg]

Thank you for the detailed report! The default case of that switch uses reflection to get the Kind: https://github.com/googleapis/google-cloud-go/blob/28a9062d1dd4272371932472056f6fc8c1780372/datastore/save.go#L443-L447

I am not sure if this change in behavior is intentional, but I can't think of any reasons we wouldn't want to support this. A benefit of the current flow is avoiding reflection for standard types and only falling back when needed. But, that doesn't preclude us from extending the logic when using reflection.

[Hexcles]

The default case of that switch uses reflection to get the Kind:

Yeah but it's only for dereferencing a pointer IIUC.

[tbpg]

Yeah but it's only for dereferencing a pointer IIUC.

Yes, hence the last paragraph of my comment. :smile:

I still haven't thought of any reasons we wouldn't want to support this. Would you be interested in sending a PR to add the functionality (and tests)?