fix(api): allow missing required variable when a brick is added + app validation

internal/api/handlers/bricks.go

@@ -146,7 +146,7 @@ func HandleBrickCreate(
 		err = brickService.BrickCreate(req, app)
 		if err != nil {
 			// TODO: handle specific errors
-			slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()))
+			slog.Error("Unable to create brick", slog.String("error", err.Error()))
 			render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "error while creating or updating brick"})
 			return
 		}
@@ -213,7 +213,7 @@ func HandleBrickUpdates(
 		req.ID = id
 		err = brickService.BrickUpdate(req, app)
 		if err != nil {
-			slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()))
+			slog.Error("Unable to update the brick", slog.String("error", err.Error()))
 			render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to update the brick"})
 
 			return

internal/orchestrator/app/validator.go

@@ -0,0 +1,45 @@
+package app
+
+import (
+	"errors"
+	"fmt"
+	"log/slog"
+
+	"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
+)
+
+// ValidateBricks checks that all bricks referenced in the given AppDescriptor exist in the provided BricksIndex,
+// It collects and returns all validation errors as a single joined error, allowing the caller to see all issues at once rather than stopping at the first error.
+func ValidateBricks(a AppDescriptor, index *bricksindex.BricksIndex) error {
+	if index == nil {
+		return fmt.Errorf("bricks index cannot be nil")
+	}
+
+	var allErrors error
+
+	for _, appBrick := range a.Bricks {
+		indexBrick, found := index.FindBrickByID(appBrick.ID)
+		if !found {
+			allErrors = errors.Join(allErrors, fmt.Errorf("brick %q not found", appBrick.ID))
+			continue // Skip further validation for this brick since it doesn't exist
+		}
+
+		for appBrickVariableName := range appBrick.Variables {
+			_, exist := indexBrick.GetVariable(appBrickVariableName)
+			if !exist {
+				// TODO: we should return warnings
+				slog.Warn("[skip] variable does not exist into the brick definition", "variable", appBrickVariableName, "brick", indexBrick.ID)
+			}
+		}
+
+		// Check that all required brick variables are provided by app
+		for _, indexBrickVariable := range indexBrick.Variables {
+			if indexBrickVariable.IsRequired() {
+				if _, exist := appBrick.Variables[indexBrickVariable.Name]; !exist {
+					allErrors = errors.Join(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID))
+				}
+			}
+		}
+	}
+	return allErrors
+}

internal/orchestrator/app/validator_test.go

@@ -0,0 +1,163 @@
+package app
+
+import (
+	"errors"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/arduino/go-paths-helper"
+
+	"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
+)
+
+func TestValidateAppDescriptorBricks(t *testing.T) {
+	bricksIndex := &bricksindex.BricksIndex{
+		Bricks: []bricksindex.Brick{
+			{
+				ID:          "arduino:arduino_cloud",
+				Name:        "Arduino Cloud",
+				Description: "Connects to Arduino Cloud",
+				Variables: []bricksindex.BrickVariable{
+					{
+						Name:         "ARDUINO_DEVICE_ID",
+						Description:  "Arduino Cloud Device ID",
+						DefaultValue: "", // Required (no default value)
+					},
+					{
+						Name:         "ARDUINO_SECRET",
+						Description:  "Arduino Cloud Secret",
+						DefaultValue: "", // Required (no default value)
+					},
+				},
+			},
+		},
+	}
+
+	testCases := []struct {
+		name          string
+		yamlContent   string
+		expectedError error
+	}{
+		{
+			name: "valid with all required filled",
+			yamlContent: `
+name: App ok
+description: App ok
+bricks:
+  - arduino:arduino_cloud:
+      variables:
+        ARDUINO_DEVICE_ID: "my-device-id"
+        ARDUINO_SECRET: "my-secret"
+`,
+			expectedError: nil,
+		},
+		{
+			name: "valid with missing bricks",
+			yamlContent: `
+name: App with no bricks
+description: App with no bricks description
+`,
+			expectedError: nil,
+		},
+		{
+			name: "valid with empty list of bricks",
+			yamlContent: `
+name: App with empty bricks
+description: App with empty bricks
+
+bricks: []
+`,
+			expectedError: nil,
+		},
+		{
+			name: "valid if required variable is empty string",
+			yamlContent: `
+name: App with an empty variable
+description: App with an empty variable
+bricks:
+  - arduino:arduino_cloud:
+      variables:
+        ARDUINO_DEVICE_ID: "my-device-id"
+        ARDUINO_SECRET:
+`,
+			expectedError: nil,
+		},
+		{
+			name: "invalid if required variable is omitted",
+			yamlContent: `
+name: App with no required variables
+description: App with no required variables
+bricks:
+  - arduino:arduino_cloud
+`,
+			expectedError: errors.Join(
+				errors.New("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\""),
+				errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
+			),
+		},
+		{
+			name: "invalid if a required variable among two is omitted",
+			yamlContent: `
+name: App only one required variable filled
+description: App only one required variable filled
+bricks:
+  - arduino:arduino_cloud:
+      variables:
+        ARDUINO_DEVICE_ID: "my-device-id"
+`,
+			expectedError: errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
+		},
+		{
+			name: "invalid if brick id not found",
+			yamlContent: `
+name: App no existing brick
+description: App no existing brick
+bricks:
+  - arduino:not_existing_brick:
+      variables:
+        ARDUINO_DEVICE_ID: "my-device-id"
+        ARDUINO_SECRET: "LAKDJ"
+`,
+			expectedError: errors.New("brick \"arduino:not_existing_brick\" not found"),
+		},
+		{
+			name: "log a warning if variable does not exist in the brick",
+			yamlContent: `
+name: App with non existing variable
+description: App with non existing variable
+bricks:
+  - arduino:arduino_cloud:
+      variables:
+        NOT_EXISTING_VARIABLE: "this-is-a-not-existing-variable-for-the-brick"
+        ARDUINO_DEVICE_ID: "my-device-id"
+        ARDUINO_SECRET: "my-secret"
+`,
+			expectedError: nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			tempDir := t.TempDir()
+			err := paths.New(tempDir).MkdirAll()
+			require.NoError(t, err)
+			appYaml := paths.New(tempDir, "app.yaml")
+			err = os.WriteFile(appYaml.String(), []byte(tc.yamlContent), 0600)
+			require.NoError(t, err)
+
+			appDescriptor, err := ParseDescriptorFile(appYaml)
+			require.NoError(t, err)
+
+			err = ValidateBricks(appDescriptor, bricksIndex)
+			if tc.expectedError == nil {
+				assert.NoError(t, err, "Expected no validation errors")
+			} else {
+				require.Error(t, err, "Expected validation error")
+				assert.Equal(t, tc.expectedError.Error(), err.Error(), "Error message should match")
+			}
+		})
+	}
+}

