Skip to content

Commit 9eeb9f2

Browse files
mirkoCrobumirkoCrobu
mirkoCrobu
and
mirkoCrobu
authored
Inconsistent Validation for icon Field in App Creation/Retrieval
Co-authored-by: mirkoCrobu <mirkocrobu@NB-0531.localdomain>
1 parent f7ccc7c commit 9eeb9f2

File tree

8 files changed

+154
-32
lines changed

8 files changed

+154
-32
lines changed

‎internal/api/handlers/app_clone.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ func HandleAppClone(
6666
render.EncodeResponse(w, http.StatusNotFound, models.ErrorResponse{Details: "app not found"})
6767
return
6868
}
69-
if errors.Is(err, orchestrator.ErrInvalidApp) {
69+
if errors.Is(err, app.ErrInvalidApp) {
7070
slog.Error("missing app.yaml", slog.String("error", err.Error()))
7171
render.EncodeResponse(w, http.StatusBadRequest, models.ErrorResponse{Details: "missing app.yaml"})
7272
return

‎internal/api/handlers/app_create.go‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,18 @@ func HandleAppCreate(
6060
cfg,
6161
)
6262
if err != nil {
63-
if errors.Is(err, orchestrator.ErrAppAlreadyExists) {
63+
switch {
64+
case errors.Is(err, orchestrator.ErrAppAlreadyExists):
6465
slog.Error("app already exists", slog.String("error", err.Error()))
6566
render.EncodeResponse(w, http.StatusConflict, models.ErrorResponse{Details: "app already exists"})
66-
return
67+
68+
case errors.Is(err, app.ErrInvalidApp):
69+
slog.Error("invalid app data", slog.String("error", err.Error()))
70+
render.EncodeResponse(w, http.StatusBadRequest, models.ErrorResponse{Details: err.Error()})
71+
default:
72+
slog.Error("unable to create app", slog.String("error", err.Error()))
73+
render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "an unexpected error occurred"})
6774
}
68-
slog.Error("unable to create app", slog.String("error", err.Error()))
69-
render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to create"})
7075
return
7176
}
7277
render.EncodeResponse(w, http.StatusCreated, resp)

‎internal/api/handlers/app_details.go‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package handlers
22

33
import (
44
"encoding/json"
5+
"errors"
6+
"fmt"
57
"log/slog"
68
"net/http"
79

@@ -64,7 +66,7 @@ func HandleAppDetailsEdits(
6466
render.EncodeResponse(w, http.StatusPreconditionFailed, models.ErrorResponse{Details: "invalid id"})
6567
return
6668
}
67-
app, err := app.Load(id.ToPath().String())
69+
appToEdit, err := app.Load(id.ToPath().String())
6870
if err != nil {
6971
slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()), slog.String("path", id.String()))
7072
render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to find the app"})
@@ -95,14 +97,25 @@ func HandleAppDetailsEdits(
9597
Description: editRequest.Description,
9698
}
9799
}
98-
err = orchestrator.EditApp(appEditRequest, &app, cfg)
100+
err = orchestrator.EditApp(appEditRequest, &appToEdit, cfg)
99101
if err != nil {
100-
slog.Error("Unable to edit the app", slog.String("error", err.Error()))
101-
render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to edit the app"})
102+
switch {
103+
case errors.Is(err, app.ErrInvalidApp):
104+
slog.Error("Unable to edit the app 1", slog.String("error", err.Error()))
105+
render.EncodeResponse(w, http.StatusBadRequest, models.ErrorResponse{Details: err.Error()})
106+
case errors.Is(err, orchestrator.ErrAppAlreadyExists):
107+
slog.Error("The name is already in use.", slog.String("error", err.Error()))
108+
render.EncodeResponse(w, http.StatusBadRequest, models.ErrorResponse{
109+
Details: fmt.Sprintf("the name %q is already in use", *editRequest.Name),
110+
})
111+
default:
112+
slog.Error("Unable to edit the app ", slog.String("error", err.Error()))
113+
render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to edit the app"})
114+
}
102115
return
103116
}
104117

