Skip to content

Commit

Permalink
address code smells, refactor error handling
Browse files Browse the repository at this point in the history 
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
  • Loading branch information
@butonic
butonic committed Sep 24, 2024
1 parent 8a28ec0 commit 34ea969
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 84 deletions.
3 changes: 2 additions & 1 deletion changelog/unreleased/fix-select-next-gateway-client.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ Bugfix: Always select next gateway client

We now use the gateway selector to always select the next gateway client. This ensures that we can always connect to the gateway during up- and downscaling.

https://github.com/owncloud/ocis/pull/10133
https://github.com/owncloud/ocis/pull/10141
https://github.com/owncloud/ocis/pull/10133
107 changes: 39 additions & 68 deletions services/collaboration/pkg/connector/contentconnector.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"crypto/tls"
"fmt"
"io"
"net/http"
"strconv"
Expand Down Expand Up @@ -105,13 +106,10 @@ func (c *ContentConnector) GetFile(ctx context.Context, w http.ResponseWriter) e
sResp, err := gwc.Stat(ctx, &providerv1beta1.StatRequest{
Ref: wopiContext.FileReference,
})
if err != nil {
logger.Error().Err(err).Msg("GetFile: Stat Request failed")
if err := requestFailed(logger, sResp.GetStatus(), err, "GetFile: Stat Request failed"); err != nil {
return err
}
if sResp.GetStatus().GetCode() != rpcv1beta1.Code_CODE_OK {
return NewConnectorError(500, sResp.GetStatus().GetCode().String()+" "+sResp.GetStatus().GetMessage())
}

// Initiate download request
req := &providerv1beta1.InitiateFileDownloadRequest{
Ref: wopiContext.FileReference,
Expand All @@ -125,19 +123,10 @@ func (c *ContentConnector) GetFile(ctx context.Context, w http.ResponseWriter) e
return err
}
resp, err := gwc.InitiateFileDownload(ctx, req)
if err != nil {
logger.Error().Err(err).Msg("GetFile: InitiateFileDownload failed")
if err := requestFailed(logger, resp.GetStatus(), err, "GetFile: InitiateFileDownload failed"); err != nil {
return err
}

if resp.GetStatus().GetCode() != rpcv1beta1.Code_CODE_OK {
logger.Error().
Str("StatusCode", resp.GetStatus().GetCode().String()).
Str("StatusMsg", resp.GetStatus().GetMessage()).
Msg("GetFile: InitiateFileDownload failed with wrong status")
return NewConnectorError(500, resp.GetStatus().GetCode().String()+" "+resp.GetStatus().GetMessage())
}

// Figure out the download endpoint and download token
downloadEndpoint := ""
downloadToken := ""
Expand All @@ -152,6 +141,10 @@ func (c *ContentConnector) GetFile(ctx context.Context, w http.ResponseWriter) e
}
}

logger = logger.With().
Str("Endpoint", downloadEndpoint).
Bool("HasDownloadToken", hasDownloadToken).Logger()