internal/orchestrator/bricks/bricks.go

@@ -282,15 +282,15 @@ func (s *Service) BrickCreate(
 		if !exist {
 			return fmt.Errorf("variable %q does not exist on brick %q", name, brick.ID)
 		}
-		if value.DefaultValue == "" && reqValue == "" {
-			return fmt.Errorf("variable %q cannot be empty", name)
+		if value.IsRequired() && reqValue == "" {
+			return fmt.Errorf("required variable %q cannot be empty", name)
 		}
 	}
 
 	for _, brickVar := range brick.Variables {
-		if brickVar.DefaultValue == "" {
+		if brickVar.IsRequired() {
 			if _, exist := req.Variables[brickVar.Name]; !exist {
-				return fmt.Errorf("required variable %q is mandatory", brickVar.Name)
+				slog.Warn("[Skip] a required variable is not set by user", "variable", brickVar.Name, "brick", brickVar.Name)
 			}
 		}
 	}
@@ -335,16 +335,21 @@ func (s *Service) BrickUpdate(
 	req BrickCreateUpdateRequest,
 	appCurrent app.ArduinoApp,
 ) error {
-	index := slices.IndexFunc(appCurrent.Descriptor.Bricks, func(b app.Brick) bool { return b.ID == req.ID })
-	if index == -1 {
-		return fmt.Errorf("brick not found with id %s", req.ID)
+	brickFromIndex, present := s.bricksIndex.FindBrickByID(req.ID)
+	if !present {
+		return fmt.Errorf("brick %q not found into the brick index", req.ID)
+	}
+
+	brickPosition := slices.IndexFunc(appCurrent.Descriptor.Bricks, func(b app.Brick) bool { return b.ID == req.ID })
+	if brickPosition == -1 {
+		return fmt.Errorf("brick %q not found into the bricks of the app", req.ID)
 	}
-	brickID := appCurrent.Descriptor.Bricks[index].ID
-	brickVariables := appCurrent.Descriptor.Bricks[index].Variables
+
+	brickVariables := appCurrent.Descriptor.Bricks[brickPosition].Variables
 	if len(brickVariables) == 0 {
 		brickVariables = make(map[string]string)
 	}
-	brickModel := appCurrent.Descriptor.Bricks[index].Model
+	brickModel := appCurrent.Descriptor.Bricks[brickPosition].Model
 
 	if req.Model != nil && *req.Model != brickModel {
 		models := s.modelsIndex.GetModelsByBrick(req.ID)
@@ -354,17 +359,14 @@ func (s *Service) BrickUpdate(
 		}
 		brickModel = *req.Model
 	}
-	brick, present := s.bricksIndex.FindBrickByID(brickID)
-	if !present {
-		return fmt.Errorf("brick not found with id %s", brickID)
-	}
+
 	for name, updateValue := range req.Variables {
-		value, exist := brick.GetVariable(name)
+		value, exist := brickFromIndex.GetVariable(name)
 		if !exist {
-			return errors.New("variable does not exist")
+			return fmt.Errorf("variable %q does not exist on brick %q", name, brickFromIndex.ID)
 		}
-		if value.DefaultValue == "" && updateValue == "" {
-			return errors.New("variable default value cannot be empty")
+		if value.IsRequired() && updateValue == "" {
+			return fmt.Errorf("required variable %q cannot be empty", name)
 		}
 		updated := false
 		for _, v := range brickVariables {
@@ -374,14 +376,22 @@ func (s *Service) BrickUpdate(
 				break
 			}
 		}
-
 		if !updated {
 			brickVariables[name] = updateValue
 		}
 	}
 
-	appCurrent.Descriptor.Bricks[index].Model = brickModel
-	appCurrent.Descriptor.Bricks[index].Variables = brickVariables
+	appCurrent.Descriptor.Bricks[brickPosition].Model = brickModel
+	appCurrent.Descriptor.Bricks[brickPosition].Variables = brickVariables
+
+	for _, brickVar := range brickFromIndex.Variables {
+		if brickVar.IsRequired() {
+			newVariables := appCurrent.Descriptor.Bricks[brickPosition].Variables
+			if _, exist := newVariables[brickVar.Name]; !exist {
+				return fmt.Errorf("required variable %q must be set", brickVar.Name)
+			}
+		}
+	}
 
 	err := appCurrent.Save()
 	if err != nil {