105-
res, err := orchestrator.AppDetails(r.Context(), dockerClient, app, bricksIndex, idProvider, cfg)
118+
res, err := orchestrator.AppDetails(r.Context(), dockerClient, appToEdit, bricksIndex, idProvider, cfg)
106119
if err != nil {
107120
slog.Error("Unable to parse the app.yaml", slog.String("error", err.Error()))
108121
render.EncodeResponse(w, http.StatusInternalServerError, models.ErrorResponse{Details: "unable to find the app"})

‎internal/e2e/daemon/app_test.go‎

Lines changed: 89 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,34 @@ func TestCreateApp(t *testing.T) {
3232
expectedStatusCode int
3333
expectedErrorDetails *string
3434
}{
35+
{
36+
name: "should return 400 bad request when icon is not a single emoji",
37+
parameters: client.CreateAppParams{
38+
SkipPython: f.Ptr(false),
39+
SkipSketch: f.Ptr(false),
40+
},
41+
body: client.CreateAppRequest{
42+
Icon: f.Ptr("invalid-icon"),
43+
Name: "HelloWorld-2",
44+
Description: f.Ptr("My HelloWorld description"),
45+
},
46+
expectedStatusCode: http.StatusBadRequest,
47+
expectedErrorDetails: f.Ptr("invalid app: icon \"invalid-icon\" is not a valid single emoji"),
48+
},
49+
{
50+
name: "should create app successfully when icon is empty",
51+
parameters: client.CreateAppParams{
52+
SkipPython: f.Ptr(false),
53+
SkipSketch: f.Ptr(false),
54+
},
55+
body: client.CreateAppRequest{
56+
Icon: nil,
57+
Name: "HelloWorld-2",
58+
Description: f.Ptr("My HelloWorld description"),
59+
},
60+
expectedStatusCode: http.StatusCreated,
61+
//expectedErrorDetails: f.Ptr("invalid app: icon cannot be empty"),
62+
},
3563
{
3664
name: "should return 201 Created on first successful creation",
3765
parameters: client.CreateAppParams{
@@ -192,7 +220,33 @@ func TestEditApp(t *testing.T) {
192220
require.Equal(t, renamedApp, detailsResp.JSON200.Name)
193221
require.Equal(t, modifedIcon, *detailsResp.JSON200.Icon)
194222
})
223+
t.Run("RequestEmptyIcon_Success", func(t *testing.T) {
224+
createResp, err := httpClient.CreateAppWithResponse(
225+
t.Context(),
226+
&client.CreateAppParams{SkipSketch: f.Ptr(true)},
227+
client.CreateAppRequest{
228+
Icon: f.Ptr("💻"),
229+
Name: "new-valid-app-1",
230+
Description: f.Ptr("My app description"),
231+
},
232+
)
233+
require.NoError(t, err)
234+
require.Equal(t, http.StatusCreated, createResp.StatusCode())
235+
require.NotNil(t, createResp.JSON201)
236+
237+
validAppId := *createResp.JSON201.Id
195238

239+
invalidIconBody := `{"icon": "","description": "modified", "example": false,"default": false}`
240+
editResp, err := httpClient.EditAppWithBody(
241+
t.Context(),
242+
validAppId,
243+
"application/json",
244+
strings.NewReader(invalidIconBody),
245+
)
246+
require.NoError(t, err)
247+
defer editResp.Body.Close()
248+
require.Equal(t, http.StatusOK, editResp.StatusCode)
249+
})
196250
t.Run("InvalidAppId_Fail", func(t *testing.T) {
197251
var actualResponseBody models.ErrorResponse
198252
editResp, err := httpClient.EditApp(
@@ -232,7 +286,7 @@ func TestEditApp(t *testing.T) {
232286
require.Equal(t, "unable to find the app", actualResponseBody.Details)
233287
})
234288

235-
t.Run("InvalidRequestBody_Fail", func(t *testing.T) {
289+
t.Run("InvalidRequestSintaxBody_Fail", func(t *testing.T) {
236290
createResp, err := httpClient.CreateAppWithResponse(
237291
t.Context(),
238292
&client.CreateAppParams{SkipSketch: f.Ptr(true)},
@@ -266,6 +320,40 @@ func TestEditApp(t *testing.T) {
266320
require.NoError(t, err)
267321
require.Equal(t, "invalid request", actualResponseBody.Details)
268322
})
323+
t.Run("InvalidRequestIcon_Fail", func(t *testing.T) {
324+
createResp, err := httpClient.CreateAppWithResponse(
325+
t.Context(),
326+
&client.CreateAppParams{SkipSketch: f.Ptr(true)},
327+
client.CreateAppRequest{
328+
Icon: f.Ptr("💻"),
329+
Name: "new-valid-app-2",
330+
Description: f.Ptr("My app description"),
331+
},
332+
)
333+
require.NoError(t, err)
334+
require.Equal(t, http.StatusCreated, createResp.StatusCode())
335+
require.NotNil(t, createResp.JSON201)
336+
337+
validAppId := *createResp.JSON201.Id
338+
339+
var actualResponseBody models.ErrorResponse
340+
invalidIconBody := `{"name": "test", "icon": "💻 invalid"}`
341+
editResp, err := httpClient.EditAppWithBody(
342+
t.Context(),
343+
validAppId,
344+
"application/json",
345+
strings.NewReader(invalidIconBody),
346+
)
347+
require.NoError(t, err)
348+
defer editResp.Body.Close()
349+
350+
require.Equal(t, http.StatusBadRequest, editResp.StatusCode)
351+
body, err := io.ReadAll(editResp.Body)
352+
require.NoError(t, err)
353+
err = json.Unmarshal(body, &actualResponseBody)
354+
require.NoError(t, err)
355+
require.Equal(t, "invalid app: icon \"💻 invalid\" is not a valid single emoji", actualResponseBody.Details)
356+
})
269357
}
270358

271359
func TestDeleteApp(t *testing.T) {

‎internal/orchestrator/app/app.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,19 @@ func (a *ArduinoApp) GetDescriptorPath() *paths.Path {
8989
return descriptorFile
9090
}
9191

92+
var ErrInvalidApp = fmt.Errorf("invalid app")
93+
9294
func (a *ArduinoApp) Save() error {
95+
if err := a.Descriptor.IsValid(); err != nil {
96+
return fmt.Errorf("%w: %v", ErrInvalidApp, err)
97+
}
98+
if err := a.writeApp(); err != nil {
99+
return err
100+
}
101+
return nil
102+
}
103+
104+
func (a *ArduinoApp) writeApp() error {
93105
descriptorPath := a.GetDescriptorPath()
94106
if descriptorPath == nil {
95107
return errors.New("app descriptor file path is not set")

‎internal/orchestrator/app/parser.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -110,14 +110,14 @@ func ParseDescriptorFile(file *paths.Path) (AppDescriptor, error) {
110110
return AppDescriptor{}, fmt.Errorf("application name is empty")
111111
}
112112

113-
return descriptor, validate(descriptor)
113+
return descriptor, descriptor.IsValid()
114114
}
115115

116-
func validate(app AppDescriptor) error {
116+
func (a *AppDescriptor) IsValid() error {
117117
var allErrors error
118-
if app.Icon != "" {
119-
if !isSingleEmoji(app.Icon) {
120-
allErrors = errors.Join(allErrors, fmt.Errorf("icon %q is not a valid single emoji", app.Icon))
118+
if a.Icon != "" {
119+
if !isSingleEmoji(a.Icon) {
120+
allErrors = errors.Join(allErrors, fmt.Errorf("icon %q is not a valid single emoji", a.Icon))
121121
}
122122
}
123123
return allErrors

‎internal/orchestrator/orchestrator.go‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ import (
4242
var (
4343
ErrAppAlreadyExists = fmt.Errorf("app already exists")
4444
ErrAppDoesntExists = fmt.Errorf("app doesn't exist")
45-
ErrInvalidApp = fmt.Errorf("invalid app")
4645
)
4746

4847
const (
@@ -700,13 +699,15 @@ func CreateApp(
700699
return CreateAppResponse{}, ErrAppAlreadyExists
701700
}
702701
appName := req.Name
703-
app := app.AppDescriptor{
702+
newApp := app.AppDescriptor{
704703
Name: appName,
705704
Description: req.Description,
706705
Ports: []int{},
707706
Icon: req.Icon, // TODO: not sure if icon will exists for bricks
708707
}
709-
708+
if err := newApp.IsValid(); err != nil {
709+
return CreateAppResponse{}, fmt.Errorf("%w: %v", app.ErrInvalidApp, err)
710+
}
710711
var options appgenerator.Opts = 0
711712

712713
if req.SkipSketch {
@@ -716,7 +717,7 @@ func CreateApp(
716717
options |= appgenerator.SkipPython
717718
}
718719

719-
if err := appgenerator.GenerateApp(basePath, app, options); err != nil {
720+
if err := appgenerator.GenerateApp(basePath, newApp, options); err != nil {
720721
return CreateAppResponse{}, fmt.Errorf("failed to create app: %w", err)
721722
}
722723
id, err := idProvider.IDFromPath(basePath)
@@ -748,7 +749,7 @@ func CloneApp(
748749
return CloneAppResponse{}, ErrAppDoesntExists
749750
}
750751
if !originPath.Join("app.yaml").Exist() && !originPath.Join("app.yml").Exist() {
751-
return CloneAppResponse{}, ErrInvalidApp
752+
return CloneAppResponse{}, app.ErrInvalidApp
752753
}
753754

754755
var dstPath *paths.Path
@@ -894,37 +895,40 @@ type AppEditRequest struct {
894895

895896
func EditApp(
896897
req AppEditRequest,
897-
app *app.ArduinoApp,
898+
editApp *app.ArduinoApp,
898899
cfg config.Configuration,
899900
) (editErr error) {
900901
if req.Default != nil {
901-
if err := editAppDefaults(app, *req.Default, cfg); err != nil {
902+
if err := editAppDefaults(editApp, *req.Default, cfg); err != nil {
902903
return fmt.Errorf("failed to edit app defaults: %w", err)
903904
}
904905
}
905906

906907
if req.Name != nil {
907-
app.Descriptor.Name = *req.Name
908-
newPath := app.FullPath.Parent().Join(slug.Make(*req.Name))
908+
editApp.Descriptor.Name = *req.Name
909+
newPath := editApp.FullPath.Parent().Join(slug.Make(*req.Name))
909910
if newPath.Exist() {
910911
return ErrAppAlreadyExists
911912
}
912-
if err := app.FullPath.Rename(newPath); err != nil {
913+
if err := editApp.FullPath.Rename(newPath); err != nil {
913914
editErr = fmt.Errorf("failed to rename app path: %w", err)
914915
return editErr
915916
}
916-
app.FullPath = newPath
917-
app.Name = app.Descriptor.Name
917+
editApp.FullPath = newPath
918+
editApp.Name = editApp.Descriptor.Name
918919
}
919920

920921
if req.Icon != nil {
921-
app.Descriptor.Icon = *req.Icon
922+
editApp.Descriptor.Icon = *req.Icon
922923
}
923924
if req.Description != nil {
924-
app.Descriptor.Description = *req.Description
925+
editApp.Descriptor.Description = *req.Description
925926
}
926927

927-
err := app.Save()
928+
if err := editApp.Descriptor.IsValid(); err != nil {
929+
return fmt.Errorf("%w: %w", app.ErrInvalidApp, err)
930+
}
931+
err := editApp.Save()
928932
if err != nil {
929933
return fmt.Errorf("failed to save app: %w", err)
930934
}

‎internal/orchestrator/orchestrator_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ func TestCloneApp(t *testing.T) {
127127
err := cfg.AppsDir().Join("app-without-yaml").Mkdir()
128128
require.NoError(t, err)
129129
_, err = CloneApp(t.Context(), CloneAppRequest{FromID: f.Must(idProvider.ParseID("user:app-without-yaml"))}, idProvider, cfg)
130-
require.ErrorIs(t, err, ErrInvalidApp)
130+
require.ErrorIs(t, err, app.ErrInvalidApp)
131131
})
132132
t.Run("name already exists", func(t *testing.T) {
133133
_, err = CloneApp(t.Context(), CloneAppRequest{

0 commit comments

Comments
 (0)