Stricter checks on libs/cores arguments

cli/globals/args.go

@@ -54,10 +54,18 @@ func ParseReferenceArgs(args []string, parseArch bool) ([]*ReferenceArg, error)
 // "packager:arch@version", useful to represent a platform (or core) name.
 func ParseReferenceArg(arg string, parseArch bool) (*ReferenceArg, error) {
 	ret := &ReferenceArg{}
-
+	if arg == "" {
+		return nil, fmt.Errorf("invalid empry core argument")
+	}
 	toks := strings.SplitN(arg, "@", 2)
+	if toks[0] == "" {
+		return nil, fmt.Errorf("invalid empty core reference '%s'", arg)
+	}
 	ret.PackageName = toks[0]
 	if len(toks) > 1 {
+		if toks[1] == "" {
+			return nil, fmt.Errorf("invalid empty core version: '%s'", arg)
+		}
 		ret.Version = toks[1]
 	}
 
@@ -66,9 +74,63 @@ func ParseReferenceArg(arg string, parseArch bool) (*ReferenceArg, error) {
 		if len(toks) != 2 {
 			return nil, fmt.Errorf("invalid item %s", arg)
 		}
+		if toks[0] == "" {
+			return nil, fmt.Errorf("invalid empty core name '%s'", arg)
+		}
 		ret.PackageName = toks[0]
+		if toks[1] == "" {
+			return nil, fmt.Errorf("invalid empty core architecture '%s'", arg)
+		}
 		ret.Architecture = toks[1]
 	}
 
 	return ret, nil
 }
+
+// LibraryReferenceArg is a command line argument that reference a library.
+type LibraryReferenceArg struct {
+	Name    string
+	Version string
+}
+
+func (r *LibraryReferenceArg) String() string {
+	if r.Version != "" {
+		return r.Name + "@" + r.Version
+	}
+	return r.Name
+}
+
+// ParseLibraryReferenceArg parse a command line argument that reference a
+// library in the form "LibName@Version" or just "LibName".
+func ParseLibraryReferenceArg(arg string) (*LibraryReferenceArg, error) {
+	tokens := strings.SplitN(arg, "@", 2)
+
+	ret := &LibraryReferenceArg{}
+	// TODO: check library Name constraints
+	// TODO: check library Version constraints
+	if tokens[0] == "" {
+		return nil, fmt.Errorf("invalid empty library name")
+	}
+	ret.Name = tokens[0]
+	if len(tokens) > 1 {
+		if tokens[1] == "" {
+			return nil, fmt.Errorf("invalid empty library version: %s", arg)
+		}
+		ret.Version = tokens[1]
+	}
+	return ret, nil
+}
+
+// ParseLibraryReferenceArgs is a convenient wrapper that operates on a slice of strings and
+// calls ParseLibraryReferenceArg for each of them. It returns at the first invalid argument.
+func ParseLibraryReferenceArgs(args []string) ([]*LibraryReferenceArg, error) {
+	ret := []*LibraryReferenceArg{}
+	for _, arg := range args {
+		if reference, err := ParseLibraryReferenceArg(arg); err == nil {
+			ret = append(ret, reference)
+		} else {
+			return nil, err
+		}
+	}
+	return ret, nil
+}

