Skip to content

statedb: handle empty index name in queries #169

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.

Sign up for GitHub

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

Draft
tklauser wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

statedb: handle empty index name in queries #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1cb296
statedb: validate primary index uniqueness earlier in NewTableAny
tklauser Jun 30, 2026
a988870
statedb: reject secondary indexes with empty name
tklauser Jun 30, 2026
7ded532
statedb: handle empty index name in queries
tklauser Jun 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1386,6 +1386,47 @@ func TestDB_EmptyKeys(t *testing.T) {

}

func TestDB_EmptyIndexName(t *testing.T) {
t.Parallel()

// A prefix index whose FromObject yields no prefixes, so that
// QueryFromObject returns a zero-value Query{} with an empty index name.
emptyPrefixIndex := NetIPPrefixIndex[*testObject]{
Name: "empty-prefix",
Unique: true,
FromObject: func(obj *testObject) iter.Seq[netip.Prefix] {
return func(yield func(netip.Prefix) bool) {}
},
}

db, table := newTestDBWithMetrics(t, &NopMetrics{}, emptyPrefixIndex)

txn := db.WriteTxn(table)
_, _, err := table.Insert(txn, &testObject{ID: 1})
require.NoError(t, err, "Insert")
txn.Commit()

// QueryFromObject returns an empty-index query when the object yields no
// keys. Querying with it must fall back to the primary index instead of
// panicking.
q := emptyPrefixIndex.QueryFromObject(&testObject{ID: 1})
rtxn := db.ReadTxn()
require.NotPanics(t, func() {
table.Get(rtxn, q)
for range table.List(rtxn, q) {
}
for range table.Prefix(rtxn, q) {
}
for range table.LowerBound(rtxn, q) {
}
})

// An explicitly empty-index query resolves to the primary index.
obj, _, ok := table.Get(rtxn, Query[*testObject]{key: index.Uint64(1)})
require.True(t, ok, "Get")
require.Equal(t, uint64(1), obj.ID)
}

func TestWriteJSON(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -1491,6 +1532,15 @@ func Test_validateTableName(t *testing.T) {
}
}

func Test_validateSecondaryIndexName(t *testing.T) {
db := New()
emptyNameIndex := tagsIndex
emptyNameIndex.Name = ""

_, err := NewTable(db, "test", idIndex, emptyNameIndex)
require.ErrorIs(t, err, ErrEmptySecondaryIndexName)
}

func Test_getAcquiredInfo(t *testing.T) {
t.Parallel()
db, table, _ := newTestDB(t)
Expand Down
3 changes: 3 additions & 0 deletions errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ var (
// ErrPrimaryIndexNotUnique indicates that the primary index for the table is not marked unique.
ErrPrimaryIndexNotUnique = errors.New("primary index not unique")

// ErrEmptySecondaryIndexName indicates that a secondary index for the table has an empty name.
ErrEmptySecondaryIndexName = errors.New("secondary index name is empty")

// ErrDuplicateIndex indicates that the table has two or more indexers that share the same name.
ErrDuplicateIndex = errors.New("index name already in use")

Expand Down
18 changes: 13 additions & 5 deletions table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ func NewTableAny[Obj any](
return nil, err
}

// Primary index must always be unique
if !primaryIndexer.isUnique() {
return nil, tableError(tableName, ErrPrimaryIndexNotUnique)
}

toAnyIndexer := func(idx Indexer[Obj], pos int) anyIndexer {
return anyIndexer{
name: idx.indexName(),
Expand Down Expand Up @@ -111,17 +116,15 @@ func NewTableAny[Obj any](
indexPos := SecondaryIndexStartPos
for _, indexer := range secondaryIndexers {
name := indexer.indexName()
if name == "" {
return nil, tableError(tableName, ErrEmptySecondaryIndexName)
}
anyIndexer := toAnyIndexer(indexer, indexPos)
table.secondaryAnyIndexers = append(table.secondaryAnyIndexers, anyIndexer)
table.indexPositions[indexPos] = name
indexPos++
}

// Primary index must always be unique
if !primaryIndexer.isUnique() {
return nil, tableError(tableName, ErrPrimaryIndexNotUnique)
}

// Validate that indexes have unique ids.
indexNames := map[string]struct{}{}
indexNames[primaryIndexer.indexName()] = struct{}{}
Expand Down Expand Up @@ -199,6 +202,11 @@ func (t *genTable[Obj]) released() {
}

func (t *genTable[Obj]) indexPos(name string) int {
// An empty index name refers to the primary index, matching getIndexer.
if name == "" {

@pippolo84 pippolo84 Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indexPos is executed for secondary indexes too. In case of a secondary index with an empty name we'll enter this if returning the primary index.
I think we should explicitly reject secondary indexes with empty names in NewTableAny.

@tklauser tklauser Jun 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should explicitly reject secondary indexes with empty names in NewTableAny.

I think rejecting secondary indexes with empty names is a worthwhile change. Fixed as suggested.

But AFAICS it still won't fix the issue here in all cases because the name passed to indexPos by callers may be the one extracted from Query.index or QueryRequest.Index and that could still be empty in some cases, depending on how the query was constructed.

@pippolo84 pippolo84 Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

Besides, looking again at the code I see that stateDB explicitly supports objects with an empty primary key (see TestDB_EmptyKeys) and with this change it is possible to have something like this:

emptyPrefixIndex := NetIPPrefixIndex[*testObject]{
	Name:   "empty-prefix",
	Unique: true,
	FromObject: func(*testObject) iter.Seq[netip.Prefix] {
		return func(func(netip.Prefix) bool) {}
	},
}

table, err := NewTable(
	db,
	"test",
	keyIndex,         // Primary index
	emptyPrefixIndex, // Secondary index
)

Now suppose that this object with an empty primary key is in the table:

testObject{
	Key:  "",
	Tags: part.NewSet("test-object"),
}

Then this query:

queryObject := &testObject{Key: "unrelated"}
q := emptyPrefixIndex.QueryFromObject(queryObject)

table.Get(rtxn, q)

returns the stored testObject, despite the object we used to generate the query had a Key value equal to "unrelated" (IOW: from a primary key POV it should not match).

Even if I don't think it is dangerous, it feels inconsistent. Getting this right seems hard, that's why I wonder if we should instead return a "no-match" representation instead of steering an empty index name toward the primary index.

@tklauser tklauser Jun 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's a good point. I didn't consider this in my original change. I agree that getting this right is rather tricky, so I'll move the PR to draft for now and will discuss it with @joamaki after he's back.

return PrimaryIndexPos
}

// By default don't consider the internal indexes.
start := PrimaryIndexPos

Expand Down
Loading