Make the field search do limits (whacks #17228) (#17283)

We're playing this whackamole because we added the middleware to universally sorta stomp on any `limit` or `offset` params, and they got stomped on. Which, in turn, led to my PR to make the default not limited. Which, in turn, led to the other issue this PR solves which makes the search things we relied upon to not have a limit, have a limit.

frontend/src/metabase/dashboard/components/add-card-sidebar/QuestionList.jsx

@@ -15,6 +15,8 @@ QuestionList.propTypes = {
   hasCollections: PropTypes.bool,
 };
 
+const SEARCH_LIMIT = 1000;
+
 export function QuestionList({
   searchText,
   collectionId,
@@ -35,6 +37,7 @@ export function QuestionList({
   query = {
     ...query,
     models: "card",
+    limit: SEARCH_LIMIT,
   };
 
   return (

frontend/src/metabase/nav/components/SearchBar.jsx

@@ -52,6 +52,7 @@ const SearchInput = styled.input`
 `;
 
 const ALLOWED_SEARCH_FOCUS_ELEMENTS = new Set(["BODY", "A"]);
+const SEARCH_LIMIT = 50;
 
 export default class SearchBar extends React.Component {
   state = {
@@ -152,7 +153,7 @@ export default class SearchBar extends React.Component {
                   py={1}
                 >
                   <Search.ListLoader
-                    query={{ q: searchText.trim() }}
+                    query={{ q: searchText.trim(), limit: SEARCH_LIMIT }}
                     wrapped
                     reload
                     debounced

frontend/src/metabase/query_builder/components/data-search/SearchResults.jsx

@@ -18,6 +18,8 @@ const propTypes = {
   ),
 };
 
+const SEARCH_LIMIT = 1000;
+
 export function SearchResults({
   searchQuery,
   onSelect,
@@ -27,6 +29,7 @@ export function SearchResults({
   const query = {
     q: searchQuery,
     models: searchModels,
+    limit: SEARCH_LIMIT,
   };
 
   if (databaseId) {

src/metabase/api/field.clj

@@ -12,6 +12,7 @@
             [metabase.models.table :as table :refer [Table]]
             [metabase.query-processor :as qp]
             [metabase.related :as related]
+            [metabase.server.middleware.offset-paging :as offset-paging]
             [metabase.types :as types]
             [metabase.util :as u]
             [metabase.util.i18n :refer [trs]]
@@ -21,8 +22,11 @@
             [toucan.hydrate :refer [hydrate]])
   (:import java.text.NumberFormat))
 
+
 ;;; --------------------------------------------- Basic CRUD Operations ----------------------------------------------
 
+(def ^:private default-max-field-search-limit 1000)
+
 (def ^:private FieldVisibilityType
   "Schema for a valid `Field` visibility type."
   (apply s/enum (map name field/visibility-types)))
@@ -137,7 +141,6 @@
     ;; return updated field
     (hydrate (Field id) :dimensions)))
 
-
 ;;; ------------------------------------------------- Field Metadata -------------------------------------------------
 
 (api/defendpoint GET "/:id/summary"
@@ -297,6 +300,7 @@
     (db/select-one Field :id fk-target-field-id)
     field))
 
+
 (defn- search-values-query
   "Generate the MBQL query used to power FieldValues search in `search-values` below. The actual query generated differs
   slightly based on whether the two Fields are the same Field."
@@ -326,9 +330,10 @@
       ;; -> ((14 \"Marilyne Mohr\")
              (36 \"Margot Farrell\")
              (48 \"Maryam Douglas\"))"
-  [field search-field value & [limit]]
+  [field search-field value maybe-limit]
   (try
     (let [field   (follow-fks field)
+          limit   (or maybe-limit default-max-field-search-limit)
           results (qp/process-query (search-values-query field search-field value limit))
           rows    (get-in results [:data :rows])]
       ;; if the two Fields are different, we'll get results like [[v1 v2] [v1 v2]]. That is the expected format and we can
@@ -344,17 +349,17 @@
       (log/debug e (trs "Error searching field values"))
       nil)))
 
+
 (api/defendpoint GET "/:id/search/:search-id"
   "Search for values of a Field with `search-id` that start with `value`. See docstring for
   `metabase.api.field/search-values` for a more detailed explanation."
-  [id search-id value limit]
-  {value su/NonBlankString
-   limit (s/maybe su/IntStringGreaterThanZero)}
+  [id search-id value]
+  {value su/NonBlankString}
   (let [field        (api/check-404 (Field id))
         search-field (api/check-404 (Field search-id))]
     (throw-if-no-read-or-segmented-perms field)
     (throw-if-no-read-or-segmented-perms search-field)
-    (search-values field search-field value (when limit (Integer/parseInt limit)))))
+    (search-values field search-field value offset-paging/*limit*)))
 
 (defn remapped-value
   "Search for one specific remapping where the value of `field` exactly matches `value`. Returns a pair like

test/metabase/api/field_test.clj

@@ -595,7 +595,8 @@
              (mt/format-rows-by [int str]
                (field-api/search-values (Field (mt/id :venues :id))
                                         (Field (mt/id :venues :name))
-                                        "Red")))))
+                                        "Red"
+                                        nil)))))
     (tqp.test/test-timeseries-drivers
       (is (= [["139" "Red Medicine"]
               ["148" "Fred 62"]
@@ -608,7 +609,16 @@
               ["977" "Fred 62"]]
              (field-api/search-values (Field (mt/id :checkins :id))
                                       (Field (mt/id :checkins :venue_name))
-                                      "Red"))))))
+                                      "Red"
+                                      nil)))))
+  (testing "make sure limit works"
+    (mt/test-drivers (mt/normal-drivers)
+      (is (= [[1 "Red Medicine"]]
+             (mt/format-rows-by [int str]
+                                (field-api/search-values (Field (mt/id :venues :id))
+                                                         (Field (mt/id :venues :name))
+                                                         "Red"
+                                                         1)))))))
 
 (deftest search-values-with-field-same-as-search-field-test
   (testing "make sure it also works if you use the same Field twice"
@@ -616,9 +626,11 @@
       (is (= [["Fred 62" "Fred 62"] ["Red Medicine" "Red Medicine"]]
              (field-api/search-values (Field (mt/id :venues :name))
                                       (Field (mt/id :venues :name))
-                                      "Red"))))
+                                      "Red"
+                                      nil))))
     (tqp.test/test-timeseries-drivers
       (is (= [["Fred 62" "Fred 62"] ["Red Medicine" "Red Medicine"]]
              (field-api/search-values (Field (mt/id :checkins :venue_name))
                                       (Field (mt/id :checkins :venue_name))
-                                      "Red"))))))
+                                      "Red"
+                                      nil))))))

test/metabase/api/search_test.clj

@@ -222,6 +222,12 @@
   (testing "It offsets matches properly"
     (with-search-items-in-root-collection "test"
       (is (<= 4 (count (search-request-data :crowberto :q "test" :limit "100" :offset "2"))))))
+  (testing "It offsets without limit properly"
+    (with-search-items-in-root-collection "test"
+      (is (= 4 (count (search-request-data :crowberto :q "test" :offset "2"))))))
+  (testing "It limits without offset properly"
+    (with-search-items-in-root-collection "test"
+      (is (= 2 (count (search-request-data :crowberto :q "test" :limit "2"))))))
   (testing "It subsets matches for model"
     (with-search-items-in-root-collection "test"
       (is (= 0 (count (search-request-data :crowberto :q "test" :models "database"))))