Skip to content

Commit

Permalink
fix: Improve performance of content destination collision detection (#…
Browse files Browse the repository at this point in the history
…388)

* Fix: Improve performance of content destination collision detection.

* Remove duplicate info.Validate call

* Fix tests.

* Remove another Validate call.
  • Loading branch information
erikgeiser authored Nov 6, 2021
1 parent 72daac2 commit 3f068f0
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 42 deletions.
52 changes: 23 additions & 29 deletions files/files.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,33 +116,35 @@ func (c *Content) Sys() interface{} {

// ExpandContentGlobs gathers all of the real files to be copied into the package.
func ExpandContentGlobs(contents Contents, disableGlobbing bool) (files Contents, err error) {
options := []fileglob.OptFunc{fileglob.MatchDirectoryIncludesContents}
if disableGlobbing {
options = append(options, fileglob.QuoteMeta)
}

for _, f := range contents {
var globbed map[string]string

switch f.Type {
case "ghost", "symlink":
// Ghost and symlink files need to be in the list, but dont glob them because they do not really exist
files, err = appendWithUniqueDestination(files, f.WithFileInfoDefaults())
files = append(files, f.WithFileInfoDefaults())
default:
globbed, err = glob.Glob(f.Source, f.Destination, options...)
if err != nil {
return nil, err
}

continue
files, err = appendGlobbedFiles(files, globbed, f)
if err != nil {
return nil, err
}
}

options := []fileglob.OptFunc{fileglob.MatchDirectoryIncludesContents}
if disableGlobbing {
options = append(options, fileglob.QuoteMeta)
}
globbed, err = glob.Glob(f.Source, f.Destination, options...)
if err != nil {
return nil, err
}
}

files, err = appendGlobbedFiles(globbed, f, files)
if err != nil {
return nil, err
}
err = checkNoCollisions(files)
if err != nil {
return nil, err
}

// sort the files for reproducibility and general cleanliness
Expand All @@ -151,8 +153,7 @@ func ExpandContentGlobs(contents Contents, disableGlobbing bool) (files Contents
return files, nil
}

func appendGlobbedFiles(globbed map[string]string, origFile *Content, incFiles Contents) (files Contents, err error) {
files = append(files, incFiles...)
func appendGlobbedFiles(all Contents, globbed map[string]string, origFile *Content) (Contents, error) {
for src, dst := range globbed {
newFile := &Content{
Destination: ToNixPath(dst),
Expand All @@ -162,36 +163,29 @@ func appendGlobbedFiles(globbed map[string]string, origFile *Content, incFiles C
Packager: origFile.Packager,
}

files, err = appendWithUniqueDestination(files, newFile.WithFileInfoDefaults())
if err != nil {
return nil, err
}
all = append(all, newFile.WithFileInfoDefaults())
}

return files, nil
return all, nil
}

var ErrContentCollision = fmt.Errorf("content collision")

func appendWithUniqueDestination(slice []*Content, elems ...*Content) ([]*Content, error) {
func checkNoCollisions(contents Contents) error {
alreadyPresent := map[string]*Content{}

for _, presentElem := range slice {
alreadyPresent[presentElem.Destination] = presentElem
}

for _, elem := range elems {
for _, elem := range contents {
present, ok := alreadyPresent[elem.Destination]
if ok && (present.Packager == "" || elem.Packager == "" || present.Packager == elem.Packager) {
return nil, fmt.Errorf(
return fmt.Errorf(
"cannot add %s because %s is already present at the same destination (%s): %w",
elem.Source, present.Source, present.Destination, ErrContentCollision)
}

alreadyPresent[elem.Destination] = elem
}

return append(slice, elems...), nil
return nil
}

// ToNixPath converts the given path to a nix-style path.
Expand Down
4 changes: 0 additions & 4 deletions internal/cmd/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,6 @@ func doPackage(configPath, target, packager string) error {

info = nfpm.WithDefaults(info)

if err = nfpm.Validate(info); err != nil {
return err
}

fmt.Printf("using %s packager...\n", packager)
pkg, err := nfpm.Get(packager)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion nfpm.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func ParseWithEnvMapping(in io.Reader, mapping func(string) string) (config Conf

WithDefaults(&config.Info)

return config, config.Validate()
return config, nil
}

// ParseFile decodes YAML data from a file path into a configuration struct.
Expand Down
25 changes: 17 additions & 8 deletions nfpm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,21 +153,30 @@ func TestValidateError(t *testing.T) {
}
}

func parseAndValidate(filename string) (nfpm.Config, error) {
config, err := nfpm.ParseFile(filename)
if err == nil {
err = config.Validate()
}

return config, err
}

func TestParseFile(t *testing.T) {
nfpm.ClearPackagers()
_, err := nfpm.ParseFile("./testdata/overrides.yaml")
_, err := parseAndValidate("./testdata/overrides.yaml")
require.Error(t, err)
nfpm.RegisterPackager("deb", &fakePackager{})
nfpm.RegisterPackager("rpm", &fakePackager{})
nfpm.RegisterPackager("apk", &fakePackager{})
_, err = nfpm.ParseFile("./testdata/overrides.yaml")
_, err = parseAndValidate("./testdata/overrides.yaml")
require.NoError(t, err)
_, err = nfpm.ParseFile("./testdata/doesnotexist.yaml")
_, err = parseAndValidate("./testdata/doesnotexist.yaml")
require.Error(t, err)
os.Setenv("RPM_KEY_FILE", "my/rpm/key/file")
os.Setenv("TEST_RELEASE_ENV_VAR", "1234")
os.Setenv("TEST_PRERELEASE_ENV_VAR", "beta1")
config, err := nfpm.ParseFile("./testdata/env-fields.yaml")
config, err := parseAndValidate("./testdata/env-fields.yaml")
require.NoError(t, err)
require.Equal(t, fmt.Sprintf("v%s", os.Getenv("GOROOT")), config.Version)
require.Equal(t, "1234", config.Release)
Expand All @@ -178,22 +187,22 @@ func TestParseFile(t *testing.T) {
}

func TestParseEnhancedFile(t *testing.T) {
config, err := nfpm.ParseFile("./testdata/contents.yaml")
config, err := parseAndValidate("./testdata/contents.yaml")
require.NoError(t, err)
require.Equal(t, config.Name, "contents foo")
require.Equal(t, "contents foo", config.Name)
shouldFind := 5
require.Len(t, config.Contents, shouldFind)
}

func TestParseEnhancedNestedGlobFile(t *testing.T) {
config, err := nfpm.ParseFile("./testdata/contents_glob.yaml")
config, err := parseAndValidate("./testdata/contents_glob.yaml")
require.NoError(t, err)
shouldFind := 3
require.Len(t, config.Contents, shouldFind)
}

func TestParseEnhancedNestedNoGlob(t *testing.T) {
config, err := nfpm.ParseFile("./testdata/contents_directory.yaml")
config, err := parseAndValidate("./testdata/contents_directory.yaml")
require.NoError(t, err)
shouldFind := 3
require.Len(t, config.Contents, shouldFind)
Expand Down

0 comments on commit 3f068f0

Please sign in to comment.