Unify App validation across the codebase

internal/orchestrator/app/app.go

@@ -65,51 +65,88 @@ func Load(appPath *paths.Path) (ArduinoApp, error) {
 		return ArduinoApp{}, fmt.Errorf("cannot get absolute path for app: %w", err)
 	}
 
+	if err := ValidateApp(appPath); err != nil {
+		return ArduinoApp{}, err
+	}
+
 	app := ArduinoApp{
 		FullPath:   appPath,
 		Descriptor: AppDescriptor{},
 	}
 
-	if descriptorFile := app.GetDescriptorPath(); descriptorFile.Exist() {
-		desc, err := ParseDescriptorFile(descriptorFile)
+	desc, err := ParseDescriptorFile(app.GetDescriptorPath())
+	if err != nil {
+		return ArduinoApp{}, err
+	}
+	app.Descriptor = desc
+	app.Name = desc.Name
+
+	if app.Descriptor.Description == "" {
+		description, err := app.getAppDescriptionFromReadme()
 		if err != nil {
-			return ArduinoApp{}, fmt.Errorf("error loading app descriptor file: %w", err)
-		}
-		app.Descriptor = desc
-		app.Name = desc.Name
-
-		if app.Descriptor.Description == "" {
-			description, err := app.getAppDescriptionFromReadme()
-			if err != nil {
-				// Log the error but don't fail the loading process, as the description is optional
-				slog.Warn("cannot extract app description from README.md", "error", err)
-			} else {
-				app.Descriptor.Description = description
-			}
+			slog.Warn("cannot extract app description from README.md", "error", err)
+		} else {
+			app.Descriptor.Description = description
 		}
+	}
 
-	} else {
-		return ArduinoApp{}, errors.New("descriptor app.yaml file missing from app")
+	app.MainPythonFile = appPath.Join("python", "main.py")
+
+	sketchPath := appPath.Join("sketch")
+	if sketchPath.IsDir() {
+		app.mainSketchPath = sketchPath
 	}
 
-	if appPath.Join("python", "main.py").Exist() {
-		app.MainPythonFile = appPath.Join("python", "main.py")
+	if appPath.Join("bricks").Exist() {
+		app.LocalBricks = loadBricksFromFolder(appPath.Join("bricks"))
 	}
 
-	if appPath.Join("sketch", "sketch.ino").Exist() {
-		// TODO: check sketch casing?
-		app.mainSketchPath = appPath.Join("sketch")
+	return app, nil
+}
+
+func ValidateApp(appPath *paths.Path) error {
+	descriptorFile := appPath.Join("app.yaml")
+	if _, err := validateAndParseDescriptor(descriptorFile); err != nil {
+		return err
 	}
 
-	if app.MainPythonFile == nil && app.mainSketchPath == nil {
-		return ArduinoApp{}, errors.New("main python file and sketch file missing from app")
+	sketchPath := appPath.Join("sketch")
+	if err := isValidSketchFolder(sketchPath); err != nil {
+		return err
 	}
 
-	if appPath.Join("bricks").Exist() {
-		app.LocalBricks = loadBricksFromFolder(appPath.Join("bricks"))
+	if !appPath.Join("python", "main.py").Exist() {
+		return errors.New("main python file missing from app")
 	}
 
-	return app, nil
+	return nil
+}
+
+func validateAndParseDescriptor(descriptorFile *paths.Path) (AppDescriptor, error) {
+	if !descriptorFile.Exist() {
+		return AppDescriptor{}, errors.New("descriptor app.yaml file missing from app")
+	}
+	appDescriptor, err := ParseDescriptorFile(descriptorFile)
+	if err != nil {
+		return AppDescriptor{}, fmt.Errorf("error loading app descriptor file: %w", err)
+	}
+	return appDescriptor, nil
+}
+
+func isValidSketchFolder(sketchDir *paths.Path) error {
+	if sketchDir == nil {
+		return nil
+	}
+
+	sketchIno := sketchDir.Join("sketch.ino")
+	sketchYaml := sketchDir.Join("sketch.yaml")
+
+	if sketchIno.Exist() || sketchYaml.Exist() {
+		if !sketchIno.Exist() || !sketchYaml.Exist() {
+			return errors.New("sketch folder is incomplete: both sketch.ino and sketch.yaml are required")
+		}
+	}
+	return nil
 }
 
 func (a *ArduinoApp) GetSketchPath() (*paths.Path, bool) {
@@ -119,15 +156,9 @@ func (a *ArduinoApp) GetSketchPath() (*paths.Path, bool) {
 	return a.mainSketchPath, true
 }
 
-// GetDescriptorPath returns the path to the app descriptor file (app.yaml or app.yml)
+// GetDescriptorPath returns the path to the app descriptor file (app.yaml)
 func (a *ArduinoApp) GetDescriptorPath() *paths.Path {
 	descriptorFile := a.FullPath.Join("app.yaml")
-	if !descriptorFile.Exist() {
-		alternateDescriptorFile := a.FullPath.Join("app.yml")
-		if alternateDescriptorFile.Exist() {
-			return alternateDescriptorFile
-		}
-	}
 	return descriptorFile
 }
 

internal/orchestrator/app/app_test.go

@@ -122,12 +122,12 @@ func TestMissingDescriptor(t *testing.T) {
 }
 
 func TestMissingMains(t *testing.T) {
-	appFolderPath := paths.New("testdata", "MissingMains")
+	appFolderPath := paths.New("testdata", "MissingMain")
 
 	// Load app
 	app, err := Load(appFolderPath)
 	assert.Error(t, err)
-	assert.ErrorContains(t, err, "main python file and sketch file missing from app")
+	assert.ErrorContains(t, err, "main python file missing from app")
 	assert.Empty(t, app)
 }
 

internal/orchestrator/app/find.go

@@ -42,9 +42,9 @@ func FindAppsInFolder(pathToExplore *paths.Path) (paths.PathList, error) {
 const tmpAppPrefix = ".tmp_"
 
 // DirHasAppDescriptor returns true if the given directory contains
-// an app descriptor file (app.yaml or app.yml).
+// an app descriptor file (app.yaml).
 func DirHasAppDescriptor(p *paths.Path) bool {
-	return p.Join("app.yaml").Exist() || p.Join("app.yml").Exist()
+	return p.Join("app.yaml").Exist()
 }
 
 // IsTmpAppDir returns true if the app path is a temporary app

internal/orchestrator/app/parser.go

@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"regexp"
 
 	emoji "github.com/Andrew-M-C/go.emoji"
 	"github.com/arduino/go-paths-helper"
@@ -126,10 +127,6 @@ func ParseDescriptorFile(file *paths.Path) (AppDescriptor, error) {
 		return AppDescriptor{}, fmt.Errorf("cannot decode descriptor: %w", err)
 	}
 
-	if descriptor.Name == "" {
-		return AppDescriptor{}, fmt.Errorf("application name is empty")
-	}
-
 	return descriptor, descriptor.IsValid()
 }
 
@@ -140,6 +137,12 @@ func (a *AppDescriptor) IsValid() error {
 			allErrors = errors.Join(allErrors, fmt.Errorf("icon %q is not a valid single emoji", a.Icon))
 		}
 	}
+	if a.Name != "" {
+		if !isValidName(a.Name) {
+			allErrors = errors.Join(allErrors, fmt.Errorf("name %q is not valid", a.Name))
+		}
+	}
+
 	return allErrors
 }
 
@@ -157,3 +160,8 @@ func isSingleEmoji(s string) bool {
 	}
 	return emojis == 1
 }
+
+func isValidName(s string) bool {
+	matched, _ := regexp.MatchString(`^[a-zA-Z0-9][a-zA-Z0-9_ -]*$`, s)
+	return matched
+}

internal/orchestrator/app/parser_test.go

@@ -144,3 +144,32 @@ bricks:
 	require.Equal(t, 1, len(desc.Bricks))
 	require.Equal(t, []string{"my-dev-1", "my-dev-2"}, desc.Bricks[0].Devices)
 }
+
+func TestIsValidName(t *testing.T) {
+	tests := []struct {
+		input    string
+		expected bool
+	}{
+		{"ValidName", true},
+		{"valid_name", true},
+		{"valid-name", true},
+		{"valid name", true},
+		{"Valid Name With Spaces", true},
+		{"a", true},
+		{"A1", true},
+		{"Test App", true},
+		{"Image detection with UI", true},
+		{"-invalid", false},
+		{"", false},
+		{"_invalid", false},
+		{"name!", false},
+		{"name@invalid", false},
+	}
+
+	for _, test := range tests {
+		t.Run(test.input, func(t *testing.T) {
+			result := isValidName(test.input)
+			require.Equal(t, test.expected, result, "Input: %s", test.input)
+		})
+	}
+}

internal/orchestrator/archive.go

@@ -112,7 +112,7 @@ func zipAppToBuffer(bricksIndex *bricksindex.BricksIndex, sourcePath string, roo
 			return nil
 		}
 
-		if d.Name() == "app.yaml" || d.Name() == "app.yml" { // nolint:goconst
+		if d.Name() == "app.yaml" { // nolint:goconst
 			desc, err := app.ParseDescriptorFile(paths.New(path))
 			if err != nil {
 				return err
@@ -169,10 +169,6 @@ func ImportAppFromZip(
 		rawAppName = strings.TrimSuffix(originalZipName, filepath.Ext(originalZipName))
 	}
 
-	if err := validateAppZipContent(&r.Reader, rootPrefix); err != nil {
-		return app.ID{}, fmt.Errorf("%w:%v", ErrBadRequest, err)
-	}
-
 	appDescriptor, err := readAppDescriptorFromZip(&r.Reader, rootPrefix)
 	if err != nil {
 		return app.ID{}, fmt.Errorf("failed to read app.yaml: %w", err)
@@ -202,6 +198,9 @@ func ImportAppFromZip(
 	if finalDestPath.Exist() {
 		return app.ID{}, ErrAppAlreadyExists
 	}
+	if err := app.ValidateApp(tempDestDir); err != nil {
+		return app.ID{}, fmt.Errorf("%w: %v", ErrBadRequest, err)
+	}
 
 	if err := tempDestDir.Rename(finalDestPath); err != nil {
 		return app.ID{}, fmt.Errorf("failed to finalize app import (swap): %w", err)
@@ -287,12 +286,11 @@ func readAppDescriptorFromZip(r *zip.Reader, rootPrefix string) (app.AppDescript
 	var descriptor app.AppDescriptor
 
 	targetAppYaml := paths.New(rootPrefix, "app.yaml")
-	targetAppYml := paths.New(rootPrefix, "app.yml")
 
 	for _, f := range r.File {
 		name := filepath.ToSlash(f.Name)
 
-		if name == targetAppYaml.String() || name == targetAppYml.String() {
+		if name == targetAppYaml.String() {
 			rc, err := f.Open()
 			if err != nil {
 				return descriptor, err
@@ -311,62 +309,6 @@ func readAppDescriptorFromZip(r *zip.Reader, rootPrefix string) (app.AppDescript
 	return descriptor, fmt.Errorf("app.yaml not found in archive")
 }
 
-// TODO implement centralized app validator to use everywhere is needed
-// validateAppZipContent checks for mandatory files respecting the rootPrefix
-func validateAppZipContent(r *zip.Reader, rootPrefix string) error {
-	hasAppYaml := false
-	hasMainPy := false
-
-	hasSketchFolder := false
-	hasSketchIno := false
-	hasSketchYaml := false
-
-	targetAppYaml := paths.New(rootPrefix, "app.yaml")
-	targetAppYml := paths.New(rootPrefix, "app.yml")
-	targetMainPy := paths.New(rootPrefix, "python/main.py")
-
-	targetSketchPrefix := paths.New(rootPrefix, "sketch").String() + "/"
-	for _, f := range r.File {
-		name := filepath.ToSlash(f.Name)
-
-		if name == targetAppYaml.String() || name == targetAppYml.String() {
-			hasAppYaml = true
-		}
-		if name == targetMainPy.String() {
-			hasMainPy = true
-		}
-
-		if strings.HasPrefix(name, targetSketchPrefix) {
-			hasSketchFolder = true
-			if name == paths.New(rootPrefix, "sketch/sketch.ino").String() {
-				hasSketchIno = true
-			}
-
-			if name == paths.New(rootPrefix, "sketch/sketch.yaml").String() {
-				hasSketchYaml = true
-			}
-		}
-	}
-
-	if !hasAppYaml {
-		return errors.New("missing app.yaml")
-	}
-	if !hasMainPy {
-		return errors.New("missing python/main.py")
-	}
-
-	if hasSketchFolder {
-		if !hasSketchIno {
-			return errors.New("sketch folder present but missing .ino file")
-		}
-		if !hasSketchYaml {
-			return errors.New("sketch folder present but missing .yaml file")
-		}
-	}
-
-	return nil
-}
-
 func redactSecrets(bricksindex *bricksindex.BricksIndex, desc *app.AppDescriptor) {
 	for i := range desc.Bricks {
 		brick := &desc.Bricks[i]
@@ -392,7 +334,7 @@ func redactSecrets(bricksindex *bricksindex.BricksIndex, desc *app.AppDescriptor
 func findZipRoot(r *zip.Reader) (string, error) {
 	for _, f := range r.File {
 		name := filepath.ToSlash(f.Name)
-		if filepath.Base(name) != "app.yaml" && filepath.Base(name) != "app.yml" {
+		if filepath.Base(name) != "app.yaml" {
 			continue
 		}
 		slashCount := strings.Count(name, "/")