Skip to content

Commit

Permalink
Add context to the IOException when creating temp files
Browse files Browse the repository at this point in the history
When sending an email with an attachment, we write the attachment as a
temp file. This uses the `java.io.File/createTempFile` method. If for
some reason we're not able to write that file, maybe because we don't
have sufficient permissions, that method will throw an
IOException. The IOException doesn't include the path of the file it
was trying to create, which makes it difficult to diagnose and
fix. This commit catches that original IOException and rethrows it
with the path of the temp directory that is being used.

Fixes metabase#8405
  • Loading branch information
iethree committed Sep 19, 2018
1 parent d59fdb6 commit c61da69
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 5 deletions.
20 changes: 16 additions & 4 deletions src/metabase/email/messages.clj
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@
[metabase.util
[date :as du]
[export :as export]
[i18n :refer [tru]]
[quotation :as quotation]
[urls :as url]]
[stencil
[core :as stencil]
[loader :as stencil-loader]]
[toucan.db :as db])
(:import [java.io File FileOutputStream]
(:import [java.io File FileOutputStream IOException]
java.util.Arrays))

;; Dev only -- disable template caching
Expand Down Expand Up @@ -195,10 +196,21 @@
(random-quote-context)))

(defn- create-temp-file
"Separate from `create-temp-file-or-throw` primarily so that we can simulate exceptions in tests"
[suffix]
(doto (java.io.File/createTempFile "metabase_attachment" suffix)
(doto (File/createTempFile "metabase_attachment" suffix)
.deleteOnExit))

(defn- create-temp-file-or-throw
"Tries to create a temp file, will give the users a better error message if we are unable to create the temp file"
[suffix]
(try
(create-temp-file suffix)
(catch IOException e
(let [ex-msg (str (tru "Unable to create temp file in `{0}` for email attachments "
(System/getProperty "java.io.tmpdir")))]
(throw (IOException. ex-msg e))))))

(defn- create-result-attachment-map [export-type card-name ^File attachment-file]
(let [{:keys [content-type ext]} (get export/export-formats export-type)]
{:type :attachment
Expand All @@ -214,12 +226,12 @@
:let [{:keys [rows] :as result-data} (get-in result [:result :data])]
:when (seq rows)]
[(when-let [temp-file (and (render/include-csv-attachment? card result-data)
(create-temp-file "csv"))]
(create-temp-file-or-throw "csv"))]
(export/export-to-csv-writer temp-file result)
(create-result-attachment-map "csv" card-name temp-file))

(when-let [temp-file (and (:include_xls card)
(create-temp-file "xlsx"))]
(create-temp-file-or-throw "xlsx"))]
(export/export-to-xlsx-file temp-file result)
(create-result-attachment-map "xlsx" card-name temp-file))]))))

Expand Down
24 changes: 23 additions & 1 deletion test/metabase/email/messages_test.clj
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
(ns metabase.email.messages-test
(:require [expectations :refer :all]
[metabase.email-test :refer [inbox with-fake-inbox]]
[metabase.email.messages :refer [send-new-user-email! send-password-reset-email!]]))
[metabase.email.messages :as msgs :refer [send-new-user-email! send-password-reset-email!]]
[metabase.test.util :as tu])
(:import java.io.IOException))

;; new user email
;; NOTE: we are not validating the content of the email body namely because it's got randomized elements and thus
Expand All @@ -28,3 +30,23 @@
(send-password-reset-email! "[email protected]" (not :google-auth) "test.domain.com" "http://localhost/some/url")
(-> (@inbox "[email protected]")
(update-in [0 :body 0] dissoc :content))))

(defmacro ^:private with-create-temp-failure [& body]
`(with-redefs [msgs/create-temp-file (fn [_#]
(throw (IOException. "Failed to write file")))]
~@body))

;; Test that IOException bubbles up
(expect
IOException
(with-create-temp-failure
(#'msgs/create-temp-file-or-throw "txt")))

;; When we fail to create the temp file, include the directory in the error message
(expect
(re-pattern (format "Unable to create temp file in `%s`" (System/getProperty "java.io.tmpdir")))
(try
(with-create-temp-failure
(#'msgs/create-temp-file-or-throw "txt"))
(catch Exception e
(.getMessage e))))

0 comments on commit c61da69

Please sign in to comment.