From dcf306e50cdb45b2e29fe8e739abef8df45f4081 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Tue, 5 Jul 2022 14:07:28 +0700
Subject: [PATCH] Store Sandboxed FieldValues (#23435)
* Store Sandboxed FieldValues
* locked-filter -> linked-filter
* resolve Noah and Braden comments
* Ensure current code does not confuse with the new added FieldValues type (#23601)
* makes sure the old code returns the correct FieldValues after adding new FieldValues types
* use threading macro to make at test easier to read
* Job to clean expired advanced field values (#23437)
* add a clean job
* merge upstream
* use java-time for max-age var
* simplify a reduce function and update some tests to make it easier to understand
---
.../sandbox/models/params/field_values.clj | 97 ++++----
.../sandbox/api/field_test.clj | 76 +++++--
resources/migrations/000_migrations.yaml | 40 ++++
src/metabase/api/field.clj | 8 +-
src/metabase/models/field.clj | 18 +-
src/metabase/models/field_values.clj | 209 +++++++++++++-----
src/metabase/models/params/field_values.clj | 29 +--
src/metabase/sync/field_values.clj | 67 +++++-
test/metabase/models/field_values_test.clj | 72 +++++-
test/metabase/models/on_demand_test.clj | 2 +-
.../models/params/chain_filter_test.clj | 2 +-
test/metabase/sync/field_values_test.clj | 84 ++++++-
12 files changed, 517 insertions(+), 187 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj b/enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj
index d4b9a34056c..b6c1f453c46 100644
--- a/enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj
+++ b/enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj
@@ -1,48 +1,47 @@
(ns metabase-enterprise.sandbox.models.params.field-values
- (:require [clojure.core.memoize :as memoize]
- [metabase-enterprise.sandbox.api.table :as table]
+ (:require [metabase-enterprise.sandbox.api.table :as table]
[metabase.api.common :as api]
- [metabase.db.connection :as mdb.connection]
- [metabase.models.field :as field :refer [Field]]
+ [metabase.models.field :as field]
[metabase.models.field-values :as field-values :refer [FieldValues]]
[metabase.models.params.field-values :as params.field-values]
[metabase.public-settings.premium-features :refer [defenterprise]]
- [metabase.util :as u]
- [toucan.db :as db]
- [toucan.hydrate :refer [hydrate]]))
+ [toucan.db :as db]))
(comment api/keep-me)
-(comment mdb.connection/keep-me) ; used for [[memoize/ttl]]
-(def ^:private ^{:arglist '([last-updated field])} fetch-sandboxed-field-values*
- (memoize/ttl
- ;; use a custom key fn for memoization. The custom key includes current User ID and a hash of their permissions
- ;; set, so we cache per-User (and so changes to that User's permissions will result in a cache miss), and Field ID
- ;; instead of an entire Field object (so maps with slightly different keys are still considered equal)
- ^{::memoize/args-fn (fn [[updated-at field]]
- [(mdb.connection/unique-identifier)
- api/*current-user-id*
- (hash @api/*current-user-permissions-set*)
- updated-at
- (u/the-id field)])}
- (fn [_ field]
- (let [{:keys [values has_more_values]} (field-values/distinct-values field)]
- {:values values
- :field_id (u/the-id field)
- :has_more_values (boolean has_more_values)}))
- ;; TODO -- shouldn't we return sandboxed human-readable values as well??
- ;;
- ;; Expire entries older than 30 days so we don't have entries for users and/or fields that
- ;; no longer exists hanging around.
- ;; (`clojure.core.cache/TTLCacheQ` (which `memoize` uses underneath) evicts all stale entries on
- ;; every cache miss)
- :ttl/threshold (* 1000 60 60 24 30)))
-
-(defn- fetch-sandboxed-field-values
- [field]
- (fetch-sandboxed-field-values*
- (db/select-one-field :updated_at FieldValues :field_id (u/the-id field))
- field))
+(defn- create-sandboxed-field-values
+ [field hash-key]
+ (when-let [{:keys [values has_more_values]} (field-values/distinct-values field)]
+ (let [;; If the full FieldValues of this field has human-readable-values, fix it with the sandboxed values
+ human-readable-values (field-values/fixup-human-readable-values
+ (db/select-one FieldValues
+ :field_id (:id field)
+ :type :full)
+ values)]
+ (db/insert! FieldValues
+ :field_id (:id field)
+ :type :sandbox
+ :hash_key hash-key
+ :has_more_values has_more_values
+ :human_readable_values human-readable-values
+ :values values))))
+
+(defn- get-or-create-sandboxed-field-values!
+ "Returns a sandboxed FieldValues for a field if exists, otherwise try to create one."
+ [field user-id user-permissions-set]
+ (let [hash-key (field-values/hash-key-for-sandbox (:id field) user-id user-permissions-set)
+ fv (or (FieldValues :field_id (:id field)
+ :type :sandbox
+ :hash_key hash-key)
+ (create-sandboxed-field-values field hash-key))]
+ (cond
+ (nil? fv) nil
+
+ ;; If it's expired, delete then try to re-create it
+ (field-values/advanced-field-values-expired? fv) (do
+ (db/delete! FieldValues :id (:id fv))
+ (recur field user-id user-permissions-set))
+ :else fv)))
(defn- field-is-sandboxed?
[{:keys [table], :as field}]
@@ -51,33 +50,11 @@
;; back to fetching it manually with `field/table`
(table/only-segmented-perms? (or table (field/table field))))
-(defenterprise field-id->field-values-for-current-user*
- "Fetch *existing* FieldValues for a sequence of `field-ids` for the current User. Values are returned as a map of
-
- {field-id FieldValues-instance}
-
- Returns `nil` if `field-ids` is empty of no matching FieldValues exist."
- :feature :sandboxes
- [field-ids]
- (let [fields (when (seq field-ids)
- (hydrate (db/select Field :id [:in (set field-ids)]) :table))
- {unsandboxed-fields false
- sandboxed-fields true} (group-by (comp boolean field-is-sandboxed?) fields)]
- (merge
- ;; use the normal OSS batched implementation for any Fields that aren't subject to sandboxing.
- (when (seq unsandboxed-fields)
- (params.field-values/default-field-id->field-values-for-current-user
- (set (map u/the-id unsandboxed-fields))))
- ;; for sandboxed fields, fetch the sandboxed values individually.
- (into {} (for [{field-id :id, :as field} sandboxed-fields]
- [field-id (fetch-sandboxed-field-values field)])))))
-
(defenterprise get-or-create-field-values-for-current-user!*
"Fetch cached FieldValues for a `field`, creating them if needed if the Field should have FieldValues. These
should be filtered as appropriate for the current User (currently this only applies to the EE impl)."
:feature :sandboxes
[field]
(if (field-is-sandboxed? field)
- ;; if sandboxing is in effect we never actually "save" the FieldValues the way the OSS/non-sandboxed impl does.
- (fetch-sandboxed-field-values field)
+ (get-or-create-sandboxed-field-values! field api/*current-user-id* @api/*current-user-permissions-set*)
(params.field-values/default-get-or-create-field-values-for-current-user! field)))
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj
index 17137a42ccb..c6edcaf5b8e 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj
@@ -2,8 +2,8 @@
"Tests for special behavior of `/api/metabase/field` endpoints in the Metabase Enterprise Edition."
(:require [clojure.test :refer :all]
[metabase-enterprise.sandbox.test-util :as mt.tu]
- [metabase.models :refer [Field User]]
- [metabase.models.field-values :as field-values :refer [FieldValues]]
+ [metabase.models :refer [Field FieldValues User]]
+ [metabase.models.field-values :as field-values]
[metabase.test :as mt]
[toucan.db :as db]))
@@ -50,7 +50,7 @@
(fetch-values :rasta :price))))
(testing "Reset field values; if another User fetches them first, do I still see sandboxed values? (metabase/metaboat#128)"
- (field-values/clear-field-values! (mt/id :venues :name))
+ (field-values/clear-field-values-for-field! (mt/id :venues :name))
;; fetch Field values with an admin
(testing "Admin should see all Field values"
(is (= {:field_id (mt/id :venues :name)
@@ -84,6 +84,22 @@
(format "field/%d/values" (mt/id :venues :name)))
(update :values (partial take 3)))))))))))))))
+(deftest human-readable-values-test
+ (testing "GET /api/field/:id/values should returns correct human readable mapping if exists"
+ (mt/with-temp-copy-of-db
+ (let [field-id (mt/id :venues :price)
+ full-fv-id (db/select-one-id FieldValues :field_id field-id :type :full)]
+ (db/update! FieldValues full-fv-id
+ :human_readable_values ["$" "$$" "$$$" "$$$$"])
+ ;; sanity test without gtap
+ (is (= [[1 "$"] [2 "$$"] [3 "$$$"] [4 "$$$$"]]
+ (:values (mt/user-http-request :rasta :get 200 (format "field/%d/values" field-id)))))
+ (mt/with-gtaps {:gtaps {:venues
+ {:remappings {:cat [:variable [:field-id (mt/id :venues :category_id)]]}}}
+ :attributes {:cat 4}}
+ (is (= [[1 "$"] [3 "$$$"]]
+ (:values (mt/user-http-request :rasta :get 200 (format "field/%d/values" (mt/id :venues :price)))))))))))
+
(deftest search-test
(testing "GET /api/field/:id/search/:search-id"
(mt/with-gtaps {:gtaps {:venues
@@ -111,29 +127,55 @@
:attributes {:cat 50}}
(let [field (Field (mt/id :venues :name))]
;; Make sure FieldValues are populated
- (field-values/get-or-create-field-values! field)
+ (field-values/get-or-create-full-field-values! field)
;; Warm up the cache
- (mt/user-http-request :rasta :get 200 (str "field/" (mt/id :venues :name) "/values"))
+ (mt/user-http-request :rasta :get 200 (str "field/" (:id field) "/values"))
(testing "Do we use cached values when available?"
(with-redefs [field-values/distinct-values (fn [_] (assert false "Should not be called"))]
- (is (some? (:values (mt/user-http-request :rasta :get 200 (str "field/" (mt/id :venues :name) "/values")))))))
- (testing "Do we invalidate the cache when FieldValues change"
+ (is (some? (:values (mt/user-http-request :rasta :get 200 (str "field/" (:id field) "/values")))))
+ (is (= 1 (db/count FieldValues
+ :field_id (:id field)
+ :type :sandbox)))))
+
+ (testing "Do different users has different sandbox FieldValues"
+ (let [password (mt/random-name)]
+ (mt/with-temp User [another-user {:password password}]
+ (mt/with-gtaps-for-user another-user {:gtaps {:venues
+ {:remappings {:cat [:variable [:field-id (mt/id :venues :category_id)]]}
+ :query (mt.tu/restricted-column-query (mt/id))}}
+ :attributes {:cat 5}}
+ (mt/user-http-request another-user :get 200 (str "field/" (:id field) "/values"))
+ ;; create another one for the new user
+ (is (= 2 (db/count FieldValues
+ :field_id (:id field)
+ :type :sandbox)))))))
+
+ (testing "Do we invalidate the cache when full FieldValues change"
(try
- (let [ ;; Updating FieldValues which should invalidate the cache
- fv-id (db/select-one-id FieldValues :field_id (mt/id :venues :name))
- old-updated-at (db/select-one-field :updated_at FieldValues :field_id (mt/id :venues :name))
- new-values ["foo" "bar"]]
+ (let [;; Updating FieldValues which should invalidate the cache
+ fv-id (db/select-one-id FieldValues :field_id (:id field) :type :full)
+ new-values ["foo" "bar"]]
(testing "Sanity check: make sure FieldValues exist"
(is (some? fv-id)))
(db/update! FieldValues fv-id
- {:values new-values})
- (testing "Sanity check: make sure updated_at has been updated"
- (is (not= (db/select-one-field :updated_at FieldValues :field_id (mt/id :venues :name))
- old-updated-at)))
+ {:values new-values})
(with-redefs [field-values/distinct-values (constantly {:values new-values
:has_more_values false})]
(is (= (map vector new-values)
- (:values (mt/user-http-request :rasta :get 200 (str "field/" (mt/id :venues :name) "/values")))))))
+ (:values (mt/user-http-request :rasta :get 200 (str "field/" (:id field) "/values")))))))
(finally
;; Put everything back as it was
- (field-values/get-or-create-field-values! field)))))))
+ (field-values/get-or-create-full-field-values! field))))
+
+ (testing "When a sandbox fieldvalues expired, do we delete it then create a new one?"
+ (#'field-values/clear-advanced-field-values-for-field! field)
+ ;; make sure we have a cache
+ (mt/user-http-request :rasta :get 200 (str "field/" (:id field) "/values"))
+ (let [old-sandbox-fv-id (db/select-one-id FieldValues :field_id (:id field) :type :sandbox)]
+ (with-redefs [field-values/advanced-field-values-expired? (fn [fv]
+ (= (:id fv) old-sandbox-fv-id))]
+ (mt/user-http-request :rasta :get 200 (str "field/" (:id field) "/values"))
+ ;; did the old one get deleted?
+ (is (not (db/exists? FieldValues :id old-sandbox-fv-id)))
+ ;; make sure we created a new one
+ (is (= 1 (db/count FieldValues :field_id (:id field) :type :sandbox)))))))))
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index 2ae970821a6..21e01137029 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -11791,6 +11791,7 @@ databaseChangeLog:
nullable: true
unique: true
tableName: report_dashboardcard
+
- changeSet:
id: v44.00-023
author: qnkhuat
@@ -11907,6 +11908,45 @@ databaseChangeLog:
AND p.object = '/collection/namespace/snippets/root/'
WHERE p.object IS NULL;
+ - changeSet:
+ id: v44.00-035
+ author: qnkhuat
+ comment: Added 0.44.0. Add type to fieldvalues
+ changes:
+ - addColumn:
+ tableName: metabase_fieldvalues
+ columns:
+ - column:
+ name: type
+ type: ${text.type}
+ remarks: Type of FieldValues
+ constraints:
+ nullable: true
+ - changeSet:
+ id: v44.00-036
+ author: qnkhuat
+ comment: Added 0.44.0 - Add default value for metabase_fieldvalues.type
+ changes:
+ - addNotNullConstraint:
+ columnDataType: ${text.type}
+ columnName: type
+ defaultNullValue: 'full'
+ tableName: metabase_fieldvalues
+ - changeSet:
+ id: v44.00-037
+ author: qnkhuat
+ comment: Added 0.44.0. Add type to fieldvalues
+ changes:
+ - addColumn:
+ tableName: metabase_fieldvalues
+ columns:
+ - column:
+ name: hash_key
+ type: ${text.type}
+ remarks: Hash key for a cached fieldvalues
+ constraints:
+ nullable: true
+
- changeSet:
id: v44.00-038
author: metamben
diff --git a/src/metabase/api/field.clj b/src/metabase/api/field.clj
index 86b9cc51cc5..165f6e33b2d 100644
--- a/src/metabase/api/field.clj
+++ b/src/metabase/api/field.clj
@@ -258,6 +258,7 @@
[field-or-id value-pairs]
(let [human-readable-values? (validate-human-readable-pairs value-pairs)]
(db/insert! FieldValues
+ :type :full
:field_id (u/the-id field-or-id)
:values (map first value-pairs)
:human_readable_values (when human-readable-values?
@@ -272,7 +273,7 @@
(api/check (field-values/field-should-have-field-values? field)
[400 (str "You can only update the human readable values of a mapped values of a Field whose value of "
"`has_field_values` is `list` or whose 'base_type' is 'type/Boolean'.")])
- (if-let [field-value-id (db/select-one-id FieldValues, :field_id id)]
+ (if-let [field-value-id (db/select-one-id FieldValues, :field_id id :type :full)]
(update-field-values! field-value-id value-pairs)
(create-field-values! field value-pairs)))
{:status :success})
@@ -286,17 +287,16 @@
;; but no data perms, they should stll be able to trigger a sync of field values. This is fine because we don't
;; return any actual field values from this API. (#21764)
(binding [api/*current-user-permissions-set* (atom #{"/"})]
- (field-values/create-or-update-field-values! field)))
+ (field-values/create-or-update-full-field-values! field)))
{:status :success})
(api/defendpoint POST "/:id/discard_values"
"Discard the FieldValues belonging to this Field. Only applies to fields that have FieldValues. If this Field's
Database is set up to automatically sync FieldValues, they will be recreated during the next cycle."
[id]
- (field-values/clear-field-values! (api/write-check (Field id)))
+ (field-values/clear-field-values-for-field! (api/write-check (Field id)))
{:status :success})
-
;;; --------------------------------------------------- Searching ----------------------------------------------------
(defn- table-id [field]
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index 8a247f36901..b9b97c258be 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -219,11 +219,14 @@
instance. This only returns a single instance for each Field! Duplicates are discarded!
(select-field-id->instance [(Field 1) (Field 2)] FieldValues)
- ;; -> {1 #FieldValues{...}, 2 #FieldValues{...}}"
- [fields model]
+ ;; -> {1 #FieldValues{...}, 2 #FieldValues{...}}
+
+ (select-field-id->instance [(Field 1) (Field 2)] FieldValues :type :full)
+ -> returns Fieldvalues of type :full for fields: [(Field 1) (Field 2)] "
+ [fields model & conditions]
(let [field-ids (set (map :id fields))]
(u/key-by :field_id (when (seq field-ids)
- (db/select model :field_id [:in field-ids])))))
+ (apply db/select model :field_id [:in field-ids] conditions)))))
(defn nfc-field->parent-identifier
"Take a nested field column field corresponding to something like an inner key within a JSON column,
@@ -248,7 +251,11 @@
"Efficiently hydrate the `FieldValues` for a collection of `fields`."
{:batched-hydrate :values}
[fields]
- (let [id->field-values (select-field-id->instance fields FieldValues)]
+ ;; In 44 we added a new concept of Advanced FieldValues, so FieldValues are no longer have an one-to-one relationship
+ ;; with Field. See the doc in [[metabase.models.field-values]] for more.
+ ;; Adding an explicity filter by :type =:full for FieldValues here bc I believe this hydration does not concern
+ ;; the new Advanced FieldValues.
+ (let [id->field-values (select-field-id->instance fields FieldValues :type :full)]
(for [field fields]
(assoc field :values (get id->field-values (:id field) [])))))
@@ -257,7 +264,8 @@
{:batched-hydrate :normal_values}
[fields]
(let [id->field-values (select-field-id->instance (filter field-values/field-should-have-field-values? fields)
- [FieldValues :id :human_readable_values :values :field_id])]
+ [FieldValues :id :human_readable_values :values :field_id]
+ :type :full)]
(for [field fields]
(assoc field :values (get id->field-values (:id field) [])))))
diff --git a/src/metabase/models/field_values.clj b/src/metabase/models/field_values.clj
index b5dd8fc5e90..60908887478 100644
--- a/src/metabase/models/field_values.clj
+++ b/src/metabase/models/field_values.clj
@@ -1,8 +1,27 @@
(ns metabase.models.field-values
+ "FieldValues is used to store a cached list of values of Fields that has `has_field_values=:auto-list or :list`.
+ Check the doc in [[metabase.models.field/has-field-values-options]] for more info about `has_field_values`.
+
+ There are 2 main classes of FieldValues: Full and Advanced.
+ - Full FieldValues store a list of distinct values of a Field without any constraints.
+ - Whereas Advanced FieldValues has additional constraints:
+ - sandbox: FieldValues of a field but is sandboxed for a specific user
+ - linked-filter: FieldValues for a param that connects to a Field that is constrained by the values of other Field.
+ It's currently being used on Dashboard or Embedding, but it could be used to power any parameters that connect to a Field.
+
+ * Life cycle
+ - Full FieldValues are created by the fingerprint or scanning process.
+ Once it's created the values will be updated by the scanning process that runs daily.
+ - Advanced FieldValues are created on demand: for example the Sandbox FieldValues are created when a user with
+ sandboxed permission try to get values of a Field.
+ Normally these FieldValues will be deleted after [[advanced-field-values-max-age]] days by the scanning process.
+ But they will also be automatically deleted when the Full FieldValues of the same Field got updated."
(:require [clojure.tools.logging :as log]
+ [java-time :as t]
[metabase.models.serialization.hash :as serdes.hash]
[metabase.plugins.classloader :as classloader]
[metabase.util :as u]
+ [metabase.util.date-2 :as u.date]
[metabase.util.i18n :refer [trs tru]]
[metabase.util.schema :as su]
[schema.core :as s]
@@ -28,8 +47,24 @@
"Maximum total length for a FieldValues entry (combined length of all values for the field)."
(int (* auto-list-cardinality-threshold entry-max-length)))
+(def advanced-field-values-max-age
+ "Age of an advanced FieldValues in days.
+ After this time, these field values should be deleted by the `delete-expired-advanced-field-values` job."
+ (t/days 30))
-;; ## Entity + DB Multimethods
+(def advanced-field-values-types
+ "A class of fieldvalues that has additional constraints/filters."
+ #{:sandbox ;; are fieldvalues but filtered by sandbox permissions
+ :linked-filter}) ;; are fieldvalues but has constraints from other linked parameters on dashboard/embedding
+
+(def ^:private field-values-types
+ "All FieldValues type."
+ (into #{:full} ;; default type for fieldvalues where it contains values for a field without constraints
+ advanced-field-values-types))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Entity & Lifecycle |
+;;; +----------------------------------------------------------------------------------------------------------------+
(models/defmodel FieldValues :metabase_fieldvalues)
@@ -39,13 +74,61 @@
{:human-readable-values human-readable-values
:status-code 400}))))
-(defn- pre-insert [field-values]
- (u/prog1 field-values
- (assert-valid-human-readable-values field-values)))
+(defn- assert-valid-field-values-type
+ [{:keys [type hash_key] :as _field-values}]
+ (when type
+ (when-not (contains? field-values-types type)
+ (throw (ex-info (tru "Invalid field-values type.")
+ {:type type
+ :stauts-code 400})))
+
+ (when (and (= type :full)
+ hash_key)
+ (throw (ex-info (tru "Full FieldValues shouldn't have hash_key.")
+ {:type type
+ :hash_key hash_key
+ :status-code 400})))
+
+ (when (and (advanced-field-values-types type)
+ (empty? hash_key))
+ (throw (ex-info (tru "Advanced FieldValues requires a hash_key.")
+ {:type type
+ :status-code 400})))))
+
+(defn clear-advanced-field-values-for-field!
+ "Remove all advanced FieldValues for a `field-or-id`."
+ [field-or-id]
+ (db/delete! FieldValues :field_id (u/the-id field-or-id)
+ :type [:in advanced-field-values-types]))
+
+(defn clear-field-values-for-field!
+ "Remove all FieldValues for a `field-or-id`, including the advanced fieldvalues."
+ [field-or-id]
+ (db/delete! FieldValues :field_id (u/the-id field-or-id)))
+
+(defn- pre-insert [{:keys [field_id] :as field-values}]
+ (u/prog1 (merge {:type :full}
+ field-values)
+ (assert-valid-human-readable-values field-values)
+ (assert-valid-field-values-type field-values)
+ ;; if inserting a new full fieldvalues, make sure all the advanced field-values of this field is deleted
+ (when (= (:type <>) :full)
+ (clear-advanced-field-values-for-field! field_id))))
-(defn- pre-update [field-values]
+(defn- pre-update [{:keys [id type field_id values hash_key] :as field-values}]
(u/prog1 field-values
- (assert-valid-human-readable-values field-values)))
+ (assert-valid-human-readable-values field-values)
+ (when (or type hash_key)
+ (throw (ex-info (tru "Can't update type or hash_key for a FieldValues.")
+ {:type type
+ :hash_key hash_key
+ :status-code 400})))
+ ;; if we're updating the values of a Full FieldValues, delete all Advanced FieldValues of this field
+ (when (and values
+ (= (or type (db/select-one-field :type FieldValues :id id))
+ :full))
+ (clear-advanced-field-values-for-field! (or field_id
+ (db/select-one-field :field_id FieldValues :id id))))))
(defn- post-select [field-values]
(cond-> field-values
@@ -73,7 +156,9 @@
models/IModel
(merge models/IModelDefaults
{:properties (constantly {:timestamped? true})
- :types (constantly {:human_readable_values :json-no-keywordization, :values :json})
+ :types (constantly {:human_readable_values :json-no-keywordization
+ :values :json
+ :type :keyword})
:pre-insert pre-insert
:pre-update pre-update
:post-select post-select})
@@ -81,8 +166,9 @@
serdes.hash/IdentityHashable
{:identity-hash-fields (constantly [(serdes.hash/hydrated-hash :field)])})
-
-;; ## FieldValues Helper Functions
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Utils fns |
+;;; +----------------------------------------------------------------------------------------------------------------+
(defn field-should-have-field-values?
"Should this `field` be backed by a corresponding FieldValues object?"
@@ -106,7 +192,6 @@
(not (isa? (keyword base-type) :type/Temporal))
(#{:list :auto-list} (keyword has-field-values)))))))
-
(defn take-by-length
"Like `take` but condition by the total length of elements.
Returns a stateful transducer when no collection is provided.
@@ -135,6 +220,50 @@
(cons f (take-by-length new-length
(rest s)))))))))
+(defn fixup-human-readable-values
+ "Field values and human readable values are lists that are zipped together. If the field values have changes, the
+ human readable values will need to change too. This function reconstructs the `human_readable_values` to reflect
+ `new-values`. If a new field value is found, a string version of that is used"
+ [{old-values :values, old-hrv :human_readable_values} new-values]
+ (when (seq old-hrv)
+ (let [orig-remappings (zipmap old-values old-hrv)]
+ (map #(get orig-remappings % (str %)) new-values))))
+
+(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]}]
+ (if (seq human_readable_values)
+ (map vector values human_readable_values)
+ (map vector values)))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Advanced FieldValues |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defn advanced-field-values-expired?
+ "Checks if an advanced FieldValues expired."
+ [fv]
+ {:pre [(advanced-field-values-types (:type fv))]}
+ (u.date/older-than? (:created_at fv) advanced-field-values-max-age))
+
+(defn hash-key-for-sandbox
+ "Return a hash-key that will be used for sandboxed fieldvalues."
+ [field-id user-id user-permissions-set]
+ (str (hash [field-id
+ user-id
+ (hash user-permissions-set)])))
+
+(defn hash-key-for-linked-filters
+ "Return a hash-key that will be used for linked-filters fieldvalues."
+ [field-id constraints]
+ (str (hash [field-id
+ constraints])))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | CRUD fns |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
(defn distinct-values
"Fetch a sequence of distinct values for `field` that are below the [[total-max-length]] threshold. If the values are
past the threshold, this returns a subset of possible values values where the total length of all items is less than [[total-max-length]].
@@ -169,21 +298,14 @@
(log/error e (trs "Error fetching field values"))
nil)))
-(defn- fixup-human-readable-values
- "Field values and human readable values are lists that are zipped together. If the field values have changes, the
- human readable values will need to change too. This function reconstructs the `human_readable_values` to reflect
- `new-values`. If a new field value is found, a string version of that is used"
- [{old-values :values, old-hrv :human_readable_values} new-values]
- (when (seq old-hrv)
- (let [orig-remappings (zipmap old-values old-hrv)]
- (map #(get orig-remappings % (str %)) new-values))))
-
-(defn create-or-update-field-values!
- "Create or update the FieldValues object for `field`. If the FieldValues object already exists, then update values for
+(defn create-or-update-full-field-values!
+ "Create or update the full FieldValues object for `field`. If the FieldValues object already exists, then update values for
it; otherwise create a new FieldValues object with the newly fetched values. Returns whether the field values were
- created/updated/deleted as a result of this call."
+ created/updated/deleted as a result of this call.
+
+ Note that if the full FieldValues are create/updated/deleted, it'll delete all the Advanced FieldValues of the same `field`."
[field & [human-readable-values]]
- (let [field-values (FieldValues :field_id (u/the-id field))
+ (let [field-values (FieldValues :field_id (u/the-id field) :type :full)
{:keys [values has_more_values]} (distinct-values field)
field-name (or (:name field) (:id field))]
(cond
@@ -205,7 +327,8 @@
field-name (count values))
(trs "Switching Field to use a search widget instead."))
(db/update! 'Field (u/the-id field) :has_field_values nil)
- (db/delete! FieldValues :field_id (u/the-id field)))
+ (clear-field-values-for-field! field)
+ ::fv-deleted)
(and (= (:values field-values) values)
(= (:has_more_values field-values) has_more_values))
@@ -226,6 +349,7 @@
(do
(log/debug (trs "Storing FieldValues for Field {0}..." field-name))
(db/insert! FieldValues
+ :type :full
:field_id (u/the-id field)
:has_more_values has_more_values
:values values
@@ -235,40 +359,23 @@
;; otherwise this Field isn't eligible, so delete any FieldValues that might exist
:else
(do
- (db/delete! FieldValues :field_id (u/the-id field))
+ (clear-field-values-for-field! field)
::fv-deleted))))
-(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]}]
- (if (seq human_readable_values)
- (map vector values human_readable_values)
- (map vector values)))
-
-(defn get-or-create-field-values!
+(defn get-or-create-full-field-values!
"Create FieldValues for a `Field` if they *should* exist but don't already exist. Returns the existing or newly
created FieldValues for `Field`."
{:arglists '([field] [field human-readable-values])}
[{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)
- (when (#{::fv-created ::fv-updated} (create-or-update-field-values! field human-readable-values))
- (FieldValues :field_id field-id)))))
-
-(defn save-field-values!
- "Save the FieldValues for `field-id`, creating them if needed, otherwise updating them."
- [field-id values]
- {:pre [(integer? field-id) (coll? values)]}
- (if-let [field-values (FieldValues :field_id field-id)]
- (db/update! FieldValues (u/the-id field-values), :values values)
- (db/insert! FieldValues :field_id field-id, :values values)))
-
-(defn clear-field-values!
- "Remove the FieldValues for `field-or-id`."
- [field-or-id]
- (db/delete! FieldValues :field_id (u/the-id field-or-id)))
+ (or (FieldValues :field_id field-id :type :full)
+ (when (#{::fv-created ::fv-updated} (create-or-update-full-field-values! field human-readable-values))
+ (FieldValues :field_id field-id :type :full)))))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | On Demand |
+;;; +----------------------------------------------------------------------------------------------------------------+
(defn- table-ids->table-id->is-on-demand?
"Given a collection of `table-ids` return a map of Table ID to whether or not its Database is subject to 'On Demand'
@@ -299,4 +406,4 @@
(log/debug
(trs "Field {0} ''{1}'' should have FieldValues and belongs to a Database with On-Demand FieldValues updating."
(u/the-id field) (:name field)))
- (create-or-update-field-values! field)))))
+ (create-or-update-full-field-values! field)))))
diff --git a/src/metabase/models/params/field_values.clj b/src/metabase/models/params/field_values.clj
index d204b900426..3da3fffc8c7 100644
--- a/src/metabase/models/params/field_values.clj
+++ b/src/metabase/models/params/field_values.clj
@@ -12,7 +12,7 @@
"OSS implementation; used as a fallback for the EE implementation if the field isn't sandboxed."
[field]
(when (field-values/field-should-have-field-values? field)
- (field-values/get-or-create-field-values! field)))
+ (field-values/get-or-create-full-field-values! field)))
(defenterprise get-or-create-field-values-for-current-user!*
"Fetch cached FieldValues for a `field`, creating them if needed if the Field should have FieldValues."
@@ -26,7 +26,8 @@
(when (seq field-ids)
(not-empty
(let [field-values (db/select [FieldValues :values :human_readable_values :field_id]
- :field_id [:in (set field-ids)])
+ :field_id [:in (set field-ids)]
+ :type :full)
readable-fields (when (seq field-values)
(field/readable-fields-only (db/select [Field :id :table_id]
:id [:in (set (map :field_id field-values))])))
@@ -35,16 +36,6 @@
(filter #(contains? readable-field-ids (:field_id %)))
(u/key-by :field_id))))))
-(defenterprise field-id->field-values-for-current-user*
- "Fetch *existing* FieldValues for a sequence of `field-ids` for the current User. Values are returned as a map of
-
- {field-id FieldValues-instance}
-
- Returns `nil` if `field-ids` is empty of no matching FieldValues exist."
- metabase-enterprise.sandbox.models.params.field-values
- [field-ids]
- (default-field-id->field-values-for-current-user field-ids))
-
(defn current-user-can-fetch-field-values?
"Whether the current User has permissions to fetch FieldValues for a `field`."
[field]
@@ -52,7 +43,7 @@
(mi/can-read? field))
(defn get-or-create-field-values-for-current-user!
- "Fetch cached FieldValues for a `field`, creating them if needed if the Field should have FieldValues. These are
+ "Fetch FieldValues for a `field`, creating them if needed if the Field should have FieldValues. These are
filtered as appropriate for the current User, depending on MB version (e.g. EE sandboxing will filter these values).
If the Field has a human-readable values remapping (see documentation at the top of
`metabase.models.params.chain-filter` for an explanation of what this means), values are returned in the format
@@ -70,15 +61,5 @@
(if-let [field-values (get-or-create-field-values-for-current-user!* field)]
(-> field-values
(assoc :values (field-values/field-values->pairs field-values))
- (dissoc :human_readable_values :created_at :updated_at :id))
+ (select-keys [:values :field_id :has_more_values]))
{:values [], :field_id (u/the-id field), :has_more_values false}))
-
-(defn field-id->field-values-for-current-user
- "Fetch *existing* FieldValues for a sequence of `field-ids` for the current User. Values are returned as a map of
-
- {field-id FieldValues-instance}
-
- Returns `nil` if `field-ids` is empty of no matching FieldValues exist."
- [field-ids]
- (when (seq field-ids)
- (not-empty (field-id->field-values-for-current-user* (set field-ids)))))
diff --git a/src/metabase/sync/field_values.clj b/src/metabase/sync/field_values.clj
index 4c88a5e227a..2e0fdfc966f 100644
--- a/src/metabase/sync/field_values.clj
+++ b/src/metabase/sync/field_values.clj
@@ -1,6 +1,9 @@
(ns metabase.sync.field-values
- "Logic for updating cached FieldValues for fields in a database."
+ "Logic for updating FieldValues for fields in a database."
(:require [clojure.tools.logging :as log]
+ [java-time :as t]
+ [metabase.db :as mdb]
+ [metabase.driver.sql.query-processor :as sql.qp]
[metabase.models.field :refer [Field]]
[metabase.models.field-values :as field-values :refer [FieldValues]]
[metabase.sync.interface :as i]
@@ -15,12 +18,12 @@
(log/debug (format "Based on cardinality and/or type information, %s should no longer have field values.\n"
(sync-util/name-for-logging field))
"Deleting FieldValues...")
- (db/delete! FieldValues :field_id (u/the-id field))
+ (field-values/clear-field-values-for-field! field)
::field-values/fv-deleted))
(s/defn ^:private update-field-values-for-field! [field :- i/FieldInstance]
(log/debug (u/format-color 'green "Looking into updating FieldValues for %s" (sync-util/name-for-logging field)))
- (field-values/create-or-update-field-values! field))
+ (field-values/create-or-update-full-field-values! field))
(defn- update-field-value-stats-count [counts-map result]
(if (instance? Exception result)
@@ -35,8 +38,12 @@
counts-map)))
+(defn- table->fields-to-scan
+ [table]
+ (db/select Field :table_id (u/the-id table), :active true, :visibility_type "normal"))
+
(s/defn update-field-values-for-table!
- "Update the cached FieldValues for all Fields (as needed) for TABLE."
+ "Update the FieldValues for all Fields (as needed) for TABLE."
[table :- i/TableInstance]
(reduce (fn [fv-change-counts field]
(let [result (sync-util/with-error-handling (format "Error updating field values for %s" (sync-util/name-for-logging field))
@@ -45,23 +52,61 @@
(clear-field-values-for-field! field)))]
(update-field-value-stats-count fv-change-counts result)))
{:errors 0, :created 0, :updated 0, :deleted 0}
- (db/select Field :table_id (u/the-id table), :active true, :visibility_type "normal")))
+ (table->fields-to-scan table)))
(s/defn ^:private update-field-values-for-database!
- [database :- i/DatabaseInstance]
- (apply merge-with + (map update-field-values-for-table! (sync-util/db->sync-tables database))))
+ [_database :- i/DatabaseInstance
+ tables :- [i/TableInstance]]
+ (apply merge-with + (map update-field-values-for-table! tables)))
(defn- update-field-values-summary [{:keys [created updated deleted errors]}]
(trs "Updated {0} field value sets, created {1}, deleted {2} with {3} errors"
updated created deleted errors))
-(def ^:private field-values-steps
- [(sync-util/create-sync-step "update-field-values" update-field-values-for-database! update-field-values-summary)])
+(defn- delete-expired-advanced-field-values-summary [{:keys [deleted]}]
+ (trs "Deleted {0} expired advanced fieldvalues" deleted))
+
+(defn- delete-expired-advanced-field-values-for-field!
+ [field]
+ (sync-util/with-error-handling (format "Error deleting expired advanced field values for %s" (sync-util/name-for-logging field))
+ (let [conditions [:field_id (:id field),
+ :type [:in field-values/advanced-field-values-types],
+ :created_at [:< (sql.qp/add-interval-honeysql-form
+ (mdb/db-type)
+ :%now
+ (- (t/as field-values/advanced-field-values-max-age :days))
+ :day)]]
+ rows-count (apply db/count FieldValues conditions)]
+ (apply db/delete! FieldValues conditions)
+ rows-count)))
+
+(s/defn delete-expired-advanced-field-values-for-table!
+ "Delete all expired advanced FieldValues for a table and returns the number of deleted rows.
+ For more info about advanced FieldValues, check the docs in [[metabase.models.field-values/field-values-types]]"
+ [table :- i/TableInstance]
+ (->> (table->fields-to-scan table)
+ (map delete-expired-advanced-field-values-for-field!)
+ (reduce +)))
+
+(s/defn ^:private delete-expired-advanced-field-values-for-database!
+ [_database :- i/DatabaseInstance
+ tables :- [i/TableInstance]]
+ {:deleted (apply + (map delete-expired-advanced-field-values-for-table! tables))})
+
+(defn- make-sync-field-values-steps
+ [tables]
+ [(sync-util/create-sync-step "delete-expired-advanced-field-values"
+ #(delete-expired-advanced-field-values-for-database! % tables)
+ delete-expired-advanced-field-values-summary)
+ (sync-util/create-sync-step "update-field-values"
+ #(update-field-values-for-database! % tables)
+ update-field-values-summary)])
(s/defn update-field-values!
- "Update the cached FieldValues (distinct values for categories and certain other fields that are shown
+ "Update the advanced FieldValues (distinct values for categories and certain other fields that are shown
in widgets like filters) for the Tables in DATABASE (as needed)."
[database :- i/DatabaseInstance]
(sync-util/sync-operation :cache-field-values database (format "Cache field values in %s"
(sync-util/name-for-logging database))
- (sync-util/run-sync-operation "field values scanning" database field-values-steps)))
+ (let [tables (sync-util/db->sync-tables database)]
+ (sync-util/run-sync-operation "field values scanning" database (make-sync-field-values-steps tables)))))
diff --git a/test/metabase/models/field_values_test.clj b/test/metabase/models/field_values_test.clj
index 39f321b5aae..1f9e52bdcb6 100644
--- a/test/metabase/models/field_values_test.clj
+++ b/test/metabase/models/field_values_test.clj
@@ -104,14 +104,14 @@
:has_more_values true}
(#'field-values/distinct-values {})))))))
-(deftest clear-field-values!-test
+(deftest clear-field-values-for-field!-test
(mt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Field [{field-id :id} {:table_id table-id}]
FieldValues [_ {:field_id field-id, :values "[1,2,3]"}]]
(is (= [1 2 3]
(db/select-one-field :values FieldValues, :field_id field-id)))
- (#'field-values/clear-field-values! field-id)
+ (field-values/clear-field-values-for-field! field-id)
(is (= nil
(db/select-one-field :values FieldValues, :field_id field-id)))))
@@ -123,6 +123,20 @@
(sync/sync-database! db)
(find-values field-values-id))
+(deftest get-or-create-full-field-values!-test
+ (testing "create a full Fieldvalues if it does not exist"
+ (db/delete! FieldValues :field_id (mt/id :categories :name) :type :full)
+ (is (= :full (-> (Field (mt/id :categories :name))
+ field-values/get-or-create-full-field-values!
+ :type))
+ (is (= 1 (db/count FieldValues :field_id (mt/id :categories :name) :type :full))))
+
+ (testing "if an Advanced FeildValues Exists, make sure we still returns the full FieldValues"
+ (mt/with-temp FieldValues [_ {:field_id (mt/id :categories :name)
+ :type :sandbox
+ :hash_key "random-hash"}])
+ (is (= :full (:type (field-values/get-or-create-full-field-values! (Field (mt/id :categories :name)))))))))
+
(deftest normalize-human-readable-values-test
(testing "If FieldValues were saved as a map, normalize them to a sequence on the way out"
(mt/with-temp FieldValues [fv {:field_id (mt/id :venues :id)
@@ -210,12 +224,64 @@
:type "external"}]
(mt/with-temp-vals-in-db Field (mt/id :orders :product_id) {:has_field_values "list"}
(is (= ::field-values/fv-created
- (field-values/create-or-update-field-values! (Field (mt/id :orders :product_id)))))
+ (field-values/create-or-update-full-field-values! (Field (mt/id :orders :product_id)))))
(is (partial= {:field_id (mt/id :orders :product_id)
:values [1 2 3 4]
:human_readable_values []}
(field-values))))))))))
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Life Cycle |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(deftest insert-field-values-type-test
+ (testing "fieldvalues type=:full shouldn't have hash_key"
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"Full FieldValues shouldnt have hash_key"
+ (mt/with-temp FieldValues [_ {:field_id (mt/id :venues :id)
+ :type :full
+ :hash_key "random-hash"}]))))
+
+ (testing "Advanced fieldvalues requires a hash_key"
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"Advanced FieldValues requires a hash_key"
+ (mt/with-temp FieldValues [_ {:field_id (mt/id :venues :id)
+ :type :sandbox}])))))
+
+(deftest insert-full-field-values-should-remove-all-cached-field-values
+ (mt/with-temp* [FieldValues [sandbox-fv {:field_id (mt/id :venues :id)
+ :type :sandbox
+ :hash_key "random-hash"}]]
+ (db/insert! FieldValues {:field_id (mt/id :venues :id)
+ :type :full})
+ (is (not (db/exists? FieldValues :id (:id sandbox-fv))))))
+
+(deftest update-full-field-values-should-remove-all-cached-field-values
+ (mt/with-temp* [FieldValues [fv {:field_id (mt/id :venues :id)
+ :type :full}]
+ FieldValues [sandbox-fv {:field_id (mt/id :venues :id)
+ :type :sandbox
+ :hash_key "random-hash"}]]
+ (db/update! FieldValues (:id fv) :values [1 2 3])
+ (is (not (db/exists? FieldValues :id (:id sandbox-fv))))))
+
+(deftest cant-update-type-or-has-of-a-field-values-test
+ (mt/with-temp FieldValues [fv {:field_id (mt/id :venues :id)
+ :type :sandbox
+ :hash_key "random-hash"}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"Cant update type or hash_key for a FieldValues."
+ (db/update! FieldValues (:id fv) :type :full)))
+
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"Cant update type or hash_key for a FieldValues."
+ (db/update! FieldValues (:id fv) :hash_key "new-hash")))))
+
+
(deftest identity-hash-test
(testing "Field hashes are composed of the name and the table's identity-hash"
(mt/with-temp* [Database [db {:name "field-db" :engine :h2}]
diff --git a/test/metabase/models/on_demand_test.clj b/test/metabase/models/on_demand_test.clj
index 57a38fdcc12..aebd808a06c 100644
--- a/test/metabase/models/on_demand_test.clj
+++ b/test/metabase/models/on_demand_test.clj
@@ -18,7 +18,7 @@
{:style/indent 0}
[f]
(let [updated-field-names (atom #{})]
- (with-redefs [field-values/create-or-update-field-values! (fn [field]
+ (with-redefs [field-values/create-or-update-full-field-values! (fn [field]
(swap! updated-field-names conj (:name field)))]
(f updated-field-names)
@updated-field-names)))
diff --git a/test/metabase/models/params/chain_filter_test.clj b/test/metabase/models/params/chain_filter_test.clj
index 85d26da9682..a38b33a6d2d 100644
--- a/test/metabase/models/params/chain_filter_test.clj
+++ b/test/metabase/models/params/chain_filter_test.clj
@@ -316,7 +316,7 @@
(testing "Field-to-field remapping: venues.category_id -> categories.name\n"
(testing "Show me venue IDs (names)"
(is (= [[29 "20th Century Cafe"]
- [ 8 "25°"]
+ [8 "25°"]
[93 "33 Taps"]]
(take 3 (chain-filter/chain-filter (mt/id :venues :id) nil)))))
(testing "Show me expensive venue IDs (names)"
diff --git a/test/metabase/sync/field_values_test.clj b/test/metabase/sync/field_values_test.clj
index 7b73d31d4a8..0609f383411 100644
--- a/test/metabase/sync/field_values_test.clj
+++ b/test/metabase/sync/field_values_test.clj
@@ -2,10 +2,10 @@
"Tests around the way Metabase syncs FieldValues, and sets the values of `field.has_field_values`."
(:require [clojure.string :as str]
[clojure.test :refer :all]
+ [java-time :as t]
[metabase.db.metadata-queries :as metadata-queries]
- [metabase.models.field :refer [Field]]
- [metabase.models.field-values :as field-values :refer [FieldValues]]
- [metabase.models.table :refer [Table]]
+ [metabase.models :refer [Field FieldValues Table]]
+ [metabase.models.field-values :as field-values]
[metabase.sync :as sync]
[metabase.sync.util-test :as sync.util-test]
[metabase.test :as mt]
@@ -14,7 +14,7 @@
[toucan.db :as db]))
(defn- venues-price-field-values []
- (db/select-one-field :values FieldValues, :field_id (mt/id :venues :price)))
+ (db/select-one-field :values FieldValues, :field_id (mt/id :venues :price), :type :full))
(defn- sync-database!' [step database]
(let [{:keys [step-info task-history]} (sync.util-test/sync-database! step database)]
@@ -44,7 +44,7 @@
(is (= [1 2 3 4]
(venues-price-field-values))))
(testing "Update the FieldValues, remove one of the values that should be there"
- (db/update! FieldValues (db/select-one-id FieldValues :field_id (mt/id :venues :price)) :values [1 2 3])
+ (db/update! FieldValues (db/select-one-id FieldValues :field_id (mt/id :venues :price) :type :full) :values [1 2 3])
(is (= [1 2 3]
(venues-price-field-values))))
(testing "Now re-sync the table and validate the field values updated"
@@ -54,6 +54,59 @@
(is (= [1 2 3 4]
(venues-price-field-values))))))
+(deftest sync-should-delete-expired-advanced-field-values-test
+ (testing "Test that the expired Advanced FieldValues should be removed"
+ (let [field-id (mt/id :venues :price)
+ expired-created-at (t/minus (t/offset-date-time) (t/plus field-values/advanced-field-values-max-age (t/days 1)))
+ now (t/offset-date-time)
+ [expired-sandbox-id
+ expired-linked-filter-id
+ valid-sandbox-id
+ valid-linked-filter-id
+ old-full-id
+ new-full-id] (db/simple-insert-many!
+ FieldValues
+ [;; expired sandbox fieldvalues
+ {:field_id field-id
+ :type "sandbox"
+ :hash_key "random-hash"
+ :created_at expired-created-at
+ :updated_at expired-created-at}
+ ;; expired linked-filter fieldvalues
+ {:field_id field-id
+ :type "linked-filter"
+ :hash_key "random-hash"
+ :created_at expired-created-at
+ :updated_at expired-created-at}
+ ;; valid sandbox fieldvalues
+ {:field_id field-id
+ :type "sandbox"
+ :hash_key "random-hash"
+ :created_at now
+ :updated_at now}
+ ;; valid linked-filter fieldvalues
+ {:field_id field-id
+ :type "linked-filter"
+ :hash_key "random-hash"
+ :created_at now
+ :updated_at now}
+ ;; old full fieldvalues
+ {:field_id field-id
+ :type "full"
+ :created_at expired-created-at
+ :updated_at expired-created-at}
+ ;; new full fieldvalues
+ {:field_id field-id
+ :type "full"
+ :created_at now
+ :updated_at now}])]
+ (is (= (repeat 2 {:deleted 2})
+ (sync-database!' "delete-expired-advanced-field-values" (data/db))))
+ (testing "The expired Advanced FieldValues should be deleted"
+ (is (not (db/exists? FieldValues :id [:in [expired-sandbox-id expired-linked-filter-id]]))))
+ (testing "The valid Advanced FieldValues and full Fieldvalues(both old and new) should not be deleted"
+ (is (db/exists? FieldValues :id [:in [valid-sandbox-id valid-linked-filter-id new-full-id old-full-id]]))))))
+
(deftest auto-list-with-cardinality-threshold-test
;; A Field with 50 values should get marked as `auto-list` on initial sync, because it should be 'list', but was
;; marked automatically, as opposed to explicitly (`list`)
@@ -71,8 +124,13 @@
(into {} (db/select-one [FieldValues :values :human_readable_values :has_more_values]
:field_id (mt/id :blueberries_consumed :str))))))
- (testing (str "if the number grows past the cardinality threshold & we sync again it should get unmarked as auto-list and set "
- "back to `nil` (#3215)\n")
+ ;; Manually add an advanced field values to test whether or not it got deleted later
+ (db/insert! FieldValues {:field_id (mt/id :blueberries_consumed :str)
+ :type :sandbox
+ :hash_key "random-key"})
+
+ (testing (str "if the number grows past the cardinality threshold & we sync again it should get unmarked as auto-list "
+ "and set back to `nil` (#3215)\n")
;; now insert enough bloobs to put us over the limit and re-sync.
(one-off-dbs/insert-rows-and-sync! (one-off-dbs/range-str 50 (+ 100 field-values/auto-list-cardinality-threshold)))
(testing "has_field_values should have been set to nil."
@@ -105,7 +163,7 @@
(is (= nil
(db/select-one-field :has_field_values Field :id (mt/id :blueberries_consumed :str)))))
- (testing "its FieldValues should also get deleted."
+ (testing "All of its FieldValues should also get deleted."
(is (= nil
(db/select-one FieldValues
:field_id (mt/id :blueberries_consumed :str))))))))
@@ -120,7 +178,10 @@
(testing "has_more_values should initially be false"
(is (= false
(db/select-one-field :has_more_values FieldValues :field_id (mt/id :blueberries_consumed :str)))))
-
+ ;; Manually add an advanced field values to test whether or not it got deleted later
+ (db/insert! FieldValues {:field_id (mt/id :blueberries_consumed :str)
+ :type :sandbox
+ :hash_key "random-key"})
(testing "adding more values even if it's exceed our cardinality limit, "
(one-off-dbs/insert-rows-and-sync! (one-off-dbs/range-str 50 (+ 100 metadata-queries/absolute-max-distinct-values-limit)))
(testing "has_field_values shouldn't change and has_more_values should be true"
@@ -133,7 +194,10 @@
:human_readable_values []
:has_more_values true}
(into {} (db/select-one [FieldValues :values :human_readable_values :has_more_values]
- :field_id (mt/id :blueberries_consumed :str))))))))))
+ :field_id (mt/id :blueberries_consumed :str))))))
+ (testing "The advanced field values of this field should be deleted"
+ (is (= 0 (db/count FieldValues :field_id (mt/id :blueberries_consumed :str)
+ :type [:not= :full]))))))))
(deftest list-with-max-length-threshold-test
(testing "If we had explicitly marked the Field as `list` (instead of `auto-list`) "
--
GitLab