Skip to content

Commit cd29de0

Browse files
dido18mirkoCrobu
dido18
and
mirkoCrobu
authored
fix(api): allow missing required variable when a brick is added + app validation (#74)
--------- Co-authored-by: mirkoCrobu <[email protected]>
1 parent f761ee6 commit cd29de0

File tree

8 files changed

+389
-38
lines changed

8 files changed

+389
-38
lines changed

‎internal/api/handlers/bricks.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ func HandleBrickCreate(
146146
err = brickService.BrickCreate(req, app)
147147
if err != nil {
148148
// TODO: handle specific errors
149-
slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()))
149+
slog.Error("Unable to create brick", slog.String("error", err.Error()))
150150
render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "error while creating or updating brick"})
151151
return
152152
}
@@ -213,7 +213,7 @@ func HandleBrickUpdates(
213213
req.ID = id
214214
err = brickService.BrickUpdate(req, app)
215215
if err != nil {
216-
slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()))
216+
slog.Error("Unable to update the brick", slog.String("error", err.Error()))
217217
render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to update the brick"})
218218

219219
return

‎internal/orchestrator/app/validator.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package app
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"log/slog"
7+
8+
"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
9+
)
10+
11+
// ValidateBricks checks that all bricks referenced in the given AppDescriptor exist in the provided BricksIndex,
12+
// 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.
13+
func ValidateBricks(a AppDescriptor, index *bricksindex.BricksIndex) error {
14+
if index == nil {
15+
return fmt.Errorf("bricks index cannot be nil")
16+
}
17+
18+
var allErrors error
19+
20+
for _, appBrick := range a.Bricks {
21+
indexBrick, found := index.FindBrickByID(appBrick.ID)
22+
if !found {
23+
allErrors = errors.Join(allErrors, fmt.Errorf("brick %q not found", appBrick.ID))
24+
continue // Skip further validation for this brick since it doesn't exist
25+
}
26+
27+
for appBrickVariableName := range appBrick.Variables {
28+
_, exist := indexBrick.GetVariable(appBrickVariableName)
29+
if !exist {
30+
// TODO: we should return warnings
31+
slog.Warn("[skip] variable does not exist into the brick definition", "variable", appBrickVariableName, "brick", indexBrick.ID)
32+
}
33+
}
34+
35+
// Check that all required brick variables are provided by app
36+
for _, indexBrickVariable := range indexBrick.Variables {
37+
if indexBrickVariable.IsRequired() {
38+
if _, exist := appBrick.Variables[indexBrickVariable.Name]; !exist {
39+
allErrors = errors.Join(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID))
40+
}
41+
}
42+
}
43+
}
44+
return allErrors
45+
}

