From 6331c3b13076d00819eee78fb29b9c0431d35a92 Mon Sep 17 00:00:00 2001
From: Howon Lee <hlee.howon@gmail.com>
Date: Tue, 3 Aug 2021 13:59:58 -0700
Subject: [PATCH] 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.
---
.../add-card-sidebar/QuestionList.jsx | 3 +++
.../src/metabase/nav/components/SearchBar.jsx | 3 ++-
.../components/data-search/SearchResults.jsx | 3 +++
src/metabase/api/field.clj | 17 ++++++++++------
test/metabase/api/field_test.clj | 20 +++++++++++++++----
test/metabase/api/search_test.clj | 6 ++++++
6 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/frontend/src/metabase/dashboard/components/add-card-sidebar/QuestionList.jsx b/frontend/src/metabase/dashboard/components/add-card-sidebar/QuestionList.jsx
index 470a3eeca91..0395a9e4ddc 100644
--- a/frontend/src/metabase/dashboard/components/add-card-sidebar/QuestionList.jsx
+++ b/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 (
diff --git a/frontend/src/metabase/nav/components/SearchBar.jsx b/frontend/src/metabase/nav/components/SearchBar.jsx
index 6437003376f..35943215357 100644
--- a/frontend/src/metabase/nav/components/SearchBar.jsx
+++ b/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
diff --git a/frontend/src/metabase/query_builder/components/data-search/SearchResults.jsx b/frontend/src/metabase/query_builder/components/data-search/SearchResults.jsx
index 60e31ff0114..e5973072fb2 100644
--- a/frontend/src/metabase/query_builder/components/data-search/SearchResults.jsx
+++ b/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) {
diff --git a/src/metabase/api/field.clj b/src/metabase/api/field.clj
index d92c02cbf35..71209a9e3a3 100644
--- a/src/metabase/api/field.clj
+++ b/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
diff --git a/test/metabase/api/field_test.clj b/test/metabase/api/field_test.clj
index 53543135c87..00235007c8d 100644
--- a/test/metabase/api/field_test.clj
+++ b/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))))))
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 1b4f0776356..ad072e50023 100644
--- a/test/metabase/api/search_test.clj
+++ b/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"))))
--
GitLab