Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Commit c253f7e

Browse files
darkowlzzdarkowlzz
authored
Merge pull request #1175 from darkowlzz/concurrent-ensure-update-validation
fix(ensure): concurrent update args validation
2 parents fcbc4be + ebf6207 commit c253f7e

File tree

2 files changed

+185
-31
lines changed

2 files changed

+185
-31
lines changed

‎cmd/dep/ensure.go

+79-31
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"path/filepath"
1616
"sort"
1717
"strings"
18+
"sync"
1819

1920
"github.com/golang/dep"
2021
"github.com/golang/dep/internal/gps"
@@ -111,6 +112,8 @@ dep ensure -update -no-vendor
111112
112113
`
113114

115+
var errUpdateArgsValidation = errors.New("update arguments validation failed")
116+
114117
func (cmd *ensureCommand) Name() string { return "ensure" }
115118
func (cmd *ensureCommand) Args() string {
116119
return "[-update | -add] [-no-vendor | -vendor-only] [-dry-run] [<spec>...]"
@@ -345,37 +348,8 @@ func (cmd *ensureCommand) runUpdate(ctx *dep.Ctx, args []string, p *dep.Project,
345348
params.ChangeAll = true
346349
}
347350

348-
// Allow any of specified project versions to change, regardless of the lock
349-
// file.
350-
for _, arg := range args {
351-
// Ensure the provided path has a deducible project root
352-
// TODO(sdboyer) do these concurrently
353-
pc, path, err := getProjectConstraint(arg, sm)
354-
if err != nil {
355-
// TODO(sdboyer) return all errors, not just the first one we encounter
356-
// TODO(sdboyer) ensure these errors are contextualized in a sensible way for -update
357-
return err
358-
}
359-
if path != string(pc.Ident.ProjectRoot) {
360-
// TODO(sdboyer): does this really merit an abortive error?
361-
return errors.Errorf("%s is not a project root, try %s instead", path, pc.Ident.ProjectRoot)
362-
}
363-
364-
if !p.Lock.HasProjectWithRoot(pc.Ident.ProjectRoot) {
365-
return errors.Errorf("%s is not present in %s, cannot -update it", pc.Ident.ProjectRoot, dep.LockName)
366-
}
367-
368-
if pc.Ident.Source != "" {
369-
return errors.Errorf("cannot specify alternate sources on -update (%s)", pc.Ident.Source)
370-
}
371-
372-
if !gps.IsAny(pc.Constraint) {
373-
// TODO(sdboyer) constraints should be allowed to allow solves that
374-
// target particular versions while remaining within declared constraints
375-
return errors.Errorf("version constraint %s passed for %s, but -update follows constraints declared in %s, not CLI arguments", pc.Constraint, pc.Ident.ProjectRoot, dep.ManifestName)
376-
}
377-
378-
params.ToChange = append(params.ToChange, gps.ProjectRoot(arg))
351+
if err := validateUpdateArgs(ctx, args, p, sm, &params); err != nil {
352+
return err
379353
}
380354

381355
// Re-prepare a solver now that our params are complete.
@@ -793,3 +767,77 @@ func (e pkgtreeErrs) Error() string {
793767

794768
return fmt.Sprintf("found %d errors in the package tree:\n%s", len(e), strings.Join(errs, "\n"))
795769
}
770+
771+
func validateUpdateArgs(ctx *dep.Ctx, args []string, p *dep.Project, sm gps.SourceManager, params *gps.SolveParameters) error {
772+
// Channel for receiving all the valid arguments.
773+
argsCh := make(chan string, len(args))
774+
775+
// Channel for receiving all the validation errors.
776+
errCh := make(chan error, len(args))
777+
778+
var wg sync.WaitGroup
779+
780+
// Allow any of specified project versions to change, regardless of the lock
781+
// file.
782+
for _, arg := range args {
783+
wg.Add(1)
784+
785+
go func(arg string) {
786+
defer wg.Done()
787+
788+
// Ensure the provided path has a deducible project root.
789+
pc, path, err := getProjectConstraint(arg, sm)
790+
if err != nil {
791+
// TODO(sdboyer) ensure these errors are contextualized in a sensible way for -update
792+
errCh <- err
793+
return
794+
}
795+
if path != string(pc.Ident.ProjectRoot) {
796+
// TODO(sdboyer): does this really merit an abortive error?
797+
errCh <- errors.Errorf("%s is not a project root, try %s instead", path, pc.Ident.ProjectRoot)
798+
return
799+
}
800+
801+
if !p.Lock.HasProjectWithRoot(pc.Ident.ProjectRoot) {
802+
errCh <- errors.Errorf("%s is not present in %s, cannot -update it", pc.Ident.ProjectRoot, dep.LockName)
803+
return
804+
}
805+
806+
if pc.Ident.Source != "" {
807+
errCh <- errors.Errorf("cannot specify alternate sources on -update (%s)", pc.Ident.Source)
808+
return
809+
}
810+
811+
if !gps.IsAny(pc.Constraint) {
812+
// TODO(sdboyer) constraints should be allowed to allow solves that
813+
// target particular versions while remaining within declared constraints.
814+
errCh <- errors.Errorf("version constraint %s passed for %s, but -update follows constraints declared in %s, not CLI arguments", pc.Constraint, pc.Ident.ProjectRoot, dep.ManifestName)
815+
return
816+
}
817+
818+
// Valid argument.
819+
argsCh <- arg
820+
}(arg)
821+
}
822+
823+
wg.Wait()
824+
close(errCh)
825+
close(argsCh)
826+
827+
// Log all the errors.
828+
if len(errCh) > 0 {
829+
ctx.Err.Printf("Invalid arguments passed to ensure -update:\n\n")
830+
for err := range errCh {
831+
ctx.Err.Println(" ✗", err.Error())
832+
}
833+
ctx.Err.Println()
834+
return errUpdateArgsValidation
835+
}
836+
837+
// Add all the valid arguments to solve params.
838+
for arg := range argsCh {
839+
params.ToChange = append(params.ToChange, gps.ProjectRoot(arg))
840+
}
841+
842+
return nil
843+
}

‎cmd/dep/ensure_test.go

+106
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,18 @@
55
package main
66

77
import (
8+
"bytes"
89
"errors"
910
"go/build"
11+
"io/ioutil"
12+
"log"
13+
"strings"
1014
"testing"
1115

16+
"github.com/golang/dep"
1217
"github.com/golang/dep/internal/gps"
1318
"github.com/golang/dep/internal/gps/pkgtree"
19+
"github.com/golang/dep/internal/test"
1420
)
1521

1622
func TestInvalidEnsureFlagCombinations(t *testing.T) {
@@ -139,3 +145,103 @@ func TestCheckErrors(t *testing.T) {
139145
})
140146
}
141147
}
148+
149+
func TestValidateUpdateArgs(t *testing.T) {
150+
cases := []struct {
151+
name string
152+
args []string
153+
wantError error
154+
wantWarn []string
155+
lockedProjects []string
156+
}{
157+
{
158+
name: "empty args",
159+
args: []string{},
160+
wantError: nil,
161+
},
162+
{
163+
name: "not project root",
164+
args: []string{"github.com/golang/dep/cmd"},
165+
wantError: errUpdateArgsValidation,
166+
wantWarn: []string{
167+
"github.com/golang/dep/cmd is not a project root, try github.com/golang/dep instead",
168+
},
169+
},
170+
{
171+
name: "not present in lock",
172+
args: []string{"github.com/golang/dep"},
173+
wantError: errUpdateArgsValidation,
174+
wantWarn: []string{
175+
"github.com/golang/dep is not present in Gopkg.lock, cannot -update it",
176+
},
177+
},
178+
{
179+
name: "cannot specify alternate sources",
180+
args: []string{"github.com/golang/dep:github.com/example/dep"},
181+
wantError: errUpdateArgsValidation,
182+
wantWarn: []string{
183+
"cannot specify alternate sources on -update (github.com/example/dep)",
184+
},
185+
lockedProjects: []string{"github.com/golang/dep"},
186+
},
187+
{
188+
name: "version constraint passed",
189+
args: []string{"github.com/golang/dep@master"},
190+
wantError: errUpdateArgsValidation,
191+
wantWarn: []string{
192+
"version constraint master passed for github.com/golang/dep, but -update follows constraints declared in Gopkg.toml, not CLI arguments",
193+
},
194+
lockedProjects: []string{"github.com/golang/dep"},
195+
},
196+
}
197+
198+
h := test.NewHelper(t)
199+
defer h.Cleanup()
200+
201+
h.TempDir("src")
202+
pwd := h.Path(".")
203+
204+
stderrOutput := &bytes.Buffer{}
205+
errLogger := log.New(stderrOutput, "", 0)
206+
ctx := &dep.Ctx{
207+
GOPATH: pwd,
208+
Out: log.New(ioutil.Discard, "", 0),
209+
Err: errLogger,
210+
}
211+
212+
sm, err := ctx.SourceManager()
213+
h.Must(err)
214+
defer sm.Release()
215+
216+
p := new(dep.Project)
217+
params := p.MakeParams()
218+
219+
for _, c := range cases {
220+
t.Run(c.name, func(t *testing.T) {
221+
// Empty the buffer for every case
222+
stderrOutput.Reset()
223+
224+
// Fill up the locked projects
225+
lockedProjects := []gps.LockedProject{}
226+
for _, lp := range c.lockedProjects {
227+
pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(lp)}
228+
lockedProjects = append(lockedProjects, gps.NewLockedProject(pi, gps.NewVersion("v1.0.0"), []string{}))
229+
}
230+
231+
// Add lock to project
232+
p.Lock = &dep.Lock{P: lockedProjects}
233+
234+
err := validateUpdateArgs(ctx, c.args, p, sm, &params)
235+
if err != c.wantError {
236+
t.Fatalf("Unexpected error while validating update args:\n\t(GOT): %v\n\t(WNT): %v", err, c.wantError)
237+
}
238+
239+
warnings := stderrOutput.String()
240+
for _, warn := range c.wantWarn {
241+
if !strings.Contains(warnings, warn) {
242+
t.Fatalf("Expected validateUpdateArgs errors to contain: %q", warn)
243+
}
244+
}
245+
})
246+
}
247+
}

0 commit comments

Comments
 (0)