Skip to content
Snippets Groups Projects
Commit db4f3222 authored by Ryan Senior's avatar Ryan Senior
Browse files

Add dimensions/human_readable_values to the query_metadata response 

This moves values to be nested under the related fields object, and
includes dimension along with human_readable_values in the field as
well.
parent dba73da1
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/field.clj
+ 2
− 2
View file @ db4f3222
......@@ -71,8 +71,8 @@
[id]
(let [field (api/read-check Field id)]
(if-not (field-should-have-field-values? field)
{:values {} :human_readable_values {}}
(create-field-values-if-needed! field))))
{:values []}
{:values (create-field-values-if-needed! field)})))
;; TODO - not sure this is used anymore
......
src/metabase/api/table.clj
+ 11
− 1
View file @ db4f3222
......@@ -11,6 +11,7 @@
[card :refer [Card]]
[database :as database]
[field :refer [Field]]
[field-values :as fv]
[interface :as mi]
[table :as table :refer [Table]]]
[metabase.util.schema :as su]
......@@ -79,6 +80,14 @@
(sync-database/sync-table! updated-table))
updated-table)))
(defn- format-fields-for-response [resp]
(update resp :fields
(fn [fields]
(map (fn [{:keys [values] :as field}]
(if (seq values)
(update field :values fv/field-values->pairs)
field))
fields))))
(api/defendpoint GET "/:id/query_metadata"
"Get metadata about a `Table` useful for running queries.
......@@ -89,8 +98,9 @@
[id include_sensitive_fields]
{include_sensitive_fields (s/maybe su/BooleanString)}
(-> (api/read-check Table id)
(hydrate :db [:fields :target] :field_values :segments :metrics)
(hydrate :db [:fields :target :normal_values :dimensions] :segments :metrics)
(m/dissoc-in [:db :details])
format-fields-for-response
(update-in [:fields] (if (Boolean/parseBoolean include_sensitive_fields)
;; If someone passes include_sensitive_fields return hydrated :fields as-is
identity
......
src/metabase/models/field.clj
+ 14
− 1
View file @ db4f3222
......@@ -104,11 +104,24 @@
(for [field fields]
(assoc field :values (get id->field-values (:id field) [])))))
(defn with-normal-values
"Efficiently hydrate the `FieldValues` for visibility_type normal FIELDS."
{:batched-hydrate :normal_values}
[fields]
(let [field-ids (set (for [{:keys [id visibility_type]} fields
:when (= :normal visibility_type)]
id))
id->field-values (u/key-by :field_id (when (seq field-ids)
(db/select [FieldValues :id :human_readable_values :values :field_id]
:field_id [:in field-ids])))]
(for [field fields]
(assoc field :values (get id->field-values (:id field) [])))))
(defn with-dimensions
"Efficiently hydrate the `FieldValues` for a collection of FIELDS."
{:batched-hydrate :dimensions}
[fields]
(let [field-ids (set (map :id fields))
(let [field-ids (set (map :id fields))
id->dimensions (u/key-by :field_id (when (seq field-ids)
(db/select [Dimensions :id :name :field_id :human_readable_field_id :type]
:field_id [:in field-ids])))]
......
src/metabase/models/field_values.clj
+ 11
− 2
View file @ db4f3222
......@@ -59,6 +59,14 @@
:values ((resolve 'metabase.db.metadata-queries/field-distinct-values) field))
(create-field-values! field)))
(defn field-values->pairs
"Returns a list of pairs (or single element vectors if there are no
human_readable_values) for the given `FIELD-VALUES` instance"
[{:keys [values human_readable_values] :as field-values}]
(if (seq human_readable_values)
(map vector values human_readable_values)
(map vector values)))
(defn create-field-values-if-needed!
"Create `FieldValues` for a `Field` if they *should* exist but don't already exist.
Returns the existing or newly created `FieldValues` for `Field`."
......@@ -66,8 +74,9 @@
[{field-id :id :as field} & [human-readable-values]]
{:pre [(integer? field-id)]}
(when (field-should-have-field-values? field)
(or (FieldValues :field_id field-id)
(create-field-values! field human-readable-values))))
(field-values->pairs
(or (db/select-one [FieldValues :values :human_readable_values] :field_id field-id)
(create-field-values! field human-readable-values)))))
(defn save-field-values!
"Save the `FieldValues` for FIELD-ID, creating them if needed, otherwise updating them."
......
test/metabase/api/field_test.clj
+ 9
− 24
View file @ db4f3222
......@@ -149,22 +149,17 @@
;; ## GET /api/field/:id/values
;; Should return something useful for a field that has special_type :type/Category
(expect
{:field_id (id :venues :price)
:human_readable_values {}
:values [1 2 3 4]
:id (field-values-id :venues :price)}
{:values (mapv vector [1 2 3 4])}
(do
;; clear out existing human_readable_values in case they're set
(db/update! FieldValues (field-values-id :venues :price)
:human_readable_values nil)
;; now update the values via the API
(-> ((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))
(dissoc :created_at :updated_at))))
((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))))
;; Should return nothing for a field whose special_type is *not* :type/Category
(expect
{:values {}
:human_readable_values {}}
{:values []}
((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :id))))
......@@ -173,36 +168,26 @@
;; Check that we can set values
(expect
[{:status "success"}
{:field_id (id :venues :price)
:human_readable_values {:1 "$"
:2 "$$"
:3 "$$$"
:4 "$$$$"}
:values [1 2 3 4]
:id (field-values-id :venues :price)}]
{:values (mapv (fn [idx]
(vector idx [(str idx) (apply str (repeat idx \$))]))
[1 2 3 4])}]
[((user->client :crowberto) :post 200 (format "field/%d/value_map_update" (id :venues :price)) {:values_map {:1 "$"
:2 "$$"
:3 "$$$"
:4 "$$$$"}})
(-> ((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))
(dissoc :created_at :updated_at))])
((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))])
;; Check that we can unset values
(expect
[{:status "success"}
(tu/match-$ (FieldValues :field_id (id :venues :price))
{:field_id (id :venues :price)
:human_readable_values {}
:values [1 2 3 4]
:id (field-values-id :venues :price)})]
{:values (mapv vector [1 2 3 4])}]
[(do (db/update! FieldValues (:id (field->field-values :venues :price))
:human_readable_values {:1 "$" ; make sure they're set
:2 "$$"
:3 "$$$"
:4 "$$$$"})
((user->client :crowberto) :post 200 (format "field/%d/value_map_update" (id :venues :price)) {:values_map {}}))
(-> ((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))
(dissoc :created_at :updated_at))])
((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))])
;; Check that we get an error if we call value_map_update on something that isn't a category
(expect "You can only update the mapped values of a Field whose 'special_type' is 'category'/'city'/'state'/'country' or whose 'base_type' is 'type/Boolean'."
......
test/metabase/api/table_test.clj
+ 138
− 49
View file @ db4f3222
(ns metabase.api.table-test
"Tests for /api/table endpoints."
(:require [expectations :refer :all]
(:require [cheshire.core :as json]
[expectations :refer :all]
[metabase
[driver :as driver]
[http-client :as http]
......@@ -10,7 +11,9 @@
[metabase.models
[card :refer [Card]]
[database :as database :refer [Database]]
[dimensions :refer [Dimensions]]
[field :refer [Field]]
[field-values :refer [FieldValues]]
[permissions :as perms]
[permissions-group :as perms-group]
[table :refer [Table]]]
......@@ -20,7 +23,9 @@
[metabase.test.data
[dataset-definitions :as defs]
[users :refer :all]]
[toucan.hydrate :as hydrate]
[toucan
[db :as db]
[hydrate :as hydrate]]
[toucan.util.test :as tt]))
(resolve-private-vars metabase.models.table pk-field-id)
......@@ -58,7 +63,6 @@
:entity_type nil
:visibility_type nil
:db (db-details)
:field_values {}
:entity_name nil
:active true
:db_id (id)
......@@ -128,11 +132,16 @@
(perms/delete-related-permissions! (perms-group/all-users) (perms/object-path database-id))
((user->client :rasta) :get 403 (str "table/" table-id))))
(def ^:private venue-categories
(->> defs/test-data
:table-definitions
second
:rows))
;; ## GET /api/table/:id/query_metadata
(expect
(merge (table-defaults)
(match-$ (hydrate/hydrate (Table (id :categories)) :field_values)
(match-$ (Table (id :categories))
{:schema "PUBLIC"
:name "CATEGORIES"
:display_name "Categories"
......@@ -148,7 +157,9 @@
:base_type "type/BigInteger"
:fk_target_field_id $
:raw_column_id $
:last_analyzed $}))
:last_analyzed $
:dimensions ()
:values ()}))
(merge defaults (match-$ (Field (id :categories :name))
{:special_type "type/Name"
:name "NAME"
......@@ -160,13 +171,14 @@
:base_type "type/Text"
:fk_target_field_id $
:raw_column_id $
:last_analyzed $}))])
:last_analyzed $
:values venue-categories
:dimensions ()}))])
:rows 75
:updated_at $
:id (id :categories)
:raw_table_id $
:created_at $
:field_values (tu/obj->json->obj (:field_values $$))}))
:created_at $}))
((user->client :rasta) :get 200 (format "table/%d/query_metadata" (id :categories))))
......@@ -188,6 +200,13 @@
sort
vec)))
(def ^:private user-full-names
(->> defs/test-data
:table-definitions
first
:rows
(map first)))
;;; GET api/table/:id/query_metadata?include_sensitive_fields
;;; Make sure that getting the User table *does* include info about the password field, but not actual values themselves
(expect
......@@ -208,7 +227,9 @@
:visibility_type "normal"
:fk_target_field_id $
:raw_column_id $
:last_analyzed $}))
:last_analyzed $
:dimensions []
:values []}))
(merge defaults (match-$ (Field (id :users :last_login))
{:special_type nil
:name "LAST_LOGIN"
......@@ -220,7 +241,9 @@
:visibility_type "normal"
:fk_target_field_id $
:raw_column_id $
:last_analyzed $}))
:last_analyzed $
:dimensions []
:values []}))
(merge defaults (match-$ (Field (id :users :name))
{:special_type "type/Name"
:name "NAME"
......@@ -232,7 +255,9 @@
:visibility_type "normal"
:fk_target_field_id $
:raw_column_id $
:last_analyzed $}))
:last_analyzed $
:dimensions []
:values (map vector (sort user-full-names))}))
(merge defaults (match-$ (Field :table_id (id :users), :name "PASSWORD")
{:special_type "type/Category"
:name "PASSWORD"
......@@ -244,27 +269,13 @@
:visibility_type "sensitive"
:fk_target_field_id $
:raw_column_id $
:last_analyzed $}))])
:last_analyzed $
:dimensions []
:values []}))])
:rows 15
:updated_at $
:id (id :users)
:raw_table_id $
:field_values {(keyword (str (id :users :name)))
["Broen Olujimi"
"Conchúr Tihomir"
"Dwight Gresham"
"Felipinho Asklepios"
"Frans Hevel"
"Kaneonuskatew Eiran"
"Kfir Caj"
"Nils Gotam"
"Plato Yeshua"
"Quentin Sören"
"Rüstem Hebel"
"Shad Ferdynand"
"Simcha Yan"
"Spiros Teofil"
"Szymon Theutrich"]}
:created_at $}))
((user->client :rasta) :get 200 (format "table/%d/query_metadata?include_sensitive_fields=true" (id :users))))
......@@ -287,7 +298,9 @@
:base_type "type/BigInteger"
:fk_target_field_id $
:raw_column_id $
:last_analyzed $}))
:last_analyzed $
:dimensions []
:values []}))
(merge defaults (match-$ (Field (id :users :last_login))
{:special_type nil
:name "LAST_LOGIN"
......@@ -298,7 +311,9 @@
:base_type "type/DateTime"
:fk_target_field_id $
:raw_column_id $
:last_analyzed $}))
:last_analyzed $
:dimensions []
:values []}))
(merge defaults (match-$ (Field (id :users :name))
{:special_type "type/Name"
:name "NAME"
......@@ -309,27 +324,27 @@
:base_type "type/Text"
:fk_target_field_id $
:raw_column_id $
:last_analyzed $}))])
:last_analyzed $
:dimensions []
:values [["Broen Olujimi"]
["Conchúr Tihomir"]
["Dwight Gresham"]
["Felipinho Asklepios"]
["Frans Hevel"]
["Kaneonuskatew Eiran"]
["Kfir Caj"]
["Nils Gotam"]
["Plato Yeshua"]
["Quentin Sören"]
["Rüstem Hebel"]
["Shad Ferdynand"]
["Simcha Yan"]
["Spiros Teofil"]
["Szymon Theutrich"]]}))])
:rows 15
:updated_at $
:id (id :users)
:raw_table_id $
:field_values {(keyword (str (id :users :name)))
["Broen Olujimi"
"Conchúr Tihomir"
"Dwight Gresham"
"Felipinho Asklepios"
"Frans Hevel"
"Kaneonuskatew Eiran"
"Kfir Caj"
"Nils Gotam"
"Plato Yeshua"
"Quentin Sören"
"Rüstem Hebel"
"Shad Ferdynand"
"Simcha Yan"
"Spiros Teofil"
"Szymon Theutrich"]}
:created_at $}))
((user->client :rasta) :get 200 (format "table/%d/query_metadata" (id :users))))
......@@ -397,7 +412,6 @@
(test-fun "technical")
@called)))
;; ## GET /api/table/:id/fks
;; We expect a single FK from CHECKINS.USER_ID -> USERS.ID
(expect
......@@ -458,7 +472,6 @@
:created_at $}))}))}])
((user->client :rasta) :get 200 (format "table/%d/fks" (id :users))))
;; Make sure metadata for 'virtual' tables comes back as expected from GET /api/table/:id/query_metadata
(tt/expect-with-temp [Card [card {:name "Go Dubs!"
:database_id (data/id)
......@@ -504,3 +517,79 @@
(expect
[]
((user->client :crowberto) :get 200 "table/card__1000/fks"))
(defn- delete-model-instance!
"Allows deleting a row by the model instance toucan returns when
it's inserted"
[{:keys [id] :as instance}]
(db/delete! (-> instance name symbol) :id id))
(defn- call-with-data
"Takes a thunk `DATA-LOAD-FN` that returns a seq of toucan model
instances that will be deleted after `BODY-FN` finishes"
[data-load-fn body-fn]
(let [result-instances (data-load-fn)]
(try
(body-fn)
(finally
(doseq [instance result-instances]
(delete-model-instance! instance))))))
(defmacro ^:private with-data [data-load-fn & body]
`(call-with-data ~data-load-fn (fn [] ~@body)))
(defn- narrow-fields [category-names api-response]
(for [field (:fields api-response)
:when (contains? (set category-names) (:name field))]
(-> field
(select-keys [:id :table_id :name :values :dimensions])
(update :dimensions (fn [dim]
(if (map? dim)
(dissoc dim :id)
dim))))))
;; ## GET /api/table/:id/query_metadata
;; Ensure internal remapped dimensions and human_readable_values are returned
(expect
[{:table_id (id :venues)
:id (id :venues :category_id)
:name "CATEGORY_ID"
:values (map-indexed (fn [idx [category]] [idx category]) venue-categories)
:dimensions {:name "Foo", :field_id 11, :human_readable_field_id nil, :type "internal"}}
{:id (id :venues :price)
:table_id (id :venues)
:name "PRICE"
:values [[1] [2] [3] [4]]
:dimensions []}]
(with-data
(fn []
[(db/insert! Dimensions {:field_id (id :venues :category_id)
:name "Foo"
:type "internal"})
(db/insert! FieldValues {:field_id (id :venues :category_id)
:values (json/generate-string (range 0 (count venue-categories)))
:human_readable_values (json/generate-string (map first venue-categories))})])
(narrow-fields ["PRICE" "CATEGORY_ID"]
((user->client :rasta) :get 200 (format "table/%d/query_metadata" (id :venues))))))
;; ## GET /api/table/:id/query_metadata
;; Ensure FK remappings are returned
(expect
[{:table_id (id :venues)
:id (id :venues :category_id)
:name "CATEGORY_ID"
:values []
:dimensions {:name "Foo", :field_id 11, :human_readable_field_id (id :categories :name), :type "external"}}
{:id (id :venues :price)
:table_id (id :venues)
:name "PRICE"
:values [[1] [2] [3] [4]]
:dimensions []}]
(with-data
(fn []
[(db/insert! Dimensions {:field_id (id :venues :category_id)
:name "Foo"
:type "external"
:human_readable_field_id (id :categories :name)})])
(narrow-fields ["PRICE" "CATEGORY_ID"]
((user->client :rasta) :get 200 (format "table/%d/query_metadata" (id :venues))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment