jmoiron / sqlx

general purpose extensions to golang's database/sql
http://jmoiron.github.io/sqlx/
MIT License
16.14k stars 1.08k forks source link

Reflection when scanning and getting values seems to not be selecting the shallowest depth value #230

Open c4milo opened 8 years ago

c4milo commented 8 years ago

I recently updated sqlx and my queries no longer work when scanning postgres enum types into my Go structs. I get this error:

error: sql: Scan error on column index 2: unsupported Scan, storing driver.Value type []uint8 into type *stacks.StatusType"

query: db.Get(s, "SELECT * FROM stacks WHERE name = $1", name)

table:

do $$
begin
    -- creates an enum type for status
    if not exists (select 1 from pg_type where typname = 'stack_status_type') then
        create type stack_status_type as enum ('beta', 'deprecated', 'public');
    end if;
end$$;

-- this table contains the supported Hooklift stacks metadata.
create table if not exists stacks (
    -- stack ID
    id                      uuid not null,
    -- stack's name
    name                    citext unique not null,
    -- status can be one of the following: beta, deprecated, public
    status                  stack_status_type not null default 'beta',
    -- timestamp of when the stack was created
    created_at              timestamptz not null default current_timestamp,
    -- timestamp of when the stack was last updated
    updated_at              timestamptz not null,

    primary key(id)
);

What am I missing?

c4milo commented 8 years ago

I solved this changing the way how I was composing my database types. It seems the order on which reflection was being done changed and broke my code.

jmoiron commented 8 years ago

@c4milo Can you let me know what was broken and what change fixed it? It's possible there is still a bug, or there might be a breaking change that I did not understand at the time that should be documented, or maybe a treatment of some ambiguous situation which has changed.

c4milo commented 8 years ago

@jmoiron oh, sure!

I had the following type in my domain layer:

// StatusType defines a type to represent the different states of a stack.
type StatusType string

const (
    Beta StatusType = "beta"
    Deprecated StatusType = "depracated"
    Public StatusType = "public"
)

type Stack struct {
    ID        string     `json:"id"`
    Name      string     `json:"name"`
    Status    StatusType `json:"status"`
    CreatedAt time.Time  `json:"created_at" db:"created_at"`
    UpdatedAt time.Time  `json:"updated_at" db:"updated_at"`
}

and in my persistence layer I had the following type:

type stack struct {
    Stack
    Status string
}

// GetByName returns stack information by Name.
func (r *RepoPostgres) GetByName(name string) (*Stack, error) {
    s := new(stack)
    err := r.db.Get(s, "SELECT * FROM stacks WHERE name = $1", name)
    if err != nil {
        return nil, database.Error(err, errorsMap)
    }

    s.Stack.Status = StatusType(s.Status)
    return &(s.Stack), nil
}

Before upgrading sqlx, that above code worked fine, and Status in the stack type was being set properly by sqlx.

After upgrading sqlx, the Status field receiving the value was the one from the Stack type which uses a custom Go type and sqlx would fail scanning the db value into it.

In order to fix it, I had to explicitly set db:"-" on the Status field of my domain object like so:

type Stack struct {
    ID        string     `json:"id"`
    Name      string     `json:"name"`
    Status    StatusType `json:"status" db:"-"`
    CreatedAt time.Time  `json:"created_at" db:"created_at"`
    UpdatedAt time.Time  `json:"updated_at" db:"updated_at"`
}

Which I think results in better code since it is more explicit.

jmoiron commented 8 years ago

It's more explicit but this behavior from sqlx is not as documented and is wrong. It should be satisfying scan destinations based on Go attribute access rules, which are breadth first and not depth first. I'll try to get a test case for this going.

c4milo commented 8 years ago

Unless I'm misunderstanding, I think this may be actually working as intended by the Golang spec:

For a value x of type T or *T where T is not a pointer or interface type, x.f denotes the field or method at the shallowest depth in T where there is such an f. If there is not exactly one f with shallowest depth, the selector expression is illegal.

-- https://golang.org/ref/spec#Selectors

There is discussion about the topic here: https://groups.google.com/forum/#!topic/golang-nuts/1Xt6vvNCIA4

jmoiron commented 8 years ago

Yes, so in this example stack.Status should take precedence over stack.Stack.Status, but you claimed that's not what is happening. Regardless, if there's no test for this there should be one. I'll be looking into it.

jmoiron commented 8 years ago

This was almost certainly a bug and should be fixed in #232. I'll merge that when I get some documentation updated to mention it. Any code that was relying on it was relying on behavior that went against what the documentation claimed and was therefore wrong, but I want people to know about it anyway.

Thanks a lot for your detailed bug report! It made it really easy to identify and track down :+1:

c4milo commented 8 years ago

No problems, thanks for fixing this so quickly!

On Wed, Jun 15, 2016 at 1:13 AM Jason Moiron notifications@github.com wrote:

This was almost certainly a bug and should be fixed in #232 https://github.com/jmoiron/sqlx/pull/232. I'll merge that when I get some documentation updated to mention it. Any code that was relying on it was relying on behavior that went against what the documentation claimed and was therefore wrong, but I want people to know about it anyway.

Thanks a lot for your detailed bug report! It made it really easy to identify and track down 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jmoiron/sqlx/issues/230#issuecomment-226089663, or mute the thread https://github.com/notifications/unsubscribe/AACi0SGNHvCziGzIbGTwBUBlG6MkuuuLks5qL4nzgaJpZM4I1Z1C .

c4milo commented 7 years ago

@jmoiron, I'm currently trying to work with Postgres array types, using a similar strategy as detailed here for scanning. But, this time it is for insertion and it seems sqlx is passing the fields of my domain struct instead of the lib/pq array types of the embedder struct which looks like this:

type client struct {
    Client
    RedirectURIs     pq.StringArray `db:"redirect_uris"`
    ResponseTypes    pq.StringArray `db:"response_types"`
    GrantTypes       pq.StringArray `db:"grant_types"`
    DefaultACRValues pq.StringArray `db:"default_acr_values"`
    RequestURIs      pq.StringArray `db:"request_uris"`
    Contacts         pq.StringArray `db:"contacts"`
}

I tested the access rules with a simple Go program and it seems to be doing the right thing with respect to Go's selectors spec: https://play.golang.org/p/N97CjAcm-L

This is the underlined error:

sql: converting Exec argument $6 type: unsupported type []string, a slice
abraithwaite commented 7 years ago

Running into this issue for an interesting case. I've got an external struct I'm importing for which I want to override the input argument for passing into a named statement.

https://gist.github.com/abraithwaite/f9cd2dd6a9311c4bd1633e5525843a8a

The library choses the embedded struct untagged field over the top level db tagged field.

Adding \db:"-"`` to the embedded structs tags fixes the issue of course, but is undesirable because I now have to change an external package. The other workaround (which I'm currently using) is picking a different tag, but it's less desirable to me over having a clean override.