Skip to content
Snippets Groups Projects
Unverified Commit 515111f7 authored by Chris Truter's avatar Chris Truter Committed by GitHub
Browse files

Fix weight fallback, and fix the reason tests didn't run (#50370)

parent d8843159
No related branches found
No related tags found
No related merge requests found
...@@ -143,6 +143,7 @@ ...@@ -143,6 +143,7 @@
:native-query {:type :native-query, :context-key :search-native-query} :native-query {:type :native-query, :context-key :search-native-query}
:verified {:type :single-value, :supported-value? #{true}, :required-feature :content-verification}})) :verified {:type :single-value, :supported-value? #{true}, :required-feature :content-verification}}))
;; This gets called *a lot* during a search request, so we'll almost certainly need to optimize it. Maybe just TTL.
(defn weights (defn weights
"Strength of the various scorers. Copied from metabase.search.in-place.scoring, but allowing divergence." "Strength of the various scorers. Copied from metabase.search.in-place.scoring, but allowing divergence."
[context] [context]
...@@ -150,7 +151,9 @@ ...@@ -150,7 +151,9 @@
overrides (public-settings/experimental-search-weight-overrides)] overrides (public-settings/experimental-search-weight-overrides)]
(if (= :all context) (if (= :all context)
(merge-with merge static-weights overrides) (merge-with merge static-weights overrides)
(merge {} (merge (get static-weights :default)
;; Not sure which of the next two should have precedence, arguments for both "¯\_(ツ)_/¯"
(get overrides :default)
(get static-weights context) (get static-weights context)
(get overrides context))))) (get overrides context)))))
......
...@@ -1608,11 +1608,12 @@ ...@@ -1608,11 +1608,12 @@
([context params] ([context params]
(weights-url (assoc params :context (name context))))) (weights-url (assoc params :context (name context)))))
(deftest weights-test (deftest ^:synchronized weights-test
(let [base-url (weights-url) (let [base-url (weights-url)
original-weights (search.config/weights :default) original-weights (search.config/weights :default)
original-overrides (public-settings/experimental-search-weight-overrides)] original-overrides (public-settings/experimental-search-weight-overrides)]
(try (try
(public-settings/experimental-search-weight-overrides! nil)
(testing "default weights" (testing "default weights"
(is (= original-weights (mt/user-http-request :crowberto :get 200 base-url))) (is (= original-weights (mt/user-http-request :crowberto :get 200 base-url)))
(is (mt/user-http-request :rasta :get 403 (weights-url {:recency 4}))) (is (mt/user-http-request :rasta :get 403 (weights-url {:recency 4})))
...@@ -1627,6 +1628,8 @@ ...@@ -1627,6 +1628,8 @@
(let [context :none-given (let [context :none-given
context-url (weights-url context {}) context-url (weights-url context {})
original-weights (search.config/weights context)] original-weights (search.config/weights context)]
;; overrides from before will cascade
(is (= 30.0 (:text (mt/user-http-request :crowberto :get 200 context-url))))
(is (= original-weights (mt/user-http-request :crowberto :get 200 context-url))) (is (= original-weights (mt/user-http-request :crowberto :get 200 context-url)))
(is (mt/user-http-request :rasta :get 403 (weights-url context {:recency 5}))) (is (mt/user-http-request :rasta :get 403 (weights-url context {:recency 5})))
(is (= (assoc original-weights :recency 5.0) (is (= (assoc original-weights :recency 5.0)
...@@ -1643,6 +1646,10 @@ ...@@ -1643,6 +1646,10 @@
(is (= all-weights (mt/user-http-request :crowberto :get 200 context-url))) (is (= all-weights (mt/user-http-request :crowberto :get 200 context-url)))
(is (= (mt/user-http-request :crowberto :get 200 base-url) (is (= (mt/user-http-request :crowberto :get 200 base-url)
(:default (mt/user-http-request :crowberto :get 200 context-url)))) (:default (mt/user-http-request :crowberto :get 200 context-url))))
(is (= (mt/user-http-request :crowberto :get 200 (weights-url :none-given {}))
(merge
(:default (mt/user-http-request :crowberto :get 200 context-url))
(:none-given (mt/user-http-request :crowberto :get 200 context-url)))))
(is (mt/user-http-request :rasta :get 403 (weights-url context {:recency 4}))) (is (mt/user-http-request :rasta :get 403 (weights-url context {:recency 4})))
(is (mt/user-http-request :crowberto :get 400 (weights-url context {:recency 4}))) (is (mt/user-http-request :crowberto :get 400 (weights-url context {:recency 4})))
(is (mt/user-http-request :crowberto :get 400 (weights-url context {:text 30}))) (is (mt/user-http-request :crowberto :get 400 (weights-url context {:text 30})))
......
...@@ -69,6 +69,7 @@ ...@@ -69,6 +69,7 @@
:is-impersonated-user? (premium-features/impersonated-user?) :is-impersonated-user? (premium-features/impersonated-user?)
:is-sandboxed-user? (premium-features/impersonated-user?)}) :is-sandboxed-user? (premium-features/impersonated-user?)})
{:archived false {:archived false
:context :command-palette
:search-string search-string :search-string search-string
:models search.config/all-models :models search.config/all-models
:model-ancestors? false} :model-ancestors? false}
...@@ -79,8 +80,9 @@ ...@@ -79,8 +80,9 @@
(defn- search-no-weights (defn- search-no-weights
"Like search but with all weights set to 0." "Like search but with all weights set to 0."
[ranker-key & args] [ranker-key & args]
(mt/with-dynamic-redefs [search.config/weights #(assoc (:default @#'search.config/static-weights) ranker-key 0)] (let [orig-weights (or (mt/dynamic-value search.config/weights) search.config/weights)]
(apply search* ranker-key args))) (mt/with-dynamic-redefs [search.config/weights #(assoc (orig-weights %) ranker-key 0)]
(apply search* ranker-key args))))
(defn- search [& args] (defn- search [& args]
(let [result (apply search* args)] (let [result (apply search* args)]
...@@ -211,35 +213,41 @@ ...@@ -211,35 +213,41 @@
(deftest ^:parallel bookmark-test (deftest ^:parallel bookmark-test
(let [crowberto (mt/user->id :crowberto) (let [crowberto (mt/user->id :crowberto)
rasta (mt/user->id :rasta)] rasta (mt/user->id :rasta)]
(testing "bookmarked items are ranker higher" (mt/with-temp [:model/Card {c1 :id} {}
(with-index-contents :model/Card {c2 :id} {}]
[{:model "card" :id 1 :name "card normal"} (testing "bookmarked items are ranker higher"
{:model "card" :id 2 :name "card crowberto loved"}] (with-index-contents
(mt/with-temp [:model/CardBookmark _ {:card_id 2 :user_id crowberto} [{:model "card" :id c1 :name "card normal"}
:model/CardBookmark _ {:card_id 1 :user_id rasta}] {:model "card" :id c2 :name "card crowberto loved"}]
(is (= [["card" 2 "card crowberto loved"] (mt/with-temp [:model/CardBookmark _ {:card_id c2 :user_id crowberto}
["card" 1 "card normal"]] :model/CardBookmark _ {:card_id c1 :user_id rasta}]
(search :bookmarked "card" {:current-user-id crowberto})))))) (is (= [["card" c2 "card crowberto loved"]
["card" c1 "card normal"]]
(search :bookmarked "card" {:current-user-id crowberto})))))))
(testing "bookmarked dashboard" (mt/with-temp [:model/Dashboard {d1 :id} {}
(with-index-contents :model/Dashboard {d2 :id} {}]
[{:model "dashboard" :id 1 :name "dashboard normal"} (testing "bookmarked dashboard"
{:model "dashboard" :id 2 :name "dashboard crowberto loved"}] (with-index-contents
(mt/with-temp [:model/DashboardBookmark _ {:dashboard_id 2 :user_id crowberto} [{:model "dashboard" :id d1 :name "dashboard normal"}
:model/DashboardBookmark _ {:dashboard_id 1 :user_id rasta}] {:model "dashboard" :id d2 :name "dashboard crowberto loved"}]
(is (= [["dashboard" 2 "dashboard crowberto loved"] (mt/with-temp [:model/DashboardBookmark _ {:dashboard_id d2 :user_id crowberto}
["dashboard" 1 "dashboard normal"]] :model/DashboardBookmark _ {:dashboard_id d1 :user_id rasta}]
(search :bookmarked "dashboard" {:current-user-id crowberto})))))) (is (= [["dashboard" d2 "dashboard crowberto loved"]
["dashboard" d1 "dashboard normal"]]
(search :bookmarked "dashboard" {:current-user-id crowberto})))))))
(testing "bookmarked collection" (mt/with-temp [:model/Collection {c1 :id} {}
(with-index-contents :model/Collection {c2 :id} {}]
[{:model "collection" :id 1 :name "collection normal"} (testing "bookmarked collection"
{:model "collection" :id 2 :name "collection crowberto loved"}] (with-index-contents
(mt/with-temp [:model/CollectionBookmark _ {:collection_id 2 :user_id crowberto} [{:model "collection" :id c1 :name "collection normal"}
:model/CollectionBookmark _ {:collection_id 1 :user_id rasta}] {:model "collection" :id c2 :name "collection crowberto loved"}]
(is (= [["collection" 2 "collection crowberto loved"] (mt/with-temp [:model/CollectionBookmark _ {:collection_id c2 :user_id crowberto}
["collection" 1 "collection normal"]] :model/CollectionBookmark _ {:collection_id c1 :user_id rasta}]
(search :bookmarked "collection" {:current-user-id crowberto})))))))) (is (= [["collection" c2 "collection crowberto loved"]
["collection" c1 "collection normal"]]
(search :bookmarked "collection" {:current-user-id crowberto})))))))))
(deftest ^:parallel user-recency-test (deftest ^:parallel user-recency-test
(let [user-id (mt/user->id :crowberto) (let [user-id (mt/user->id :crowberto)
...@@ -251,19 +259,23 @@ ...@@ -251,19 +259,23 @@
:model_id model-id :model_id model-id
:user_id user-id :user_id user-id
:timestamp timestamp})] :timestamp timestamp})]
(with-index-contents (mt/with-temp [:model/Card {c1 :id} {}
[{:model "card" :id 1 :name "card ancient"} :model/Card {c2 :id} {}
{:model "metric" :id 2 :name "card recent"} :model/Card {c3 :id} {}
{:model "dataset" :id 3 :name "card unseen"} :model/Card {c4 :id} {}
{:model "dataset" :id 4 :name "card old"}] :model/RecentViews _ (recent-view c1 forever-ago)
(mt/with-temp [:model/RecentViews _ (recent-view 1 forever-ago) :model/RecentViews _ (recent-view c2 right-now)
:model/RecentViews _ (recent-view 2 right-now) :model/RecentViews _ (recent-view c2 forever-ago)
:model/RecentViews _ (recent-view 2 forever-ago) :model/RecentViews _ (recent-view c4 forever-ago)
:model/RecentViews _ (recent-view 4 forever-ago) :model/RecentViews _ (recent-view c4 long-ago)]
:model/RecentViews _ (recent-view 4 long-ago)] (with-index-contents
[{:model "card" :id c1 :name "card ancient"}
{:model "metric" :id c2 :name "card recent"}
{:model "dataset" :id c3 :name "card unseen"}
{:model "dataset" :id c4 :name "card old"}]
(testing "We prefer results more recently viewed by the current user" (testing "We prefer results more recently viewed by the current user"
(is (= [["metric" 2 "card recent"] (is (= [["metric" c2 "card recent"]
["dataset" 4 "card old"] ["dataset" c4 "card old"]
["card" 1 "card ancient"] ["card" c1 "card ancient"]
["dataset" 3 "card unseen"]] ["dataset" c3 "card unseen"]]
(search :user-recency "card" {:current-user-id user-id})))))))) (search :user-recency "card" {:current-user-id user-id}))))))))
...@@ -18,7 +18,7 @@ ...@@ -18,7 +18,7 @@
(deftest ^:parallel parse-engine-test (deftest ^:parallel parse-engine-test
(testing "Default engine" (testing "Default engine"
(is (= :search.engine/in-place (#'search.impl/parse-engine nil)))) (is (= "search.engine" (namespace (#'search.impl/parse-engine nil)))))
(testing "Unknown engine resolves to the default" (testing "Unknown engine resolves to the default"
(is (= (#'search.impl/parse-engine nil) (is (= (#'search.impl/parse-engine nil)
(#'search.impl/parse-engine "vespa")))) (#'search.impl/parse-engine "vespa"))))
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment