From fd5a6cd93decb3565a112497fdc92e3e6877d07e Mon Sep 17 00:00:00 2001
From: Joseph Schorr <josephschorr@users.noreply.github.com>
Date: Mon, 29 Jan 2024 18:14:33 -0500
Subject: [PATCH] Ensure NewForHLC returns an error if the incoming decimal is
 invalid

This shouldn't be possible, but there are some indications it might be
---
 internal/datastore/crdb/crdb.go               |  2 +-
 internal/datastore/crdb/stats.go              |  2 +-
 internal/datastore/revisions/hlcrevision.go   | 10 ++++++---
 .../datastore/revisions/hlcrevision_test.go   |  3 ++-
 pkg/zedtoken/zedtoken_test.go                 | 22 ++++++++++++++-----
 5 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/internal/datastore/crdb/crdb.go b/internal/datastore/crdb/crdb.go
index b74423fd7d..069f21f625 100644
--- a/internal/datastore/crdb/crdb.go
+++ b/internal/datastore/crdb/crdb.go
@@ -478,7 +478,7 @@ func readCRDBNow(ctx context.Context, reader pgxcommon.DBFuncQuerier) (datastore
 		return datastore.NoRevision, fmt.Errorf("unable to read timestamp: %w", err)
 	}
 
-	return revisions.NewForHLC(hlcNow), nil
+	return revisions.NewForHLC(hlcNow)
 }
 
 func readClusterTTLNanos(ctx context.Context, conn pgxcommon.DBFuncQuerier) (int64, error) {
diff --git a/internal/datastore/crdb/stats.go b/internal/datastore/crdb/stats.go
index 4d9f35a1f8..abebe1b1f5 100644
--- a/internal/datastore/crdb/stats.go
+++ b/internal/datastore/crdb/stats.go
@@ -105,5 +105,5 @@ func updateCounter(ctx context.Context, tx pgx.Tx, change int64) (datastore.Revi
 		return datastore.NoRevision, fmt.Errorf("unable to executed upsert counter query: %w", err)
 	}
 
-	return revisions.NewForHLC(timestamp), nil
+	return revisions.NewForHLC(timestamp)
 }
diff --git a/internal/datastore/revisions/hlcrevision.go b/internal/datastore/revisions/hlcrevision.go
index 6f14ced5f8..03a1299a07 100644
--- a/internal/datastore/revisions/hlcrevision.go
+++ b/internal/datastore/revisions/hlcrevision.go
@@ -73,9 +73,13 @@ func HLCRevisionFromString(revisionStr string) (HLCRevision, error) {
 }
 
 // NewForHLC creates a new revision for the given hybrid logical clock.
-func NewForHLC(decimal decimal.Decimal) HLCRevision {
-	rev, _ := HLCRevisionFromString(decimal.String())
-	return rev
+func NewForHLC(decimal decimal.Decimal) (HLCRevision, error) {
+	rev, err := HLCRevisionFromString(decimal.String())
+	if err != nil {
+		return zeroHLC, fmt.Errorf("invalid HLC decimal: %v (%s) => %w", decimal, decimal.String(), err)
+	}
+
+	return rev, nil
 }
 
 // NewHLCForTime creates a new revision for the given time.
diff --git a/internal/datastore/revisions/hlcrevision_test.go b/internal/datastore/revisions/hlcrevision_test.go
index c74e391e5f..962abdfa1f 100644
--- a/internal/datastore/revisions/hlcrevision_test.go
+++ b/internal/datastore/revisions/hlcrevision_test.go
@@ -24,7 +24,8 @@ func TestNewForHLC(t *testing.T) {
 			d, err := decimal.NewFromString(tc)
 			require.NoError(t, err)
 
-			rev := NewForHLC(d)
+			rev, err := NewForHLC(d)
+			require.NoError(t, err)
 			require.Equal(t, tc, rev.String())
 		})
 	}
diff --git a/pkg/zedtoken/zedtoken_test.go b/pkg/zedtoken/zedtoken_test.go
index 2fdf22a659..09bbc4d48e 100644
--- a/pkg/zedtoken/zedtoken_test.go
+++ b/pkg/zedtoken/zedtoken_test.go
@@ -166,10 +166,16 @@ var hlcDecodeTests = []struct {
 	expectError      bool
 }{
 	{
-		format:           "V1 ZedToken",
-		token:            "CAIaFQoTMTYyMTUzODE4OTAyODkyODAwMA==",
-		expectedRevision: revisions.NewForHLC(decimal.NewFromInt(1621538189028928000)),
-		expectError:      false,
+		format: "V1 ZedToken",
+		token:  "CAIaFQoTMTYyMTUzODE4OTAyODkyODAwMA==",
+		expectedRevision: func() datastore.Revision {
+			r, err := revisions.NewForHLC(decimal.NewFromInt(1621538189028928000))
+			if err != nil {
+				panic(err)
+			}
+			return r
+		}(),
+		expectError: false,
 	},
 	{
 		format: "V1 ZedToken",
@@ -179,7 +185,13 @@ var hlcDecodeTests = []struct {
 			if err != nil {
 				panic(err)
 			}
-			return revisions.NewForHLC(v)
+
+			r, err := revisions.NewForHLC(v)
+			if err != nil {
+				panic(err)
+			}
+
+			return r
 		})(),
 		expectError: false,
 	},