Key tag issue

[Ale1ster]

As referenced in the README, the key field tag for a struct can be either badgerholdKey or badgerhold:"key", but in the package tests the tag `badgerholdKey:"Key" is used.

Is that an issue of outdated documentation?

When a uint64 struct field is tagged as a struct key, it behaves differently (badgerhold.Store.Get and badgerhold.Store.Find) depending on which tag is used.

What is the correct way to declare a key field?

package badgerhold_test

import (
    "testing"
    "time"

    "github.com/timshannon/badgerhold"
)

type BHTag struct {
    ID uint64 `badgerholdKey`
    Data string
    Created time.Time
}

type BHKTag struct {
    ID uint64 `badgerholdKey:"Key"`
    Data string
    Created time.Time
}

type BKTag struct {
    ID uint64 `badgerhold:"key"`
    Data string
    Created time.Time
}

func TestGetKeyTag1(t *testing.T) {
    testWrap(t, func(store *badgerhold.Store, t *testing.T) {
        data := &BHTag{
            Data: `tag badgerholdKey`,
            Created: time.Now(),
        }

        err := store.Insert(badgerhold.NextSequence(), &BKTag{})
        if err != nil {
            t.Fatalf("Error on empty value consumption from sequence: %s", err)
        }

        err = store.Insert(badgerhold.NextSequence(), data)
        if err != nil {
            t.Fatalf("Error inserting data: %s", err)
        }

        key := data.ID
        if key == 0 {
            t.Fatalf("Error on insert: empty key")
        }

        findResult := make([]BHTag, 0)
        err = store.Find(&findResult, badgerhold.Where(badgerhold.Key).Eq(key))
        if err != nil {
            t.Fatalf("Error on find: %s", err)
        }
        if len(findResult) != 1 {
            t.Fatalf("Error: find returned %d results, expected 1", len(findResult))
        }
        if findResult[0].ID == 0 {
            t.Fatalf("Error: empty value for key from find")
        }

        getResult := &BHTag{}
        err = store.Get(key, getResult)
        if err != nil {
            t.Fatalf("Error on get: %s", err)
        }
        if getResult.ID == 0 {
            t.Fatalf("Error: empty value for key from get")
        }
    })
}

func TestGetKeyTag2(t *testing.T) {
    testWrap(t, func(store *badgerhold.Store, t *testing.T) {
        data := &BHKTag{
            Data: `tag badgerholdKey:"Key"`,
            Created: time.Now(),
        }

        err := store.Insert(badgerhold.NextSequence(), &BHKTag{})
        if err != nil {
            t.Fatalf("Error on empty value consumption from sequence: %s", err)
        }

        err = store.Insert(badgerhold.NextSequence(), data)
        if err != nil {
            t.Fatalf("Error inserting data: %s", err)
        }

        key := data.ID
        if key == 0 {
            t.Fatalf("Error on insert: empty key")
        }

        findResult := make([]BHKTag, 0)
        err = store.Find(&findResult, badgerhold.Where(badgerhold.Key).Eq(key))
        if err != nil {
            t.Fatalf("Error on find: %s", err)
        }
        if len(findResult) != 1 {
            t.Fatalf("Error: find returned %d results, expected 1", len(findResult))
        }
        if findResult[0].ID == 0 {
            t.Fatalf("Error: empty value for key from find")
        }

        getResult := &BHKTag{}
        err = store.Get(key, getResult)
        if err != nil {
            t.Fatalf("Error on get: %s", err)
        }
        if getResult.ID == 0 {
            t.Fatalf("Error: empty value for key from get")
        }
    })
}

func TestGetKeyTag3(t *testing.T) {
    testWrap(t, func(store *badgerhold.Store, t *testing.T) {
        data := &BKTag{
            Data: `tag badgerhold:"key"`,
            Created: time.Now(),
        }

        err := store.Insert(badgerhold.NextSequence(), &BKTag{})
        if err != nil {
            t.Fatalf("Error on empty value consumption from sequence: %s", err)
        }

        err = store.Insert(badgerhold.NextSequence(), data)
        if err != nil {
            t.Fatalf("Error inserting data: %s", err)
        }

        key := data.ID
        if key == 0 {
            t.Fatalf("Error on insert: empty key")
        }

        findResult := make([]BKTag, 0)
        err = store.Find(&findResult, badgerhold.Where(badgerhold.Key).Eq(key))
        if err != nil {
            t.Fatalf("Error on find: %s", err)
        }
        if len(findResult) != 1 {
            t.Fatalf("Error: find returned %d results, expected 1", len(findResult))
        }
        if findResult[0].ID == 0 {
            t.Fatalf("Error: empty value for key from find")
        }

        getResult := &BKTag{}
        err = store.Get(key, getResult)
        if err != nil {
            t.Fatalf("Error on get: %s", err)
        }
        if getResult.ID == 0 {
            t.Fatalf("Error: empty value for key from get")
        }
    })
}

Sorry for the long test, but I am not well-versed with reflection in Go.

[timshannon]

This is bug, thanks for bringing it up. Per the documentation, both should be supported until I rev the major version. If there is a scenario where the new tag (badgerhold:"key") works but the old one (badgerholdKey) doesn't, that's a bug.

Thanks,

[Ale1ster]

So, the supported keys are badgerholdKey and badgerhold:"key". But in that case, why are all tags in the package test structs badgerholdKey:"Key"?

If it is an issue with reflect's field tag retrieval, it seems Lookup is supported for both formats (both badgerholdKey:"anything-here" and badgerholdKey).

If so, I think the documentation could be a little clearer.

[timshannon]

I agree there are missing tests for the future deprecated key tag format, otherwise this issue would've been caught.

What additional info do you think should be added to the documentation?

[Ale1ster]

I think that the current documentation (as included in your previous post) is complete enough, but it should note the proper way to use the tag as badgerholdKey:"", which seems to be the tag conventional format.

Otherwise there will be inconsistencies with reflect.StructTag.Lookup and reflect.StructTag.Get, since the tag is looked up using Lookup in some places and using the StructTag value itself in others.

[timshannon]

Yeah the solution isn't to force everyone to use the proper tag format, because I documented it wrong, and wrote it wrong originally. If I don't want to break backwards compatibility with existing software using this library, I have to support the wrong way of doing it everywhere, or I need to rev the major version of the library.

[Ale1ster]

Wouldn't supporting both the unmapped tag and the conventional tag format for badgerholdKey, as well as the badgerhold:"key" format across all uses of key tags be a solution?

Attached is a proposed patch that seems to do that, and from what I understand shouldn't break backwards compatibility.

Changing the tag check to substring check on the tag string representation, should ensure that it is permissive enough to accept both badgerholdKey and badgerholdKey:"".

patch.tar.gz