Properly delete artifacts from s3

We have admin functionality to delete artifacts from the repo that we
use rarely. This code had a bug where it would not delete from the s3
repo. This was because the s3-client assumes that a string path that
does not end in / is a single object, and the presence of the / means we
need to delete multiple objects. The admin delete code was generating a
path that did not end in /, but assumed that the s3-client would delete
everything under that path.

This fixes that issue by generating a path with a trailing /, and
expands the admin deletion test to test against s3.

Fixes #881.

Author: tobias

src/clojars/admin.clj

@@ -45,7 +45,7 @@
 
 (defn segments->path [segments]
   (let [[group & rest] (remove nil? segments)]
-    (str/join "/" (concat [(fu/group->path group)] rest))))
+    (str (str/join "/" (concat [(fu/group->path group)] rest)) "/")))
 
 (defn help []
   (println

test/clojars/test_helper.clj

@@ -17,6 +17,7 @@
    [clojure.java.shell :as shell]
    [clojure.string :as str]
    [clojure.test :refer [is]]
+   [cognitect.aws.client.api :as aws]
    [com.stuartsierra.component :as component]
    [matcher-combinators.test])
   (:import
@@ -208,3 +209,18 @@
 (defn assert-status
   [session status]
   (is (= status (get-in session [:response :status]))))
+
+(defn real-s3-client
+  "This creates a real s3 client for testing s3-specific functionality. It
+  requires minio to running. See docker-compose.yml."
+  [bucket]
+  (let [client (s3/s3-client bucket
+                             {:credentials {:access-key-id     "fake-access-key"
+                                            :secret-access-key "fake-secret-key"}
+                              :endpoint {:protocol "http"
+                                         :hostname "localhost"
+                                         :port     9000}
+                              :region "us-east-1"})]
+    (aws/invoke (:s3 client) {:op      :CreateBucket
+                              :request {:Bucket bucket}})
+    client))

test/clojars/unit/admin_test.clj

@@ -3,6 +3,7 @@
    [clojars.admin :as admin]
    [clojars.config :refer [config]]
    [clojars.db :as db]
+   [clojars.s3 :as s3]
    [clojars.search :as search]
    [clojars.storage :as storage]
    [clojars.test-helper :as help]
@@ -12,17 +13,27 @@
 
 (def ^:dynamic *search-removals* nil)
 
-(defn with-repo-setup2
+(def ^:dynamic *s3-client* nil)
+
+;; Note: these tests exercise the s3 client end-to-end, and require minio to
+;; running. See docker-compose.yml
+
+(defn with-repo-setup
   [f]
-  (let [jar (io/file (io/resource "fake.jar"))]
+  (let [config (config)
+        jar (io/file (io/resource "fake.jar"))
+        s3-client (help/real-s3-client (get-in config [:s3 :repo-bucket]))]
     (binding [*search-removals* (atom #{})
+              *s3-client* s3-client
               admin/*db* help/*db*
               admin/*search* (reify search/Search
                                (delete! [_ group#]
                                  (swap! *search-removals* conj group#))
                                (delete! [_ group# artifact#]
                                  (swap! *search-removals* conj (format "%s/%s" group# artifact#))))
-              admin/*storage* (storage/fs-storage (:repo (config)))]
+              admin/*storage* (storage/multi-storage
+                               (storage/fs-storage (:repo config))
+                               (storage/s3-storage s3-client))]
       (db/add-user admin/*db* "testuser@example.com" "testuser" "password")
       (help/add-verified-group "testuser" "org.ham")
       (db/add-jar admin/*db* "testuser" {:group "org.ham" :name "biscuit" :version "1" :description "delete me"})
@@ -41,14 +52,14 @@
 (use-fixtures :each
   help/default-fixture
   help/with-clean-database
-  with-repo-setup2)
+  with-repo-setup)
 
 (deftest segments->path-should-work
   (are [exp given] (= exp (admin/segments->path given))
-    "a/b" ["a" "b"]
-    "a/b/c" ["a.b" "c"]
-    "a/b/c" ["a.b.c"]
-    "a/b/c" ["a.b" nil "c" nil]))
+    "a/b/" ["a" "b"]
+    "a/b/c/" ["a.b" "c"]
+    "a/b/c/" ["a.b.c"]
+    "a/b/c/" ["a.b" nil "c" nil]))
 
 (deftest backup-dir-should-work
   (with-redefs [admin/current-date-str (constantly "20160827")]
@@ -71,6 +82,13 @@
   (is (backup-exists? "org/ham" "sandwich/1/sandwich-1.jar"))
   (is (backup-exists? "org/ham" "sandwich/1/sandwich-1.pom"))
 
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/1/biscuit-1.jar")))
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/1/biscuit-1.pom")))
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/2/biscuit-2.jar")))
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/2/biscuit-2.pom")))
+  (is (not (s3/object-exists? *s3-client* "org/ham/sandwich/1/sandwich-1.jar")))
+  (is (not (s3/object-exists? *s3-client* "org/ham/sandwich/1/sandwich-1.pom")))
+
   (is (not (.exists (io/file (:repo (config)) "org/ham"))))
 
   (is (not (db/find-jar admin/*db* "org.ham" "biscuit")))
@@ -96,6 +114,13 @@
   (is  (.exists (io/file (:repo (config)) "org/ham/sandwich/1/sandwich-1.jar")))
   (is  (.exists (io/file (:repo (config)) "org/ham/sandwich/1/sandwich-1.pom")))
 
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/1/biscuit-1.jar")))
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/1/biscuit-1.pom")))
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/2/biscuit-2.jar")))
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/2/biscuit-2.pom")))
+  (is (s3/object-exists? *s3-client* "org/ham/sandwich/1/sandwich-1.jar"))
+  (is (s3/object-exists? *s3-client* "org/ham/sandwich/1/sandwich-1.pom"))
+
   (is (not (db/find-jar admin/*db* "org.ham" "biscuit")))
   (is (db/find-jar admin/*db* "org.ham" "sandwich"))
   (is (seq (db/group-activenames admin/*db* "org.ham")))
@@ -119,6 +144,13 @@
   (is  (.exists (io/file (:repo (config)) "org/ham/sandwich/1/sandwich-1.jar")))
   (is  (.exists (io/file (:repo (config)) "org/ham/sandwich/1/sandwich-1.pom")))
 
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/1/biscuit-1.jar")))
+  (is (not (s3/object-exists? *s3-client* "org/ham/biscuit/1/biscuit-1.pom")))
+  (is (s3/object-exists? *s3-client* "org/ham/biscuit/2/biscuit-2.jar"))
+  (is (s3/object-exists? *s3-client* "org/ham/biscuit/2/biscuit-2.pom"))
+  (is (s3/object-exists? *s3-client* "org/ham/sandwich/1/sandwich-1.jar"))
+  (is (s3/object-exists? *s3-client* "org/ham/sandwich/1/sandwich-1.pom"))
+
   (is (not (db/find-jar admin/*db* "org.ham" "biscuit" "1")))
   (is (db/find-jar admin/*db* "org.ham" "biscuit" "2"))
   (is (db/find-jar admin/*db* "org.ham" "sandwich"))

test/clojars/unit/s3_test.clj

@@ -1,6 +1,7 @@
 (ns clojars.unit.s3-test
   (:require
    [clojars.s3 :as s3]
+   [clojars.test-helper :as help]
    [clojure.java.io :as io]
    [clojure.test :refer [deftest is testing use-fixtures]]
    [cognitect.aws.client.api :as aws]
@@ -17,19 +18,7 @@
     v))
 
 (def ^:private real-s3-client
-  (memoize
-   (fn []
-     (let [bucket "testing-bucket"
-           client (s3/s3-client bucket
-                                {:credentials {:access-key-id     "fake-access-key"
-                                               :secret-access-key "fake-secret-key"}
-                                 :endpoint {:protocol "http"
-                                            :hostname "localhost"
-                                            :port     9000}
-                                 :region "us-east-1"})]
-       (aws/invoke (:s3 client) {:op      :CreateBucket
-                                 :request {:Bucket bucket}})
-       client))))
+  (memoize (fn [] (help/real-s3-client "testing-bucket"))))
 
 (def ^:private page-size 1000)