Skip to content

Commit

Permalink
refactor: Address first batch of changes from orm audit (#490)
Browse files Browse the repository at this point in the history
Address first batch of updates from ORM internal API audit #490:

Change Save to Update
Refactor TableExportable interface to not expose private table structure
Add Set function
Change Create to check for non-existence
Rename AfterSaveInterceptor to AfterSetInterceptor for consistency

Closes #490
  • Loading branch information
ruhatch authored Sep 7, 2021
1 parent cab89d9 commit 4e3666b
Show file tree
Hide file tree
Showing 17 changed files with 255 additions and 225 deletions.
61 changes: 45 additions & 16 deletions orm/auto_uint64.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package orm
import (
"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/errors"
)

var _ Indexable = &AutoUInt64TableBuilder{}
Expand Down Expand Up @@ -37,7 +38,6 @@ func (a AutoUInt64TableBuilder) Build() AutoUInt64Table {
}
}

var _ SequenceExportable = &AutoUInt64Table{}
var _ TableExportable = &AutoUInt64Table{}

// AutoUInt64Table is the table type which an auto incrementing ID.
Expand All @@ -46,8 +46,11 @@ type AutoUInt64Table struct {
seq Sequence
}

// Create a new persistent object with an auto generated uint64 primary key. They key is returned.
// Create iterates though the registered callbacks and may add secondary index keys by them.
// Create a new persistent object with an auto generated uint64 primary key. The
// key is returned.
//
// Create iterates through the registered callbacks that may add secondary index
// keys.
func (a AutoUInt64Table) Create(ctx HasKVStore, obj codec.ProtoMarshaler) (uint64, error) {
autoIncID := a.seq.NextVal(ctx)
err := a.table.Create(ctx, EncodeSequence(autoIncID), obj)
Expand All @@ -57,20 +60,32 @@ func (a AutoUInt64Table) Create(ctx HasKVStore, obj codec.ProtoMarshaler) (uint6
return autoIncID, nil
}

// Save updates the given object under the rowID key. It expects the key to exists already
// and fails with an `ErrNotFound` otherwise. Any caller must therefore make sure that this contract
// is fulfilled. Parameters must not be nil.
// Update updates the given object under the rowID key. It expects the key to
// exists already and fails with an `ErrNotFound` otherwise. Any caller must
// therefore make sure that this contract is fulfilled. Parameters must not be
// nil.
//
// Save iterates though the registered callbacks and may add or remove secondary index keys by them.
func (a AutoUInt64Table) Save(ctx HasKVStore, rowID uint64, newValue codec.ProtoMarshaler) error {
return a.table.Save(ctx, EncodeSequence(rowID), newValue)
// Update iterates through the registered callbacks that may add or remove
// secondary index keys.
func (a AutoUInt64Table) Update(ctx HasKVStore, rowID uint64, newValue codec.ProtoMarshaler) error {
return a.table.Update(ctx, EncodeSequence(rowID), newValue)
}

// Delete removes the object under the rowID key. It expects the key to exists already
// and fails with a `ErrNotFound` otherwise. Any caller must therefore make sure that this contract
// is fulfilled.
// Set persists the given object under the rowID key. It does not check if the
// key already exists and overwrites the value if it does.
//
// Delete iterates though the registered callbacks and removes secondary index keys by them.
// Set iterates through the registered callbacks that may add secondary index
// keys.
func (a AutoUInt64Table) Set(ctx HasKVStore, rowID uint64, newValue codec.ProtoMarshaler) error {
return a.table.Set(ctx, EncodeSequence(rowID), newValue)
}

// Delete removes the object under the rowID key. It expects the key to exists
// already and fails with a `ErrNotFound` otherwise. Any caller must therefore
// make sure that this contract is fulfilled.
//
// Delete iterates through the registered callbacks that remove secondary index
// keys.
func (a AutoUInt64Table) Delete(ctx HasKVStore, rowID uint64) error {
return a.table.Delete(ctx, EncodeSequence(rowID))
}
Expand Down Expand Up @@ -128,7 +143,21 @@ func (a AutoUInt64Table) Sequence() Sequence {
return a.seq
}

// Table satisfies the TableExportable interface and must not be used otherwise.
func (a AutoUInt64Table) Table() table {
return a.table
// Export stores all the values in the table in the passed ModelSlicePtr and
// returns the current value of the associated sequence.
func (a AutoUInt64Table) Export(ctx HasKVStore, dest ModelSlicePtr) (uint64, error) {
_, err := a.table.Export(ctx, dest)
if err != nil {
return 0, err
}
return a.seq.CurVal(ctx), nil
}

// Import clears the table and initializes it from the given data interface{}.
// data should be a slice of structs that implement PrimaryKeyed.
func (a AutoUInt64Table) Import(ctx HasKVStore, data interface{}, seqValue uint64) error {
if err := a.seq.InitVal(ctx, seqValue); err != nil {
return errors.Wrap(err, "sequence")
}
return a.table.Import(ctx, data, seqValue)
}
93 changes: 10 additions & 83 deletions orm/genesis.go
Original file line number Diff line number Diff line change
@@ -1,88 +1,15 @@
package orm

import (
"reflect"

"github.com/cosmos/cosmos-sdk/store/prefix"
"github.com/cosmos/cosmos-sdk/types/errors"
)

// TableExportable
type TableExportable interface {
// Table returns the table to export
Table() table
}

// SequenceExportable
type SequenceExportable interface {
// Sequence returns the sequence to export
Sequence() Sequence
}

// ExportTableData iterates over the given table entries and stores them at the passed ModelSlicePtr.
// When the given table implements the `SequenceExportable` interface then it's current value
// is returned as well or otherwise defaults to 0.
func ExportTableData(ctx HasKVStore, t TableExportable, dest ModelSlicePtr) (uint64, error) {
it, err := t.Table().PrefixScan(ctx, nil, nil)
if err != nil {
return 0, errors.Wrap(err, "table PrefixScan failure when exporting table data")
}
_, err = ReadAll(it, dest)
if err != nil {
return 0, err
}
var seqValue uint64
if st, ok := t.(SequenceExportable); ok {
seqValue = st.Sequence().CurVal(ctx)
}
return seqValue, err
}

// ImportTableData initializes a table and attaches indexers from the given data interface{}.
// data should be a slice of structs that implement PrimaryKeyed (eg []*GroupInfo).
// The seqValue is optional and only used with tables that implement the `SequenceExportable` interface.
func ImportTableData(ctx HasKVStore, t TableExportable, data interface{}, seqValue uint64) error {
table := t.Table()
if err := clearAllInTable(ctx, table); err != nil {
return errors.Wrap(err, "clear old entries")
}

if st, ok := t.(SequenceExportable); ok {
if err := st.Sequence().InitVal(ctx, seqValue); err != nil {
return errors.Wrap(err, "sequence")
}
}

// Provided data must be a slice
modelSlice := reflect.ValueOf(data)
if modelSlice.Kind() != reflect.Slice {
return errors.Wrap(ErrArgument, "data must be a slice")
}

// Create table entries
for i := 0; i < modelSlice.Len(); i++ {
obj, ok := modelSlice.Index(i).Interface().(PrimaryKeyed)
if !ok {
return errors.Wrapf(ErrArgument, "unsupported type :%s", reflect.TypeOf(data).Elem().Elem())
}
err := table.Create(ctx, PrimaryKey(obj), obj)
if err != nil {
return err
}
}

return nil
}

// clearAllInTable deletes all entries in a table with delete interceptors called
func clearAllInTable(ctx HasKVStore, table table) error {
store := prefix.NewStore(ctx.KVStore(table.storeKey), []byte{table.prefix})
it := store.Iterator(nil, nil)
defer it.Close()
for ; it.Valid(); it.Next() {
if err := table.Delete(ctx, it.Key()); err != nil {
return err
}
}
return nil
// Export stores all the values in the table in the passed
// ModelSlicePtr. If the table has an associated sequence, then its
// current value is returned, otherwise 0 is returned by default.
Export(HasKVStore, ModelSlicePtr) (uint64, error)

// Import clears the table and initializes it from the given data
// interface{}. data should be a slice of structs that implement
// PrimaryKeyed (eg []*GroupInfo). The seqValue is optional and only
// used with tables that have an associated sequence.
Import(HasKVStore, interface{}, uint64) error
}
4 changes: 2 additions & 2 deletions orm/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestImportExportTableData(t *testing.T) {
},
}

err = orm.ImportTableData(ctx, table, groups, 2)
err = table.Import(ctx, groups, 2)
require.NoError(t, err)

for _, g := range groups {
Expand All @@ -46,7 +46,7 @@ func TestImportExportTableData(t *testing.T) {
}

var exported []*testdata.GroupInfo
seq, err := orm.ExportTableData(ctx, table, &exported)
seq, err := table.Export(ctx, &exported)
require.NoError(t, err)
require.Equal(t, seq, uint64(2))

Expand Down
2 changes: 1 addition & 1 deletion orm/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ require (
github.com/gogo/protobuf v1.3.3
github.com/stretchr/testify v1.7.0
github.com/tendermint/tm-db v0.6.4
pgregory.net/rapid v0.4.7 // indirect
pgregory.net/rapid v0.4.7
)

replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
4 changes: 2 additions & 2 deletions orm/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func newIndex(builder Indexable, prefix byte, indexer *Indexer) (MultiKeyIndex,
indexer: indexer,
indexKeyCodec: codec,
}
builder.AddAfterSaveInterceptor(idx.onSave)
builder.AddAfterSetInterceptor(idx.onSet)
builder.AddAfterDeleteInterceptor(idx.onDelete)
return idx, nil
}
Expand Down Expand Up @@ -135,7 +135,7 @@ func (i MultiKeyIndex) ReversePrefixScan(ctx HasKVStore, start []byte, end []byt
return indexIterator{ctx: ctx, it: it, rowGetter: i.rowGetter, keyCodec: i.indexKeyCodec}, nil
}

func (i MultiKeyIndex) onSave(ctx HasKVStore, rowID RowID, newValue, oldValue codec.ProtoMarshaler) error {
func (i MultiKeyIndex) onSet(ctx HasKVStore, rowID RowID, newValue, oldValue codec.ProtoMarshaler) error {
store := prefix.NewStore(ctx.KVStore(i.storeKey), []byte{i.prefix})
if oldValue == nil {
return i.indexer.OnCreate(store, rowID, newValue)
Expand Down
6 changes: 3 additions & 3 deletions orm/index_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (b *nilCodecBuilder) RowGetter() orm.RowGetter {
return func(a orm.HasKVStore, b orm.RowID, c codec.ProtoMarshaler) error { return nil }
}
func (b *nilCodecBuilder) IndexKeyCodec() orm.IndexKeyCodec { return nil }
func (b *nilCodecBuilder) AddAfterSaveInterceptor(orm.AfterSaveInterceptor) {}
func (b *nilCodecBuilder) AddAfterSetInterceptor(orm.AfterSetInterceptor) {}
func (b *nilCodecBuilder) AddAfterDeleteInterceptor(orm.AfterDeleteInterceptor) {}

type nilStoreKeyBuilder struct{}
Expand All @@ -39,7 +39,7 @@ func (b *nilStoreKeyBuilder) RowGetter() orm.RowGetter {
func (b *nilStoreKeyBuilder) IndexKeyCodec() orm.IndexKeyCodec {
return orm.Max255DynamicLengthIndexKeyCodec{}
}
func (b *nilStoreKeyBuilder) AddAfterSaveInterceptor(orm.AfterSaveInterceptor) {}
func (b *nilStoreKeyBuilder) AddAfterSetInterceptor(orm.AfterSetInterceptor) {}
func (b *nilStoreKeyBuilder) AddAfterDeleteInterceptor(orm.AfterDeleteInterceptor) {}

type nilRowGetterBuilder struct{}
Expand All @@ -53,7 +53,7 @@ func (b *nilRowGetterBuilder) RowGetter() orm.RowGetter {
func (b *nilRowGetterBuilder) IndexKeyCodec() orm.IndexKeyCodec {
return orm.Max255DynamicLengthIndexKeyCodec{}
}
func (b *nilRowGetterBuilder) AddAfterSaveInterceptor(orm.AfterSaveInterceptor) {}
func (b *nilRowGetterBuilder) AddAfterSetInterceptor(orm.AfterSetInterceptor) {}
func (b *nilRowGetterBuilder) AddAfterDeleteInterceptor(orm.AfterDeleteInterceptor) {}

func TestNewIndex(t *testing.T) {
Expand Down
8 changes: 4 additions & 4 deletions orm/orm.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func (r RowID) Bytes() []byte {
return r
}

// Validateable is an interface that Persistent types can implement and is called on any orm save or update operation.
// Validateable is an interface that Persistent types can implement and is called on any orm set operation.
type Validateable interface {
// ValidateBasic is a sanity check on the data. Any error returned prevents create or updates.
ValidateBasic() error
Expand Down Expand Up @@ -131,12 +131,12 @@ type Indexable interface {
StoreKey() sdk.StoreKey
RowGetter() RowGetter
IndexKeyCodec() IndexKeyCodec
AddAfterSaveInterceptor(interceptor AfterSaveInterceptor)
AddAfterSetInterceptor(interceptor AfterSetInterceptor)
AddAfterDeleteInterceptor(interceptor AfterDeleteInterceptor)
}

// AfterSaveInterceptor defines a callback function to be called on Create + Update.
type AfterSaveInterceptor func(ctx HasKVStore, rowID RowID, newValue, oldValue codec.ProtoMarshaler) error
// AfterSetInterceptor defines a callback function to be called on Create + Update.
type AfterSetInterceptor func(ctx HasKVStore, rowID RowID, newValue, oldValue codec.ProtoMarshaler) error

// AfterDeleteInterceptor defines a callback function to be called on Delete operations.
type AfterDeleteInterceptor func(ctx HasKVStore, rowID RowID, value codec.ProtoMarshaler) error
Expand Down
14 changes: 7 additions & 7 deletions orm/orm_scenario_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func TestKeeperEndToEndWithAutoUInt64Table(t *testing.T) {

// when updated
g.Admin = []byte("new-admin-address")
err = k.groupTable.Save(ctx, rowID, &g)
err = k.groupTable.Update(ctx, rowID, &g)
require.NoError(t, err)

// then indexes are updated, too
Expand Down Expand Up @@ -152,7 +152,7 @@ func TestKeeperEndToEndWithPrimaryKeyTable(t *testing.T) {
Weight: m.Weight,
}
// then it should fail as the primary key is immutable
err = k.groupMemberTable.Save(ctx, updatedMember)
err = k.groupMemberTable.Update(ctx, updatedMember)
require.Error(t, err)

// and when entity updated with non primary key attribute modified
Expand All @@ -162,7 +162,7 @@ func TestKeeperEndToEndWithPrimaryKeyTable(t *testing.T) {
Weight: 99,
}
// then it should not fail
err = k.groupMemberTable.Save(ctx, updatedMember)
err = k.groupMemberTable.Update(ctx, updatedMember)
require.NoError(t, err)

// and when entity deleted
Expand Down Expand Up @@ -298,13 +298,13 @@ func TestExportImportStateAutoUInt64Table(t *testing.T) {
require.Equal(t, uint64(i), groupRowID)
}
var groups []*testdata.GroupInfo
seqVal, err := orm.ExportTableData(ctx, k.groupTable, &groups)
seqVal, err := k.groupTable.Export(ctx, &groups)
require.NoError(t, err)

// when a new db seeded
ctx = orm.NewMockContext()

err = orm.ImportTableData(ctx, k.groupTable, groups, seqVal)
err = k.groupTable.Import(ctx, groups, seqVal)
require.NoError(t, err)
// then all data is set again

Expand Down Expand Up @@ -354,13 +354,13 @@ func TestExportImportStatePrimaryKeyTable(t *testing.T) {
testRecords[i-1] = g
}
var groupMembers []*testdata.GroupMember
_, err := orm.ExportTableData(ctx, k.groupMemberTable, &groupMembers)
_, err := k.groupMemberTable.Export(ctx, &groupMembers)
require.NoError(t, err)

// when a new db seeded
ctx = orm.NewMockContext()

err = orm.ImportTableData(ctx, k.groupMemberTable, groupMembers, 0)
err = k.groupMemberTable.Import(ctx, groupMembers, 0)
require.NoError(t, err)

// then all data is set again
Expand Down
Loading

0 comments on commit 4e3666b

Please sign in to comment.