Merge branch 'master' of github.com:metabase/metabase into dashboard-drill

project.clj

@@ -88,7 +88,10 @@
   :main ^:skip-aot metabase.core
   :manifest {"Liquibase-Package" "liquibase.change,liquibase.changelog,liquibase.database,liquibase.parser,liquibase.precondition,liquibase.datatype,liquibase.serializer,liquibase.sqlgenerator,liquibase.executor,liquibase.snapshot,liquibase.logging,liquibase.diff,liquibase.structure,liquibase.structurecompare,liquibase.lockservice,liquibase.sdk,liquibase.ext"}
   :target-path "target/%s"
-  :jvm-opts ["-server"                                                ; Run JVM in server mode as opposed to client -- see http://stackoverflow.com/questions/198577/real-differences-between-java-server-and-java-client for a good explanation of this
+  :jvm-opts ["-XX:MaxPermSize=256m"                                   ; give the JVM a little more PermGen space to avoid PermGen OutOfMemoryErrors
+             "-Xverify:none"                                          ; disable bytecode verification when running in dev so it starts slightly faster
+             "-XX:+CMSClassUnloadingEnabled"                          ; let Clojure's dynamically generated temporary classes be GC'ed from PermGen
+             "-XX:+UseConcMarkSweepGC"                                ; Concurrent Mark Sweep GC needs to be used for Class Unloading (above)
              "-Djava.awt.headless=true"]                              ; prevent Java icon from randomly popping up in dock when running `lein ring server`
   :javac-options ["-target" "1.7", "-source" "1.7"]
   :uberjar-name "metabase.jar"
@@ -122,10 +125,7 @@
                    :env {:mb-run-mode "dev"}
                    :jvm-opts ["-Dlogfile.path=target/log"
                               "-Xms1024m"                             ; give JVM a decent heap size to start with
-                              "-Xmx2048m"                             ; hard limit of 2GB so we stop hitting the 4GB container limit on CircleCI
-                              "-Xverify:none"                         ; disable bytecode verification when running in dev so it starts slightly faster
-                              "-XX:+CMSClassUnloadingEnabled"         ; let Clojure's dynamically generated temporary classes be GC'ed from PermGen
-                              "-XX:+UseConcMarkSweepGC"]              ; Concurrent Mark Sweep GC needs to be used for Class Unloading (above)
+                              "-Xmx2048m"]                            ; hard limit of 2GB so we stop hitting the 4GB container limit on CircleCI
                    :aot [metabase.logger]}                            ; Log appender class needs to be compiled for log4j to use it
              :reflection-warnings {:global-vars {*warn-on-reflection* true}} ; run `lein check-reflection-warnings` to check for reflection warnings
              :expectations {:injections [(require 'metabase.test-setup)]

src/metabase/driver/postgres.clj

@@ -114,8 +114,8 @@
     :seconds      (hsql/call :to_timestamp expr)
     :milliseconds (recur (hx// expr 1000) :seconds)))
 
-(defn- date-trunc [unit expr] (hsql/call :date_trunc (hx/literal unit) expr))
-(defn- extract    [unit expr] (hsql/call :extract    unit              expr))
+(defn- date-trunc [unit expr] (hsql/call :date_trunc (hx/literal unit) (hx/cast :timestamp expr)))
+(defn- extract    [unit expr] (hsql/call :extract    unit              (hx/cast :timestamp expr)))
 
 (def ^:private extract-integer (comp hx/->integer extract))
 

test/metabase/api/card_test.clj

@@ -211,11 +211,11 @@
   ((user->client :rasta) :get 200 (str "card/" (u/get-id card))))
 
 ;; Check that a user without permissions isn't allowed to fetch the card
-(tt/expect-with-temp [Database  [{database-id :id}]
-                      Table     [{table-id :id}    {:db_id database-id}]
-                      Card      [card              {:dataset_query (mbql-count-query database-id table-id)}]]
+(expect
   "You don't have permissions to do that."
-  (do
+  (tt/with-temp* [Database [{database-id :id}]
+                  Table    [{table-id :id}    {:db_id database-id}]
+                  Card     [card              {:dataset_query (mbql-count-query database-id table-id)}]]
     ;; revoke permissions for default group to this database
     (perms/delete-related-permissions! (perms-group/all-users) (perms/object-path database-id))
     ;; now a non-admin user shouldn't be able to fetch this card
@@ -480,13 +480,14 @@
         (db/delete! Card :id card-id)))))
 
 ;; Make sure we card creation fails if we try to set a `collection_id` we don't have permissions for
-(tt/expect-with-temp [Collection [collection]]
+(expect
   "You don't have permissions to do that."
-  ((user->client :rasta) :post 403 "card" {:name                   "My Cool Card"
-                                           :display                "scalar"
-                                           :dataset_query          (mbql-count-query (id) (id :venues))
-                                           :visualization_settings {:global {:title nil}}
-                                           :collection_id          (u/get-id collection)}))
+  (tt/with-temp Collection [collection]
+    ((user->client :rasta) :post 403 "card" {:name                   "My Cool Card"
+                                             :display                "scalar"
+                                             :dataset_query          (mbql-count-query (id) (id :venues))
+                                             :visualization_settings {:global {:title nil}}
+                                             :collection_id          (u/get-id collection)})))
 
 ;; Make sure we can change the `collection_id` of a Card if it's not in any collection
 (tt/expect-with-temp [Card       [card]
@@ -497,26 +498,27 @@
     (db/select-one-field :collection_id Card :id (u/get-id card))))
 
 ;; Make sure we can still change *anything* for a Card if we don't have permissions for the Collection it belongs to
-(tt/expect-with-temp [Collection [collection]
-                      Card       [card       {:collection_id (u/get-id collection)}]]
+(expect
   "You don't have permissions to do that."
-  ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:name "Number of Blueberries Consumed Per Month"}))
+  (tt/with-temp* [Collection [collection]
+                  Card       [card       {:collection_id (u/get-id collection)}]]
+    ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:name "Number of Blueberries Consumed Per Month"})))
 
 ;; Make sure that we can't change the `collection_id` of a Card if we don't have write permissions for the new collection
