From e563b9dd2f3aa0484e6cdc08869991b5e438023e Mon Sep 17 00:00:00 2001 From: Evgeny Antyshev Date: Mon, 14 Nov 2022 20:56:35 +0300 Subject: [PATCH] [Tour Of Beam] verify that unit exists when saving progress (#24118) * AIO * Update learning/tour-of-beam/backend/integration_tests/auth_test.go Co-authored-by: Danny McCormick * nit Co-authored-by: Danny McCormick --- learning/tour-of-beam/backend/function.go | 8 ++ .../backend/integration_tests/auth_test.go | 111 +++++++++++------- .../backend/integration_tests/client.go | 23 +++- .../integration_tests/function_test.go | 18 +-- .../backend/internal/service/content.go | 8 ++ .../backend/internal/storage/datastore.go | 27 ++++- .../backend/internal/storage/iface.go | 2 + .../backend/internal/storage/index.yaml | 5 + .../backend/internal/storage/mock.go | 9 ++ .../samples/api/get_user_progress.json | 4 +- 10 files changed, 157 insertions(+), 58 deletions(-) diff --git a/learning/tour-of-beam/backend/function.go b/learning/tour-of-beam/backend/function.go index 2138ba19da0a..2c562ac74b72 100644 --- a/learning/tour-of-beam/backend/function.go +++ b/learning/tour-of-beam/backend/function.go @@ -197,6 +197,10 @@ func postUnitComplete(w http.ResponseWriter, r *http.Request, sdk tob.Sdk, uid s unitId := r.URL.Query().Get("id") err := svc.SetUnitComplete(r.Context(), sdk, unitId, uid) + if errors.Is(err, tob.ErrNoUnit) { + finalizeErrResponse(w, http.StatusNotFound, NOT_FOUND, "unit not found") + return + } if err != nil { log.Println("Set unit complete error:", err) finalizeErrResponse(w, http.StatusInternalServerError, INTERNAL_ERROR, "storage error") @@ -219,6 +223,10 @@ func postUserCode(w http.ResponseWriter, r *http.Request, sdk tob.Sdk, uid strin } err = svc.SaveUserCode(r.Context(), sdk, unitId, uid, userCodeRequest) + if errors.Is(err, tob.ErrNoUnit) { + finalizeErrResponse(w, http.StatusNotFound, NOT_FOUND, "unit not found") + return + } if err != nil { log.Println("Save user code error:", err) message := "storage error" diff --git a/learning/tour-of-beam/backend/integration_tests/auth_test.go b/learning/tour-of-beam/backend/integration_tests/auth_test.go index abfecd5e4fa6..6e5c7881d58a 100644 --- a/learning/tour-of-beam/backend/integration_tests/auth_test.go +++ b/learning/tour-of-beam/backend/integration_tests/auth_test.go @@ -17,6 +17,7 @@ package main import ( "flag" + "net/http" "os" "path/filepath" "testing" @@ -36,68 +37,92 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } +func makeUserCodeRequest() UserCodeRequest { + return UserCodeRequest{ + Files: []UserCodeFile{ + {Name: "main.py", Content: "import sys; sys.exit(0)", IsMain: true}, + }, + PipelineOptions: "some opts", + } +} + +func checkBadHttpCode(t *testing.T, err error, code int) { + if err == nil { + t.Fatal("error expected") + } + if err, ok := err.(*ErrBadResponse); ok { + if err.Code == code { + return + } + } + t.Fatalf("Expected ErrBadResponse with code %v, got %v", code, err) +} + func TestSaveGetProgress(t *testing.T) { idToken := emulator.getIDToken() + // postUnitCompleteURL + port := os.Getenv(PORT_POST_UNIT_COMPLETE) + if port == "" { + t.Fatal(PORT_POST_UNIT_COMPLETE, "env not set") + } + postUnitCompleteURL := "http://localhost:" + port + + // postUserCodeURL + port = os.Getenv(PORT_POST_USER_CODE) + if port == "" { + t.Fatal(PORT_POST_USER_CODE, "env not set") + } + postUserCodeURL := "http://localhost:" + port + + // getUserProgressURL + port = os.Getenv(PORT_GET_USER_PROGRESS) + if port == "" { + t.Fatal(PORT_GET_USER_PROGRESS, "env not set") + } + getUserProgressURL := "http://localhost:" + port + + t.Run("save_complete_no_unit", func(t *testing.T) { + resp, err := PostUnitComplete(postUnitCompleteURL, "python", "unknown_unit_id_1", idToken) + checkBadHttpCode(t, err, http.StatusNotFound) + assert.Equal(t, "NOT_FOUND", resp.Code) + assert.Equal(t, "unit not found", resp.Message) + }) t.Run("save_complete", func(t *testing.T) { - port := os.Getenv(PORT_POST_UNIT_COMPLETE) - if port == "" { - t.Fatal(PORT_POST_UNIT_COMPLETE, "env not set") - } - url := "http://localhost:" + port - - err := PostUnitComplete(url, "python", "unit_id_1", idToken) + _, err := PostUnitComplete(postUnitCompleteURL, "python", "challenge1", idToken) if err != nil { t.Fatal(err) } }) t.Run("save_code", func(t *testing.T) { - port := os.Getenv(PORT_POST_USER_CODE) - if port == "" { - t.Fatal(PORT_POST_USER_CODE, "env not set") - } - url := "http://localhost:" + port - req := UserCodeRequest{ - Files: []UserCodeFile{ - {Name: "main.py", Content: "import sys; sys.exit(0)", IsMain: true}, - }, - PipelineOptions: "some opts", - } - - _, err := PostUserCode(url, "python", "unit_id_2", idToken, req) + req := makeUserCodeRequest() + _, err := PostUserCode(postUserCodeURL, "python", "example1", idToken, req) if err != nil { t.Fatal(err) } }) - t.Run("save_code_fail", func(t *testing.T) { - port := os.Getenv(PORT_POST_USER_CODE) - if port == "" { - t.Fatal(PORT_POST_USER_CODE, "env not set") - } - url := "http://localhost:" + port - req := UserCodeRequest{ - Files: []UserCodeFile{ - // empty content doesn't pass validation - {Name: "main.py", Content: "", IsMain: true}, - }, - PipelineOptions: "some opts", - } + t.Run("save_code_playground_fail", func(t *testing.T) { + req := makeUserCodeRequest() - resp, err := PostUserCode(url, "python", "unit_id_1", idToken, req) - if err != nil { - t.Fatal(err) - } + // empty content doesn't pass validation + req.Files[0].Content = "" + + resp, err := PostUserCode(postUserCodeURL, "python", "example1", idToken, req) + checkBadHttpCode(t, err, http.StatusInternalServerError) assert.Equal(t, "INTERNAL_ERROR", resp.Code) msg := "playground api error" assert.Equal(t, msg, resp.Message[:len(msg)]) + }) + t.Run("save_code_no_unit", func(t *testing.T) { + req := makeUserCodeRequest() + resp, err := PostUserCode(postUserCodeURL, "python", "unknown_unit_id_1", idToken, req) + checkBadHttpCode(t, err, http.StatusNotFound) + assert.Equal(t, "NOT_FOUND", resp.Code) + assert.Equal(t, "unit not found", resp.Message) + }) t.Run("get", func(t *testing.T) { - port := os.Getenv(PORT_GET_USER_PROGRESS) - if port == "" { - t.Fatal(PORT_GET_USER_PROGRESS, "env not set") - } - url := "http://localhost:" + port mock_path := filepath.Join("..", "samples", "api", "get_user_progress.json") var exp SdkProgress @@ -105,7 +130,7 @@ func TestSaveGetProgress(t *testing.T) { t.Fatal(err) } - resp, err := GetUserProgress(url, "python", idToken) + resp, err := GetUserProgress(getUserProgressURL, "python", idToken) if err != nil { t.Fatal(err) } diff --git a/learning/tour-of-beam/backend/integration_tests/client.go b/learning/tour-of-beam/backend/integration_tests/client.go index 081ff6182a3d..5058929eca46 100644 --- a/learning/tour-of-beam/backend/integration_tests/client.go +++ b/learning/tour-of-beam/backend/integration_tests/client.go @@ -21,6 +21,14 @@ import ( "os" ) +type ErrBadResponse struct { + Code int +} + +func (e *ErrBadResponse) Error() string { + return fmt.Sprintf("http code %d", e.Code) +} + var ( ExpectedHeaders = map[string]string{ "Access-Control-Allow-Origin": "*", @@ -81,11 +89,11 @@ func GetUserProgress(url, sdk, token string) (SdkProgress, error) { return result, err } -func PostUnitComplete(url, sdk, unitId, token string) error { - var result interface{} +func PostUnitComplete(url, sdk, unitId, token string) (ErrorResponse, error) { + var result ErrorResponse err := Do(&result, http.MethodPost, url, map[string]string{"sdk": sdk, "id": unitId}, map[string]string{"Authorization": "Bearer " + token}, nil) - return err + return result, err } func PostUserCode(url, sdk, unitId, token string, body UserCodeRequest) (ErrorResponse, error) { @@ -166,5 +174,12 @@ func Do(dst interface{}, method, url string, queryParams, headers map[string]str tee := io.TeeReader(resp.Body, os.Stdout) defer os.Stdout.WriteString("\n") - return json.NewDecoder(tee).Decode(dst) + if err := json.NewDecoder(tee).Decode(dst); err != nil { + return fmt.Errorf("response decode err: %w", err) + } + + if resp.StatusCode != http.StatusOK { + return &ErrBadResponse{resp.StatusCode} + } + return nil } diff --git a/learning/tour-of-beam/backend/integration_tests/function_test.go b/learning/tour-of-beam/backend/integration_tests/function_test.go index fcda014ec33e..ee99769f8e83 100644 --- a/learning/tour-of-beam/backend/integration_tests/function_test.go +++ b/learning/tour-of-beam/backend/integration_tests/function_test.go @@ -17,6 +17,7 @@ package main import ( "encoding/json" + "net/http" "os" "path/filepath" "testing" @@ -113,12 +114,14 @@ func TestGetUnitContent(t *testing.T) { func TestNegative(t *testing.T) { for i, params := range []struct { - portEnvName string - queryParams map[string]string - headers map[string]string - expected ErrorResponse + portEnvName string + queryParams map[string]string + headers map[string]string + expectedCode int + expected ErrorResponse }{ {PORT_GET_CONTENT_TREE, nil, nil, + http.StatusBadRequest, ErrorResponse{ Code: "BAD_FORMAT", Message: "unknown sdk", @@ -126,10 +129,12 @@ func TestNegative(t *testing.T) { }, {PORT_GET_CONTENT_TREE, map[string]string{"sdk": "scio"}, nil, // TODO: actually here should be a NOT_FOUND error + http.StatusInternalServerError, ErrorResponse{Code: "INTERNAL_ERROR", Message: "storage error"}, }, {PORT_GET_UNIT_CONTENT, map[string]string{"sdk": "python", "id": "unknown_unitId"}, nil, + http.StatusNotFound, ErrorResponse{ Code: "NOT_FOUND", Message: "unit not found", @@ -140,6 +145,7 @@ func TestNegative(t *testing.T) { {PORT_GET_USER_PROGRESS, map[string]string{"sdk": "python"}, map[string]string{"authorization": "bad_header"}, + http.StatusUnauthorized, ErrorResponse{ Code: "UNAUTHORIZED", Message: "bad auth header", @@ -155,9 +161,7 @@ func TestNegative(t *testing.T) { var resp ErrorResponse err := Get(&resp, url, params.queryParams, params.headers) - if err != nil { - t.Fatal(err) - } + checkBadHttpCode(t, err, params.expectedCode) assert.Equal(t, params.expected, resp) } } diff --git a/learning/tour-of-beam/backend/internal/service/content.go b/learning/tour-of-beam/backend/internal/service/content.go index 675addbf17fe..3afc106b70ff 100644 --- a/learning/tour-of-beam/backend/internal/service/content.go +++ b/learning/tour-of-beam/backend/internal/service/content.go @@ -70,10 +70,18 @@ func (s *Svc) GetUserProgress(ctx context.Context, sdk tob.Sdk, userId string) ( } func (s *Svc) SetUnitComplete(ctx context.Context, sdk tob.Sdk, unitId, uid string) error { + if err := s.Repo.CheckUnitExists(ctx, sdk, unitId); err != nil { + return err + } + return s.Repo.SetUnitComplete(ctx, sdk, unitId, uid) } func (s *Svc) SaveUserCode(ctx context.Context, sdk tob.Sdk, unitId, uid string, userRequest tob.UserCodeRequest) error { + if err := s.Repo.CheckUnitExists(ctx, sdk, unitId); err != nil { + return err + } + req := MakePgSaveRequest(userRequest, sdk) resp, err := s.PgClient.SaveSnippet(ctx, &req) if err != nil { diff --git a/learning/tour-of-beam/backend/internal/storage/datastore.go b/learning/tour-of-beam/backend/internal/storage/datastore.go index 477dc368012e..8116580f0e82 100644 --- a/learning/tour-of-beam/backend/internal/storage/datastore.go +++ b/learning/tour-of-beam/backend/internal/storage/datastore.go @@ -229,8 +229,9 @@ func (d *DatastoreDb) SaveContentTrees(ctx context.Context, trees []tob.ContentT return nil } -// Get learning unit content by unitId -func (d *DatastoreDb) GetUnitContent(ctx context.Context, sdk tob.Sdk, unitId string) (unit *tob.Unit, err error) { +// get a custom projection of a learning unit +func (d *DatastoreDb) getUnit(ctx context.Context, sdk tob.Sdk, unitId string, + projectionFunc func(*datastore.Query) *datastore.Query) (unit *tob.Unit, err error) { var tbNodes []TbLearningNode rootKey := pgNameKey(TbLearningPathKind, sdk.StorageID(), nil) @@ -238,6 +239,7 @@ func (d *DatastoreDb) GetUnitContent(ctx context.Context, sdk tob.Sdk, unitId st Namespace(PgNamespace). Ancestor(rootKey). FilterField("id", "=", unitId) + query = projectionFunc(query) _, err = d.Client.GetAll(ctx, query, &tbNodes) if err != nil { @@ -258,6 +260,27 @@ func (d *DatastoreDb) GetUnitContent(ctx context.Context, sdk tob.Sdk, unitId st return node.Unit, nil } +// Get learning unit content by unitId +func (d *DatastoreDb) GetUnitContent(ctx context.Context, sdk tob.Sdk, unitId string) (unit *tob.Unit, err error) { + return d.getUnit(ctx, sdk, unitId, func(q *datastore.Query) *datastore.Query { + return q + }) +} + +// Check if the unit exists, returns ErrNoUnit if not +func (d *DatastoreDb) CheckUnitExists(ctx context.Context, sdk tob.Sdk, unitId string) (err error) { + unit, err := d.getUnit(ctx, sdk, unitId, func(q *datastore.Query) *datastore.Query { + return q.Project("__key__", "type") + }) + if err != nil { + return err + } + if unit == nil { + return tob.ErrNoUnit + } + return nil +} + func (d *DatastoreDb) SaveUser(ctx context.Context, uid string) error { userKey := pgNameKey(TbUserKind, uid, nil) diff --git a/learning/tour-of-beam/backend/internal/storage/iface.go b/learning/tour-of-beam/backend/internal/storage/iface.go index c18e093ca7aa..ceb7230f0f32 100644 --- a/learning/tour-of-beam/backend/internal/storage/iface.go +++ b/learning/tour-of-beam/backend/internal/storage/iface.go @@ -26,6 +26,8 @@ type Iface interface { SaveContentTrees(ctx context.Context, trees []tob.ContentTree) error GetUnitContent(ctx context.Context, sdk tob.Sdk, unitId string) (*tob.Unit, error) + // Check if the unit exists, returns ErrNoUnit if not + CheckUnitExists(ctx context.Context, sdk tob.Sdk, unitId string) error SaveUser(ctx context.Context, uid string) error GetUserProgress(ctx context.Context, sdk tob.Sdk, uid string) (*tob.SdkProgress, error) diff --git a/learning/tour-of-beam/backend/internal/storage/index.yaml b/learning/tour-of-beam/backend/internal/storage/index.yaml index 65658f14a6ea..109b7b9fb85e 100644 --- a/learning/tour-of-beam/backend/internal/storage/index.yaml +++ b/learning/tour-of-beam/backend/internal/storage/index.yaml @@ -6,6 +6,11 @@ indexes: # index.yaml file manually, remove the "# AUTOGENERATED" marker line above. # If you want to manage some indexes manually, move them above the marker line. +- kind: "tb_learning_node" + ancestor: yes + properties: + - name: "id" + - name: "type" - kind: "tb_learning_module" ancestor: yes properties: diff --git a/learning/tour-of-beam/backend/internal/storage/mock.go b/learning/tour-of-beam/backend/internal/storage/mock.go index 1a8d4193bd11..6f0a6e182f5a 100644 --- a/learning/tour-of-beam/backend/internal/storage/mock.go +++ b/learning/tour-of-beam/backend/internal/storage/mock.go @@ -60,6 +60,15 @@ func (d *Mock) GetUnitContent(_ context.Context, sdk tob.Sdk, unitId string) (u return u, err } +func (d *Mock) CheckUnitExists(ctx context.Context, sdk tob.Sdk, unitId string) error { + for _, existingId := range []string{"intro-unit", "example1", "challenge1"} { + if unitId == existingId { + return nil + } + } + return tob.ErrNoUnit +} + func (d *Mock) SaveUser(ctx context.Context, uid string) error { return nil } diff --git a/learning/tour-of-beam/backend/samples/api/get_user_progress.json b/learning/tour-of-beam/backend/samples/api/get_user_progress.json index fc8027c9eb9f..6d1f04a424b7 100644 --- a/learning/tour-of-beam/backend/samples/api/get_user_progress.json +++ b/learning/tour-of-beam/backend/samples/api/get_user_progress.json @@ -1,11 +1,11 @@ { "units": [ { - "id": "unit_id_1", + "id": "challenge1", "isCompleted": true }, { - "id": "unit_id_2", + "id": "example1", "userSnippetId": "QY2dskTNu_w" } ]