Stop using detectableChunk in processResult (#4691)

The processResult function took a detectableChunk as a parameter, but that was purely out of convenience, because callers originally happened to have that type available. We later added more callers that didn't have a detectableChunk available, so they had to synthesize one. This was tolerable, but we're exploring more refactors that will introduce processResult callers that cannot synthesize a detectableChunk (because they don't have the right information available). This PR therefore modifies the signature of processResult to require only the necessary information so that we're not sandbagged by this unrelated structure down the road.

Author: rosecodym

pkg/engine/engine.go

@@ -1000,13 +1000,10 @@ func (e *Engine) verificationOverlapWorker(ctx context.Context) {
 						res.SetVerificationError(errOverlap)
 						e.processResult(
 							ctx,
-							detectableChunk{
-								chunk:    chunk.chunk,
-								detector: detector,
-								decoder:  chunk.decoder,
-								wgDoneFn: wgDetect.Done,
-							},
 							res,
+							chunk.chunk,
+							chunk.decoder,
+							detector.Detector.Description(),
 							isFalsePositive,
 						)
 
@@ -1133,7 +1130,7 @@ func (e *Engine) detectChunk(ctx context.Context, data detectableChunk) {
 		}
 
 		for _, res := range results {
-			e.processResult(ctx, data, res, isFalsePositive)
+			e.processResult(ctx, res, data.chunk, data.decoder, data.detector.Detector.Description(), isFalsePositive)
 		}
 	}
 
@@ -1172,20 +1169,18 @@ func (e *Engine) filterResults(
 
 // processResult generates a detectors.ResultWithMetadata from the provided chunk and result and puts it on the results
 // channel, unless the result exists on a line with an ignore tag, in which case no result is generated.
-//
-// CMR: The provided chunk is wrapped in a detectableChunk, but I'm pretty sure that's purely out of convenience
-// (because that's what this function's callers are using when they call this function). We're past detection at this
-// point in the engine, so we should probably refactor that parameter into a less confusing data type.
 func (e *Engine) processResult(
 	ctx context.Context,
-	data detectableChunk,
 	res detectors.Result,
+	chunk sources.Chunk,
+	decoderType detectorspb.DecoderType,
+	detectorDescription string,
 	isFalsePositive func(detectors.Result) (bool, string),
 ) {
 	ignoreLinePresent := false
-	if SupportsLineNumbers(data.chunk.SourceType) {
-		copyChunk := data.chunk
-		copyMetaDataClone := proto.Clone(data.chunk.SourceMetadata)
+	if SupportsLineNumbers(chunk.SourceType) {
+		copyChunk := chunk
+		copyMetaDataClone := proto.Clone(chunk.SourceMetadata)
 		if copyMetaData, ok := copyMetaDataClone.(*source_metadatapb.MetaData); ok {
 			copyChunk.SourceMetadata = copyMetaData
 		}
@@ -1195,15 +1190,15 @@ func (e *Engine) processResult(
 			ctx.Logger().Error(err, "error setting link")
 			return
 		}
-		data.chunk = copyChunk
+		chunk = copyChunk
 	}
 	if ignoreLinePresent {
 		return
 	}
 
-	secret := detectors.CopyMetadata(&data.chunk, res)
-	secret.DecoderType = data.decoder
-	secret.DetectorDescription = data.detector.Detector.Description()
+	secret := detectors.CopyMetadata(&chunk, res)
+	secret.DecoderType = decoderType
+	secret.DetectorDescription = detectorDescription
 
 	if !res.Verified && res.Raw != nil {
 		isFp, _ := isFalsePositive(res)

pkg/engine/engine_test.go

@@ -559,21 +559,18 @@ func TestProcessResult_SourceSupportsLineNumbers_LinkUpdated(t *testing.T) {
 	// Arrange: Create an engine
 	e := Engine{results: make(chan detectors.ResultWithMetadata, 1)}
 
-	// Arrange: Create a detectableChunk
-	data := detectableChunk{
-		chunk: sources.Chunk{
-			Data: []byte("abcde\nswordfish"),
-			SourceMetadata: &source_metadatapb.MetaData{
-				Data: &source_metadatapb.MetaData_Github{
-					Github: &source_metadatapb.Github{
-						Line: 1,
-						Link: "https://github.com/org/repo/blob/abcdef/file.txt#L1",
-					},
+	// Arrange: Create a Chunk
+	chunk := sources.Chunk{
+		Data: []byte("abcde\nswordfish"),
+		SourceMetadata: &source_metadatapb.MetaData{
+			Data: &source_metadatapb.MetaData_Github{
+				Github: &source_metadatapb.Github{
+					Line: 1,
+					Link: "https://github.com/org/repo/blob/abcdef/file.txt#L1",
 				},
 			},
-			SourceType: sourcespb.SourceType_SOURCE_TYPE_GIT,
 		},
-		detector: &ahocorasick.DetectorMatch{Detector: fakeDetectorV1{}},
+		SourceType: sourcespb.SourceType_SOURCE_TYPE_GIT,
 	}
 
 	// Arrange: Create a Result
@@ -583,7 +580,7 @@ func TestProcessResult_SourceSupportsLineNumbers_LinkUpdated(t *testing.T) {
 	}
 
 	// Act
-	e.processResult(context.AddLogger(t.Context()), data, result, nil)
+	e.processResult(context.AddLogger(t.Context()), result, chunk, 0, "", nil)
 
 	// Assert that the link has been correctly updated
 	require.Len(t, e.results, 1)
@@ -595,20 +592,17 @@ func TestProcessResult_IgnoreLinePresent_NothingGenerated(t *testing.T) {
 	// Arrange: Create an engine
 	e := Engine{results: make(chan detectors.ResultWithMetadata, 1)}
 
-	// Arrange: Create a detectableChunk
-	data := detectableChunk{
-		chunk: sources.Chunk{
-			Data: []byte("swordfish trufflehog:ignore"),
-			SourceMetadata: &source_metadatapb.MetaData{
-				Data: &source_metadatapb.MetaData_Git{
-					Git: &source_metadatapb.Git{
-						Line: 1,
-					},
+	// Arrange: Create a Chunk
+	chunk := sources.Chunk{
+		Data: []byte("swordfish trufflehog:ignore"),
+		SourceMetadata: &source_metadatapb.MetaData{
+			Data: &source_metadatapb.MetaData_Git{
+				Git: &source_metadatapb.Git{
+					Line: 1,
 				},
 			},
-			SourceType: sourcespb.SourceType_SOURCE_TYPE_GIT,
 		},
-		detector: &ahocorasick.DetectorMatch{Detector: fakeDetectorV1{}},
+		SourceType: sourcespb.SourceType_SOURCE_TYPE_GIT,
 	}
 
 	// Arrange: Create a Result
@@ -618,7 +612,7 @@ func TestProcessResult_IgnoreLinePresent_NothingGenerated(t *testing.T) {
 	}
 
 	// Act
-	e.processResult(context.AddLogger(t.Context()), data, result, nil)
+	e.processResult(context.AddLogger(t.Context()), result, chunk, 0, "", nil)
 
 	// Assert that no results were generated
 	assert.Empty(t, e.results)
@@ -628,27 +622,23 @@ func TestProcessResult_AllFieldsCopied(t *testing.T) {
 	// Arrange: Create an engine
 	e := Engine{results: make(chan detectors.ResultWithMetadata, 1)}
 
-	// Arrange: Create a detectableChunk
-	data := detectableChunk{
-		chunk: sources.Chunk{
-			SourceName: "test source",
-			SourceID:   1,
-			JobID:      2,
-			SecretID:   3,
-			SourceMetadata: &source_metadatapb.MetaData{
-				Data: &source_metadatapb.MetaData_Docker{
-					Docker: &source_metadatapb.Docker{
-						File:  "file",
-						Image: "image",
-						Layer: "layer",
-						Tag:   "tag",
-					},
+	// Arrange: Create a Chunk
+	chunk := sources.Chunk{
+		SourceName: "test source",
+		SourceID:   1,
+		JobID:      2,
+		SecretID:   3,
+		SourceMetadata: &source_metadatapb.MetaData{
+			Data: &source_metadatapb.MetaData_Docker{
+				Docker: &source_metadatapb.Docker{
+					File:  "file",
+					Image: "image",
+					Layer: "layer",
+					Tag:   "tag",
 				},
 			},
-			SourceType: sourcespb.SourceType_SOURCE_TYPE_DOCKER,
 		},
-		decoder:  detectorspb.DecoderType_PLAIN,
-		detector: &ahocorasick.DetectorMatch{Detector: fakeDetectorV1{}},
+		SourceType: sourcespb.SourceType_SOURCE_TYPE_DOCKER,
 	}
 
 	// Arrange: Create a Result
@@ -662,12 +652,12 @@ func TestProcessResult_AllFieldsCopied(t *testing.T) {
 	}
 
 	// Act
-	e.processResult(context.AddLogger(t.Context()), data, result, nil)
+	e.processResult(context.AddLogger(t.Context()), result, chunk, detectorspb.DecoderType_PLAIN, "a detector that detects", nil)
 
 	// Assert that the single generated result has the correct fields
 	require.Len(t, e.results, 1)
 	r := <-e.results
-	if diff := cmp.Diff(data.chunk.SourceMetadata, r.SourceMetadata, protocmp.Transform()); diff != "" {
+	if diff := cmp.Diff(chunk.SourceMetadata, r.SourceMetadata, protocmp.Transform()); diff != "" {
 		t.Errorf("metadata mismatch (-want +got):\n%s", diff)
 	}
 	assert.Equal(t, map[string]string{"key": "value"}, r.ExtraData)
@@ -682,6 +672,7 @@ func TestProcessResult_AllFieldsCopied(t *testing.T) {
 	assert.Equal(t, "test source", r.SourceName)
 	assert.Equal(t, sourcespb.SourceType_SOURCE_TYPE_DOCKER, r.SourceType)
 	assert.Equal(t, detectorspb.DecoderType_PLAIN, r.DecoderType)
+	assert.Equal(t, "a detector that detects", r.DetectorDescription)
 }
 
 func TestProcessResult_FalsePositiveFlagSetCorrectly(t *testing.T) {
@@ -722,10 +713,6 @@ func TestProcessResult_FalsePositiveFlagSetCorrectly(t *testing.T) {
 			// Arrange: Create an Engine
 			e := Engine{results: make(chan detectors.ResultWithMetadata, 1)}
 
-			// Arrange: Create a detectableChunk
-			// (It needs a DetectorMatch to avoid a panic.)
-			data := detectableChunk{detector: &ahocorasick.DetectorMatch{Detector: fakeDetectorV1{}}}
-
 			// Arrange: Create a Result
 			res := detectors.Result{
 				Raw:      []byte("something not nil"), // The false positive check is not run when Raw is nil
@@ -736,7 +723,7 @@ func TestProcessResult_FalsePositiveFlagSetCorrectly(t *testing.T) {
 			isFalsePositive := func(_ detectors.Result) (bool, string) { return tt.isFalsePositive, "" }
 
 			// Act
-			e.processResult(context.AddLogger(t.Context()), data, res, isFalsePositive)
+			e.processResult(context.AddLogger(t.Context()), res, sources.Chunk{}, 0, "", isFalsePositive)
 
 			// Assert that the single generated result has the correct false positive flag
 			require.Len(t, e.results, 1)