From 8b20406f45374e5e6203596afab2d739d3a7c974 Mon Sep 17 00:00:00 2001 From: Seebs Date: Tue, 30 Nov 2021 11:38:07 -0600 Subject: [PATCH] experiment: handle errors more gracefully, but don't stream CreateKeys We were trying to write an error to a ResponseWriter After attempting to write to it, and this produces messages about superfluous WriteHeaders, which is correct. This patch changes things so that we report messages more clearly and verbosely if we hit them before writing, and if we try to write and fail, we log the message because that's all we can do. This does change semantics slightly, in that now we're marshalling separately from trying to write the marshalled data. I think this is probably a reasonable call because it lets us get diagnostics about a hypothetical encoding problem, but in practice I don't think there should be any encoding problems. So my guess is the actual error will occur in that last line, and be logged to the server console instead of failing to write over HTTP. Also note that this changes some of the messages to include the underlying error they're complaining about. We also merge the create/find and index/field cases because only a couple of lines of code changed between four largeish functions, and we test some of the failure cases. We don't have test coverage on the "field isn't provided" type things because the mux won't actually route things there without them, so far as I know. --- http/handler.go | 214 +++++++++++-------------------------------- http/handler_test.go | 112 +++++++++++++++++++++- 2 files changed, 167 insertions(+), 159 deletions(-) diff --git a/http/handler.go b/http/handler.go index d3aa20d6a..f000d3738 100644 --- a/http/handler.go +++ b/http/handler.go @@ -3091,7 +3091,7 @@ func (h *Handler) handlePostTranslateIndexDB(w http.ResponseWriter, r *http.Requ resp.write(w, err) } -func (h *Handler) handleFindIndexKeys(w http.ResponseWriter, r *http.Request) { +func (h *Handler) handleFindOrCreateKeys(w http.ResponseWriter, r *http.Request, requireField bool, create bool) { // Verify input and output types if r.Header.Get("Content-Type") != "application/json" { http.Error(w, "Unsupported media type", http.StatusUnsupportedMediaType) @@ -3101,178 +3101,76 @@ func (h *Handler) handleFindIndexKeys(w http.ResponseWriter, r *http.Request) { http.Error(w, "Not acceptable", http.StatusNotAcceptable) return } - - indexName, ok := mux.Vars(r)["index"] - if !ok { - http.Error(w, "index name is required", http.StatusBadRequest) - return - } - - bd, err := readBody(r) - if err != nil { - http.Error(w, "failed to read body", http.StatusBadRequest) - return - } - + var indexName, fieldName string var keys []string - err = json.Unmarshal(bd, &keys) - if err != nil { - http.Error(w, "failed to decode request", http.StatusBadRequest) - return - } + err := func() error { + var ok bool + indexName, ok = mux.Vars(r)["index"] + if !ok { + return errors.New("index name is required") + } - translations, err := h.api.FindIndexKeys(r.Context(), indexName, keys...) - if err != nil { - http.Error(w, "translating keys", http.StatusBadRequest) - return - } + if requireField { + fieldName, ok = mux.Vars(r)["field"] + if !ok { + return errors.New("field name is required") + } + } - err = json.NewEncoder(w).Encode(translations) + bd, err := readBody(r) + if err != nil { + return fmt.Errorf("failed to read body: %v", err) + } + + err = json.Unmarshal(bd, &keys) + if err != nil { + return fmt.Errorf("failed to decode request: %v", err) + } + return nil + }() if err != nil { - http.Error(w, "encoding result", http.StatusBadRequest) + http.Error(w, err.Error(), http.StatusBadRequest) + } + var translations map[string]uint64 + switch { + case requireField && create: + translations, err = h.api.CreateFieldKeys(r.Context(), indexName, fieldName, keys...) + case requireField && !create: + translations, err = h.api.FindFieldKeys(r.Context(), indexName, fieldName, keys...) + case !requireField && create: + translations, err = h.api.CreateIndexKeys(r.Context(), indexName, keys...) + case !requireField && !create: + translations, err = h.api.FindIndexKeys(r.Context(), indexName, keys...) + + } + if err != nil { + http.Error(w, fmt.Sprintf("translating keys: %v", err), http.StatusInternalServerError) return } + data, err := json.Marshal(translations) + if err != nil { + http.Error(w, fmt.Sprintf("encoding response: %v", err), http.StatusInternalServerError) + } + _, err = w.Write(data) + if err != nil { + h.logger.Printf("writing CreateFieldKeys response: %v", err) + } +} + +func (h *Handler) handleFindIndexKeys(w http.ResponseWriter, r *http.Request) { + h.handleFindOrCreateKeys(w, r, false, false) } func (h *Handler) handleFindFieldKeys(w http.ResponseWriter, r *http.Request) { - // Verify input and output types - if r.Header.Get("Content-Type") != "application/json" { - http.Error(w, "Unsupported media type", http.StatusUnsupportedMediaType) - return - } - if !validHeaderAcceptJSON(r.Header) { - http.Error(w, "Not acceptable", http.StatusNotAcceptable) - return - } - - indexName, ok := mux.Vars(r)["index"] - if !ok { - http.Error(w, "index name is required", http.StatusBadRequest) - return - } - - fieldName, ok := mux.Vars(r)["field"] - if !ok { - http.Error(w, "field name is required", http.StatusBadRequest) - return - } - - bd, err := readBody(r) - if err != nil { - http.Error(w, "failed to read body", http.StatusBadRequest) - return - } - - var keys []string - err = json.Unmarshal(bd, &keys) - if err != nil { - http.Error(w, "failed to decode request", http.StatusBadRequest) - return - } - - translations, err := h.api.FindFieldKeys(r.Context(), indexName, fieldName, keys...) - if err != nil { - http.Error(w, "translating keys", http.StatusBadRequest) - return - } - - err = json.NewEncoder(w).Encode(translations) - if err != nil { - http.Error(w, "encoding result", http.StatusBadRequest) - return - } + h.handleFindOrCreateKeys(w, r, true, false) } func (h *Handler) handleCreateIndexKeys(w http.ResponseWriter, r *http.Request) { - // Verify input and output types - if r.Header.Get("Content-Type") != "application/json" { - http.Error(w, "Unsupported media type", http.StatusUnsupportedMediaType) - return - } - if !validHeaderAcceptJSON(r.Header) { - http.Error(w, "Not acceptable", http.StatusNotAcceptable) - return - } - - indexName, ok := mux.Vars(r)["index"] - if !ok { - http.Error(w, "index name is required", http.StatusBadRequest) - return - } - - bd, err := readBody(r) - if err != nil { - http.Error(w, "failed to read body", http.StatusBadRequest) - return - } - - var keys []string - err = json.Unmarshal(bd, &keys) - if err != nil { - http.Error(w, "failed to decode request", http.StatusBadRequest) - return - } - - translations, err := h.api.CreateIndexKeys(r.Context(), indexName, keys...) - if err != nil { - http.Error(w, "translating keys", http.StatusBadRequest) - return - } - - err = json.NewEncoder(w).Encode(translations) - if err != nil { - http.Error(w, "encoding result", http.StatusBadRequest) - return - } + h.handleFindOrCreateKeys(w, r, false, true) } func (h *Handler) handleCreateFieldKeys(w http.ResponseWriter, r *http.Request) { - // Verify input and output types - if r.Header.Get("Content-Type") != "application/json" { - http.Error(w, "Unsupported media type", http.StatusUnsupportedMediaType) - return - } - if !validHeaderAcceptJSON(r.Header) { - http.Error(w, "Not acceptable", http.StatusNotAcceptable) - return - } - - indexName, ok := mux.Vars(r)["index"] - if !ok { - http.Error(w, "index name is required", http.StatusBadRequest) - return - } - - fieldName, ok := mux.Vars(r)["field"] - if !ok { - http.Error(w, "field name is required", http.StatusBadRequest) - return - } - - bd, err := readBody(r) - if err != nil { - http.Error(w, "failed to read body", http.StatusBadRequest) - return - } - - var keys []string - err = json.Unmarshal(bd, &keys) - if err != nil { - http.Error(w, "failed to decode request", http.StatusBadRequest) - return - } - - translations, err := h.api.CreateFieldKeys(r.Context(), indexName, fieldName, keys...) - if err != nil { - http.Error(w, "translating keys", http.StatusBadRequest) - return - } - - err = json.NewEncoder(w).Encode(translations) - if err != nil { - http.Error(w, "encoding result", http.StatusBadRequest) - return - } + h.handleFindOrCreateKeys(w, r, true, true) } func (h *Handler) handleMatchField(w http.ResponseWriter, r *http.Request) { diff --git a/http/handler_test.go b/http/handler_test.go index bfea699d3..ca841268b 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -19,6 +19,7 @@ import ( "fmt" "net" gohttp "net/http" + "strings" "testing" pilosa "github.com/molecula/featurebase/v2" @@ -167,6 +168,115 @@ func TestIngestSchemaHandler(t *testing.T) { schemaURL := fmt.Sprintf("%s/internal/schema", m.URL()) resp := test.Do(t, "POST", schemaURL, string(schema)) if resp.StatusCode != gohttp.StatusOK { - t.Errorf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + t.Errorf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + // now, try again, expecting a failure: + resp = test.Do(t, "POST", schemaURL, string(schema)) + if resp.StatusCode != gohttp.StatusConflict { + t.Errorf("invalid status: expected 409, got %d, body=%s", resp.StatusCode, resp.Body) + } +} + +func TestTranslationHandlers(t *testing.T) { + // reusable data for the tests + nameBytes, err := json.Marshal([]string{"a", "b", "c"}) + if err != nil { + t.Fatalf("marshalling json: %v", err) + } + names := string(nameBytes) + + c := test.MustRunCluster(t, 1) + defer c.Close() + + schema := ` +{ + "index-name": "example", + "primary-key-type": "string", + "index-action": "create", + "fields": [ + { + "field-name": "stringset", + "field-type": "string", + "field-options": { + "cache-type": "ranked", + "cache-size": 100000 + } + } + ] +} +` + m := c.GetPrimary() + schemaURL := fmt.Sprintf("%s/internal/schema", m.URL()) + resp := test.Do(t, "POST", schemaURL, string(schema)) + if resp.StatusCode != gohttp.StatusOK { + t.Errorf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + baseURLs := []string{ + fmt.Sprintf("%s/internal/translate/index/example/", m.URL()), + fmt.Sprintf("%s/internal/translate/field/example/stringset/", m.URL()), + fmt.Sprintf("%s/internal/translate/field/example/nonexistent/", m.URL()), + } + for _, url := range baseURLs { + expectFailure := strings.HasSuffix(url, "/nonexistent/") + createURL := url + "keys/create" + findURL := url + "keys/find" + var results map[string]uint64 + + if expectFailure { + resp := test.Do(t, "POST", findURL, names) + if resp.StatusCode != gohttp.StatusInternalServerError { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + resp = test.Do(t, "POST", createURL, names) + if resp.StatusCode != gohttp.StatusInternalServerError { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + continue + } + + // try to find them when they don't exist + resp := test.Do(t, "POST", findURL, names) + if resp.StatusCode != gohttp.StatusOK { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + err := json.Unmarshal([]byte(resp.Body), &results) + if err != nil { + t.Fatalf("unmarshalling result: %v", err) + } + if len(results) != 0 { + t.Fatalf("finding keys before any were set: expected no results, got %d (%q)", len(results), results) + } + + // try to create them, but malformed, so we expect an error + resp = test.Do(t, "POST", createURL, names[:6]) + if resp.StatusCode != gohttp.StatusBadRequest { + t.Fatalf("invalid status: expected 400, got %d, body=%s", resp.StatusCode, resp.Body) + } + + // try to create them + resp = test.Do(t, "POST", createURL, names) + if resp.StatusCode != gohttp.StatusOK { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + err = json.Unmarshal([]byte(resp.Body), &results) + if err != nil { + t.Fatalf("unmarshalling result: %v", err) + } + if len(results) != 3 { + t.Fatalf("finding keys before any were set: expected 3 results, got %d (%q)", len(results), results) + } + + // try to find them now that they exist + resp = test.Do(t, "POST", findURL, names) + if resp.StatusCode != gohttp.StatusOK { + t.Fatalf("invalid status: %d, body=%s", resp.StatusCode, resp.Body) + } + err = json.Unmarshal([]byte(resp.Body), &results) + if err != nil { + t.Fatalf("unmarshalling result: %v", err) + } + if len(results) != 3 { + t.Fatalf("finding keys before any were set: expected 3 results, got %d (%q)", len(results), results) + } } }