‎internal/orchestrator/app/validator_test.go‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
package app
2+
3+
import (
4+
"errors"
5+
"os"
6+
"testing"
7+
8+
"github.com/stretchr/testify/assert"
9+
"github.com/stretchr/testify/require"
10+
11+
"github.com/arduino/go-paths-helper"
12+
13+
"github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex"
14+
)
15+
16+
func TestValidateAppDescriptorBricks(t *testing.T) {
17+
bricksIndex := &bricksindex.BricksIndex{
18+
Bricks: []bricksindex.Brick{
19+
{
20+
ID: "arduino:arduino_cloud",
21+
Name: "Arduino Cloud",
22+
Description: "Connects to Arduino Cloud",
23+
Variables: []bricksindex.BrickVariable{
24+
{
25+
Name: "ARDUINO_DEVICE_ID",
26+
Description: "Arduino Cloud Device ID",
27+
DefaultValue: "", // Required (no default value)
28+
},
29+
{
30+
Name: "ARDUINO_SECRET",
31+
Description: "Arduino Cloud Secret",
32+
DefaultValue: "", // Required (no default value)
33+
},
34+
},
35+
},
36+
},
37+
}
38+
39+
testCases := []struct {
40+
name string
41+
yamlContent string
42+
expectedError error
43+
}{
44+
{
45+
name: "valid with all required filled",
46+
yamlContent: `
47+
name: App ok
48+
description: App ok
49+
bricks:
50+
- arduino:arduino_cloud:
51+
variables:
52+
ARDUINO_DEVICE_ID: "my-device-id"
53+
ARDUINO_SECRET: "my-secret"
54+
`,
55+
expectedError: nil,
56+
},
57+
{
58+
name: "valid with missing bricks",
59+
yamlContent: `
60+
name: App with no bricks
61+
description: App with no bricks description
62+
`,
63+
expectedError: nil,
64+
},
65+
{
66+
name: "valid with empty list of bricks",
67+
yamlContent: `
68+
name: App with empty bricks
69+
description: App with empty bricks
70+
71+
bricks: []
72+
`,
73+
expectedError: nil,
74+
},
75+
{
76+
name: "valid if required variable is empty string",
77+
yamlContent: `
78+
name: App with an empty variable
79+
description: App with an empty variable
80+
bricks:
81+
- arduino:arduino_cloud:
82+
variables:
83+
ARDUINO_DEVICE_ID: "my-device-id"
84+
ARDUINO_SECRET:
85+
`,
86+
expectedError: nil,
87+
},
88+
{
89+
name: "invalid if required variable is omitted",
90+
yamlContent: `
91+
name: App with no required variables
92+
description: App with no required variables
93+
bricks:
94+
- arduino:arduino_cloud
95+
`,
96+
expectedError: errors.Join(
97+
errors.New("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\""),
98+
errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
99+
),
100+
},
101+
{
102+
name: "invalid if a required variable among two is omitted",
103+
yamlContent: `
104+
name: App only one required variable filled
105+
description: App only one required variable filled
106+
bricks:
107+
- arduino:arduino_cloud:
108+
variables:
109+
ARDUINO_DEVICE_ID: "my-device-id"
110+
`,
111+
expectedError: errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
112+
},
113+
{
114+
name: "invalid if brick id not found",
115+
yamlContent: `
116+
name: App no existing brick
117+
description: App no existing brick
118+
bricks:
119+
- arduino:not_existing_brick:
120+
variables:
121+
ARDUINO_DEVICE_ID: "my-device-id"
122+
ARDUINO_SECRET: "LAKDJ"
123+
`,
124+
expectedError: errors.New("brick \"arduino:not_existing_brick\" not found"),
125+
},
126+
{
127+
name: "log a warning if variable does not exist in the brick",
128+
yamlContent: `
129+
name: App with non existing variable
130+
description: App with non existing variable
131+
bricks:
132+
- arduino:arduino_cloud:
133+
variables:
134+
NOT_EXISTING_VARIABLE: "this-is-a-not-existing-variable-for-the-brick"
135+
ARDUINO_DEVICE_ID: "my-device-id"
136+
ARDUINO_SECRET: "my-secret"
137+
`,
138+
expectedError: nil,
139+
},
140+
}
141+
142+
for _, tc := range testCases {
143+
t.Run(tc.name, func(t *testing.T) {
144+
tempDir := t.TempDir()
145+
err := paths.New(tempDir).MkdirAll()
146+
require.NoError(t, err)
147+
appYaml := paths.New(tempDir, "app.yaml")
148+
err = os.WriteFile(appYaml.String(), []byte(tc.yamlContent), 0600)
149+
require.NoError(t, err)
150+
151+
appDescriptor, err := ParseDescriptorFile(appYaml)
152+
require.NoError(t, err)
153+
154+
err = ValidateBricks(appDescriptor, bricksIndex)
155+
if tc.expectedError == nil {
156+
assert.NoError(t, err, "Expected no validation errors")
157+
} else {
158+
require.Error(t, err, "Expected validation error")
159+
assert.Equal(t, tc.expectedError.Error(), err.Error(), "Error message should match")
160+
}
161+
})
162+
}
163+
}

