Cleanup code (#2516)

* pre-compile the chunking regex

* reduce type conversions

* add changelog

changelog/unreleased/cleanup-code.md

@@ -0,0 +1,6 @@
+Enhancement: Cleaned up some code
+
+- Reduced type conversions []byte <-> string
+- pre-compile chunking regex
+
+https://github.com/cs3org/reva/pull/2516

internal/http/services/owncloud/ocdav/errors/error.go

@@ -19,6 +19,7 @@
 package errors
 
 import (
+	"bytes"
 	"encoding/xml"
 	"net/http"
 
@@ -76,9 +77,12 @@ func Marshal(code code, message string, header string) ([]byte, error) {
 		Header:    header,
 	})
 	if err != nil {
-		return []byte(""), err
+		return nil, err
 	}
-	return []byte(xml.Header + string(xmlstring)), err
+	var buf bytes.Buffer
+	buf.WriteString(xml.Header)
+	buf.Write(xmlstring)
+	return buf.Bytes(), err
 }
 
 // ErrorXML holds the xml representation of an error

internal/http/services/owncloud/ocdav/propfind/propfind.go

@@ -19,6 +19,7 @@
 package propfind
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"encoding/xml"
@@ -218,7 +219,7 @@ func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r
 	}
 
 	w.WriteHeader(http.StatusMultiStatus)
-	if _, err := w.Write([]byte(propRes)); err != nil {
+	if _, err := w.Write(propRes); err != nil {
 		log.Err(err).Msg("error writing response")
 	}
 }
@@ -545,24 +546,26 @@ func ReadPropfind(r io.Reader) (pf XML, status int, err error) {
 }
 
 // MultistatusResponse converts a list of resource infos into a multistatus response string
-func MultistatusResponse(ctx context.Context, pf *XML, mds []*provider.ResourceInfo, publicURL, ns string, linkshares map[string]struct{}) (string, error) {
+func MultistatusResponse(ctx context.Context, pf *XML, mds []*provider.ResourceInfo, publicURL, ns string, linkshares map[string]struct{}) ([]byte, error) {
 	responses := make([]*ResponseXML, 0, len(mds))
 	for i := range mds {
 		res, err := mdToPropResponse(ctx, pf, mds[i], publicURL, ns, linkshares)
 		if err != nil {
-			return "", err
+			return nil, err
 		}
 		responses = append(responses, res)
 	}
 	responsesXML, err := xml.Marshal(&responses)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
-	msg := `<?xml version="1.0" encoding="utf-8"?><d:multistatus xmlns:d="DAV:" `
-	msg += `xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">`
-	msg += string(responsesXML) + `</d:multistatus>`
-	return msg, nil
+	var buf bytes.Buffer
+	buf.WriteString(`<?xml version="1.0" encoding="utf-8"?><d:multistatus xmlns:d="DAV:" `)
+	buf.WriteString(`xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">`)
+	buf.Write(responsesXML)
+	buf.WriteString(`</d:multistatus>`)
+	return buf.Bytes(), nil
 }
 
 // mdToPropResponse converts the CS3 metadata into a webdav PropResponse
@@ -590,7 +593,7 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p
 	// -2 indicates unknown (default)
 	// -3 indicates unlimited
 	quota := net.PropQuotaUnknown
-	size := fmt.Sprintf("%d", md.Size)
+	size := strconv.FormatUint(md.Size, 10)
 	// TODO refactor helper functions: GetOpaqueJSONEncoded(opaque, key string, *struct) err, GetOpaquePlainEncoded(opaque, key) value, err
 	// or use ok like pattern and return bool?
 	if md.Opaque != nil && md.Opaque.Map != nil {
@@ -693,15 +696,15 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p
 				} else {
 					checksums.WriteString(" MD5:")
 				}
-				checksums.WriteString(string(e.Value))
+				checksums.Write(e.Value)
 			}
 			if e, ok := md.Opaque.Map["adler32"]; ok {
 				if checksums.Len() == 0 {
 					checksums.WriteString("<oc:checksum>ADLER32:")
 				} else {
 					checksums.WriteString(" ADLER32:")
 				}
-				checksums.WriteString(string(e.Value))
+				checksums.Write(e.Value)
 			}
 		}
 		if checksums.Len() > 0 {
@@ -861,15 +864,15 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p
 							} else {
 								checksums.WriteString(" MD5:")
 							}
