Use values in relationship To/From fields (#2871)

* use pkg values in relationship fields

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

* add linter rule for using values in relationships

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

* use new cmptest package for comparing relationships

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

* create cmptest for common cmp.Diff options in test

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

* condense matches for relationship ruleguard

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

* remove relationship type from rules

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

* restore build tag

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

* suggest using values

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

* nil check pkgs

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

@@ -0,0 +1,71 @@
+package cmptest
+
+import (
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
+	"github.com/anchore/syft/syft/file"
+	"github.com/anchore/syft/syft/pkg"
+)
+
+func DefaultCommonOptions() []cmp.Option {
+	return CommonOptions(nil, nil)
+}
+
+func CommonOptions(licenseCmp LicenseComparer, locationCmp LocationComparer) []cmp.Option {
+	if licenseCmp == nil {
+		licenseCmp = DefaultLicenseComparer
+	}
+
+	if locationCmp == nil {
+		locationCmp = DefaultLocationComparer
+	}
+
+	return []cmp.Option{
+		cmpopts.IgnoreFields(pkg.Package{}, "id"), // note: ID is not deterministic for test purposes
+		cmpopts.SortSlices(pkg.Less),
+		cmpopts.SortSlices(DefaultRelationshipComparer),
+		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 !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/diff_reporter.go

@@ -0,0 +1,37 @@
+package cmptest
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+// DiffReporter is a simple custom reporter that only records differences detected during comparison.
+type DiffReporter struct {
+	path  cmp.Path
+	diffs []string
+}
+
+func NewDiffReporter() DiffReporter {
+	return DiffReporter{}
+}
+
+func (r *DiffReporter) PushStep(ps cmp.PathStep) {
+	r.path = append(r.path, ps)
+}
+
+func (r *DiffReporter) Report(rs cmp.Result) {
+	if !rs.Equal() {
+		vx, vy := r.path.Last().Values()
+		r.diffs = append(r.diffs, fmt.Sprintf("%#v:\n\t-: %+v\n\t+: %+v\n", r.path, vx, vy))
+	}
+}
+
+func (r *DiffReporter) PopStep() {
+	r.path = r.path[:len(r.path)-1]
+}
+
+func (r *DiffReporter) String() string {
+	return strings.Join(r.diffs, "\n")
+}

internal/cmptest/license.go

@@ -0,0 +1,29 @@
+package cmptest
+
+import (
+	"github.com/google/go-cmp/cmp"
+
+	"github.com/anchore/syft/syft/file"
+	"github.com/anchore/syft/syft/pkg"
+)
+
+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(
+		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 !DefaultLocationComparer(xe, ye) {
+					return false
+				}
+			}
+			return true
+		},
+	))
+}

internal/cmptest/location.go

@@ -0,0 +1,13 @@
+package cmptest
+
+import (
+	"github.com/google/go-cmp/cmp"
+
+	"github.com/anchore/syft/syft/file"
+)
+
+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)
+}

internal/cmptest/relationship.go

@@ -0,0 +1,26 @@
+package cmptest
+
+import (
+	"github.com/sanity-io/litter"
+
+	"github.com/anchore/syft/syft/artifact"
+)
+
+type RelationshipComparer func(x, y artifact.Relationship) bool
+
+var relationshipStringer = litter.Options{
+	Compact:           true,
+	StripPackageNames: false,
+	HidePrivateFields: true, // we want to ignore package IDs
+	HideZeroValues:    true,
+	StrictGo:          true,
+	//FieldExclusions: ...  // these can be added for future values that need to be ignored
+	//FieldFilter: ...
+}
+
+func DefaultRelationshipComparer(x, y artifact.Relationship) bool {
+	// we just need a stable sort, the ordering does not need to be sensible
+	xStr := relationshipStringer.Sdump(x)
+	yStr := relationshipStringer.Sdump(y)
+	return xStr < yStr
+}

internal/relationship/by_file_ownership.go