‎internal/orchestrator/bricks/bricks.go‎

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -282,15 +282,15 @@ func (s *Service) BrickCreate(
282282
if !exist {
283283
return fmt.Errorf("variable %q does not exist on brick %q", name, brick.ID)
284284
}
285-
if value.DefaultValue == "" && reqValue == "" {
286-
return fmt.Errorf("variable %q cannot be empty", name)
285+
if value.IsRequired() && reqValue == "" {
286+
return fmt.Errorf("required variable %q cannot be empty", name)
287287
}
288288
}
289289

290290
for _, brickVar := range brick.Variables {
291-
if brickVar.DefaultValue == "" {
291+
if brickVar.IsRequired() {
292292
if _, exist := req.Variables[brickVar.Name]; !exist {
293-
return fmt.Errorf("required variable %q is mandatory", brickVar.Name)
293+
slog.Warn("[Skip] a required variable is not set by user", "variable", brickVar.Name, "brick", brickVar.Name)
294294
}
295295
}
296296
}
@@ -335,16 +335,21 @@ func (s *Service) BrickUpdate(
335335
req BrickCreateUpdateRequest,
336336
appCurrent app.ArduinoApp,
337337
) error {
338-
index := slices.IndexFunc(appCurrent.Descriptor.Bricks, func(b app.Brick) bool { return b.ID == req.ID })
339-
if index == -1 {
340-
return fmt.Errorf("brick not found with id %s", req.ID)
338+
brickFromIndex, present := s.bricksIndex.FindBrickByID(req.ID)
339+
if !present {
340+
return fmt.Errorf("brick %q not found into the brick index", req.ID)
341+
}
342+
343+
brickPosition := slices.IndexFunc(appCurrent.Descriptor.Bricks, func(b app.Brick) bool { return b.ID == req.ID })
344+
if brickPosition == -1 {
345+
return fmt.Errorf("brick %q not found into the bricks of the app", req.ID)
341346
}
342-
brickID := appCurrent.Descriptor.Bricks[index].ID
343-
brickVariables := appCurrent.Descriptor.Bricks[index].Variables
347+
348+
brickVariables := appCurrent.Descriptor.Bricks[brickPosition].Variables
344349
if len(brickVariables) == 0 {
345350
brickVariables = make(map[string]string)
346351
}
347-
brickModel := appCurrent.Descriptor.Bricks[index].Model
352+
brickModel := appCurrent.Descriptor.Bricks[brickPosition].Model
348353

349354
if req.Model != nil && *req.Model != brickModel {
350355
models := s.modelsIndex.GetModelsByBrick(req.ID)
@@ -354,17 +359,14 @@ func (s *Service) BrickUpdate(
354359
}
355360
brickModel = *req.Model
356361
}
357-
brick, present := s.bricksIndex.FindBrickByID(brickID)
358-
if !present {
359-
return fmt.Errorf("brick not found with id %s", brickID)
360-
}
362+
361363
for name, updateValue := range req.Variables {
362-
value, exist := brick.GetVariable(name)
364+
value, exist := brickFromIndex.GetVariable(name)
363365
if !exist {
364-
return errors.New("variable does not exist")
366+
return fmt.Errorf("variable %q does not exist on brick %q", name, brickFromIndex.ID)
365367
}
366-
if value.DefaultValue == "" && updateValue == "" {
367-
return errors.New("variable default value cannot be empty")
368+
if value.IsRequired() && updateValue == "" {
369+
return fmt.Errorf("required variable %q cannot be empty", name)
368370
}
369371
updated := false
370372
for _, v := range brickVariables {
@@ -374,14 +376,13 @@ func (s *Service) BrickUpdate(
374376
break
375377
}
376378
}
377-
378379
if !updated {
379380
brickVariables[name] = updateValue
380381
}
381382
}
382383

383-
appCurrent.Descriptor.Bricks[index].Model = brickModel
384-
appCurrent.Descriptor.Bricks[index].Variables = brickVariables
384+
appCurrent.Descriptor.Bricks[brickPosition].Model = brickModel
385+
appCurrent.Descriptor.Bricks[brickPosition].Variables = brickVariables
385386

386387
err := appCurrent.Save()
387388
if err != nil {

0 commit comments

Comments
 (0)