-(tt/expect-with-temp [Collection [original-collection]
-                      Collection [new-collection]
-                      Card       [card                {:collection_id (u/get-id original-collection)}]]
+(expect
   "You don't have permissions to do that."
-  (do
+  (tt/with-temp* [Collection [original-collection]
+                  Collection [new-collection]
+                  Card       [card                {:collection_id (u/get-id original-collection)}]]
     (perms/grant-collection-readwrite-permissions! (perms-group/all-users) original-collection)
     ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:collection_id (u/get-id new-collection)})))
 
 ;; Make sure that we can't change the `collection_id` of a Card if we don't have write permissions for the current collection
-(tt/expect-with-temp [Collection [original-collection]
-                      Collection [new-collection]
-                      Card       [card                {:collection_id (u/get-id original-collection)}]]
+(expect
   "You don't have permissions to do that."
-  (do
+  (tt/with-temp* [Collection [original-collection]
+                  Collection [new-collection]
+                  Card       [card                {:collection_id (u/get-id original-collection)}]]
     (perms/grant-collection-readwrite-permissions! (perms-group/all-users) new-collection)
     ((user->client :rasta) :put 403 (str "card/" (u/get-id card)) {:collection_id (u/get-id new-collection)})))
 
@@ -606,20 +608,22 @@
   (POST-card-collections! :crowberto 200 new-collection [card-1 card-2]))
 
 ;; Test that we can bulk remove some Cards from a collection
-(tt/expect-with-temp [Collection [collection]
-                      Card       [card-1     {:collection_id (u/get-id collection)}]
-                      Card       [card-2     {:collection_id (u/get-id collection)}]]
+(expect
   [{:status "ok"}
    [nil nil]]
-  (POST-card-collections! :crowberto 200 nil [card-1 card-2]))
+  (tt/with-temp* [Collection [collection]
+                  Card       [card-1     {:collection_id (u/get-id collection)}]
+                  Card       [card-2     {:collection_id (u/get-id collection)}]]
+    (POST-card-collections! :crowberto 200 nil [card-1 card-2])))
 
 ;; Check that we aren't allowed to move Cards if we don't have permissions for destination collection
-(tt/expect-with-temp [Collection [collection]
-                      Card       [card-1]
-                      Card       [card-2]]
+(expect
   ["You don't have permissions to do that."
    [nil nil]]
-  (POST-card-collections! :rasta 403 collection [card-1 card-2]))
+  (tt/with-temp* [Collection [collection]
+                  Card       [card-1]
+                  Card       [card-2]]
+    (POST-card-collections! :rasta 403 collection [card-1 card-2])))
 
 ;; Check that we aren't allowed to move Cards if we don't have permissions for source collection
 (tt/expect-with-temp [Collection [collection]
@@ -630,14 +634,14 @@
   (POST-card-collections! :rasta 403 nil [card-1 card-2]))
 
 ;; Check that we aren't allowed to move Cards if we don't have permissions for the Card
-(tt/expect-with-temp [Collection [collection]
-                      Database   [database]
-                      Table      [table      {:db_id (u/get-id database)}]
-                      Card       [card-1     {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]
-                      Card       [card-2     {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]]
+(expect
   ["You don't have permissions to do that."
    [nil nil]]
-  (do
+  (tt/with-temp* [Collection [collection]
+                  Database   [database]
+                  Table      [table      {:db_id (u/get-id database)}]
+                  Card       [card-1     {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]
+                  Card       [card-2     {:dataset_query (mbql-count-query (u/get-id database) (u/get-id table))}]]
     (perms/revoke-permissions! (perms-group/all-users) (u/get-id database))
     (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
     (POST-card-collections! :rasta 403 collection [card-1 card-2])))

test/metabase/api/collection_test.clj

@@ -18,27 +18,27 @@
   ((user->client :crowberto) :get 200 "collection"))
 
 ;; check that we don't see collections if we don't have permissions for them
-(tt/expect-with-temp [Collection [collection-1 {:name "Collection 1"}]
-                      Collection [collection-2 {:name "Collection 2"}]]
+(expect
   ["Collection 1"]
-  (do
+  (tt/with-temp* [Collection [collection-1 {:name "Collection 1"}]
+                  Collection [collection-2 {:name "Collection 2"}]]
     (perms/grant-collection-read-permissions! (group/all-users) collection-1)
     (map :name ((user->client :rasta) :get 200 "collection"))))
 
 ;; check that we don't see collections if they're archived
-(tt/expect-with-temp [Collection [collection-1 {:name "Archived Collection", :archived true}]
-                      Collection [collection-2 {:name "Regular Collection"}]]
+(expect
   ["Regular Collection"]
-  (do
+  (tt/with-temp* [Collection [collection-1 {:name "Archived Collection", :archived true}]
+                  Collection [collection-2 {:name "Regular Collection"}]]
     (perms/grant-collection-read-permissions! (group/all-users) collection-1)
     (perms/grant-collection-read-permissions! (group/all-users) collection-2)
     (map :name ((user->client :rasta) :get 200 "collection"))))
 
 ;; Check that if we pass `?archived=true` we instead see archived cards
-(tt/expect-with-temp [Collection [collection-1 {:name "Archived Collection", :archived true}]
-                      Collection [collection-2 {:name "Regular Collection"}]]
+(expect
   ["Archived Collection"]
-  (do
+  (tt/with-temp* [Collection [collection-1 {:name "Archived Collection", :archived true}]
+                  Collection [collection-2 {:name "Regular Collection"}]]
     (perms/grant-collection-read-permissions! (group/all-users) collection-1)
     (perms/grant-collection-read-permissions! (group/all-users) collection-2)
     (map :name ((user->client :rasta) :get 200 "collection" :archived :true))))
@@ -100,7 +100,8 @@
    {:name "My Beautiful Collection", :color "#ABCDEF"}))
 
 ;; check that non-admins aren't allowed to update a collection
-(tt/expect-with-temp [Collection [collection]]
+(expect
   "You don't have permissions to do that."
-  ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
-   {:name "My Beautiful Collection", :color "#ABCDEF"}))
+  (tt/with-temp Collection [collection]
+    ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
+     {:name "My Beautiful Collection", :color "#ABCDEF"})))

test/metabase/api/label_test.clj

@@ -9,8 +9,8 @@
 
 ;;; GET /api/label -- list all labels
 (tt/expect-with-temp [Label [{label-1-id :id} {:name "Toucan-Approved"}]
-                   Label [{label-2-id :id} {:name "non-Toucan-Approved"}]]
-  [{:id label-2-id, :name "non-Toucan-Approved", :slug "non_toucan_approved", :icon nil}  ; should be sorted by name, case-insensitive
+                      Label [{label-2-id :id} {:name "non-Toucan-Approved"}]]
+  [{:id label-2-id, :name "non-Toucan-Approved", :slug "non_toucan_approved", :icon nil} ; should be sorted by name, case-insensitive
    {:id label-1-id, :name "Toucan-Approved",     :slug "toucan_approved",     :icon nil}]
   ((user->client :rasta) :get 200, "label"))
 
@@ -27,7 +27,8 @@
   ((user->client :rasta) :put 200, (str "label/" label-id) {:name "Bird-Friendly"}))
 
 ;;; DELETE /api/label/:id -- delete a label
-(tt/expect-with-temp [Label [{label-id :id} {:name "This will make the toucan very cross!"}]]
+(expect
   nil
-  (do ((user->client :rasta) :delete 204, (str "label/" label-id))
-      (Label label-id)))
+  (tt/with-temp Label [{label-id :id} {:name "This will make the toucan very cross!"}]
+    ((user->client :rasta) :delete 204, (str "label/" label-id))
+    (Label label-id)))

test/metabase/api/pulse_test.clj

@@ -174,9 +174,9 @@
 
 
 ;; ## DELETE /api/pulse/:id
-(tt/expect-with-temp [Pulse [pulse]]
+(expect
   nil
-  (do
+  (tt/with-temp Pulse [pulse]
     ((user->client :rasta) :delete 204 (format "pulse/%d" (:id pulse)))
     (pulse/retrieve-pulse (:id pulse))))
 

test/metabase/api/segment_test.clj

@@ -372,10 +372,11 @@
 
 
 ;;; PUT /api/segment/id. Can I update a segment's name without specifying `:points_of_interest` and `:show_in_getting_started`?
-(tt/expect-with-temp [Segment [segment]]
-  :ok
-  (do ((user->client :crowberto) :put 200 (str "segment/" (u/get-id segment))
-       {:name             "Cool name"
-        :revision_message "WOW HOW COOL"
-        :definition       {}})
-      :ok))
+(expect
+  (tt/with-temp Segment [segment]
+    ;; just make sure API call doesn't barf
+    ((user->client :crowberto) :put 200 (str "segment/" (u/get-id segment))
+     {:name             "Cool name"
+      :revision_message "WOW HOW COOL"
+      :definition       {}})
+    true))

test/metabase/query_processor_test/filter_test.clj

@@ -343,3 +343,11 @@
                                           (ql/aggregation (ql/count))
                                           (ql/filter (ql/and (ql/not (ql/> $id 32))
                                                              (ql/contains $name "BBQ")))))))
+
+
+;; make sure that filtering with dates truncating to minutes works (#4632)
+(expect-with-non-timeseries-dbs [107] (first-row
+                                        (format-rows-by [int]
+                                          (data/run-query checkins
+                                            (ql/aggregation (ql/count))
+                                            (ql/filter (ql/between (ql/datetime-field $date :minute) "2015-01-01T12:30:00" "2015-05-31"))))))

test/metabase/sync_database/analyze_test.clj

 (ns metabase.sync-database.analyze-test
   (:require [clojure.string :as str]
             [expectations :refer :all]
+            [metabase
+             [driver :as driver]
+             [util :as u]]
             [metabase.db.metadata-queries :as metadata-queries]
+            [metabase.models
+             [field :refer [Field]]
+             [table :as table :refer [Table]]]
             [metabase.sync-database.analyze :refer :all]
-            [metabase.test.util :as tu]))
-
+            [metabase.test
+             [data :as data]
+             [util :as tu]]
+            [metabase.test.data.users :refer :all]
+            [toucan.db :as db]
+            [toucan.util.test :as tt]))
 
 ;; test:cardinality-and-extract-field-values
 ;; (#2332) check that if field values are long we skip over them
@@ -22,8 +32,8 @@
 
 ;;; ## mark-json-field!
 
-(tu/resolve-private-vars metabase.sync-database.analyze values-are-valid-json?)
-(tu/resolve-private-vars metabase.sync-database.analyze values-are-valid-emails?)
+(tu/resolve-private-vars metabase.sync-database.analyze
+  values-are-valid-json? values-are-valid-emails?)
 
 (def ^:const ^:private fake-values-seq-json
   "A sequence of values that should be marked is valid JSON.")
@@ -72,3 +82,85 @@
 (expect false (values-are-valid-emails? [100]))
 (expect false (values-are-valid-emails? ["true"]))
 (expect false (values-are-valid-emails? ["false"]))
+
+;; Tests to avoid analyzing hidden tables
+(defn- unanalyzed-fields-count [table]
+  (assert (pos? ;; don't let ourselves be fooled if the test passes because the table is
+           ;; totally broken or has no fields. Make sure we actually test something
+           (db/count Field :table_id (u/get-id table))))
+  (db/count Field :last_analyzed nil, :table_id (u/get-id table)))
+
+(defn- latest-sync-time [table]
+  (db/select-one-field :last_analyzed Field
+    :last_analyzed [:not= nil]
+    :table_id      (u/get-id table)
+    {:order-by [[:last_analyzed :desc]]}))
+
+(defn- set-table-visibility-type! [table visibility-type]
+  ((user->client :crowberto) :put 200 (format "table/%d" (:id table)) {:display_name    "hiddentable"
+                                                                       :entity_type     "person"
+                                                                       :visibility_type visibility-type
+                                                                       :description     "What a nice table!"}))
+
+(defn- api-sync! [table]
+  ((user->client :crowberto) :post 200 (format "database/%d/sync" (:db_id table))))
+
+(defn- analyze! [table]
+  (let [db-id (:db_id table)]
+    (analyze-data-shape-for-tables! (driver/database-id->driver db-id) {:id db-id})))
+
+;; expect all the kinds of hidden tables to stay un-analyzed through transitions and repeated syncing
+(expect
+  1
+  (tt/with-temp* [Table [table {:rows 15}]
+                  Field [field {:table_id (:id table)}]]
+    (set-table-visibility-type! table "hidden")
+    (api-sync! table)
+    (set-table-visibility-type! table "cruft")
+    (set-table-visibility-type! table "cruft")
+    (api-sync! table)
+    (set-table-visibility-type! table "technical")
+    (api-sync! table)
+    (set-table-visibility-type! table "technical")
+    (api-sync! table)
+    (api-sync! table)
+    (unanalyzed-fields-count table)))
+
+;; same test not coming through the api
+(expect
+  1
+  (tt/with-temp* [Table [table {:rows 15}]
+                  Field [field {:table_id (:id table)}]]
+    (set-table-visibility-type! table "hidden")
+    (analyze! table)
+    (set-table-visibility-type! table "cruft")
+    (set-table-visibility-type! table "cruft")
+    (analyze! table)
+    (set-table-visibility-type! table "technical")
+    (analyze! table)
+    (set-table-visibility-type! table "technical")
+    (analyze! table)
+    (analyze! table)
+    (unanalyzed-fields-count table)))
+
+;; un-hiding a table should cause it to be analyzed
+(expect
+  0
+  (tt/with-temp* [Table [table {:rows 15}]
+                  Field [field {:table_id (:id table)}]]
+    (set-table-visibility-type! table "hidden")
+    (set-table-visibility-type! table nil)
+    (unanalyzed-fields-count table)))
+
+;; re-hiding a table should not cause it to be analyzed
+(expect
+  ;; create an initially hidden table
+  (tt/with-temp* [Table [table {:rows 15, :visibility_type "hidden"}]
+                  Field [field {:table_id (:id table)}]]
+    ;; switch the table to visible (triggering a sync) and get the last sync time
+    (let [last-sync-time (do (set-table-visibility-type! table nil)
+                             (latest-sync-time table))]
+      ;; now make it hidden again
+      (set-table-visibility-type! table "hidden")
+      ;; sync time shouldn't change
+      (= last-sync-time (latest-sync-time table)))))

test/metabase/test/data.clj

@@ -9,6 +9,7 @@
              [query-processor :as qp]
              [sync-database :as sync-database]
              [util :as u]]
+            metabase.driver.h2
             [metabase.models
              [database :refer [Database]]
              [field :as field :refer [Field]]
@@ -19,6 +20,7 @@
             [metabase.test.data
              [dataset-definitions :as defs]
              [datasets :refer [*driver*]]
+             h2
              [interface :as i]]
             [schema.core :as s]
             [toucan.db :as db])