httpClient := http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{
Expand All @@ -164,29 +157,20 @@ func (c *ContentConnector) GetFile(ctx context.Context, w http.ResponseWriter) e
// public link downloads have the token in the download endpoint
httpReq, err := newHttpRequest(ctx, wopiContext, http.MethodGet, downloadEndpoint, downloadToken, bytes.NewReader([]byte("")))
if err != nil {
logger.Error().
Err(err).
Str("Endpoint", downloadEndpoint).
Bool("HasDownloadToken", hasDownloadToken).
Msg("GetFile: Could not create the request to the endpoint")
logger.Error().Err(err).Msg("GetFile: Could not create the request to the endpoint")
return err
}

httpResp, err := httpClient.Do(httpReq)
if err != nil {
logger.Error().
Err(err).
Str("Endpoint", downloadEndpoint).
Bool("HasDownloadToken", hasDownloadToken).
Msg("GetFile: Get request to the download endpoint failed")
logger.Error().Err(err).Msg("GetFile: Get request to the download endpoint failed")
return err
}

defer httpResp.Body.Close()

if httpResp.StatusCode != http.StatusOK {
logger.Error().
Err(err).
Int("HttpCode", httpResp.StatusCode).
Msg("GetFile: downloading the file failed")
return NewConnectorError(500, "GetFile: Downloading the file failed")
Expand Down Expand Up @@ -248,19 +232,10 @@ func (c *ContentConnector) PutFile(ctx context.Context, stream io.Reader, stream
statRes, err := gwc.Stat(ctx, &providerv1beta1.StatRequest{
Ref: wopiContext.FileReference,
})
if err != nil {
logger.Error().Err(err).Msg("PutFile: stat failed")
if err := requestFailed(logger, statRes.GetStatus(), err, "PutFile: stat failed"); err != nil {
return nil, err
}

if statRes.GetStatus().GetCode() != rpcv1beta1.Code_CODE_OK && statRes.GetStatus().GetCode() != rpcv1beta1.Code_CODE_NOT_FOUND {
logger.Error().
Str("StatusCode", statRes.GetStatus().GetCode().String()).
Str("StatusMsg", statRes.GetStatus().GetMessage()).
Msg("PutFile: stat failed with unexpected status")
return NewResponse(500), nil
}

mtime := statRes.GetInfo().GetMtime()
// If there is a lock and it mismatches, return 409
if statRes.GetInfo().GetLock() != nil && statRes.GetInfo().GetLock().GetLockId() != lockID {
Expand Down Expand Up @@ -308,19 +283,10 @@ func (c *ContentConnector) PutFile(ctx context.Context, stream io.Reader, stream
}
// Initiate the upload request
resp, err := gwc.InitiateFileUpload(ctx, req)
if err != nil {
logger.Error().Err(err).Msg("UploadHelper: InitiateFileUpload failed")
if err := requestFailed(logger, resp.GetStatus(), err, "PutFile: InitiateFileUpload failed"); err != nil {
return nil, err
}

if resp.GetStatus().GetCode() != rpcv1beta1.Code_CODE_OK {
logger.Error().
Str("StatusCode", resp.GetStatus().GetCode().String()).
Str("StatusMsg", resp.GetStatus().GetMessage()).
Msg("UploadHelper: InitiateFileUpload failed with wrong status")
return NewResponse(500), nil
}

// if the content length is greater than 0, we need to upload the content to the
// target endpoint, otherwise we're done
if streamLength > 0 {
Expand All @@ -338,6 +304,10 @@ func (c *ContentConnector) PutFile(ctx context.Context, stream io.Reader, stream
}
}

logger = logger.With().
Str("Endpoint", uploadEndpoint).
Bool("HasUploadToken", hasUploadToken).Logger()

httpClient := http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{
Expand All @@ -351,11 +321,7 @@ func (c *ContentConnector) PutFile(ctx context.Context, stream io.Reader, stream
// public link uploads have the token in the upload endpoint
httpReq, err := newHttpRequest(ctx, wopiContext, http.MethodPut, uploadEndpoint, uploadToken, stream)
if err != nil {
logger.Error().
Err(err).
Str("Endpoint", uploadEndpoint).
Bool("HasUploadToken", hasUploadToken).
Msg("UploadHelper: Could not create the request to the endpoint")
logger.Error().Err(err).Msg("UploadHelper: Could not create the request to the endpoint")
return nil, err
}
// "stream" is an *http.body and doesn't fill the httpReq.ContentLength automatically
Expand All @@ -371,22 +337,16 @@ func (c *ContentConnector) PutFile(ctx context.Context, stream io.Reader, stream

httpResp, err := httpClient.Do(httpReq)
if err != nil {
logger.Error().
Err(err).
Str("Endpoint", uploadEndpoint).
Bool("HasUploadToken", hasUploadToken).
Msg("UploadHelper: Put request to the upload endpoint failed")
logger.Error().Err(err).Msg("UploadHelper: Put request to the upload endpoint failed")
return nil, err
}
defer httpResp.Body.Close()

if httpResp.StatusCode != http.StatusOK {
logger.Error().
Str("Endpoint", uploadEndpoint).
Bool("HasUploadToken", hasUploadToken).
Int("HttpCode", httpResp.StatusCode).
Msg("UploadHelper: Put request to the upload endpoint failed with unexpected status")
return NewResponse(500), nil
return nil, NewConnectorError(500, fmt.Sprintf("unexpected status code %d from the upload endpoint", httpResp.StatusCode))
}
gwc, err = c.gws.Next()
if err != nil {
Expand All @@ -397,20 +357,31 @@ func (c *ContentConnector) PutFile(ctx context.Context, stream io.Reader, stream
statResAfter, err := gwc.Stat(ctx, &providerv1beta1.StatRequest{
Ref: wopiContext.FileReference,
})
if err != nil {
logger.Error().Err(err).Msg("PutFile: stat after upload failed")
if err := requestFailed(logger, statResAfter.GetStatus(), err, "PutFile: stat after upload failed"); err != nil {
return nil, err
}
if statResAfter.GetStatus().GetCode() != rpcv1beta1.Code_CODE_OK {
logger.Error().
Str("StatusCode", statRes.GetStatus().GetCode().String()).
Str("StatusMsg", statRes.GetStatus().GetMessage()).
Msg("PutFile: stat after upload failed with unexpected status")
return NewResponse(500), nil
}
mtime = statResAfter.GetInfo().GetMtime()
}

logger.Debug().Msg("PutFile: success")
return NewResponseWithVersion(mtime), nil
}

func requestFailed(logger zerolog.Logger, s *rpcv1beta1.Status, err error, msg string) error {
if err != nil {
logger.Error().Err(err).Msg(msg)
return err
}
if s == nil {
logger.Error().Msg(msg + ": nil status")
return NewConnectorError(500, msg+": nil status")
}
if s.GetCode() != rpcv1beta1.Code_CODE_OK {
logger.Error().
Str("StatusCode", s.GetCode().String()).
Str("StatusMsg", s.GetMessage()).
Msg(msg)
return NewConnectorError(500, s.GetCode().String()+" "+s.GetMessage())
}
return nil
}
29 changes: 14 additions & 15 deletions services/collaboration/pkg/connector/contentconnector_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,8 @@ var _ = Describe("ContentConnector", func() {
}, nil)

err := cc.GetFile(ctx, sb)
Expect(err).To(HaveOccurred())
conErr := err.(*connector.ConnectorError)
Expect(conErr.HttpCodeOut).To(Equal(500))
targetErr := connector.NewConnectorError(500, "CODE_INTERNAL Something failed")
Expect(err).To(Equal(targetErr))
})

It("Missing download endpoint", func() {
Expand Down Expand Up @@ -264,9 +263,9 @@ var _ = Describe("ContentConnector", func() {
}, nil)

response, err := cc.PutFile(ctx, reader, reader.Size(), "notARandomLockId")
Expect(err).ToNot(HaveOccurred())
Expect(response.Status).To(Equal(500))
Expect(response.Headers).To(BeNil())
targetErr := connector.NewConnectorError(500, "CODE_INTERNAL Something failed")
Expect(err).To(Equal(targetErr))
Expect(response).To(BeNil())
})

It("Mismatched lockId", func() {
Expand Down Expand Up @@ -354,9 +353,9 @@ var _ = Describe("ContentConnector", func() {
}, nil)

response, err := cc.PutFile(ctx, reader, reader.Size(), "goodAndValidLock")
Expect(err).ToNot(HaveOccurred())
Expect(response.Status).To(Equal(500))
Expect(response.Headers).To(BeNil())
targetErr := connector.NewConnectorError(500, "CODE_INTERNAL Something failed")
Expect(err).To(Equal(targetErr))
Expect(response).To(BeNil())
})

It("Empty upload successful", func() {
Expand Down Expand Up @@ -406,9 +405,9 @@ var _ = Describe("ContentConnector", func() {
}, nil)

response, err := cc.PutFile(ctx, reader, reader.Size(), "goodAndValidLock")
Expect(err).ToNot(HaveOccurred())
Expect(response.Status).To(Equal(500))
Expect(response.Headers).To(BeNil())
targetErr := connector.NewConnectorError(500, "url is missing")
Expect(err).To(Equal(targetErr))
Expect(response).To(BeNil())
})

It("upload request failed", func() {
Expand Down Expand Up @@ -438,9 +437,9 @@ var _ = Describe("ContentConnector", func() {

response, err := cc.PutFile(ctx, reader, reader.Size(), "goodAndValidLock")
Expect(srvReqHeader.Get("X-Access-Token")).To(Equal(wopiCtx.AccessToken))
Expect(err).ToNot(HaveOccurred())
Expect(response.Status).To(Equal(500))
Expect(response.Headers).To(BeNil())
targetErr := connector.NewConnectorError(500, "unexpected status code 404 from the upload endpoint")
Expect(err).To(Equal(targetErr))
Expect(response).To(BeNil())
})

It("upload request success", func() {
Expand Down

0 comments on commit 34ea969

Please sign in to comment.