Location order on packages should consider evidence annotations when sorting (#3720)

* fix: sorting locations should consider pkg evidence

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* simplify location test options for comparison

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

---------

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

internal/cmptest/common_options.go

@@ -8,11 +8,15 @@ import (
 	"github.com/anchore/syft/syft/pkg"
 )
 
-func DefaultCommonOptions() []cmp.Option {
+func DefaultOptions() []cmp.Option {
-	return CommonOptions(nil, nil)
+	return BuildOptions(nil, nil)
 }
 
-func CommonOptions(licenseCmp LicenseComparer, locationCmp LocationComparer) []cmp.Option {
+func DefaultIgnoreLocationLayerOptions() []cmp.Option {
+	return BuildOptions(LicenseComparerWithoutLocationLayer, LocationComparerWithoutLayer)
+}
+
+func BuildOptions(licenseCmp LicenseComparer, locationCmp LocationComparer) []cmp.Option {
 	if licenseCmp == nil {
 		licenseCmp = DefaultLicenseComparer
 	}
@@ -25,47 +29,9 @@ func CommonOptions(licenseCmp LicenseComparer, locationCmp LocationComparer) []c
 		cmpopts.IgnoreFields(pkg.Package{}, "id"), // note: ID is not deterministic for test purposes
 		cmpopts.SortSlices(pkg.Less),
 		cmpopts.SortSlices(DefaultRelationshipComparer),
-		cmp.Comparer(
+		cmp.Comparer(buildSetComparer[file.Location, file.LocationSet](locationCmp)),
-			func(x, y file.LocationSet) bool {
+		cmp.Comparer(buildSetComparer[pkg.License, pkg.LicenseSet](licenseCmp)),
-				xs := x.ToSlice()
+		cmp.Comparer(locationCmp),
-				ys := y.ToSlice()
+		cmp.Comparer(licenseCmp),
-
-				if len(xs) != len(ys) {
-					return false
-				}
-				for i, xe := range xs {
-					ye := ys[i]
-					if !locationCmp(xe, ye) {
-						return false
-					}
-				}
-
-				return true
-			},
-		),
-		cmp.Comparer(
-			func(x, y pkg.LicenseSet) bool {
-				xs := x.ToSlice()
-				ys := y.ToSlice()
-
-				if len(xs) != len(ys) {
-					return false
-				}
-				for i, xe := range xs {
-					ye := ys[i]
-					if !licenseCmp(xe, ye) {
-						return false
-					}
-				}
-
-				return true
-			},
-		),
-		cmp.Comparer(
-			locationCmp,
-		),
-		cmp.Comparer(
-			licenseCmp,
-		),
 	}
 }

internal/cmptest/license.go

@@ -10,20 +10,17 @@ import (
 type LicenseComparer func(x, y pkg.License) bool
 
 func DefaultLicenseComparer(x, y pkg.License) bool {
-	return cmp.Equal(x, y, cmp.Comparer(DefaultLocationComparer), cmp.Comparer(
+	return cmp.Equal(
-		func(x, y file.LocationSet) bool {
+		x, y,
-			xs := x.ToSlice()
+		cmp.Comparer(DefaultLocationComparer),
-			ys := y.ToSlice()
+		cmp.Comparer(buildSetComparer[file.Location, file.LocationSet](DefaultLocationComparer)),
-			if len(xs) != len(ys) {
+	)
-				return false
 }
-			for i, xe := range xs {
+
-				ye := ys[i]
+func LicenseComparerWithoutLocationLayer(x, y pkg.License) bool {
-				if !DefaultLocationComparer(xe, ye) {
+	return cmp.Equal(
-					return false
+		x, y,
-				}
+		cmp.Comparer(LocationComparerWithoutLayer),
-			}
+		cmp.Comparer(buildSetComparer[file.Location, file.LocationSet](LocationComparerWithoutLayer)),
-			return true
+	)
-		},
-	))
 }

internal/cmptest/location.go

@@ -11,3 +11,7 @@ type LocationComparer func(x, y file.Location) bool
 func DefaultLocationComparer(x, y file.Location) bool {
 	return cmp.Equal(x.Coordinates, y.Coordinates) && cmp.Equal(x.AccessPath, y.AccessPath)
 }
+
+func LocationComparerWithoutLayer(x, y file.Location) bool {
+	return cmp.Equal(x.Coordinates.RealPath, y.Coordinates.RealPath) && cmp.Equal(x.AccessPath, y.AccessPath)
+}

internal/cmptest/set.go

@@ -0,0 +1,24 @@
+package cmptest
+
+type slicer[T any] interface {
+	ToSlice() []T
+}
+
+func buildSetComparer[T any, S slicer[T]](l func(x, y T) bool) func(x, y S) bool {
+	return func(x, y S) bool {
+		xs := x.ToSlice()
+		ys := y.ToSlice()
+
+		if len(xs) != len(ys) {
+			return false
+		}
+		for i, xe := range xs {
+			ye := ys[i]
+			if !l(xe, ye) {
+				return false
+			}
+		}
+
+		return true
+	}
+}

internal/evidence/constants.go

@@ -0,0 +1,9 @@
+package evidence
+
+// this package exists so that the file package can reference package evidence in tests without creating a circular dependency.
+
+const (
+	AnnotationKey        = "evidence"
+	PrimaryAnnotation    = "primary"
+	SupportingAnnotation = "supporting"
+)

internal/relationship/by_file_ownership_test.go

@@ -145,7 +145,7 @@ func TestOwnershipByFilesRelationship(t *testing.T) {
 			assert.Len(t, relationships, len(expectedRelations))
 			for idx, expectedRelationship := range expectedRelations {
 				actualRelationship := relationships[idx]
-				if d := cmp.Diff(expectedRelationship, actualRelationship, cmptest.DefaultCommonOptions()...); d != "" {
+				if d := cmp.Diff(expectedRelationship, actualRelationship, cmptest.DefaultOptions()...); d != "" {
 					t.Errorf("unexpected relationship (-want, +got): %s", d)
 				}
 			}

syft/file/location_set_test.go

@@ -1,15 +1,17 @@
 package file
 
 import (
+	"sort"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/anchore/syft/internal/evidence"
 	"github.com/anchore/syft/syft/artifact"
 )
 
-func TestLocationSet(t *testing.T) {
+func TestLocationSet_SortPaths(t *testing.T) {
 
 	etcHostsLinkVar := Location{
 		LocationData: LocationData{
@@ -93,6 +95,117 @@ func TestLocationSet(t *testing.T) {
 	}
 }
 
+func TestLocationSet_SortEvidence(t *testing.T) {
+	primaryEvidence := map[string]string{evidence.AnnotationKey: evidence.PrimaryAnnotation}
+	secondaryEvidence := map[string]string{evidence.AnnotationKey: evidence.SupportingAnnotation}
+
+	binPrimary := Location{
+		LocationData: LocationData{
+			Coordinates: Coordinates{
+				RealPath:     "/bin",
+				FileSystemID: "a",
+			},
+			AccessPath: "/usr/bin",
+		},
+		LocationMetadata: LocationMetadata{
+			Annotations: primaryEvidence,
+		},
+	}
+
+	binSecondary := Location{
+		LocationData: LocationData{
+			Coordinates: Coordinates{
+				RealPath:     "/bin",
+				FileSystemID: "a",
+			},
+			AccessPath: "/usr/bin",
+		},
+		LocationMetadata: LocationMetadata{
+			Annotations: secondaryEvidence,
+		},
+	}
+
+	binNoEvidence := Location{
+		LocationData: LocationData{
+			Coordinates: Coordinates{
+				RealPath:     "/bin",
+				FileSystemID: "a",
+			},
+			AccessPath: "/usr/bin",
+		},
+	}
+
+	etcHostsPrimary := Location{
+		LocationData: LocationData{
+			Coordinates: Coordinates{
+				RealPath:     "/etc/hosts",
+				FileSystemID: "a",
+			},
+			AccessPath: "/var/etc/hosts",
+		},
+		LocationMetadata: LocationMetadata{
+			Annotations: primaryEvidence,
+		},
+	}
+
+	etcHostsSecondary := Location{
+		LocationData: LocationData{
+			Coordinates: Coordinates{
+				RealPath:     "/etc/hosts",
+				FileSystemID: "a",
+			},
+			AccessPath: "/var/etc/hosts",
+		},
+		LocationMetadata: LocationMetadata{
+			Annotations: secondaryEvidence,
+		},
+	}
+
+	etcHostsNoEvidence := Location{
+		LocationData: LocationData{
+			Coordinates: Coordinates{
+				RealPath:     "/etc/hosts",
+				FileSystemID: "a",
+			},
+			AccessPath: "/var/etc/hosts",
+		},
+	}
+
+	tests := []struct {
+		name     string
+		input    []Location
+		expected []Location
+	}{
+		{
+			name: "sort primary, secondary, tertiary, no evidence",
+			input: []Location{
+				binNoEvidence, binPrimary, binSecondary,
+			},
+			expected: []Location{
+				binPrimary, binSecondary, binNoEvidence,
+			},
+		},
+		{
+			name: "sort by evidence, then path",
+			input: []Location{
+				etcHostsNoEvidence, etcHostsSecondary,
+				binSecondary, binNoEvidence,
+				binPrimary, etcHostsPrimary,
+			},
+			expected: []Location{
+				binPrimary, etcHostsPrimary, binSecondary, etcHostsSecondary, binNoEvidence, etcHostsNoEvidence,
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			sort.Sort(Locations(test.input))
+			assert.Equal(t, test.expected, test.input)
+		})
+	}
+}
+
 func TestLocationSet_Hash(t *testing.T) {
 	etcAlink := Location{
 		LocationData: LocationData{

syft/file/locations.go

@@ -1,5 +1,7 @@
 package file
 
+import "github.com/anchore/syft/internal/evidence"
+
 type Locations []Location
 
 func (l Locations) Len() int {
@@ -7,6 +9,9 @@ func (l Locations) Len() int {
 }
 
 func (l Locations) Less(i, j int) bool {
+	liEvidence := l[i].Annotations[evidence.AnnotationKey]
+	ljEvidence := l[j].Annotations[evidence.AnnotationKey]
+	if liEvidence == ljEvidence {
 		if l[i].RealPath == l[j].RealPath {
 			if l[i].AccessPath == l[j].AccessPath {
 				return l[i].FileSystemID < l[j].FileSystemID
@@ -15,6 +20,15 @@ func (l Locations) Less(i, j int) bool {
 		}
 		return l[i].RealPath < l[j].RealPath
 	}
+	if liEvidence == evidence.PrimaryAnnotation {
+		return true
+	}
+	if ljEvidence == evidence.PrimaryAnnotation {
+		return false
+	}
+
+	return liEvidence > ljEvidence
+}
 
 func (l Locations) Swap(i, j int) {
 	l[i], l[j] = l[j], l[i]

syft/pkg/cataloger/debian/cataloger_test.go

@@ -30,11 +30,11 @@ func TestDpkgCataloger(t *testing.T) {
 						pkg.NewLicenseFromLocations("LGPL-2.1", file.NewLocation("/usr/share/doc/libpam-runtime/copyright")),
 					),
 					Locations: file.NewLocationSet(
-						file.NewLocation("/var/lib/dpkg/status"),
+						file.NewLocation("/var/lib/dpkg/status").WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation),
-						file.NewLocation("/var/lib/dpkg/info/libpam-runtime.preinst"),
+						file.NewLocation("/var/lib/dpkg/info/libpam-runtime.preinst").WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation),
-						file.NewLocation("/var/lib/dpkg/info/libpam-runtime.md5sums"),
+						file.NewLocation("/var/lib/dpkg/info/libpam-runtime.md5sums").WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation),
-						file.NewLocation("/var/lib/dpkg/info/libpam-runtime.conffiles"),
+						file.NewLocation("/var/lib/dpkg/info/libpam-runtime.conffiles").WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation),
-						file.NewLocation("/usr/share/doc/libpam-runtime/copyright"),
+						file.NewLocation("/usr/share/doc/libpam-runtime/copyright").WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation),
 					),
 					Type: pkg.DebPkg,
 					Metadata: pkg.DpkgDBEntry{
@@ -104,10 +104,10 @@ func TestDpkgCataloger(t *testing.T) {
 						pkg.NewLicenseFromLocations("GPL-2", file.NewLocation("/usr/share/doc/libsqlite3-0/copyright")),
 					),
 					Locations: file.NewLocationSet(
-						file.NewLocation("/var/lib/dpkg/status.d/libsqlite3-0"),
+						file.NewLocation("/var/lib/dpkg/status.d/libsqlite3-0").WithAnnotation(pkg.EvidenceAnnotationKey, pkg.PrimaryEvidenceAnnotation),
-						file.NewLocation("/var/lib/dpkg/status.d/libsqlite3-0.md5sums"),
+						file.NewLocation("/var/lib/dpkg/status.d/libsqlite3-0.md5sums").WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation),
-						file.NewLocation("/var/lib/dpkg/status.d/libsqlite3-0.preinst"),
+						file.NewLocation("/var/lib/dpkg/status.d/libsqlite3-0.preinst").WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation),
-						file.NewLocation("/usr/share/doc/libsqlite3-0/copyright"),
+						file.NewLocation("/usr/share/doc/libsqlite3-0/copyright").WithAnnotation(pkg.EvidenceAnnotationKey, pkg.SupportingEvidenceAnnotation),
 					),
 					Type: pkg.DebPkg,
 					Metadata: pkg.DpkgDBEntry{

syft/pkg/cataloger/golang/stdlib_package_test.go

@@ -139,14 +139,14 @@ func Test_stdlibPackageAndRelationships_values(t *testing.T) {
 	require.Len(t, gotPkgs, 1)
 
 	gotPkg := gotPkgs[0]
-	if d := cmp.Diff(expectedPkg, gotPkg, cmptest.DefaultCommonOptions()...); d != "" {
+	if d := cmp.Diff(expectedPkg, gotPkg, cmptest.DefaultOptions()...); d != "" {
 		t.Errorf("unexpected package (-want +got): %s", d)
 	}
 
 	require.Len(t, gotRels, 1)
 	gotRel := gotRels[0]
 
-	if d := cmp.Diff(expectedRel, gotRel, cmptest.DefaultCommonOptions()...); d != "" {
+	if d := cmp.Diff(expectedRel, gotRel, cmptest.DefaultOptions()...); d != "" {
 		t.Errorf("unexpected relationship (-want +got): %s", d)
 	}
 

syft/pkg/cataloger/internal/pkgtest/test_generic_parser.go

@@ -146,29 +146,8 @@ func (p *CatalogTester) ExpectsAssertion(a func(t *testing.T, pkgs []pkg.Package
 }
 
 func (p *CatalogTester) IgnoreLocationLayer() *CatalogTester {
-	p.locationComparer = func(x, y file.Location) bool {
+	p.locationComparer = cmptest.LocationComparerWithoutLayer
-		return cmp.Equal(x.Coordinates.RealPath, y.Coordinates.RealPath) && cmp.Equal(x.AccessPath, y.AccessPath)
+	p.licenseComparer = cmptest.LicenseComparerWithoutLocationLayer
-	}
-
-	// we need to update the license comparer to use the ignored location layer
-	p.licenseComparer = func(x, y pkg.License) bool {
-		return cmp.Equal(x, y, cmp.Comparer(p.locationComparer), cmp.Comparer(
-			func(x, y file.LocationSet) bool {
-				xs := x.ToSlice()
-				ys := y.ToSlice()
-				if len(xs) != len(ys) {
-					return false
-				}
-				for i, xe := range xs {
-					ye := ys[i]
-					if !p.locationComparer(xe, ye) {
-						return false
-					}
-				}
-
-				return true
-			}))
-	}
 	return p
 }
 
@@ -272,7 +251,7 @@ func (p *CatalogTester) TestCataloger(t *testing.T, cataloger pkg.Cataloger) {
 func (p *CatalogTester) assertPkgs(t *testing.T, pkgs []pkg.Package, relationships []artifact.Relationship) {
 	t.Helper()
 
-	p.compareOptions = append(p.compareOptions, cmptest.CommonOptions(p.licenseComparer, p.locationComparer)...)
+	p.compareOptions = append(p.compareOptions, cmptest.BuildOptions(p.licenseComparer, p.locationComparer)...)
 
 	{
 		r := cmptest.NewDiffReporter()
@@ -325,53 +304,20 @@ func TestFileParserWithEnv(t *testing.T, fixturePath string, parser generic.Pars
 	NewCatalogTester().FromFile(t, fixturePath).WithEnv(env).Expects(expectedPkgs, expectedRelationships).TestParser(t, parser)
 }
 
-func AssertPackagesEqual(t *testing.T, a, b pkg.Package) {
+func AssertPackagesEqual(t *testing.T, a, b pkg.Package, userOpts ...cmp.Option) {
 	t.Helper()
-	opts := []cmp.Option{
+	opts := cmptest.DefaultOptions()
-		cmpopts.IgnoreFields(pkg.Package{}, "id"), // note: ID is not deterministic for test purposes
+	opts = append(opts, userOpts...)
-		cmp.Comparer(
-			func(x, y file.LocationSet) bool {
-				xs := x.ToSlice()
-				ys := y.ToSlice()
 
-				if len(xs) != len(ys) {
+	if diff := cmp.Diff(a, b, opts...); diff != "" {
-					return false
+		t.Errorf("unexpected packages from parsing (-expected +actual)\n%s", diff)
-				}
-				for i, xe := range xs {
-					ye := ys[i]
-					if !cmptest.DefaultLocationComparer(xe, ye) {
-						return false
 	}
 }
 
-				return true
+func AssertPackagesEqualIgnoreLayers(t *testing.T, a, b pkg.Package, userOpts ...cmp.Option) {
-			},
+	t.Helper()
-		),
+	opts := cmptest.DefaultIgnoreLocationLayerOptions()
-		cmp.Comparer(
+	opts = append(opts, userOpts...)
-			func(x, y pkg.LicenseSet) bool {
-				xs := x.ToSlice()
-				ys := y.ToSlice()
-
-				if len(xs) != len(ys) {
-					return false
-				}
-				for i, xe := range xs {
-					ye := ys[i]
-					if !cmptest.DefaultLicenseComparer(xe, ye) {
-						return false
-					}
-				}
-
-				return true
-			},
-		),
-		cmp.Comparer(
-			cmptest.DefaultLocationComparer,
-		),
-		cmp.Comparer(
-			cmptest.DefaultLicenseComparer,
-		),
-	}
 
 	if diff := cmp.Diff(a, b, opts...); diff != "" {
 		t.Errorf("unexpected packages from parsing (-expected +actual)\n%s", diff)

syft/pkg/cataloger/python/parse_wheel_egg_metadata_test.go

@@ -76,7 +76,7 @@ func TestParseWheelEggMetadata(t *testing.T) {
 				t.Fatalf("failed to parse: %+v", err)
 			}
 
-			if d := cmp.Diff(test.ExpectedMetadata, actual, cmptest.DefaultCommonOptions()...); d != "" {
+			if d := cmp.Diff(test.ExpectedMetadata, actual, cmptest.DefaultOptions()...); d != "" {
 				t.Errorf("metadata mismatch (-want +got):\n%s", d)
 			}
 		})

syft/pkg/evidence.go

@@ -1,7 +1,9 @@
 package pkg
 
+import "github.com/anchore/syft/internal/evidence"
+
 const (
-	EvidenceAnnotationKey        = "evidence"
+	EvidenceAnnotationKey        = evidence.AnnotationKey
-	PrimaryEvidenceAnnotation    = "primary"
+	PrimaryEvidenceAnnotation    = evidence.PrimaryAnnotation
-	SupportingEvidenceAnnotation = "supporting"
+	SupportingEvidenceAnnotation = evidence.SupportingAnnotation
 )