From ec70bfa4cc54a5e190ce40bdaab761f54f3f5d46 Mon Sep 17 00:00:00 2001 From: Evgeniy Kulikov Date: Tue, 26 Jan 2021 18:36:53 +0300 Subject: [PATCH] Fixes after review - It's confusing. If there is no difference, I suggest having the route named after the protocol verb or resulting handler. So it should be either post or upload. - Don't you find that it would be more understandable without else ifs? - Why do we need a temporary file here? - etc Signed-off-by: Evgeniy Kulikov --- app.go | 4 ++-- filter.go | 18 ++++++++++++++--- upload.go | 60 ++++++++++++++++++++++++++++++------------------------- 3 files changed, 50 insertions(+), 32 deletions(-) diff --git a/app.go b/app.go index f565793..6d666eb 100644 --- a/app.go +++ b/app.go @@ -181,8 +181,8 @@ func (a *app) Serve(ctx context.Context) { r := router.New() r.RedirectTrailingSlash = true - a.log.Info("enabled /put/{cid}") - r.POST("/put/{cid}", a.upload) + a.log.Info("enabled /upload/{cid}") + r.POST("/upload/{cid}", a.upload) a.log.Info("enabled /get/{cid}/{oid}") r.GET("/get/{cid}/{oid}", a.byAddress) diff --git a/filter.go b/filter.go index 2f12eb5..7e13766 100644 --- a/filter.go +++ b/filter.go @@ -61,13 +61,24 @@ func (h *headerFilter) Filter(header *fasthttp.RequestHeader) map[string]string prefix := []byte(userAttributeHeaderPrefix) header.VisitAll(func(key, val []byte) { + // checks that key and val not empty if len(key) == 0 || len(val) == 0 { return - } else if !bytes.HasPrefix(key, prefix) { + } + + // checks that key has attribute prefix + if !bytes.HasPrefix(key, prefix) { return - } else if key = bytes.TrimPrefix(key, prefix); len(key) == 0 { + } + + // checks that after removing attribute prefix we had not empty key + if key = bytes.TrimPrefix(key, prefix); len(key) == 0 { return - } else if name, ok := h.mapping[string(key)]; ok { + } + + // checks mapping table and if we found record store it + // at resulting hashmap + if name, ok := h.mapping[string(key)]; ok { result[name] = string(val) h.logger.Debug("add attribute to result object", @@ -77,6 +88,7 @@ func (h *headerFilter) Filter(header *fasthttp.RequestHeader) map[string]string return } + // otherwise inform that attribute will be ignored h.logger.Debug("ignore attribute", zap.String("key", string(key)), zap.String("val", string(val))) diff --git a/upload.go b/upload.go index b1330fc..6d72a61 100644 --- a/upload.go +++ b/upload.go @@ -3,9 +3,7 @@ package main import ( "encoding/json" "io" - "io/ioutil" "mime/multipart" - "os" "strconv" "time" @@ -40,7 +38,7 @@ func (a *app) upload(c *fasthttp.RequestCtx) { var ( err error name string - tmp *os.File + tmp io.Reader addr *object.Address form *multipart.Form cid = container.NewID() @@ -55,50 +53,51 @@ func (a *app) upload(c *fasthttp.RequestCtx) { return } - if tmp, err = ioutil.TempFile("", "http-gate-upload-*"); err != nil { - log.Error("could not prepare temporary file", zap.Error(err)) - c.Error("could not prepare temporary file", fasthttp.StatusBadRequest) - return - } - defer func() { - tmpName := tmp.Name() - - log.Debug("close temporary file", zap.Error(tmp.Close())) - log.Debug("remove temporary file", zap.Error(os.RemoveAll(tmpName))) + // if temporary reader can be closed - close it + if closer := tmp.(io.Closer); closer != nil { + log.Debug("close temporary multipart/form file", zap.Error(closer.Close())) + } if form == nil { return } - log.Debug("cleanup multipart form", zap.Error(form.RemoveAll())) + log.Debug("cleanup multipart/form", zap.Error(form.RemoveAll())) }() + // tries to receive multipart/form or throw error if form, err = c.MultipartForm(); err != nil { - log.Error("could not receive multipart form", zap.Error(err)) - c.Error("could not receive multipart form: "+err.Error(), fasthttp.StatusBadRequest) + log.Error("could not receive multipart/form", zap.Error(err)) + c.Error("could not receive multipart/form: "+err.Error(), fasthttp.StatusBadRequest) return - } else if ln := len(form.File); ln != 1 { - log.Error("received multipart form with more then one file", zap.Int("count", ln)) - c.Error("received multipart form with more then one file", fasthttp.StatusBadRequest) + } + + // checks that received multipart/form contains only one `file` per request + if ln := len(form.File); ln != 1 { + log.Error("received multipart/form with more then one file", zap.Int("count", ln)) + c.Error("received multipart/form with more then one file", fasthttp.StatusBadRequest) return } for _, file := range form.File { + // because multipart/form can contains multiple FileHeader records + // we should check that we have only one per request or throw error if ln := len(file); ln != 1 { - log.Error("received multipart form file should contains one record", zap.Int("count", ln)) - c.Error("received multipart form file should contains one record", fasthttp.StatusBadRequest) + log.Error("received multipart/form file should contains one record", zap.Int("count", ln)) + c.Error("received multipart/form file should contains one record", fasthttp.StatusBadRequest) return } name = file[0].Filename - if err = fasthttp.SaveMultipartFile(file[0], tmp.Name()); err != nil { - log.Error("could not store uploaded file into temporary", zap.Error(err)) - c.Error("could not store uploaded file into temporary", fasthttp.StatusBadRequest) + // opens multipart/form file to work within or throw error + if tmp, err = file[0].Open(); err != nil { + log.Error("could not prepare uploaded file", zap.Error(err)) + c.Error("could not prepare uploaded file", fasthttp.StatusBadRequest) return } @@ -107,6 +106,7 @@ func (a *app) upload(c *fasthttp.RequestCtx) { filtered := a.hdr.Filter(&c.Request.Header) attributes := make([]*object.Attribute, 0, len(filtered)) + // prepares attributes from filtered headers for key, val := range filtered { attribute := object.NewAttribute() attribute.SetKey(key) @@ -115,7 +115,7 @@ func (a *app) upload(c *fasthttp.RequestCtx) { attributes = append(attributes, attribute) } - // Attribute FileName wasn't set from header + // sets FileName attribute if it wasn't set from header if _, ok := filtered[object.AttributeFileName]; ok { filename := object.NewAttribute() filename.SetKey(object.AttributeFileName) @@ -124,7 +124,7 @@ func (a *app) upload(c *fasthttp.RequestCtx) { attributes = append(attributes, filename) } - // Attribute Timestamp wasn't set from header and enabled by settings + // sets Timestamp attribute if it wasn't set from header and enabled by settings if _, ok := filtered[object.AttributeTimestamp]; ok && a.enableDefaultTimestamp { timestamp := object.NewAttribute() timestamp.SetKey(object.AttributeTimestamp) @@ -133,23 +133,29 @@ func (a *app) upload(c *fasthttp.RequestCtx) { attributes = append(attributes, timestamp) } + // prepares new object and fill it raw := object.NewRaw() raw.SetContainerID(cid) raw.SetOwnerID(a.cli.Owner()) // should be various: from sdk / BearerToken raw.SetAttributes(attributes...) + // tries to put file into NeoFS or throw error if addr, err = a.cli.Object().Put(c, raw.Object(), sdk.WithPutReader(tmp)); err != nil { log.Error("could not store file in NeoFS", zap.Error(err)) c.Error("could not store file in NeoFS", fasthttp.StatusBadRequest) return - } else if err = newPutResponse(addr).Encode(c); err != nil { + } + + // tries to return response, otherwise, if something went wrong throw error + if err = newPutResponse(addr).Encode(c); err != nil { log.Error("could not prepare response", zap.Error(err)) c.Error("could not prepare response", fasthttp.StatusBadRequest) return } + // reports status code and content type c.Response.SetStatusCode(fasthttp.StatusOK) c.Response.Header.SetContentType(jsonHeader) }