-							checksums.WriteString(string(e.Value))
+							checksums.Write(e.Value)
 						}
 						if e, ok := md.Opaque.Map["adler32"]; ok {
 							if checksums.Len() == 0 {
 								checksums.WriteString("<oc:checksum>ADLER32:")
 							} else {
 								checksums.WriteString(" ADLER32:")
 							}
-							checksums.WriteString(string(e.Value))
+							checksums.Write(e.Value)
 						}
 					}
 					if checksums.Len() > 13 {

internal/http/services/owncloud/ocdav/proppatch.go

@@ -19,6 +19,7 @@
 package ocdav
 
 import (
+	"bytes"
 	"context"
 	"encoding/xml"
 	"fmt"
@@ -309,12 +310,12 @@ func (s *svc) handleProppatchResponse(ctx context.Context, w http.ResponseWriter
 	w.Header().Set(net.HeaderDav, "1, 3, extended-mkcol")
 	w.Header().Set(net.HeaderContentType, "application/xml; charset=utf-8")
 	w.WriteHeader(http.StatusMultiStatus)
-	if _, err := w.Write([]byte(propRes)); err != nil {
+	if _, err := w.Write(propRes); err != nil {
 		log.Err(err).Msg("error writing response")
 	}
 }
 
-func (s *svc) formatProppatchResponse(ctx context.Context, acceptedProps []xml.Name, removedProps []xml.Name, ref string) (string, error) {
+func (s *svc) formatProppatchResponse(ctx context.Context, acceptedProps []xml.Name, removedProps []xml.Name, ref string) ([]byte, error) {
 	responses := make([]propfind.ResponseXML, 0, 1)
 	response := propfind.ResponseXML{
 		Href:     net.EncodePath(ref),
@@ -346,13 +347,15 @@ func (s *svc) formatProppatchResponse(ctx context.Context, acceptedProps []xml.N
 	responses = append(responses, response)
 	responsesXML, err := xml.Marshal(&responses)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
-	msg := `<?xml version="1.0" encoding="utf-8"?><d:multistatus xmlns:d="DAV:" `
-	msg += `xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">`
-	msg += string(responsesXML) + `</d:multistatus>`
-	return msg, nil
+	var buf bytes.Buffer
+	buf.WriteString(`<?xml version="1.0" encoding="utf-8"?><d:multistatus xmlns:d="DAV:" `)
+	buf.WriteString(`xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">`)
+	buf.Write(responsesXML)
+	buf.WriteString(`</d:multistatus>`)
+	return buf.Bytes(), nil
 }
 
 func (s *svc) isBooleanProperty(prop string) bool {

internal/http/services/owncloud/ocdav/publicfile.go

@@ -122,7 +122,7 @@ func (s *svc) handlePropfindOnToken(w http.ResponseWriter, r *http.Request, ns s
 	w.Header().Set(net.HeaderDav, "1, 3, extended-mkcol")
 	w.Header().Set(net.HeaderContentType, "application/xml; charset=utf-8")
 	w.WriteHeader(http.StatusMultiStatus)
-	if _, err := w.Write([]byte(propRes)); err != nil {
+	if _, err := w.Write(propRes); err != nil {
 		sublog.Err(err).Msg("error writing response")
 	}
 }

internal/http/services/owncloud/ocdav/put.go

@@ -298,12 +298,7 @@ func (s *svc) handlePut(ctx context.Context, w http.ResponseWriter, r *http.Requ
 		return
 	}
 
-	ok, err := chunking.IsChunked(ref.Path)
-	if err != nil {
-		w.WriteHeader(http.StatusInternalServerError)
-		return
-	}
-	if ok {
+	if chunking.IsChunked(ref.Path) {
 		chunk, err := chunking.GetChunkBLOBInfo(ref.Path)
 		if err != nil {
 			w.WriteHeader(http.StatusInternalServerError)

internal/http/services/owncloud/ocdav/report.go

@@ -124,7 +124,7 @@ func (s *svc) doFilterFiles(w http.ResponseWriter, r *http.Request, ff *reportFi
 		w.Header().Set(net.HeaderDav, "1, 3, extended-mkcol")
 		w.Header().Set(net.HeaderContentType, "application/xml; charset=utf-8")
 		w.WriteHeader(http.StatusMultiStatus)
-		if _, err := w.Write([]byte(responsesXML)); err != nil {
+		if _, err := w.Write(responsesXML); err != nil {
 			log.Err(err).Msg("error writing response")
 		}
 	}

internal/http/services/owncloud/ocdav/trashbin.go

@@ -19,6 +19,7 @@
 package ocdav
 
 import (
+	"bytes"
 	"context"
 	"encoding/xml"
 	"fmt"
@@ -197,7 +198,7 @@ func (h *TrashbinHandler) listTrashbin(w http.ResponseWriter, r *http.Request, s
 		w.Header().Set(net.HeaderDav, "1, 3, extended-mkcol")
 		w.Header().Set(net.HeaderContentType, "application/xml; charset=utf-8")
 		w.WriteHeader(http.StatusMultiStatus)
-		_, err = w.Write([]byte(propRes))
+		_, err = w.Write(propRes)
 		if err != nil {
 			sublog.Error().Err(err).Msg("error writing body")
 			return
@@ -302,14 +303,14 @@ func (h *TrashbinHandler) listTrashbin(w http.ResponseWriter, r *http.Request, s
 	w.Header().Set(net.HeaderDav, "1, 3, extended-mkcol")
 	w.Header().Set(net.HeaderContentType, "application/xml; charset=utf-8")
 	w.WriteHeader(http.StatusMultiStatus)
-	_, err = w.Write([]byte(propRes))
+	_, err = w.Write(propRes)
 	if err != nil {
 		sublog.Error().Err(err).Msg("error writing body")
 		return
 	}
 }
 
-func (h *TrashbinHandler) formatTrashPropfind(ctx context.Context, s *svc, u *userpb.User, pf *propfind.XML, items []*provider.RecycleItem) (string, error) {
+func (h *TrashbinHandler) formatTrashPropfind(ctx context.Context, s *svc, u *userpb.User, pf *propfind.XML, items []*provider.RecycleItem) ([]byte, error) {
 	responses := make([]*propfind.ResponseXML, 0, len(items)+1)
 	// add trashbin dir . entry
 	responses = append(responses, &propfind.ResponseXML{
@@ -336,19 +337,21 @@ func (h *TrashbinHandler) formatTrashPropfind(ctx context.Context, s *svc, u *us
 	for i := range items {
 		res, err := h.itemToPropResponse(ctx, s, u, pf, items[i])
 		if err != nil {
-			return "", err
+			return nil, err
 		}
 		responses = append(responses, res)
 	}
 	responsesXML, err := xml.Marshal(&responses)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
-	msg := `<?xml version="1.0" encoding="utf-8"?><d:multistatus xmlns:d="DAV:" `
-	msg += `xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">`
-	msg += string(responsesXML) + `</d:multistatus>`
-	return msg, nil
+	var buf bytes.Buffer
+	buf.WriteString(`<?xml version="1.0" encoding="utf-8"?><d:multistatus xmlns:d="DAV:" `)
+	buf.WriteString(`xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">`)
+	buf.Write(responsesXML)
+	buf.WriteString(`</d:multistatus>`)
+	return buf.Bytes(), nil
 }
 
 // itemToPropResponse needs to create a listing that contains a key and destination

internal/http/services/owncloud/ocdav/versions.go

@@ -174,7 +174,7 @@ func (h *VersionsHandler) doListVersions(w http.ResponseWriter, r *http.Request,
 	w.Header().Set(net.HeaderDav, "1, 3, extended-mkcol")
 	w.Header().Set(net.HeaderContentType, "application/xml; charset=utf-8")
 	w.WriteHeader(http.StatusMultiStatus)
-	_, err = w.Write([]byte(propRes))
+	_, err = w.Write(propRes)
 	if err != nil {
 		sublog.Error().Err(err).Msg("error writing body")
 		return

internal/http/services/owncloud/ocs/response/response.go

@@ -189,7 +189,7 @@ func encodeXML(res Response) ([]byte, error) {
 		return nil, err
 	}
 	b := new(bytes.Buffer)
-	b.Write([]byte(xml.Header))
+	b.WriteString(xml.Header)
 	b.Write(marshalled)
 	return b.Bytes(), nil
 }

pkg/storage/fs/owncloudsql/upload.go

@@ -55,11 +55,7 @@ func (fs *owncloudsqlfs) Upload(ctx context.Context, ref *provider.Reference, r
 	uploadInfo := upload.(*fileUpload)
 
 	p := uploadInfo.info.Storage["InternalDestination"]
-	ok, err := chunking.IsChunked(p)
-	if err != nil {
-		return errors.Wrap(err, "owncloudsql: error checking path")
-	}
-	if ok {
+	if chunking.IsChunked(p) {
 		var assembledFile string
 		p, assembledFile, err = fs.chunkHandler.WriteChunk(p, r)
 		if err != nil {

pkg/storage/utils/chunking/chunking.go

@@ -29,10 +29,14 @@ import (
 	"strings"
 )
 
+var (
+	chunkingPathRE = regexp.MustCompile(`-chunking-\w+-[0-9]+-[0-9]+$`)
+)
+
 // IsChunked checks if a given path refers to a chunk or not
-func IsChunked(fn string) (bool, error) {
+func IsChunked(fn string) bool {
 	// FIXME: also need to check whether the OC-Chunked header is set
-	return regexp.MatchString(`-chunking-\w+-[0-9]+-[0-9]+$`, fn)
+	return chunkingPathRE.MatchString(fn)
 }
 
 // ChunkBLOBInfo stores info about a particular chunk

pkg/storage/utils/decomposedfs/upload.go

@@ -64,11 +64,7 @@ func (fs *Decomposedfs) Upload(ctx context.Context, ref *provider.Reference, r i
 	uploadInfo := upload.(*fileUpload)
 
 	p := uploadInfo.info.Storage["NodeName"]
-	ok, err := chunking.IsChunked(p) // check chunking v1
-	if err != nil {
-		return errors.Wrap(err, "Decomposedfs: error checking path")
-	}
-	if ok {
+	if chunking.IsChunked(p) { // check chunking v1
 		var assembledFile string
 		p, assembledFile, err = fs.chunkHandler.WriteChunk(p, r)
 		if err != nil {
@@ -360,10 +356,7 @@ func (fs *Decomposedfs) GetUpload(ctx context.Context, id string) (tusd.Upload,
 // This method can also handle lookups for paths which contain chunking information.
 func (fs *Decomposedfs) lookupNode(ctx context.Context, spaceRoot *node.Node, path string) (*node.Node, error) {
 	p := path
-	isChunked, err := chunking.IsChunked(path)
-	if err != nil {
-		return nil, err
-	}
+	isChunked := chunking.IsChunked(path)
 	if isChunked {
 		chunkInfo, err := chunking.GetChunkBLOBInfo(path)
 		if err != nil {

pkg/storage/utils/eosfs/upload.go

@@ -39,11 +39,7 @@ func (fs *eosfs) Upload(ctx context.Context, ref *provider.Reference, r io.ReadC
 		return errtypes.PermissionDenied("eos: cannot upload under the virtual share folder")
 	}
 
-	ok, err := chunking.IsChunked(p)
-	if err != nil {
-		return errors.Wrap(err, "eos: error checking path")
-	}
-	if ok {
+	if chunking.IsChunked(p) {
 		var assembledFile string
 		p, assembledFile, err = fs.chunkHandler.WriteChunk(p, r)
 		if err != nil {

pkg/storage/utils/localfs/upload.go

@@ -49,11 +49,7 @@ func (fs *localfs) Upload(ctx context.Context, ref *provider.Reference, r io.Rea
 	uploadInfo := upload.(*fileUpload)
 
 	p := uploadInfo.info.Storage["InternalDestination"]
-	ok, err := chunking.IsChunked(p)
-	if err != nil {
-		return errors.Wrap(err, "localfs: error checking path")
-	}
-	if ok {
+	if chunking.IsChunked(p) {
 		var assembledFile string
 		p, assembledFile, err = fs.chunkHandler.WriteChunk(p, r)
 		if err != nil {