diff --git a/internal/cmptest/common_options.go b/internal/cmptest/common_options.go new file mode 100644 index 000000000..ecfb54edf --- /dev/null +++ b/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, + ), + } +} diff --git a/internal/cmptest/diff_reporter.go b/internal/cmptest/diff_reporter.go new file mode 100644 index 000000000..c4f186763 --- /dev/null +++ b/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") +} diff --git a/internal/cmptest/license.go b/internal/cmptest/license.go new file mode 100644 index 000000000..f63cda296 --- /dev/null +++ b/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 + }, + )) +} diff --git a/internal/cmptest/location.go b/internal/cmptest/location.go new file mode 100644 index 000000000..b7654ab46 --- /dev/null +++ b/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) +} diff --git a/internal/cmptest/relationship.go b/internal/cmptest/relationship.go new file mode 100644 index 000000000..583b28dca --- /dev/null +++ b/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 +} diff --git a/internal/relationship/by_file_ownership.go b/internal/relationship/by_file_ownership.go index 3c420217c..2db6e660a 100644 --- a/internal/relationship/by_file_ownership.go +++ b/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, - To: child, + From: *parent, + To: *child, Type: artifact.OwnershipByFileOverlapRelationship, Data: ownershipByFilesMetadata{ Files: fs, diff --git a/internal/relationship/by_file_ownership_test.go b/internal/relationship/by_file_ownership_test.go index 396150240..f87b228a4 100644 --- a/internal/relationship/by_file_ownership_test.go +++ b/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()) - assert.Equal(t, expectedRelationship.To.ID(), actualRelationship.To.ID()) - assert.Equal(t, expectedRelationship.Type, actualRelationship.Type) - assert.Equal(t, expectedRelationship.Data, actualRelationship.Data) + if d := cmp.Diff(expectedRelationship, actualRelationship, cmptest.DefaultCommonOptions()...); d != "" { + t.Errorf("unexpected relationship (-want, +got): %s", d) + } } }) } diff --git a/syft/pkg/cataloger/internal/pkgtest/test_generic_parser.go b/syft/pkg/cataloger/internal/pkgtest/test_generic_parser.go index 227620aa5..4384e3e21 100644 --- a/syft/pkg/cataloger/internal/pkgtest/test_generic_parser.go +++ b/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 - licenseComparer licenseComparer + locationComparer cmptest.LocationComparer + licenseComparer cmptest.LicenseComparer customAssertions []func(t *testing.T, pkgs []pkg.Package, relationships []artifact.Relationship) } func NewCatalogTester() *CatalogTester { return &CatalogTester{ wantErr: require.NoError, - locationComparer: DefaultLocationComparer, - licenseComparer: DefaultLicenseComparer, + locationComparer: cmptest.DefaultLocationComparer, + 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, - 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, - ), - ) + p.compareOptions = append(p.compareOptions, cmptest.CommonOptions(p.licenseComparer, p.locationComparer)...) { - 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") -} diff --git a/test/rules/rules.go b/test/rules/rules.go index 24e98ae20..b102c68c7 100644 --- a/test/rules/rules.go +++ b/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)") +}