Skip to content

Commit

Permalink
reproduce bug in HCLRevisionFromString when parsing zero logical clock
Browse files Browse the repository at this point in the history
While developing the test-case for `EmitImmediatelyStrategy` with CRDB,
I detected something I wasn't expecting: when comparing the revision out of the
datastore write, and what came out of Watch API, the revisions were different.

We use `SHOW COMMIT TIMESTAMP` to retrieve a CRDB [transaction timestamp]
(cockroachdb/cockroach#80848). The value coming out of
the `update` field in change streams had the same value, but the difference was
the notation: one included the logical clock, the other didn't, but logical
clocks were the same (zero):

- revision generated out of the transaction in `readTransactionCommitRev` uses
  `NewForHLC(hlcNow)`, which takes a `Decimal`. This in turn calls
  `decimal.String()`, which returns a number stripped out of the decimal part if
  it's zero.
- revision obtained out of the changefeeds uses `revisions.HLCRevisionFromString
  (details.Updated)`, and `details.Updated` always comes with the decimal part,
  even when it's zero

Both timestamps were the same, but had a different string representation,
and led to a different `HCLRevision` logical lock,
hence `.Equal()` method failing.
  • Loading branch information
vroldanbet committed Nov 6, 2024
1 parent 686ae94 commit 715e07f
Showing 1 changed file with 70 additions and 15 deletions.
85 changes: 70 additions & 15 deletions internal/datastore/revisions/hlcrevision_test.go
Original file line number Diff line number Diff line change
@@ -1,34 +1,43 @@
package revisions

import (
"strconv"
"testing"
"time"

"github.com/authzed/spicedb/pkg/datastore"

"github.com/shopspring/decimal"
"github.com/stretchr/testify/require"
)

func TestNewForHLC(t *testing.T) {
tcs := []string{
"1",
"2",
"42",
"1257894000000000000",
"-1",
"1.0000000023",
"1703283409994227985.0000000004",
"1703283409994227985.0000000040",
"1703283409994227985.0010000000",
tcs := map[string]string{
"1": "1.0000000000",
"2": "2.0000000000",
"42": "42.0000000000",
"1257894000000000000": "1257894000000000000.0000000000",
"-1": "-1.0000000000",
"1.0000000023": "1.0000000023",
"1703283409994227985.0000000004": "1703283409994227985.0000000004",
"1703283409994227985.0000000040": "1703283409994227985.0000000040",
"1703283409994227985.0010000000": "1703283409994227985.0010000000",
"1730898575294981085.0000000000": "1730898575294981085.0000000000",
}

for _, tc := range tcs {
t.Run(tc, func(t *testing.T) {
d, err := decimal.NewFromString(tc)
for inputTimestamp, expectedTimestamp := range tcs {
t.Run(inputTimestamp, func(t *testing.T) {
d, err := decimal.NewFromString(inputTimestamp)
require.NoError(t, err)

rev, err := NewForHLC(d)
require.NoError(t, err)
require.Equal(t, tc, rev.String())
revFromString, err := HLCRevisionFromString(inputTimestamp)
require.NoError(t, err)
require.True(t, rev.Equal(revFromString), "expected equal, got %v and %v", rev, revFromString)

require.Equal(t, expectedTimestamp, rev.String())
require.Equal(t, expectedTimestamp, revFromString.String())
})
}
}
Expand Down Expand Up @@ -56,6 +65,28 @@ func TestTimestampNanoSec(t *testing.T) {
}
}

func TestConstructForTimestamp(t *testing.T) {
tcs := map[int64]string{
1: "1.0000000000",
2: "2.0000000000",
42: "42.0000000000",
1257894000000000000: "1257894000000000000.0000000000",
-1: "-1.0000000000",
9223372036854775807: "9223372036854775807.0000000000",
1703283409994227985: "1703283409994227985.0000000000",
}

for input, output := range tcs {
t.Run(strconv.Itoa(int(input)), func(t *testing.T) {
rev := zeroHLC
withTimestamp := rev.ConstructForTimestamp(input)
withTimestamp.String()
require.Equal(t, output, withTimestamp.String())
require.Equal(t, input, withTimestamp.TimestampNanoSec())
})
}
}

func TestInexactFloat64(t *testing.T) {
tcs := map[string]float64{
"1": 1,
Expand Down Expand Up @@ -85,10 +116,18 @@ func TestInexactFloat64(t *testing.T) {

func TestNewHLCForTime(t *testing.T) {
time := time.Now()
rev := NewForTime(time)
rev := NewHLCForTime(time)
require.Equal(t, time.UnixNano(), rev.TimestampNanoSec())
}

func TestNoRevision(t *testing.T) {
rev, err := HLCRevisionFromString("0")
require.NoError(t, err)
require.False(t, rev.Equal(datastore.NoRevision))
require.True(t, rev.GreaterThan(datastore.NoRevision))
require.False(t, rev.LessThan(datastore.NoRevision))
}

func TestHLCKeyEquals(t *testing.T) {
tcs := []struct {
left string
Expand Down Expand Up @@ -206,6 +245,22 @@ func TestHLCKeyLessThanFunc(t *testing.T) {
}
}

func TestHLCFromStringError(t *testing.T) {
tcs := map[string]string{
"1a": "invalid revision string",
"1.0.0": "invalid revision string",
"1a.0": "invalid revision string",
"1.0a": "invalid revision string",
}

for tc, expectedErr := range tcs {
t.Run(tc, func(t *testing.T) {
_, err := HLCRevisionFromString(tc)
require.ErrorContains(t, err, expectedErr)
})
}
}

func TestHLCToFromDecimal(t *testing.T) {
tcs := []string{
"1",
Expand Down

0 comments on commit 715e07f

Please sign in to comment.