cli/globals/args_test.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/arduino/arduino-cli/cli/globals"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 var goodCores = []struct {
@@ -30,16 +31,14 @@ var goodCores = []struct {
 }{
 	{"arduino:avr", &globals.ReferenceArg{"arduino", "avr", ""}},
 	{"arduino:[email protected]", &globals.ReferenceArg{"arduino", "avr", "1.6.20"}},
-	{"arduino:avr@", &globals.ReferenceArg{"arduino", "avr", ""}},
 }
 
 var goodLibs = []struct {
 	in       string
-	expected *globals.ReferenceArg
+	expected *globals.LibraryReferenceArg
 }{
-	{"mylib", &globals.ReferenceArg{"mylib", "", ""}},
-	{"[email protected]", &globals.ReferenceArg{"mylib", "", "1.0"}},
-	{"mylib@", &globals.ReferenceArg{"mylib", "", ""}},
+	{"mylib", &globals.LibraryReferenceArg{"mylib", ""}},
+	{"[email protected]", &globals.LibraryReferenceArg{"mylib", "1.0"}},
 }
 
 var badCores = []struct {
@@ -49,6 +48,28 @@ var badCores = []struct {
 	{"arduino:avr:avr", nil},
 	{"[email protected]:avr", nil},
 	{"arduino:avr:[email protected]", nil},
+	{"arduino:@1.6.20", nil},
+	{":[email protected]", nil},
+	{"@1.5.0", nil},
+	{"arduino:avr@", nil},
+	{"", nil},
+}
+
+var badLibs = []struct {
+	in       string
+	expected *globals.LibraryReferenceArg
+}{
+	{"", nil},
+	{"mylib@", nil},
+}
+
+func TestArgsStringify(t *testing.T) {
+	for _, lib := range goodLibs {
+		require.Equal(t, lib.in, lib.expected.String())
+	}
+	for _, core := range goodCores {
+		require.Equal(t, core.in, core.expected.String())
+	}
 }
 
 func TestParseReferenceArgCores(t *testing.T) {
@@ -60,28 +81,34 @@ func TestParseReferenceArgCores(t *testing.T) {
 
 	for _, tt := range badCores {
 		actual, err := globals.ParseReferenceArg(tt.in, true)
-		assert.NotNil(t, err)
-		assert.Equal(t, tt.expected, actual)
-	}
-
-	// library refs are not good as core's
-	for _, tt := range goodLibs {
-		_, err := globals.ParseReferenceArg(tt.in, true)
-		assert.NotNil(t, err)
+		require.NotNil(t, err, "Testing bad core '%s'", tt.in)
+		require.Equal(t, tt.expected, actual, "Testing bad core '%s'", tt.in)
 	}
 }
 
 func TestParseReferenceArgLibs(t *testing.T) {
 	for _, tt := range goodLibs {
-		actual, err := globals.ParseReferenceArg(tt.in, false)
-		assert.Nil(t, err)
-		assert.Equal(t, tt.expected, actual)
+		actual, err := globals.ParseLibraryReferenceArg(tt.in)
+		assert.Nil(t, err, "Testing good arg '%s'", tt.in)
+		assert.Equal(t, tt.expected, actual, "Testing good arg '%s'", tt.in)
 	}
+	for _, tt := range badLibs {
+		res, err := globals.ParseLibraryReferenceArg(tt.in)
+		require.Nil(t, res, "Testing bad arg '%s'", tt.in)
+		require.NotNil(t, err, "Testing bad arg '%s'", tt.in)
+	}
+}
 
-	// good libs are bad when requiring Arch to be present
+func TestParseLibraryReferenceArgs(t *testing.T) {
+	args := []string{}
 	for _, tt := range goodLibs {
-		_, err := globals.ParseReferenceArg(tt.in, true)
-		assert.NotNil(t, err)
+		args = append(args, tt.in)
+	}
+	refs, err := globals.ParseLibraryReferenceArgs(args)
+	require.Nil(t, err)
+	require.Len(t, refs, len(goodLibs))
+	for i, tt := range goodLibs {
+		assert.Equal(t, tt.expected, refs[i])
 	}
 }
 

cli/lib/download.go

@@ -47,7 +47,7 @@ func initDownloadCommand() *cobra.Command {
 
 func runDownloadCommand(cmd *cobra.Command, args []string) {
 	instance := instance.CreateInstaceIgnorePlatformIndexErrors()
-	refs, err := globals.ParseReferenceArgs(args, false)
+	refs, err := globals.ParseLibraryReferenceArgs(args)
 	if err != nil {
 		feedback.Errorf("Invalid argument passed: %v", err)
 		os.Exit(errorcodes.ErrBadArgument)
@@ -56,7 +56,7 @@ func runDownloadCommand(cmd *cobra.Command, args []string) {
 	for _, library := range refs {
 		libraryDownloadReq := &rpc.LibraryDownloadReq{
 			Instance: instance,
-			Name:     library.PackageName,
+			Name:     library.Name,
 			Version:  library.Version,
 		}
 		_, err := lib.LibraryDownload(context.Background(), libraryDownloadReq, output.ProgressBar(),

cli/lib/install.go

@@ -47,7 +47,7 @@ func initInstallCommand() *cobra.Command {
 
 func runInstallCommand(cmd *cobra.Command, args []string) {
 	instance := instance.CreateInstaceIgnorePlatformIndexErrors()
-	refs, err := globals.ParseReferenceArgs(args, false)
+	refs, err := globals.ParseLibraryReferenceArgs(args)
 	if err != nil {
 		feedback.Errorf("Arguments error: %v", err)
 		os.Exit(errorcodes.ErrBadArgument)
@@ -56,7 +56,7 @@ func runInstallCommand(cmd *cobra.Command, args []string) {
 	for _, library := range refs {
 		libraryInstallReq := &rpc.LibraryInstallReq{
 			Instance: instance,
-			Name:     library.PackageName,
+			Name:     library.Name,
 			Version:  library.Version,
 		}
 		err := lib.LibraryInstall(context.Background(), libraryInstallReq, output.ProgressBar(),

cli/lib/uninstall.go

@@ -48,7 +48,7 @@ func runUninstallCommand(cmd *cobra.Command, args []string) {
 	logrus.Info("Executing `arduino lib uninstall`")
 
 	instance := instance.CreateInstaceIgnorePlatformIndexErrors()
-	refs, err := globals.ParseReferenceArgs(args, false)
+	refs, err := globals.ParseLibraryReferenceArgs(args)
 	if err != nil {
 		feedback.Errorf("Invalid argument passed: %v", err)
 		os.Exit(errorcodes.ErrBadArgument)
@@ -57,7 +57,7 @@ func runUninstallCommand(cmd *cobra.Command, args []string) {
 	for _, library := range refs {
 		err := lib.LibraryUninstall(context.Background(), &rpc.LibraryUninstallReq{
 			Instance: instance,
-			Name:     library.PackageName,
+			Name:     library.Name,
 			Version:  library.Version,
 		}, output.TaskProgress())
 		if err != nil {