refactor: consolidate brick validation into AppDescriptor and update related tests

Author: dido18

internal/api/handlers/app_start.go

@@ -47,7 +47,7 @@ func HandleAppStart(
 			return
 		}
 
-		appLoaded, err := app.Load(id.ToPath().String())
+		app, err := app.Load(id.ToPath().String())
 		if err != nil {
 			slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()), slog.String("path", id.String()))
 			render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to find the app"})
@@ -62,23 +62,14 @@ func HandleAppStart(
 		}
 		defer sseStream.Close()
 
-		err = app.ValidateBricks(appLoaded.Descriptor, bricksIndex)
-		if err != nil {
-			sseStream.SendError(render.SSEErrorData{
-				Code:    render.BadRequestErr,
-				Message: err.Error(),
-			})
-			return
-		}
-
 		type progress struct {
 			Name     string  `json:"name"`
 			Progress float32 `json:"progress"`
 		}
 		type log struct {
 			Message string `json:"message"`
 		}
-		for item := range orchestrator.StartApp(r.Context(), dockerCli, provisioner, modelsIndex, bricksIndex, appLoaded, cfg, staticStore) {
+		for item := range orchestrator.StartApp(r.Context(), dockerCli, provisioner, modelsIndex, bricksIndex, app, cfg, staticStore) {
 			switch item.GetType() {
 			case orchestrator.ProgressType:
 				sseStream.Send(render.SSEEvent{Type: "progress", Data: progress(*item.GetProgress())})

internal/orchestrator/app/parser.go

@@ -21,6 +21,7 @@ import (
 	"io"
 
 	emoji "github.com/Andrew-M-C/go.emoji"
+	"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
 	"github.com/arduino/go-paths-helper"
 	"github.com/goccy/go-yaml"
 	"github.com/goccy/go-yaml/ast"
@@ -138,6 +139,38 @@ func (a *AppDescriptor) IsValid() error {
 	return allErrors
 }
 
+// ValidateBricks checks that all bricks referenced in the given AppDescriptor exist in the provided BricksIndex,
+// and that all required variables for each brick are present and valid. It returns an error if any brick is missing,
+// if any variable referenced by the app does not exist in the corresponding brick, or if any required variable is missing.
+// If the index is nil, validation is skipped and nil is returned.
+func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) error {
+	if index == nil {
+		return nil
+	}
+	for _, appBrick := range a.Bricks {
+		indexBrick, found := index.FindBrickByID(appBrick.ID)
+		if !found {
+			return fmt.Errorf("brick %q not found", appBrick.ID)
+		}
+
+		for appBrickName := range appBrick.Variables {
+			_, exist := indexBrick.GetVariable(appBrickName)
+			if !exist {
+				return fmt.Errorf("variable %q does not exist on brick %q", appBrickName, indexBrick.ID)
+			}
+		}
+
+		for _, indexBrickVariable := range indexBrick.Variables {
+			if indexBrickVariable.IsRequired() {
+				if _, exist := appBrick.Variables[indexBrickVariable.Name]; !exist {
+					return fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID)
+				}
+			}
+		}
+	}
+	return nil
+}
+
 func isSingleEmoji(s string) bool {
 	emojis := 0
 	for it := emoji.IterateChars(s); it.Next(); {

internal/orchestrator/app/validator.go

@@ -12,7 +12,7 @@ import (
 	"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
 )
 
-func TestValidateBricksOnAppDescriptor(t *testing.T) {
+func TestValidateAppDescriptorBricks(t *testing.T) {
 	bricksIndex, err := bricksindex.GenerateBricksIndexFromFile(paths.New("testdata/validator"))
 	require.Nil(t, err)
 
@@ -50,11 +50,10 @@ func TestValidateBricksOnAppDescriptor(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			app, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename))
+			appDescriptor, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename))
 			require.NoError(t, err)
 
-			err = ValidateBricks(app, bricksIndex)
-
+			appDescriptor.ValidateBricks(bricksIndex)
 			if tc.expectedErr == nil {
 				assert.NoError(t, err)
 			} else {

internal/orchestrator/app/validator_test.go

@@ -122,6 +122,12 @@ func StartApp(
 		ctx, cancel := context.WithCancel(ctx)
 		defer cancel()
 
+		err := app.Descriptor.ValidateBricks(bricksIndex)
+		if err != nil {
+			yield(StreamMessage{error: err})
+			return
+		}
+
 		running, err := getRunningApp(ctx, docker.Client())
 		if err != nil {
 			yield(StreamMessage{error: err})
@@ -446,6 +452,13 @@ func RestartApp(
 	return func(yield func(StreamMessage) bool) {
 		ctx, cancel := context.WithCancel(ctx)
 		defer cancel()
+
+		err := appToStart.Descriptor.ValidateBricks(bricksIndex)
+		if err != nil {
+			yield(StreamMessage{error: err})
+			return
+		}
+
 		runningApp, err := getRunningApp(ctx, docker.Client())
 		if err != nil {
 			yield(StreamMessage{error: err})

internal/orchestrator/orchestrator.go

@@ -30,7 +30,6 @@ type SSEErrCode string
 
 const (
 	InternalServiceErr SSEErrCode = "INTERNAL_SERVER_ERROR"
-	BadRequestErr      SSEErrCode = "BAD_REQUEST_ERROR"
 )
 
 type SSEErrorData struct {