Fix bugs in event validation and make criteria stronger

Fix a bugs where:
 * empty events in batch were considered valid
 * cid was not checked for validity
 * peer ID was not checked for validity
 * time span of start and event time was unchecked.

Refactor codes to instead check the zero valued struct for validity, as
we need to do that anyway. Using pointers doesn't make sense here
because all the pointer fields are required to have valid content and we
do not need to infer whether they were actually present in JSON or not.

Add extra tests to cover more edge cases

eventrecorder/event.go

@@ -6,119 +6,113 @@ import (
 	"time"
 
 	"github.com/filecoin-project/lassie/pkg/types"
-	"github.com/google/uuid"
+	"github.com/ipfs/go-cid"
+	"github.com/libp2p/go-libp2p/core/peer"
 )
 
 var (
-	validPhases     = []string{"indexer", "query", "retrieval"}
-	validEventNames = []string{
-		"accepted",
-		"candidates-filtered",
-		"candidates-found",
-		"connected",
-		"failure",
-		"first-byte-received",
-		"proposed",
-		"query-asked",
-		"query-asked-filtered",
-		"started",
-		"success",
+	errInvalidPhase     = fmt.Errorf("phase must be one of: [%s %s %s]", types.IndexerPhase, types.QueryPhase, types.RetrievalPhase)
+	errInvalidEventCode error
+	emptyRetrievalID    types.RetrievalID
+	eventCodes          = map[types.EventCode]any{
+		types.CandidatesFoundCode:    nil,
+		types.CandidatesFilteredCode: nil,
+		types.StartedCode:            nil,
+		types.ConnectedCode:          nil,
+		types.QueryAskedCode:         nil,
+		types.QueryAskedFilteredCode: nil,
+		types.ProposedCode:           nil,
+		types.AcceptedCode:           nil,
+		types.FirstByteCode:          nil,
+		types.FailedCode:             nil,
+		types.SuccessCode:            nil,
 	}
 )
 
+func init() {
+	codes := make([]types.EventCode, 0, len(eventCodes))
+	for code := range eventCodes {
+		codes = append(codes, code)
+	}
+	errInvalidEventCode = fmt.Errorf("eventName must be one of: %v", codes)
+}
+
 type event struct {
-	RetrievalId       *types.RetrievalID `json:"retrievalId"`
-	InstanceId        *string            `json:"instanceId,omitempty"`
-	Cid               *string            `json:"cid"`
-	StorageProviderId *string            `json:"storageProviderId"`
-	Phase             *types.Phase       `json:"phase"`
-	PhaseStartTime    *time.Time         `json:"phaseStartTime"`
-	EventName         *types.EventCode   `json:"eventName"`
-	EventTime         *time.Time         `json:"eventTime"`
-	EventDetails      interface{}        `json:"eventDetails,omitempty"`
+	RetrievalId       types.RetrievalID `json:"retrievalId"`
+	InstanceId        string            `json:"instanceId,omitempty"`
+	Cid               string            `json:"cid"`
+	StorageProviderId string            `json:"storageProviderId"`
+	Phase             types.Phase       `json:"phase"`
+	PhaseStartTime    time.Time         `json:"phaseStartTime"`
+	EventName         types.EventCode   `json:"eventName"`
+	EventTime         time.Time         `json:"eventTime"`
+	EventDetails      any               `json:"eventDetails,omitempty"`
 }
 
 func (e event) validate() error {
-	// RetrievalId
-	if e.RetrievalId == nil {
+	switch {
+	case e.RetrievalId == emptyRetrievalID:
 		return errors.New("property retrievalId is required")
-	}
-	if _, err := uuid.Parse(e.RetrievalId.String()); err != nil {
-		return errors.New("property retrievalId should be a valid v4 uuid")
-	}
-
-	// InstanceId
-	if e.InstanceId == nil {
+	case e.InstanceId == "":
 		return errors.New("property instanceId is required")
-	}
-
-	// Cid
-	if e.Cid == nil {
+	case e.Cid == "":
 		return errors.New("property cid is required")
-	}
-
-	// StorageProviderId
-	if e.StorageProviderId == nil {
+	case e.StorageProviderId == "":
 		return errors.New("property storageProviderId is required")
-	}
-
-	// Phase
-	if e.Phase == nil {
+	case e.Phase == "":
 		return errors.New("property phase is required")
-	}
-	isValidPhase := false
-	for _, phase := range validPhases {
-		if string(*e.Phase) == phase {
-			isValidPhase = true
-			break
-		}
-	}
-	if !isValidPhase {
-		return fmt.Errorf("property phase failed validation. Phase must be created with one of the following values: %v", validPhases)
-	}
-
-	// PhaseStartTime
-	if e.PhaseStartTime == nil {
+	case !validPhase(e.Phase):
+		return errInvalidPhase
+	case e.PhaseStartTime.IsZero():
 		return errors.New("property phaseStartTime is required")
-	}
-
-	// EventName
-	if e.EventName == nil {
+	case e.PhaseStartTime.After(time.Now()):
+		return errors.New("property phaseStartTime cannot be in the future")
+	case e.EventName == "":
 		return errors.New("property eventName is required")
-	}
-	isValidEventName := false
-	for _, phase := range validEventNames {
-		if string(*e.EventName) == phase {
-			isValidEventName = true
-			break
+	case !validEventCode(e.EventName):
+		return errInvalidEventCode
+	case e.EventTime.IsZero():
+		return errors.New("property eventTime is required")
+	case e.EventTime.After(time.Now()):
+		return errors.New("property eventTime cannot be in the future")
+	default:
+		_, err := cid.Decode(e.Cid)
+		if err != nil {
+			return fmt.Errorf("cid must be valid: %w", err)
 		}
+		if _, err := peer.Decode(e.StorageProviderId); err != nil {
+			return fmt.Errorf("storageProviderId must be valid: %w", err)
+		}
+		return nil
 	}
-	if !isValidEventName {
-		return fmt.Errorf("property eventName failed validation. Event name must be created with one of the following values: %v", validEventNames)
-	}
+}
 
-	// EventTime
-	if e.EventTime == nil {
-		return errors.New("property eventTime is required")
+func validPhase(phase types.Phase) bool {
+	switch phase {
+	case types.IndexerPhase, types.QueryPhase, types.RetrievalPhase:
+		return true
+	default:
+		return false
 	}
+}
 
-	return nil
+func validEventCode(code types.EventCode) bool {
+	_, ok := eventCodes[code]
+	return ok
 }
 
 type eventBatch struct {
 	Events []event `json:"events"`
 }
 
 func (e eventBatch) validate() error {
-	if e.Events == nil {
+	if len(e.Events) == 0 {
 		return errors.New("property events is required")
 	}
-
 	for _, event := range e.Events {
 		if err := event.validate(); err != nil {
 			return err
 		}
 	}
-
 	return nil
 }

eventrecorder/event_test.go

@@ -4,57 +4,105 @@ import (
 	"encoding/json"
 	"io"
 	"os"
-	"path"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 )
 
 func Test_EventValidate(t *testing.T) {
 	tests := []struct {
+		name              string
 		path              string
 		batch             bool
 		wantDecodeErr     bool
 		wantValidationErr bool
 	}{
 		{
+			name:              "bad-eventName",
 			path:              "../testdata/bad-eventName.json",
 			batch:             true,
 			wantValidationErr: true,
 		},
 		{
+			name:              "bad-phase",
 			path:              "../testdata/bad-phase.json",
 			batch:             true,
 			wantValidationErr: true,
 		},
 		{
+			name:              "bad-retrievalid",
 			path:              "../testdata/bad-retrievalid.json",
 			batch:             true,
 			wantDecodeErr:     true,
 			wantValidationErr: true,
 		},
 		{
+			name:  "good",
 			path:  "../testdata/good.json",
 			batch: true,
 		},
 		{
+			name:              "invalid-cid",
+			path:              "../testdata/invalid-cid.json",
+			batch:             true,
+			wantValidationErr: true,
+		},
+		{
+			name:              "future-start-time",
+			path:              "../testdata/future-start-time.json",
+			batch:             true,
+			wantValidationErr: true,
+		},
+		{
+			name:              "future-end-time",
+			path:              "../testdata/future-event-time.json",
+			batch:             true,
+			wantValidationErr: true,
+		},
+		{
+			name:              "invalid-provider-id",
+			path:              "../testdata/invalid-providerid.json",
+			batch:             true,
+			wantValidationErr: true,
+		},
+		{
+			name:              "missing-events",
 			path:              "../testdata/missing-events.json",
 			batch:             true,
 			wantValidationErr: true,
 		},
 		{
+			name:              "missing-events-empty-array",
+			path:              "../testdata/missing-events-empty-array.json",
+			batch:             true,
+			wantValidationErr: true,
+		},
+		{
+			name:              "missing-event",
+			path:              "../testdata/missing-events.json",
+			wantValidationErr: true,
+		},
+		{
+			name:              "missing-instanceid",
 			path:              "../testdata/missing-instanceid.json",
 			batch:             true,
 			wantValidationErr: true,
 		},
 		{
+			name:              "missing-event-time",
+			path:              "../testdata/missing-event-time.json",
+			batch:             true,
+			wantValidationErr: true,
+		},
+		{
+			name:              "missing-retrievalid",
 			path:              "../testdata/missing-retrievalid.json",
 			batch:             true,
 			wantValidationErr: true,
 		},
 	}
 	for _, test := range tests {
-		t.Run(path.Base(test.path), func(t *testing.T) {
+		t.Run(test.name, func(t *testing.T) {
 			f, err := os.Open(test.path)
 			t.Cleanup(func() {
 				require.NoError(t, f.Close())

go.mod

@@ -4,9 +4,10 @@ go 1.19
 
 require (
 	github.com/filecoin-project/lassie v0.5.0
-	github.com/google/uuid v1.3.0
+	github.com/ipfs/go-cid v0.3.2
 	github.com/ipfs/go-log/v2 v2.5.1
 	github.com/jackc/pgx/v5 v5.3.1
+	github.com/libp2p/go-libp2p v0.26.1
 	github.com/stretchr/testify v1.8.2
 )
 
@@ -25,13 +26,13 @@ require (
 	github.com/filecoin-project/go-statemachine v1.0.3 // indirect
 	github.com/filecoin-project/go-statestore v0.2.0 // indirect
 	github.com/gogo/protobuf v1.3.2 // indirect
+	github.com/google/uuid v1.3.0 // indirect
 	github.com/hannahhoward/cbor-gen-for v0.0.0-20230214144701-5d17c9d5243c // indirect
 	github.com/hannahhoward/go-pubsub v1.0.0 // indirect
 	github.com/hashicorp/golang-lru v0.5.4 // indirect
 	github.com/ipfs/bbloom v0.0.4 // indirect
 	github.com/ipfs/go-bitfield v1.1.0 // indirect
 	github.com/ipfs/go-block-format v0.1.1 // indirect
-	github.com/ipfs/go-cid v0.3.2 // indirect
 	github.com/ipfs/go-datastore v0.6.0 // indirect
 	github.com/ipfs/go-ipfs-blockstore v1.2.0 // indirect
 	github.com/ipfs/go-ipfs-ds-help v1.1.0 // indirect
@@ -52,7 +53,6 @@ require (
 	github.com/jpillora/backoff v1.0.0 // indirect
 	github.com/klauspost/cpuid/v2 v2.2.4 // indirect
 	github.com/libp2p/go-buffer-pool v0.1.0 // indirect
-	github.com/libp2p/go-libp2p v0.26.1 // indirect
 	github.com/mattn/go-isatty v0.0.17 // indirect
 	github.com/minio/blake2b-simd v0.0.0-20160723061019-3f5f724cb5b1 // indirect
 	github.com/minio/sha256-simd v1.0.0 // indirect

testdata/future-event-time.json

@@ -0,0 +1,14 @@
+{
+  "events": [
+    {
+      "retrievalId": "d05a522e-9608-401a-a33f-1eb4ac4111a0",
+      "instanceId": "test-instance-id",
+      "cid": "bafybeic4jpi2detp5n3q6rjo7ckulebtr7dsvt2tbrtcqlnnzqmi3bzz2y",
+      "storageProviderId": "12D3KooWHwRmkj4Jxfo2YnKJC4YBzTNEGDW6Et4E68r7RYVXk46h",
+      "phase": "query",
+      "phaseStartTime": "2023-03-02T00:15:50.479910907Z",
+      "eventName": "started",
+      "eventTime": "2223-03-02T00:15:52.361371026Z"
+    }
+  ]
+}

testdata/future-start-time.json

@@ -0,0 +1,14 @@
+{
+  "events": [
+    {
+      "retrievalId": "d05a522e-9608-401a-a33f-1eb4ac4111a0",
+      "instanceId": "test-instance-id",
+      "cid": "bafybeic4jpi2detp5n3q6rjo7ckulebtr7dsvt2tbrtcqlnnzqmi3bzz2y",
+      "storageProviderId": "12D3KooWHwRmkj4Jxfo2YnKJC4YBzTNEGDW6Et4E68r7RYVXk46h",
+      "phase": "query",
+      "phaseStartTime": "2223-03-02T00:15:50.479910907Z",
+      "eventName": "started",
+      "eventTime": "2023-03-02T00:15:52.361371026Z"
+    }
+  ]
+}

testdata/good.json

@@ -3,8 +3,8 @@
     {
       "retrievalId": "d05a522e-9608-401a-a33f-1eb4ac4111a0",
       "instanceId": "test-instance-id",
-      "cid": "test-cid",
-      "storageProviderId": "test-sp-id",
+      "cid": "bafybeic4jpi2detp5n3q6rjo7ckulebtr7dsvt2tbrtcqlnnzqmi3bzz2y",
+      "storageProviderId": "12D3KooWHwRmkj4Jxfo2YnKJC4YBzTNEGDW6Et4E68r7RYVXk46h",
       "phase": "query",
       "phaseStartTime": "2023-03-02T00:15:50.479910907Z",
       "eventName": "started",

testdata/invalid-cid.json

@@ -0,0 +1,14 @@
+{
+  "events": [
+    {
+      "retrievalId": "d05a522e-9608-401a-a33f-1eb4ac4111a0",
+      "instanceId": "test-instance-id",
+      "cid": "fish",
+      "storageProviderId": "12D3KooWHwRmkj4Jxfo2YnKJC4YBzTNEGDW6Et4E68r7RYVXk46h",
+      "phase": "query",
+      "phaseStartTime": "2023-03-02T00:15:50.479910907Z",
+      "eventName": "started",
+      "eventTime": "2023-03-02T00:15:52.361371026Z"
+    }
+  ]
+}