Minor fixes

src/metabase/driver/generic_sql/query_processor.clj

@@ -16,10 +16,10 @@
             [metabase.util :as u])
   (:import java.sql.Timestamp
            java.util.Date
-           (metabase.driver.query_processor.interface DateTimeField
+           (metabase.driver.query_processor.interface AgFieldRef
+                                                      DateTimeField
                                                       DateTimeValue
                                                       Field
-                                                      AgFieldRef
                                                       RelativeDateTimeValue
                                                       Value)))
 

src/metabase/driver/mongo/query_processor.clj

@@ -24,10 +24,10 @@
            (com.mongodb CommandResult DB)
            clojure.lang.PersistentArrayMap
            org.bson.types.ObjectId
-           (metabase.driver.query_processor.interface DateTimeField
+           (metabase.driver.query_processor.interface AgFieldRef
+                                                      DateTimeField
                                                       DateTimeValue
                                                       Field
-                                                      AgFieldRef
                                                       RelativeDateTimeValue
                                                       Value)))
 

src/metabase/driver/query_processor.clj

@@ -69,20 +69,31 @@
            {:ex-data data})
          additional-info))
 
+;; TODO - should this be moved to metabase.util ?
 (defn- explain-schema-validation-error
   "Return a nice error message to explain the schema validation error."
   [error]
-  (println "ERROR:" error)
   (cond
     (instance? NamedError error)      (let [nested-error (.error ^NamedError error)] ; recurse until we find the innermost nested named error, which is the reason we actually failed
                                         (if (instance? NamedError nested-error)
                                           (recur nested-error)
                                           (or (when (map? nested-error)
-                                                (when-let [nested-error (first (filter (partial instance? NamedError)
-                                                                                       (vals nested-error)))]
-                                                  (explain-schema-validation-error nested-error)))
+                                                (explain-schema-validation-error nested-error))
                                               (.name ^NamedError error))))
-    (instance? ValidationError error) (schema.utils/validation-error-explain error)))
+    (map? error)                      (first (for [e     (vals error)
+                                                   :when (or (instance? NamedError e)
+                                                             (instance? ValidationError e))
+                                                   :let  [explanation (explain-schema-validation-error e)]
+                                                   :when explanation]
+                                               explanation))
+    ;; When an exception is thrown, a ValidationError comes back like (throws? ("foreign-keys is not supported by this driver." 10))
+    ;; Extract the message if applicable
+    (instance? ValidationError error) (let [explanation (schema.utils/validation-error-explain error)]
+                                        (or (when (list? explanation)
+                                              (let [[reason [msg]] explanation]
+                                                (when (= reason 'throws?)
+                                                  msg)))
+                                            explanation))))
 
 (defn- wrap-catch-exceptions [qp]
   (fn [query]

src/metabase/driver/query_processor/expand.clj

@@ -140,7 +140,7 @@
   (i/assert-driver-supports :standard-deviation-aggregations)
   (ag-with-field :stddev f))
 
-(s/defn ^:always-validate count :- i/CountAggregation
+(s/defn ^:always-validate count :- i/Aggregation
   "Aggregation clause. Return total row count (e.g., `COUNT(*)`). If F is specified, only count rows where F is non-null (e.g. `COUNT(f)`)."
   ([]  {:aggregation-type :count})
   ([f] (ag-with-field :count f)))
@@ -213,8 +213,8 @@
 (s/defn ^:always-validate between :- BetweenFilter
   "Filter subclause. Return results where F is between MIN and MAX. MIN and MAX must be orderable, i.e. numbers or datetimes.
    This behaves like SQL `BETWEEN`, i.e. MIN and MAX are inclusive."
-  [f min max]
-  (i/map->BetweenFilter {:filter-type :between, :field (field f), :min-val (value f min), :max-val (value f max)}))
+  [f min-val max-val]
+  (i/map->BetweenFilter {:filter-type :between, :field (field f), :min-val (value f min-val), :max-val (value f max-val)}))
 
 (s/defn ^:always-validate inside :- CompoundFilter
   "Filter subclause for geo bounding. Return results where LAT-FIELD and LON-FIELD are between some set of bounding values."

src/metabase/driver/query_processor/interface.clj

@@ -73,16 +73,16 @@
 
 
 (def ^:const datetime-field-units
-  "Invalid units for a `DateTimeField`."
+  "Valid units for a `DateTimeField`."
   #{:default :minute :minute-of-hour :hour :hour-of-day :day :day-of-week :day-of-month :day-of-year
     :week :week-of-year :month :month-of-year :quarter :quarter-of-year :year})
 
 (def ^:const relative-datetime-value-units
-  "Invalid units for a `RelativeDateTimeValue`."
+  "Valid units for a `RelativeDateTimeValue`."
   #{:minute :hour :day :week :month :quarter :year})
 
-(def DatetimeFieldUnit (s/named (apply s/enum datetime-field-units)          "Invalid datetime unit for a field"))
-(def DatetimeValueUnit (s/named (apply s/enum relative-datetime-value-units) "Invalid datetime unit for a relative datetime"))
+(def DatetimeFieldUnit (s/named (apply s/enum datetime-field-units)          "Valid datetime unit for a field"))
+(def DatetimeValueUnit (s/named (apply s/enum relative-datetime-value-units) "Valid datetime unit for a relative datetime"))
 
 (defn datetime-field-unit? [unit]
   (contains? datetime-field-units (keyword unit)))
@@ -113,33 +113,32 @@
 
 ;; Replace Field IDs with these during first pass
 (s/defrecord FieldPlaceholder [field-id      :- s/Int
-                               fk-field-id   :- (s/maybe (s/both s/Int
-                                                                 (s/named (s/pred (fn [_] (or (assert-driver-supports :foreign-keys)
-                                                                                              true)))
-                                                                          "foreign-keys is not supported by this driver.")))
+                               fk-field-id   :- (s/maybe (s/constrained s/Int
+                                                                        (fn [_] (or (assert-driver-supports :foreign-keys) true))
+                                                                        "foreign-keys is not supported by this driver."))
                                datetime-unit :- (s/maybe (apply s/enum datetime-field-units))])
 
 (s/defrecord AgFieldRef [index :- s/Int]) ; e.g. 0
 
-(def FieldPlaceholderOrAgRef (s/named (s/cond-pre FieldPlaceholder AgFieldRef) "Invalid field (not a field ID or aggregate field reference)"))
+(def FieldPlaceholderOrAgRef (s/named (s/cond-pre FieldPlaceholder AgFieldRef) "Valid field (not a field ID or aggregate field reference)"))
 
 
 (s/defrecord RelativeDatetime [amount :- s/Int
                                unit   :- (s/maybe DatetimeValueUnit)])
 
-(def LiteralDatetimeString (s/both s/Str (s/named (s/pred u/date-string?)              "Invalid ISO-8601 datetime string literal")))
-(def LiteralDatetime       (s/named (s/cond-pre java.sql.Date LiteralDatetimeString)   "Invalid datetime literal (must be ISO-8601 string or java.sql.Date)"))
-(def Datetime              (s/named (s/cond-pre RelativeDatetime LiteralDatetime)      "Invalid datetime (must ISO-8601 string literal or a relative-datetime form)"))
-(def OrderableValue        (s/named (s/cond-pre s/Num Datetime)                        "Invalid orrderable value (must be number or datetime)"))
-(def AnyValue              (s/named (s/maybe (s/cond-pre s/Bool s/Str OrderableValue)) "Invalid value (must be nil, boolean, number, string, or a relative-datetime form)"))
+(def LiteralDatetimeString (s/constrained s/Str u/date-string?                         "Valid ISO-8601 datetime string literal"))
+(def LiteralDatetime       (s/named (s/cond-pre java.sql.Date LiteralDatetimeString)   "Valid datetime literal (must be ISO-8601 string or java.sql.Date)"))
+(def Datetime              (s/named (s/cond-pre RelativeDatetime LiteralDatetime)      "Valid datetime (must ISO-8601 string literal or a relative-datetime form)"))
+(def OrderableValue        (s/named (s/cond-pre s/Num Datetime)                        "Valid orderable value (must be number or datetime)"))
+(def AnyValue              (s/named (s/maybe (s/cond-pre s/Bool s/Str OrderableValue)) "Valid value (must be nil, boolean, number, string, or a relative-datetime form)"))
 
 ;; Replace values with these during first pass over Query.
 ;; Include associated Field ID so appropriate the info can be found during Field resolution
 (s/defrecord ValuePlaceholder [field-placeholder :- FieldPlaceholder
                                value             :- AnyValue])
 
-(def OrderableValuePlaceholder (s/both ValuePlaceholder {:field-placeholder s/Any, :value OrderableValue}))
-(def StringValuePlaceholder    (s/both ValuePlaceholder {:field-placeholder s/Any, :value s/Str}))
+(def OrderableValuePlaceholder (s/constrained ValuePlaceholder (comp (complement (s/checker OrderableValue)) :value) ":value must be orderable (number or datetime)"))
+(def StringValuePlaceholder    (s/constrained ValuePlaceholder (comp string? :value)                                 ":value must be a string"))
 
 ;; (def FieldOrAnyValue       (s/named (s/cond-pre FieldPlaceholder ValuePlaceholder)          "Field or value"))
 ;; (def FieldOrOrderableValue (s/named (s/cond-pre FieldPlaceholder OrderableValuePlaceholder) "Field or orderable value (number or datetime)"))
@@ -148,21 +147,19 @@
 
 ;;; # ------------------------------------------------------------ CLAUSE SCHEMAS ------------------------------------------------------------
 
-(def CountAggregation {:aggregation-type       (s/eq :count)
-                       (s/optional-key :field) FieldPlaceholder})
-
-(def OtherAggregation (s/both {:aggregation-type (s/named (s/enum :avg :cumulative-sum :distinct :stddev :sum) "Invalid aggregation type")
-                               :field            FieldPlaceholder}
-                              (s/named (s/pred (fn [{:keys [aggregation-type]}]
-                                                 (when (= aggregation-type :stddev)
-                                                   (assert-driver-supports :standard-deviation-aggregations))
-                                                 true))
-                                       "standard-deviation-aggregations is not supported by this driver.")))
-
-(def Aggregation (s/named (s/if #(= (get % :aggregation-type) :count)
-                            CountAggregation
-                            OtherAggregation)
-                          "Invalid aggregation clause."))
+(def Aggregation (s/constrained
+                  (s/constrained
+                   {:aggregation-type       (s/named (s/enum :avg :count :cumulative-sum :distinct :stddev :sum) "Valid aggregation type")
+                    (s/optional-key :field) FieldPlaceholder}
+                   (fn [{:keys [aggregation-type field]}]
+                     (or (= aggregation-type :count)
+                         field))
+                   "Missing :field.")
+                  (fn [{:keys [aggregation-type]}]
+                    (when (= aggregation-type :stddev)
+                      (assert-driver-supports :standard-deviation-aggregations))
+                    true)
+                  "standard-deviation-aggregations is not supported by this driver."))
 
 
 (s/defrecord EqualityFilter [filter-type :- (s/enum := :!=)
@@ -185,19 +182,19 @@
 (def SimpleFilter (s/cond-pre EqualityFilter ComparisonFilter BetweenFilter StringFilter))
 
 (s/defrecord CompoundFilter [compound-type :- (s/enum :and :or)
-                             subclauses    :- [(s/named (s/cond-pre SimpleFilter CompoundFilter) "Invalid filter subclause in compound (and/or) filter")]])
+                             subclauses    :- [(s/named (s/cond-pre SimpleFilter CompoundFilter) "Valid filter subclause in compound (and/or) filter")]])
 
-(def Filter (s/named (s/cond-pre SimpleFilter CompoundFilter) "Invalid filter clause"))
+(def Filter (s/named (s/cond-pre SimpleFilter CompoundFilter) "Valid filter clause"))
 
 
 (def OrderBy (s/named {:field     FieldPlaceholderOrAgRef
-                       :direction (s/named (s/enum :ascending :descending) "Invalid order-by direction")}
-                      "Invalid order-by subclause"))
+                       :direction (s/named (s/enum :ascending :descending) "Valid order-by direction")}
+                      "Valid order-by subclause"))
 
 
 (def Page (s/named {:page  s/Int
                     :items s/Int}
-                   "Invalid page clause"))
+                   "Valid page clause"))
 
 
 (def Query

src/metabase/driver/sqlite.clj

@@ -120,7 +120,7 @@
                                             #{:standard-deviation-aggregations}
                                             ;; HACK SQLite doesn't support ALTER TABLE ADD CONSTRAINT FOREIGN KEY and I don't have all day to work around this
                                             ;; so for now we'll just skip the foreign key stuff in the tests.
-                                            (when true #_(config/is-test?)
+                                            (when (config/is-test?)
                                               #{:foreign-keys})))})
   sql/ISQLDriver
   (merge (sql/ISQLDriverDefaultsMixin)

test/metabase/driver/query_processor_test.clj

@@ -1067,9 +1067,10 @@
              ["twitter"     98]
              ["yelp"        90]]
    :columns ["source.service" "count"]}
-  (->> (run-query tips
-         (ql/aggregation :count)
-         (ql/breakout $tips.source.service))
+  (->> (dataset geographical-tips
+         (run-query tips
+           (ql/aggregation :count)
+           (ql/breakout $tips.source.service)))
        :data (#(dissoc % :cols)) (format-rows-by [str int])))
 
 ;;; Nested Field in FIELDS

test/metabase/test/data.clj

@@ -46,33 +46,6 @@
   [db & body]
   `(do-with-db ~db (fn [] ~@body)))
 
-(defn do-with-dataset
-  "Bind `Database` for DATASET as the current DB and execute F.
-   DATASET is an optionally namespace-qualified *symbol*. If not namespace-qualified, `metabase.test.data.dataset-definitions` is assumed.
-
-     (do-with-dataset 'some-other-ns/some-db-def f)
-     (do-with-dataset 'sad-toucan-incidents)        ; metabase.test.data.dataset-definitions/sad-toucan-incidents"
-  [dataset f]
-  {:pre [(symbol? dataset)]}
-  (let [dataset-var (or (resolve dataset)
-                        (ns-resolve 'metabase.test.data.dataset-definitions dataset))]
-    (when-not dataset-var
-      (throw (Exception. (format "Dataset definition not found: '%s' or 'metabase.test.data.dataset-definitions/%s'" dataset dataset))))
-    (with-db (get-or-create-database! @dataset-var)
-      (f))))
-
-(defmacro dataset
-  "Convenience wrapper for `do-with-dataset`.
-   Bind `Database` for DATASET as the current DB and execute BODY.
-   DATASET is a unquoted symbol name of a dataset; if not namespace-qualified, `metabase.test.data.dataset-definitions` is assumed.
-
-     (dataset sad-toucan-incidents
-       ...)"
-  {:style/indent 1}
-  [dataset & body]
-  `(do-with-dataset '~dataset (fn [] ~@body)))
-
-
 (defn- $->id
   "Convert symbols like `$field` to `id` fn calls. Input is split into separate args by splitting the token on `.`.
    With no `.` delimiters, it is assumed we're referring to a Field belonging to TABLE-NAME, which is passed implicitly as the first arg.
@@ -237,7 +210,7 @@
   (reset! loader->loaded-db-def #{}))
 
 
-(defn -with-temp-db [^DatabaseDefinition dbdef f]
+(defn do-with-temp-db [^DatabaseDefinition dbdef f]
   (let [loader *data-loader*
         dbdef  (i/map->DatabaseDefinition (assoc dbdef :short-lived? true))]
     (swap! loader->loaded-db-def conj [loader dbdef])
@@ -262,6 +235,24 @@
                                           :aggregation  [\"count\"]
                                           :filter       [\"<\" (:id &events.timestamp) \"1765-01-01\"]}}))"
   [[db-binding ^DatabaseDefinition database-definition] & body]
-  `(-with-temp-db ~database-definition
+  `(do-with-temp-db ~database-definition
      (fn [~db-binding]
        ~@body)))
+
+(defn resolve-dbdef [symb]
+  @(or (resolve symb)
+       (ns-resolve 'metabase.test.data.dataset-definitions symb)
+       (throw (Exception. (format "Dataset definition not found: '%s' or 'metabase.test.data.dataset-definitions/%s'" symb symb)))))
+
+(defmacro dataset
+  "Bind temp `Database` for DATASET as the current DB and execute BODY.
+
+   Like `with-temp-db`, but takes an unquoted symbol naming a `DatabaseDefinition` rather than the dbef itself.
+   DATASET is optionally namespace-qualified; if not, `metabase.test.data.dataset-definitions` is assumed.
+
+     (dataset sad-toucan-incidents
+       ...)"
+  {:style/indent 1}
+  [dataset & body]
+  `(with-temp-db [_# (resolve-dbdef '~dataset)]
+     ~@body))