From b0c6941deb4e1fd1dfbc80a61d375282938f0e07 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Thu, 4 May 2023 11:26:57 -0400 Subject: [PATCH] fix(orm): fix failing tests (#16023) --- .golangci.yml | 1 + orm/CHANGELOG.md | 4 + orm/encoding/ormfield/duration.go | 16 +- orm/encoding/ormfield/duration_test.go | 259 +++++++++++++++----- orm/encoding/ormkv/key_codec.go | 7 +- orm/encoding/ormkv/unique_key.go | 5 +- x/group/internal/orm/index_property_test.go | 2 +- 7 files changed, 216 insertions(+), 78 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index cef776e97b..7d7fca0fd8 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -117,3 +117,4 @@ linters-settings: disabled-checks: - regexpMust - appendAssign + - ifElseChain diff --git a/orm/CHANGELOG.md b/orm/CHANGELOG.md index 6af499d5af..ba73a0d75a 100644 --- a/orm/CHANGELOG.md +++ b/orm/CHANGELOG.md @@ -49,3 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#12273](https://github.com/cosmos/cosmos-sdk/pull/12273) The timestamp key encoding was reworked to properly handle nil values. Existing users will need to manually migrate their data to the new encoding before upgrading. * [#15138](https://github.com/cosmos/cosmos-sdk/pull/15138) The duration key encoding was reworked to properly handle nil values. Existing users will need to manually migrate their data to the new encoding before upgrading. + +### Bug Fixes + +* [#16023](https://github.com/cosmos/cosmos-sdk/pull/16023) Fix bugs introduced by lack of CI tests in [#15138](https://github.com/cosmos/cosmos-sdk/pull/15138) and [#15813](https://github.com/cosmos/cosmos-sdk/pull/15813). This changes the duration encoding in [#15138](https://github.com/cosmos/cosmos-sdk/pull/15138) to correctly order values with negative nanos. diff --git a/orm/encoding/ormfield/duration.go b/orm/encoding/ormfield/duration.go index ad47173e3f..6b4b42fabe 100644 --- a/orm/encoding/ormfield/duration.go +++ b/orm/encoding/ormfield/duration.go @@ -14,6 +14,18 @@ const ( DurationNanosMax = 999999999 ) +// DurationCodec encodes google.protobuf.Duration values with the following +// encoding: +// - nil is encoded as []byte{0xFF} +// - seconds (which can range from -315,576,000,000 to +315,576,000,000) is encoded as 5 fixed bytes +// - nanos (which can range from 0 to 999,999,999 or -999,999,999 to 0 if seconds is negative) is encoded as: +// - []byte{0x0} for zero nanos +// - 4 fixed bytes with the bit mask 0xC0 applied to the first byte, with negative nanos scaled so that -999,999,999 +// is encoded as 1 and -1 is encoded as 999,999,999 +// +// When iterating over timestamp indexes, nil values will always be ordered last. +// +// Values for seconds and nanos outside the ranges specified by google.protobuf.Duration will be rejected. type DurationCodec struct{} func (d DurationCodec) Encode(value protoreflect.Value, w io.Writer) error { @@ -46,7 +58,7 @@ func (d DurationCodec) Encode(value protoreflect.Value, w io.Writer) error { if nanosInt < DurationNanosMin || nanosInt > 0 { return fmt.Errorf("negative duration nanos is out of range %d, must be between %d and %d", nanosInt, DurationNanosMin, 0) } - nanosInt = -nanosInt + nanosInt = DurationNanosMax + nanosInt + 1 } else if nanosInt < 0 || nanosInt > DurationNanosMax { return fmt.Errorf("duration nanos is out of range %d, must be between %d and %d", nanosInt, 0, DurationNanosMax) } @@ -78,7 +90,7 @@ func (d DurationCodec) Decode(r Reader) (protoreflect.Value, error) { } if negative { - nanos = -nanos + nanos = nanos - DurationNanosMax - 1 } msg.Set(durationNanosField, protoreflect.ValueOfInt32(nanos)) diff --git a/orm/encoding/ormfield/duration_test.go b/orm/encoding/ormfield/duration_test.go index 285d86159d..0e0d1d1217 100644 --- a/orm/encoding/ormfield/duration_test.go +++ b/orm/encoding/ormfield/duration_test.go @@ -12,79 +12,81 @@ import ( "cosmossdk.io/orm/encoding/ormfield" ) +func TestDurationNil(t *testing.T) { + t.Parallel() + + cdc := ormfield.DurationCodec{} + buf := &bytes.Buffer{} + assert.NilError(t, cdc.Encode(protoreflect.Value{}, buf)) + assert.Equal(t, 1, len(buf.Bytes())) + val, err := cdc.Decode(buf) + assert.NilError(t, err) + assert.Assert(t, !val.IsValid()) +} + func TestDuration(t *testing.T) { t.Parallel() cdc := ormfield.DurationCodec{} - // nil value - t.Run("nil value", func(t *testing.T) { - t.Parallel() - buf := &bytes.Buffer{} - assert.NilError(t, cdc.Encode(protoreflect.Value{}, buf)) - assert.Equal(t, 1, len(buf.Bytes())) - val, err := cdc.Decode(buf) - assert.NilError(t, err) - assert.Assert(t, !val.IsValid()) - }) + tt := []struct { + name string + seconds int64 + nanos int32 + wantLen int + }{ + { + "no nanos", + 100, + 0, + 6, + }, + { + "with nanos", + 3, + 879468295, + 9, + }, + { + "min seconds, -1 nanos", + -315576000000, + -1, + 9, + }, + { + "min value", + -315576000000, + -999999999, + 9, + }, + { + "max value", + 315576000000, + 999999999, + 9, + }, + { + "max seconds, 1 nanos", + 315576000000, + 1, + 9, + }, + } - // no nanos - t.Run("no nanos", func(t *testing.T) { - t.Parallel() - dur, err := time.ParseDuration("100s") - assert.NilError(t, err) - durPb := durationpb.New(dur) - val := protoreflect.ValueOfMessage(durPb.ProtoReflect()) - buf := &bytes.Buffer{} - assert.NilError(t, cdc.Encode(val, buf)) - assert.Equal(t, 6, len(buf.Bytes())) - val2, err := cdc.Decode(buf) - assert.NilError(t, err) - assert.Equal(t, 0, cdc.Compare(val, val2)) - }) - - t.Run("nanos", func(t *testing.T) { - t.Parallel() - dur, err := time.ParseDuration("3879468295ns") - assert.NilError(t, err) - durPb := durationpb.New(dur) - val := protoreflect.ValueOfMessage(durPb.ProtoReflect()) - buf := &bytes.Buffer{} - assert.NilError(t, cdc.Encode(val, buf)) - assert.Equal(t, 9, len(buf.Bytes())) - val2, err := cdc.Decode(buf) - assert.NilError(t, err) - assert.Equal(t, 0, cdc.Compare(val, val2)) - }) - - t.Run("min value", func(t *testing.T) { - t.Parallel() - durPb := &durationpb.Duration{ - Seconds: -315576000000, - Nanos: -999999999, - } - val := protoreflect.ValueOfMessage(durPb.ProtoReflect()) - buf := &bytes.Buffer{} - assert.NilError(t, cdc.Encode(val, buf)) - assert.Equal(t, 9, len(buf.Bytes())) - val2, err := cdc.Decode(buf) - assert.NilError(t, err) - assert.Equal(t, 0, cdc.Compare(val, val2)) - }) - - t.Run("max value", func(t *testing.T) { - t.Parallel() - durPb := &durationpb.Duration{ - Seconds: 315576000000, - Nanos: 999999999, - } - val := protoreflect.ValueOfMessage(durPb.ProtoReflect()) - buf := &bytes.Buffer{} - assert.NilError(t, cdc.Encode(val, buf)) - assert.Equal(t, 9, len(buf.Bytes())) - val2, err := cdc.Decode(buf) - assert.NilError(t, err) - assert.Equal(t, 0, cdc.Compare(val, val2)) - }) + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + durPb := &durationpb.Duration{ + Seconds: tc.seconds, + Nanos: tc.nanos, + } + val := protoreflect.ValueOfMessage(durPb.ProtoReflect()) + buf := &bytes.Buffer{} + assert.NilError(t, cdc.Encode(val, buf)) + assert.Equal(t, tc.wantLen, len(buf.Bytes())) + val2, err := cdc.Decode(buf) + assert.NilError(t, err) + assert.Equal(t, 0, cdc.Compare(val, val2)) + }) + } } func TestDurationOutOfRange(t *testing.T) { @@ -156,3 +158,124 @@ func TestDurationOutOfRange(t *testing.T) { }) } } + +func TestDurationCompare(t *testing.T) { + t.Parallel() + cdc := ormfield.DurationCodec{} + + tt := []struct { + name string + dur1 *durationpb.Duration + dur2 *durationpb.Duration + want int + }{ + { + name: "equal", + dur1: &durationpb.Duration{ + Seconds: 1, + Nanos: 1, + }, + dur2: &durationpb.Duration{ + Seconds: 1, + Nanos: 1, + }, + want: 0, + }, + { + name: "seconds equal, dur1 nanos less than dur2 nanos", + dur1: &durationpb.Duration{ + Seconds: 1, + Nanos: 1, + }, + dur2: &durationpb.Duration{ + Seconds: 1, + Nanos: 2, + }, + want: -1, + }, + { + name: "seconds equal, dur1 nanos greater than dur2 nanos", + dur1: &durationpb.Duration{ + Seconds: 1, + Nanos: 2, + }, + dur2: &durationpb.Duration{ + Seconds: 1, + Nanos: 1, + }, + want: 1, + }, + { + name: "seconds less than", + dur1: &durationpb.Duration{ + Seconds: 1, + Nanos: 1, + }, + dur2: &durationpb.Duration{ + Seconds: 2, + Nanos: 1, + }, + want: -1, + }, + { + name: "seconds greater than", + dur1: &durationpb.Duration{ + Seconds: 2, + Nanos: 1, + }, + dur2: &durationpb.Duration{ + Seconds: 1, + Nanos: 1, + }, + want: 1, + }, + { + name: "negative seconds equal, dur1 nanos less than dur2 nanos", + dur1: &durationpb.Duration{ + Seconds: -1, + Nanos: -2, + }, + dur2: &durationpb.Duration{ + Seconds: -1, + Nanos: -1, + }, + want: -1, + }, + } + + for _, tc := range tt { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + val1 := protoreflect.ValueOfMessage(tc.dur1.ProtoReflect()) + val2 := protoreflect.ValueOfMessage(tc.dur2.ProtoReflect()) + got := cdc.Compare(val1, val2) + assert.Equal(t, tc.want, got, "Compare(%v, %v)", tc.dur1, tc.dur2) + + bz1 := encodeValue(t, cdc, val1) + bz2 := encodeValue(t, cdc, val2) + assert.Equal(t, tc.want, bytes.Compare(bz1, bz2), "bytes.Compare(%v, %v)", bz1, bz2) + }) + } + + t.Run("nanos", func(t *testing.T) { + t.Parallel() + dur, err := time.ParseDuration("3879468295ns") + assert.NilError(t, err) + durPb := durationpb.New(dur) + val := protoreflect.ValueOfMessage(durPb.ProtoReflect()) + buf := &bytes.Buffer{} + assert.NilError(t, cdc.Encode(val, buf)) + assert.Equal(t, 9, len(buf.Bytes())) + val2, err := cdc.Decode(buf) + assert.NilError(t, err) + assert.Equal(t, 0, cdc.Compare(val, val2)) + }) +} + +func encodeValue(t *testing.T, cdc ormfield.Codec, val protoreflect.Value) []byte { + buf := &bytes.Buffer{} + assert.NilError(t, cdc.Encode(val, buf)) + return buf.Bytes() +} diff --git a/orm/encoding/ormkv/key_codec.go b/orm/encoding/ormkv/key_codec.go index cc7b87b8e6..b1056aaf3a 100644 --- a/orm/encoding/ormkv/key_codec.go +++ b/orm/encoding/ormkv/key_codec.go @@ -247,20 +247,19 @@ func (cdc KeyCodec) CheckValidRangeIterationKeys(start, end []protoreflect.Value y := end[i] cmp = fieldCdc.Compare(x, y) - switch { - case cmp > 0: + if cmp > 0 { return ormerrors.InvalidRangeIterationKeys.Wrapf( "start must be before end for field %s", cdc.fieldDescriptors[i].FullName(), ) - case !fieldCdc.IsOrdered() && cmp != 0: + } else if !fieldCdc.IsOrdered() && cmp != 0 { descriptor := cdc.fieldDescriptors[i] return ormerrors.InvalidRangeIterationKeys.Wrapf( "field %s of kind %s doesn't support ordered range iteration", descriptor.FullName(), descriptor.Kind(), ) - case cmp < 0: + } else if cmp < 0 { break } } diff --git a/orm/encoding/ormkv/unique_key.go b/orm/encoding/ormkv/unique_key.go index 76ac788a50..7a8955d1a5 100644 --- a/orm/encoding/ormkv/unique_key.go +++ b/orm/encoding/ormkv/unique_key.go @@ -160,9 +160,8 @@ func (u UniqueKeyCodec) EncodeEntry(entry Entry) (k, v []byte, err error) { if !fieldOrder.inKey { // goes in values because it is not present in the index key otherwise values = append(values, value) - } - // does not go in values, but we need to verify that the value in index values matches the primary key value - if u.keyCodec.fieldCodecs[fieldOrder.i].Compare(value, indexEntry.IndexValues[fieldOrder.i]) != 0 { + } else if u.keyCodec.fieldCodecs[fieldOrder.i].Compare(value, indexEntry.IndexValues[fieldOrder.i]) != 0 { + // does not go in values, but we need to verify that the value in index values matches the primary key value return nil, nil, ormerrors.BadDecodeEntry.Wrapf("value in primary key does not match corresponding value in index key") } } diff --git a/x/group/internal/orm/index_property_test.go b/x/group/internal/orm/index_property_test.go index 4c42bceb2f..e48de3c122 100644 --- a/x/group/internal/orm/index_property_test.go +++ b/x/group/internal/orm/index_property_test.go @@ -46,7 +46,7 @@ func TestPrefixRangeProperty(t *testing.T) { // index, one greater at overflow and 0 from // then on for i, b := range start { - if i < overflowIndex { //nolint:gocritic // ifElseChain: rewrite if-else to switch statement + if i < overflowIndex { require.Equal(t, b, end[i]) } else if i == overflowIndex { require.Equal(t, b+1, end[i])