@@ -58,9 +58,19 @@ func byFileOwnershipOverlap(catalog *pkg.Collection) []artifact.Relationship {
 			parent := catalog.Package(parentID) // TODO: this is potentially expensive
 			child := catalog.Package(childID)   // TODO: this is potentially expensive
 
+			if parent == nil {
+				log.Tracef("parent package not found: %v", parentID)
+				continue
+			}
+
+			if child == nil {
+				log.Tracef("child package not found: %v", childID)
+				continue
+			}
+
 			edges = append(edges, artifact.Relationship{
-				From: parent,
+				From: *parent,
-				To:   child,
+				To:   *child,
 				Type: artifact.OwnershipByFileOverlapRelationship,
 				Data: ownershipByFilesMetadata{
 					Files: fs,

internal/relationship/by_file_ownership_test.go

@@ -3,8 +3,10 @@ package relationship
 import (
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
 
+	"github.com/anchore/syft/internal/cmptest"
 	"github.com/anchore/syft/syft/artifact"
 	"github.com/anchore/syft/syft/file"
 	"github.com/anchore/syft/syft/pkg"
@@ -143,10 +145,9 @@ func TestOwnershipByFilesRelationship(t *testing.T) {
 			assert.Len(t, relationships, len(expectedRelations))
 			for idx, expectedRelationship := range expectedRelations {
 				actualRelationship := relationships[idx]
-				assert.Equal(t, expectedRelationship.From.ID(), actualRelationship.From.ID())
+				if d := cmp.Diff(expectedRelationship, actualRelationship, cmptest.DefaultCommonOptions()...); d != "" {
-				assert.Equal(t, expectedRelationship.To.ID(), actualRelationship.To.ID())
+					t.Errorf("unexpected relationship (-want, +got): %s", d)
-				assert.Equal(t, expectedRelationship.Type, actualRelationship.Type)
+				}
-				assert.Equal(t, expectedRelationship.Data, actualRelationship.Data)
 			}
 		})
 	}

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

@@ -2,7 +2,6 @@ package pkgtest
 
 import (
 	"context"
-	"fmt"
 	"io"
 	"os"
 	"strings"
@@ -10,11 +9,11 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
-	"github.com/sanity-io/litter"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/anchore/stereoscope/pkg/imagetest"
+	"github.com/anchore/syft/internal/cmptest"
 	"github.com/anchore/syft/internal/relationship"
 	"github.com/anchore/syft/syft/artifact"
 	"github.com/anchore/syft/syft/file"
@@ -26,9 +25,6 @@ import (
 	"github.com/anchore/syft/syft/source/stereoscopesource"
 )
 
-type locationComparer func(x, y file.Location) bool
-type licenseComparer func(x, y pkg.License) bool
-
 type CatalogTester struct {
 	expectedPkgs                   []pkg.Package
 	expectedRelationships          []artifact.Relationship
@@ -42,16 +38,16 @@ type CatalogTester struct {
 	resolver                       file.Resolver
 	wantErr                        require.ErrorAssertionFunc
 	compareOptions                 []cmp.Option
-	locationComparer               locationComparer
+	locationComparer               cmptest.LocationComparer
-	licenseComparer                licenseComparer
+	licenseComparer                cmptest.LicenseComparer
 	customAssertions               []func(t *testing.T, pkgs []pkg.Package, relationships []artifact.Relationship)
 }
 
 func NewCatalogTester() *CatalogTester {
 	return &CatalogTester{
 		wantErr:          require.NoError,
-		locationComparer: DefaultLocationComparer,
+		locationComparer: cmptest.DefaultLocationComparer,
-		licenseComparer:  DefaultLicenseComparer,
+		licenseComparer:  cmptest.DefaultLicenseComparer,
 		ignoreUnfulfilledPathResponses: map[string][]string{
 			"FilesByPath": {
 				// most catalogers search for a linux release, which will not be fulfilled in testing
@@ -65,29 +61,6 @@ func NewCatalogTester() *CatalogTester {
 	}
 }
 
-func DefaultLocationComparer(x, y file.Location) bool {
-	return cmp.Equal(x.Coordinates, y.Coordinates) && cmp.Equal(x.AccessPath, y.AccessPath)
-}
-
-func DefaultLicenseComparer(x, y pkg.License) bool {
-	return cmp.Equal(x, y, cmp.Comparer(DefaultLocationComparer), 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 !DefaultLocationComparer(xe, ye) {
-					return false
-				}
-			}
-			return true
-		},
-	))
-}
-
 func (p *CatalogTester) FromDirectory(t *testing.T, path string) *CatalogTester {
 	t.Helper()
 
@@ -270,77 +243,14 @@ func (p *CatalogTester) TestCataloger(t *testing.T, cataloger pkg.Cataloger) {
 	}
 }
 
-var relationshipStringer = litter.Options{
-	Compact:           true,
-	StripPackageNames: false,
-	HidePrivateFields: true, // we want to ignore package IDs
-	HideZeroValues:    true,
-	StrictGo:          true,
-	//FieldExclusions: ...  // these can be added for future values that need to be ignored
-	//FieldFilter: ...
-}
-
-func relationshipLess(x, y artifact.Relationship) bool {
-	// we just need a stable sort, the ordering does not need to be sensible
-	xStr := relationshipStringer.Sdump(x)
-	yStr := relationshipStringer.Sdump(y)
-	return xStr < yStr
-}
-
 // nolint:funlen
 func (p *CatalogTester) assertPkgs(t *testing.T, pkgs []pkg.Package, relationships []artifact.Relationship) {
 	t.Helper()
 
-	p.compareOptions = append(p.compareOptions,
+	p.compareOptions = append(p.compareOptions, cmptest.CommonOptions(p.licenseComparer, p.locationComparer)...)
-		cmpopts.IgnoreFields(pkg.Package{}, "id"), // note: ID is not deterministic for test purposes
-		cmpopts.SortSlices(pkg.Less),
-		cmpopts.SortSlices(relationshipLess),
-		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
-			},
-		),
-		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 !p.licenseComparer(xe, ye) {
-						return false
-					}
-				}
-
-				return true
-			},
-		),
-		cmp.Comparer(
-			p.locationComparer,
-		),
-		cmp.Comparer(
-			p.licenseComparer,
-		),
-	)
 
 	{
-		var r diffReporter
+		r := cmptest.NewDiffReporter()
 		var opts []cmp.Option
 
 		opts = append(opts, p.compareOptions...)
@@ -356,7 +266,7 @@ func (p *CatalogTester) assertPkgs(t *testing.T, pkgs []pkg.Package, relationshi
 		}
 	}
 	{
-		var r diffReporter
+		r := cmptest.NewDiffReporter()
 		var opts []cmp.Option
 
 		opts = append(opts, p.compareOptions...)
@@ -399,7 +309,7 @@ func AssertPackagesEqual(t *testing.T, a, b pkg.Package) {
 				}
 				for i, xe := range xs {
 					ye := ys[i]
-					if !DefaultLocationComparer(xe, ye) {
+					if !cmptest.DefaultLocationComparer(xe, ye) {
 						return false
 					}
 				}
@@ -417,7 +327,7 @@ func AssertPackagesEqual(t *testing.T, a, b pkg.Package) {
 				}
 				for i, xe := range xs {
 					ye := ys[i]
-					if !DefaultLicenseComparer(xe, ye) {
+					if !cmptest.DefaultLicenseComparer(xe, ye) {
 						return false
 					}
 				}
@@ -426,10 +336,10 @@ func AssertPackagesEqual(t *testing.T, a, b pkg.Package) {
 			},
 		),
 		cmp.Comparer(
-			DefaultLocationComparer,
+			cmptest.DefaultLocationComparer,
 		),
 		cmp.Comparer(
-			DefaultLicenseComparer,
+			cmptest.DefaultLicenseComparer,
 		),
 	}
 
@@ -437,28 +347,3 @@ func AssertPackagesEqual(t *testing.T, a, b pkg.Package) {
 		t.Errorf("unexpected packages from parsing (-expected +actual)\n%s", diff)
 	}
 }
-
-// diffReporter is a simple custom reporter that only records differences detected during comparison.
-type diffReporter struct {
-	path  cmp.Path
-	diffs []string
-}
-
-func (r *diffReporter) PushStep(ps cmp.PathStep) {
-	r.path = append(r.path, ps)
-}
-
-func (r *diffReporter) Report(rs cmp.Result) {
-	if !rs.Equal() {
-		vx, vy := r.path.Last().Values()
-		r.diffs = append(r.diffs, fmt.Sprintf("%#v:\n\t-: %+v\n\t+: %+v\n", r.path, vx, vy))
-	}
-}
-
-func (r *diffReporter) PopStep() {
-	r.path = r.path[:len(r.path)-1]
-}
-
-func (r *diffReporter) String() string {
-	return strings.Join(r.diffs, "\n")
-}

test/rules/rules.go

@@ -2,10 +2,15 @@
 
 package rules
 
-import "github.com/quasilyte/go-ruleguard/dsl"
+import (
+	"strings"
+
+	"github.com/quasilyte/go-ruleguard/dsl"
+)
 
 // nolint:unused
 func resourceCleanup(m dsl.Matcher) {
+	// this rule defends against use of internal.CloseAndLogError() without a defer statement
 	m.Match(`$res, $err := $resolver.FileContentsByLocation($loc); if $*_ { $*_ }; $next`).
 		Where(m["res"].Type.Implements(`io.Closer`) &&
 			m["res"].Type.Implements(`io.Reader`) &&
@@ -13,3 +18,31 @@ func resourceCleanup(m dsl.Matcher) {
 			!m["next"].Text.Matches(`defer internal.CloseAndLogError`)).
 		Report(`please call "defer internal.CloseAndLogError($res, $loc.RealPath)" right after checking the error returned from $resolver.FileContentsByLocation.`)
 }
+
+// nolint:unused
+func isPtr(ctx *dsl.VarFilterContext) bool {
+	return strings.HasPrefix(ctx.Type.String(), "*") || strings.HasPrefix(ctx.Type.Underlying().String(), "*")
+}
+
+// nolint:unused
+func packagesInRelationshipsAsValues(m dsl.Matcher) {
+	m.Import("github.com/anchore/syft/syft/artifact")
+
+	isRelationship := func(m dsl.Matcher) bool {
+		return m["x"].Type.Is("artifact.Relationship")
+	}
+
+	hasPointerType := func(m dsl.Matcher) bool {
+		return m["y"].Filter(isPtr)
+	}
+
+	// this rule defends against using pointers as values in artifact.Relationship
+	m.Match(
+		`$x{$*_, From: $y, $*_}`,
+		`$x{$*_, To: $y, $*_}`,
+		`$x.From = $y`,
+		`$x.To = $y`,
+	).
+		Where(isRelationship(m) && hasPointerType(m)).
+		Report("pointer used as a value for From/To field in artifact.Relationship (use values instead)")
+}