Fix incorrect scoring of search results (#23984)

* Correct `score-and-result` to count the weight of the text score when there is no text match

* Replace nil-punning of scores with explicit checking for zero scores at each step

* Update scoring functions that can return nil to return zero instead

* Address archived search with no search string

* Update e2e test

* Update e2e test with new ordering of results

* Fix dataset search with blank query string

frontend/test/metabase/scenarios/onboarding/search/search.cy.spec.js

@@ -46,7 +46,6 @@ describe("scenarios > auth > search", () => {
       cy.findByPlaceholderText("Search…").type("ord");
       cy.wait("@search");
 
-      cy.get("body").trigger("keydown", { key: "ArrowDown" });
       cy.get("body").trigger("keydown", { key: "ArrowDown" });
       cy.get("body").trigger("keydown", { key: "Enter" });
 

frontend/test/metabase/scenarios/question/new.cy.spec.js

@@ -80,6 +80,7 @@ describe("scenarios > question > new", () => {
 
       it("should allow to search and select tables", () => {
         cy.findAllByText("Orders")
+          .eq(1)
           .closest("li")
           .findByText("Table in")
           .parent()

src/metabase/api/search.clj

@@ -420,7 +420,7 @@
                              (map #(update % :bookmark bit->boolean))
                              (map #(update % :archived bit->boolean))
                              (map (partial scoring/score-and-result (:search-string search-ctx)))
-                             (filter some?))
+                             (filter #(pos? (:score %))))
           total-results     (scoring/top-results reducible-results xf)]
       ;; We get to do this slicing and dicing with the result data because
       ;; the pagination of search is for UI improvement, not for performance.

src/metabase/search/scoring.clj

@@ -82,6 +82,8 @@
                  :text     (tokens->string text-tokens (not is-match))})))))
 
 (defn- text-score-with
+  "Scores a search result. Returns a map with the score and other info about the text match,
+   if there is one. If there is no match, the score is 0."
   [weighted-scorers query-tokens search-result]
   (let [total-weight (reduce + (map :weight weighted-scorers))
         scores       (for [column (search-config/searchable-columns-for-model (:model search-result))
@@ -96,14 +98,14 @@
                                                              0
                                                              (map :scorer weighted-scorers)))]
                            :when  (and matched-text
-                                       (> score 0))]
+                                       (pos? score))]
                        {:score               (/ score total-weight)
                         :match               matched-text
                         :match-context-thunk #(match-context query-tokens match-tokens)
-                        :column              column
-                        :result              search-result})]
-    (when (seq scores)
-      (apply max-key :score scores))))
+                        :column              column})]
+    (if (seq scores)
+      (apply max-key :score scores)
+      {:score 0})))
 
 (defn- consecutivity-scorer
   [query-tokens match-tokens]
@@ -169,31 +171,31 @@
                      (tokenize (normalize raw-search-string))
                      result)
     {:score  0
-     :match  ""
-     :result result}))
+     :match  ""}))
 
 (defn- pinned-score
   [{:keys [model collection_position]}]
   ;; We experimented with favoring lower collection positions, but it wasn't good
   ;; So instead, just give a bonus for items that are pinned at all
-  (when (#{"card" "dashboard" "pulse"} model)
-    (if ((fnil pos? 0) collection_position)
-      1
-      0)))
+  (if (and (#{"card" "dashboard" "pulse"} model)
+           ((fnil pos? 0) collection_position))
+    1
+    0))
 
 (defn- bookmarked-score
   [{:keys [model bookmark]}]
-  (when (#{"card" "collection" "dashboard"} model)
-    (if bookmark
-      1
-      0)))
+  (if (and (#{"card" "collection" "dashboard"} model)
+           bookmark)
+    1
+    0))
 
 (defn- dashboard-count-score
   [{:keys [model dashboardcard_count]}]
-  (when (= model "card")
+  (if (= model "card")
     (min (/ dashboardcard_count
             search-config/dashboard-count-ceiling)
-         1)))
+         1)
+    0))
 
 (defn- recency-score
   [{:keys [updated_at]}]
@@ -268,15 +270,22 @@
    (weights-and-scores result))
 
 (defn score-and-result
-  "Returns a map with the `:score` and `:result`—or nil. The score is a vector of comparable things in priority order."
+  "Returns a map with the `:score` and `:result`."
   ([raw-search-string result]
-   (let [text-score (text-score-with-match raw-search-string result)
-         scores     (->> (conj (score-result result)
-                               {:score (:score text-score), :weight 10 :name "text score"})
-                         (filter :score))]
-     {:score  (/ (reduce + (map (fn [{:keys [weight score]}] (* weight score)) scores))
-                 (reduce + (map :weight scores)))
-      :result (serialize result text-score scores)})))
+   (let [text-match (text-score-with-match raw-search-string result)
+         text-score {:score  (:score text-match)
+                     :weight 10
+                     :name   "text score"}
+         scores (conj (score-result result) text-score)]
+     ;; Searches with a blank search string mean "show me everything, ranked";
+     ;; see https://github.com/metabase/metabase/pull/15604 for archived search.
+     ;; If the search string is non-blank, results with no text match have a score of zero.
+     (if (or (str/blank? raw-search-string)
+             (pos? (:score text-match)))
+       {:score  (/ (reduce + (map (fn [{:keys [weight score]}] (* weight score)) scores))
+                   (reduce + (map :weight scores)))
+        :result (serialize result text-match scores)}
+       {:score 0}))))
 
 (defn top-results
   "Given a reducible collection (i.e., from `jdbc/reducible-query`) and a transforming function for it, applies the

test/metabase/search/scoring_test.clj

@@ -53,10 +53,10 @@
              (score ["rasta"]
                     (result-row "Rasta")))))
     (testing "misses"
-      (is (nil?
+      (is (zero?
            (score ["rasta"]
                   (result-row "just a straight-up imposter"))))
-      (is (nil?
+      (is (zero?
            (score ["rasta" "the" "toucan"]
                   (result-row "")))))))
 
@@ -83,10 +83,10 @@
              (score ["rasta" "the" "toucan"]
                     (result-row "Rasta may be my favorite of the toucans")))))
     (testing "misses"
-      (is (nil?
+      (is (zero?
            (score ["rasta"]
                   (result-row "just a straight-up imposter"))))
-      (is (nil?
+      (is (zero?
            (score ["rasta" "the" "toucan"]
                   (result-row "")))))))
 
@@ -101,16 +101,16 @@
              (score ["rasta" "the" "toucan"]
                     (result-row "Rasta the Toucan")))))
     (testing "misses"
-      (is (nil?
+      (is (zero?
            (score ["rasta"]
                   (result-row "just a straight-up imposter"))))
-      (is (nil?
+      (is (zero?
            (score ["rasta" "the" "toucan"]
                   (result-row "")))))))
 
 (deftest exact-match-scorer-test
   (let [score (scorer->score #'scoring/exact-match-scorer)]
-    (is (nil?
+    (is (zero?
          (score ["rasta" "the" "toucan"]
                 (result-row "Crowberto el tucan"))))
     (is (= 1/3