Fix error handling in artifacts{,/*} pkg

- Handled a few unhandled errors (reported by gas and errcheck).

Verification:
$ gometalinter.v2 --disable-all --vendor --enable=gas --enable=errcheck  ./artifacts/...

Should return no errors.

Some errors should just be logged and are logged via standard log
library.

Additionally call to getID function and the function itself were
removed. They were not neccessary after moving ID field to ArtifactInfo
struct.

Change-Id: If0d249aa08e296c7b46bc06532fe51d835e1b5c6
Signed-off-by: Alexander Mazuruk <a.mazuruk@samsung.com>

diff --git a/artifacts/artifacts.go b/artifacts/artifacts.go
index 7f9a693..ce6ad2d 100644 (file)
--- a/artifacts/artifacts.go
+++ b/artifacts/artifacts.go
@@ -18,7 +18,9 @@
 package artifacts
 
 import (
+       "errors"
        "io/ioutil"
+       "log"
        "os"
        "path/filepath"
        "strconv"
@@ -103,11 +105,16 @@ func (s *Storage) PushArtifact(artifact weles.ArtifactDescription,
 
        err = s.downloader.Download(artifact.URI, path, ch)
        if err != nil {
-               s.db.SetStatus(weles.ArtifactStatusChange{
+               err2 := s.db.SetStatus(weles.ArtifactStatusChange{
                        Path:      path,
                        NewStatus: weles.ArtifactStatusFAILED,
                })
-               return "", err
+               if err2 != nil {
+                       return "", errors.New(
+                               "failed to download artifact: " + err.Error() +
+                                       " and failed to set artifacts status to failed: " + err2.Error())
+               }
+               return "", errors.New("failed to download artifact: " + err.Error())
        }
        return path, nil
 }
@@ -162,7 +169,13 @@ func (s *Storage) getNewPath(ad weles.ArtifactDescription) (weles.ArtifactPath,
        if err != nil {
                return "", err
        }
-       defer f.Close()
+
+       defer func() {
+               if err = f.Close(); err != nil {
+                       log.Println("failed to close file")
+                       //TODO: aalexanderr log
+               }
+       }()
        return weles.ArtifactPath(f.Name()), err
 }
 
@@ -170,7 +183,7 @@ func (s *Storage) getNewPath(ad weles.ArtifactDescription) (weles.ArtifactPath,
 // about status change.
 func (s *Storage) listenToChanges() {
        for change := range s.notifier {
-               // TODO handle errors returned by SetStatus
-               s.db.SetStatus(change)
+               // Error handled in SetStatus function.
+               _ = s.db.SetStatus(change) //nolint: gas, gosec
        }
 }

diff --git a/artifacts/database/database.go b/artifacts/database/database.go
index aab445b..3195b4b 100644 (file)
--- a/artifacts/database/database.go
+++ b/artifacts/database/database.go
@@ -20,6 +20,7 @@ package database
 import (
        "database/sql"
        "errors"
+       "log"
        "strings"
 
        "git.tizen.org/tools/weles"
@@ -41,7 +42,7 @@ func (aDB *ArtifactDB) Open(dbPath string) error {
        var err error
        aDB.handler, err = sql.Open("sqlite3", dbPath)
        if err != nil {
-               return err
+               return errors.New(dbOpenFail + err.Error())
        }
 
        aDB.dbmap = &gorp.DbMap{Db: aDB.handler, Dialect: gorp.SqliteDialect{}}
@@ -180,8 +181,7 @@ func (aDB *ArtifactDB) Filter(filter weles.ArtifactFilter, sorter weles.Artifact
        // Thats why it's done with the use prepareQuery.
        trans, err := aDB.dbmap.Begin()
        if err != nil {
-               return nil, weles.ListInfo{},
-                       errors.New("Failed to open transaction while filtering " + err.Error())
+               return nil, weles.ListInfo{}, errors.New(whileFilter + dbTransOpenFail + err.Error())
        }
        queryForTotal, argsForTotal := prepareQuery(filter, sorter, paginator, true, false, 0)
        queryForRemaining, argsForRemaining := prepareQuery(filter, sorter, paginator, false, true, 0)
@@ -189,12 +189,12 @@ func (aDB *ArtifactDB) Filter(filter weles.ArtifactFilter, sorter weles.Artifact
 
        rr, err = aDB.dbmap.SelectInt(queryForRemaining, argsForRemaining...)
        if err != nil {
-               return nil, weles.ListInfo{}, errors.New("Failed to get remaining records " + err.Error())
+               return nil, weles.ListInfo{}, errors.New(whileFilter + dbRemainingFail + err.Error())
        }
 
        tr, err = aDB.dbmap.SelectInt(queryForTotal, argsForTotal...)
        if err != nil {
-               return nil, weles.ListInfo{}, errors.New("Failed to get total records " + err.Error())
+               return nil, weles.ListInfo{}, errors.New(whileFilter + dbTotalFail + err.Error())
        }
        // TODO: refactor this file. below is to ignore pagination object when pagination is turned off.
        if paginator.Limit == 0 {
@@ -209,11 +209,12 @@ func (aDB *ArtifactDB) Filter(filter weles.ArtifactFilter, sorter weles.Artifact
        queryForData, argsForData := prepareQuery(filter, sorter, paginator, false, false, offset)
        _, err = aDB.dbmap.Select(&results, queryForData, argsForData...)
        if err != nil {
-               return nil, weles.ListInfo{}, err
+               return nil, weles.ListInfo{}, errors.New(whileFilter + dbArtifactInfoFail + err.Error())
        }
        if err := trans.Commit(); err != nil {
-               return nil, weles.ListInfo{},
-                       errors.New("Failed to commit transaction while filtering " + err.Error())
+               return nil,
+                       weles.ListInfo{},
+                       errors.New(whileFilter + dbTransCommitFail + err.Error())
 
        }
        return results,
@@ -252,29 +253,18 @@ func (aDB *ArtifactDB) Select(arg interface{}) (artifacts []weles.ArtifactInfo,
        return results, nil
 }
 
-// getID fetches ID of an artifact with provided path.
-func (aDB *ArtifactDB) getID(path weles.ArtifactPath) (int64, error) {
-       res, err := aDB.dbmap.SelectInt("select ID from artifacts where Path=?", path)
-       if err != nil {
-               return 0, err
-       }
-       return res, nil
-}
-
 // SetStatus changes artifact's status in ArtifactDB.
 func (aDB *ArtifactDB) SetStatus(change weles.ArtifactStatusChange) error {
        ai, err := aDB.SelectPath(change.Path)
        if err != nil {
-               return err
+               log.Println("failed to retrieve artifact based on its path: " + err.Error())
+               return err //TODO: aalexanderr - log  error and continue
        }
 
-       id, err := aDB.getID(ai.Path)
-       if err != nil {
-               return err
-       }
-       ai.ID = id
-
        ai.Status = change.NewStatus
-       _, err = aDB.dbmap.Update(&ai)
+       if _, err = aDB.dbmap.Update(&ai); err != nil {
+               log.Println("failed to update database" + err.Error())
+               // TODO: aalexanderr - log critical, stop weles gracefully
+       }
        return err
 }

diff --git a/artifacts/database/errors.go b/artifacts/database/errors.go
index 7f2d8a4..e507305 100644 (file)
--- a/artifacts/database/errors.go
+++ b/artifacts/database/errors.go
@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2017 Samsung Electronics Co., Ltd All Rights Reserved
+ *  Copyright (c) 2017-2018 Samsung Electronics Co., Ltd All Rights Reserved
  *
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -27,3 +27,13 @@ var (
        // ArtifactDB's Select().
        ErrUnsupportedQueryType = errors.New("unsupported argument type")
 )
+
+const (
+       dbOpenFail         = "failed to open artifacts database: "
+       whileFilter        = "while filtering, "
+       dbTransOpenFail    = "failed to open transaction with artifacts database: "
+       dbTransCommitFail  = "failed to commit transaction to artifacts database: "
+       dbRemainingFail    = "failed to get remaining records count: "
+       dbTotalFail        = "failed to get total records count: "
+       dbArtifactInfoFail = "failed to get ArtifactInfo records: "
+)

diff --git a/artifacts/downloader/downloader.go b/artifacts/downloader/downloader.go
index 65b8a44..3316fe5 100644 (file)
--- a/artifacts/downloader/downloader.go
+++ b/artifacts/downloader/downloader.go
@@ -20,6 +20,7 @@ package downloader
 import (
        "fmt"
        "io"
+       "log"
        "net/http"
        "os"
        "sync"
@@ -76,17 +77,28 @@ func (d *Downloader) getData(URI weles.ArtifactURI, path weles.ArtifactPath) err
        if err != nil {
                return err
        }
-       defer resp.Body.Close()
 
+       defer func() {
+               if erro := resp.Body.Close(); erro != nil {
+                       log.Println("failed to close response body after downloading file from: "+string(URI),
+                               erro.Error())
+               }
+       }()
        if resp.StatusCode != http.StatusOK {
-               return fmt.Errorf("server error %v %v", URI, resp.Status)
+               return fmt.Errorf("while downloading: %v server returned %v status code, expected 200 ",
+                       URI, resp.Status)
        }
 
        file, err := os.Create(string(path))
        if err != nil {
                return err
        }
-       defer file.Close()
+
+       defer func() {
+               if erro := file.Close(); erro != nil {
+                       log.Println("failed to close file: " + string(path) + " " + erro.Error())
+               }
+       }()
 
        _, err = io.Copy(file, resp.Body)
        return err
@@ -111,7 +123,9 @@ func (d *Downloader) download(URI weles.ArtifactURI, path weles.ArtifactPath,
 
        err := d.getData(URI, path)
        if err != nil {
-               os.Remove(string(path))
+               if err = os.Remove(string(path)); err != nil {
+                       log.Println("failed to remove an artifact: ", path, " due to: "+err.Error())
+               }
                change.NewStatus = weles.ArtifactStatusFAILED
        } else {
                change.NewStatus = weles.ArtifactStatusREADY