From a45f008c12bedde4daa8b0a67faf3fba8861e5df Mon Sep 17 00:00:00 2001
From: Robert Swain <rob@matrix.org>
Date: Wed, 31 May 2017 14:29:28 +0200
Subject: [PATCH] mediaapi/storage: Don't leak sql.ErrNoRows out of storage
 package

---
 .../dendrite/mediaapi/storage/storage.go      |  8 ++-
 .../dendrite/mediaapi/writers/download.go     | 56 ++++---------------
 2 files changed, 18 insertions(+), 46 deletions(-)

diff --git a/src/github.com/matrix-org/dendrite/mediaapi/storage/storage.go b/src/github.com/matrix-org/dendrite/mediaapi/storage/storage.go
index 4b86967fb..cb27ccc95 100644
--- a/src/github.com/matrix-org/dendrite/mediaapi/storage/storage.go
+++ b/src/github.com/matrix-org/dendrite/mediaapi/storage/storage.go
@@ -50,7 +50,11 @@ func (d *Database) StoreMediaMetadata(mediaMetadata *types.MediaMetadata) error
 
 // GetMediaMetadata returns metadata about media stored on this server.
 // The media could have been uploaded to this server or fetched from another server and cached here.
-// Returns sql.ErrNoRows if there is no metadata associated with this media.
+// Returns nil metadata if there is no metadata associated with this media.
 func (d *Database) GetMediaMetadata(mediaID types.MediaID, mediaOrigin gomatrixserverlib.ServerName) (*types.MediaMetadata, error) {
-	return d.statements.selectMedia(mediaID, mediaOrigin)
+	mediaMetadata, err := d.statements.selectMedia(mediaID, mediaOrigin)
+	if err != nil && err == sql.ErrNoRows {
+		return nil, nil
+	}
+	return mediaMetadata, err
 }
diff --git a/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go b/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go
index a123ae467..f1f74c4cd 100644
--- a/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go
+++ b/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go
@@ -15,7 +15,6 @@
 package writers
 
 import (
-	"database/sql"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -39,13 +38,6 @@ const mediaIDCharacters = "A-Za-z0-9_=-"
 // Note: unfortunately regex.MustCompile() cannot be assigned to a const
 var mediaIDRegex = regexp.MustCompile("[" + mediaIDCharacters + "]+")
 
-// Error types used by downloadRequest.getMediaMetadata
-// FIXME: make into types
-var (
-	errDBQuery    = fmt.Errorf("error querying database for media")
-	errDBNotFound = fmt.Errorf("media not found")
-)
-
 // downloadRequest metadata included in or derivable from an download request
 // https://matrix.org/docs/spec/client_server/r0.2.0.html#get-matrix-media-r0-download-servername-mediaid
 type downloadRequest struct {
@@ -121,15 +113,17 @@ func (r *downloadRequest) Validate() *util.JSONResponse {
 
 func (r *downloadRequest) doDownload(w http.ResponseWriter, cfg *config.MediaAPI, db *storage.Database, activeRemoteRequests *types.ActiveRemoteRequests) *util.JSONResponse {
 	// check if we have a record of the media in our database
-	mediaMetadata, err := r.getMediaMetadata(db)
-	if err == nil {
-		// If we have a record, we can respond from the local file
-		r.MediaMetadata = mediaMetadata
-		return r.respondFromLocalFile(w, cfg.AbsBasePath)
-	} else if err == errDBNotFound {
+	mediaMetadata, err := db.GetMediaMetadata(r.MediaMetadata.MediaID, r.MediaMetadata.Origin)
+	if err != nil {
+		r.Logger.WithError(err).Error("Error querying the database.")
+		return &util.JSONResponse{
+			Code: 500,
+			JSON: jsonerror.InternalServerError(),
+		}
+	}
+	if mediaMetadata == nil {
 		if r.MediaMetadata.Origin == cfg.ServerName {
 			// If we do not have a record and the origin is local, the file is not found
-			r.Logger.WithError(err).Warn("Failed to look up file in database")
 			return &util.JSONResponse{
 				Code: 404,
 				JSON: jsonerror.NotFound(fmt.Sprintf("File with media ID %q does not exist", r.MediaMetadata.MediaID)),
@@ -141,35 +135,9 @@ func (r *downloadRequest) doDownload(w http.ResponseWriter, cfg *config.MediaAPI
 			JSON: jsonerror.NotFound(fmt.Sprintf("File with media ID %q does not exist", r.MediaMetadata.MediaID)),
 		}
 	}
-	// Another error from the database
-	r.Logger.WithError(err).WithFields(log.Fields{
-		"MediaID": r.MediaMetadata.MediaID,
-		"Origin":  r.MediaMetadata.Origin,
-	}).Error("Error querying the database.")
-	return &util.JSONResponse{
-		Code: 500,
-		JSON: jsonerror.InternalServerError(),
-	}
-}
-
-// getMediaMetadata queries the database for media metadata
-func (r *downloadRequest) getMediaMetadata(db *storage.Database) (*types.MediaMetadata, error) {
-	mediaMetadata, err := db.GetMediaMetadata(r.MediaMetadata.MediaID, r.MediaMetadata.Origin)
-	if err != nil {
-		if err == sql.ErrNoRows {
-			r.Logger.WithFields(log.Fields{
-				"Origin":  r.MediaMetadata.Origin,
-				"MediaID": r.MediaMetadata.MediaID,
-			}).Info("Media not found in database.")
-			return nil, errDBNotFound
-		}
-		r.Logger.WithError(err).WithFields(log.Fields{
-			"Origin":  r.MediaMetadata.Origin,
-			"MediaID": r.MediaMetadata.MediaID,
-		}).Error("Error querying database for media.")
-		return nil, errDBQuery
-	}
-	return mediaMetadata, nil
+	// If we have a record, we can respond from the local file
+	r.MediaMetadata = mediaMetadata
+	return r.respondFromLocalFile(w, cfg.AbsBasePath)
 }
 
 // respondFromLocalFile reads a file from local storage and writes it to the http.ResponseWriter