`metabase.search` namespace #42487

camsaul · 2024-05-09T21:11:48Z

Consolidate search into a metabase.search module. Most of the important stuff related to search already lived in metabase.search namespaeces, but a significant chunk of search implementation stuff lived in metabase.api.search. I moved all of that stuff out into a new metabase.search.impl namespace, so now the search module exists completely independently of the REST API. I removed search's dependency on metabase.api/*current-user-id* and made it part of the search context instead (for some reason :current-user-perms was already part of search context but current User ID was not). This is so there are no circular deps between the metabase.search and metabase.api modules.2

I moved some of the tests over but there is probably more work to do there still.

Also I couldn't help myself and split a bunch of big tests into smaller ones and marked them ^:parallel where it made sense.

camsaul · 2024-05-09T21:12:35Z

dev/src/dev/deps_graph.clj

@@ -56,3 +56,50 @@
  "All namespaces from a module that are used outside that module."
  [module-symb]
  (into (sorted-set) (map :depends-on-namespace) (external-usages module-symb)))
+
+(defn module-dependencies


Some extra new dev tools to help me find circular references between modules

Gave some feedback on this in the other PR that used it, in case you were planning on merging this one first.

camsaul · 2024-05-09T21:13:51Z

test/metabase/api/search_test.clj

@@ -246,70 +244,69 @@
  [& args]
  (apply search-request-data-with identity args))

-(deftest order-clause-test


moved to metabase.search.impl-test

camsaul · 2024-05-09T21:13:59Z

test/metabase/api/search_test.clj

@@ -900,50 +919,6 @@
              (t2/update! Pulse (:id pulse) {:dashboard_id (:id dashboard)})
              (is (= nil (search-for-pulses pulse))))))))))

-(deftest search-db-call-count-test


moved to metabase.search.impl-test

camsaul · 2024-05-09T21:14:03Z

test/metabase/api/search_test.clj

@@ -1312,173 +1292,7 @@
        (is (= "Failed to parse datetime value: today~"
               (mt/user-http-request :crowberto :get 400 "search" :q search-term :last_edited_at "today~" :creator_id (mt/user->id :rasta))))))))

-(deftest created-at-correctness-test


moved to metabase.search.impl-test

camsaul · 2024-05-09T21:14:31Z

test/metabase/test/util.clj

@@ -788,15 +788,15 @@
          :status              status})))
    (thunk)))

-(defmacro with-verified-cards
+(defmacro with-verified-cards!


Not ok in ^:parallel tests so I gave it a !

replay-io · 2024-05-09T21:26:01Z

Status	Complete ↗︎
Commit	`c9c9a12`
Results	⚠️ 1 Flaky ✅ 2572 Passed

…iaDB.

crisptrutski

Oh man, so nice that you encapsulated the search code like this. We're about to start working on this module to put the "search engine" behind of plugable interface, so the timing couldn't be better.

crisptrutski · 2024-05-10T08:39:23Z

dev/src/dev/deps_graph.clj

@@ -56,3 +56,50 @@
  "All namespaces from a module that are used outside that module."
  [module-symb]
  (into (sorted-set) (map :depends-on-namespace) (external-usages module-symb)))
+
+(defn module-dependencies


Gave some feedback on this in the other PR that used it, in case you were planning on merging this one first.

crisptrutski · 2024-05-10T08:41:39Z

src/metabase/db/connection.clj

+  resources. This seems to be the default this is the default for Postgres, but MySQL/MariaDB seem to default to
+  `HOLD_CURSORS_OVER_COMMIT`."


Suggested change

resources. This seems to be the default this is the default for Postgres, but MySQL/MariaDB seem to default to

`HOLD_CURSORS_OVER_COMMIT`."

resources. This this is the default for Postgres, but MySQL/MariaDB seem to default to `HOLD_CURSORS_OVER_COMMIT`."

crisptrutski · 2024-05-10T08:41:55Z

src/metabase/db/connection.clj

+      (doto (.getConnection data-source)
+        (setup-connection!))


crisptrutski · 2024-05-10T08:42:20Z

src/metabase/db/connection.clj

+                    ;; otherwise we don't need our savepoint anymore and we can release it.
+                    (.releaseSavepoint connection savepoint))


crisptrutski · 2024-05-10T08:42:35Z

src/metabase/db/connection.clj

+                  (try
+                    (.rollback connection savepoint)
+                    (catch Throwable e2
+                      (log/errorf e2 "Error rolling back transaction: %s (triggered by: %s)" (ex-message e2) (ex-message e))


crisptrutski · 2024-05-10T08:44:17Z

src/metabase/search/filter.clj

+  ;; TODO FIXME -- search actually currently still requires [[metabase.api.common/*current-user*]] to be bound,
+  ;; because [[metabase.public-settings.premium-features/sandboxed-or-impersonated-user?]] requires it to be bound.
+  ;; Since it's part of the search context it would be nice if we could run search without having to bind that stuff at
+  ;; all.
+  (assert @@(requiring-resolve 'metabase.api.common/*current-user*)
+          "metabase.api.common/*current-user* must be bound in order to use search for an indexed entity")


Thanks for the detailed explanation and validation 🙏

test/metabase/analytics/snowplow_test.clj

test/metabase/api/search_test.clj

crisptrutski · 2024-05-10T08:53:50Z

Out of curiosity, did you check how much all this parallelism speeds up tests locally / in CI?

johnswanson · 2024-05-10T16:42:42Z

@camsaul reviewing this now, but just a thought - as far as I can tell, metabase.search is solely used by metabase.api.search, and it doesn't seem like a useful module outside of that context - it's pretty API specific. I wonder whether instead of moving code from metabase.api.search to the metabase.search module, we should instead be removing metabase.search and moving its code to live under metabase.api.search. WDYT?

johnswanson

Thanks for parallelizing all those tests, that's awesome!

Changes all LGTM, but I do kind of think metabase.search should probably live under metabase.api.search, so I'd honestly rather move things in the reverse direction here, unless we can think of a good reason why other parts of the system might want to use a metabase.search module. Especially since e.g. there are functions like metabase.search.scoring/serialize (which honestly, I don't know why it lives there in the first place, but nevertheless...) which seem to be pretty intimately connected to sending data to an API client.

piranha · 2024-05-10T19:11:36Z

We're about to modularize search, so it's going to have a few backends, and models will have to implement some protocol to be searchable — so it doesn't seem like moving it under metabase.api.search makes sense.

johnswanson · 2024-05-10T19:16:18Z

We're about to modularize search, so it's going to have a few backends, and models will have to implement some protocol to be searchable — so it doesn't seem like moving it under metabase.api.search makes sense.

Alright, that makes sense then - thanks!

tsmacdonald

, but a significant chunk of search implementation stuff lived in metabase.api.search. I moved all of that stuff out into a new metabase.search.impl namespace

👏

camsaul · 2024-05-21T21:50:25Z

test/metabase/api/search_test.clj

-          (test-search "past1years-from-12months" old-result)
-          (test-search "today" new-result))))))
-
-(deftest last-edited-at-correctness-test


moved to metabase.search.impl-test

github-actions · 2024-05-22T03:44:13Z

@camsaul Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

* Search API namespace * Search API namespace * Search API namespace * Fix Kondo warnings * Fix typo * Finally solve the MySQL ^:parallel test failures <3 * Make some search tests REPL-friendly * Did I finally fix `^:parallel` MySQL tests? * Ok I give up just make the search tests single-threaded for MySQL/MariaDB. * Revert the stuff to make paralell tests work for MySQL... for now. * Keep new name * Revert deps-graph changes * Remove unused namespaces * Fix Kondo linter for log/info + format * Misc fixes 🔧 * Test fixes 🔧

camsaul added 4 commits May 9, 2024 19:57

Search API namespace

0a071ec

Search API namespace

3bb1910

Search API namespace

cca7561

Fix Kondo warnings

20fee9c

camsaul requested review from johnswanson and a team May 9, 2024 21:11

metabase-bot bot assigned camsaul May 9, 2024

metabase-bot bot added the .Team/QueryProcessor :hammer_and_wrench: label May 9, 2024

camsaul mentioned this pull request May 9, 2024

[Epic] Modular Backend #42434

Open

camsaul commented May 9, 2024

View reviewed changes

Fix typo

b380862

camsaul commented May 9, 2024

View reviewed changes

camsaul added the no-backport Do not backport this PR to any branch label May 9, 2024

camsaul added 3 commits May 10, 2024 00:08

Finally solve the MySQL ^:parallel test failures <3

5b8b43b

Make some search tests REPL-friendly

1b7af26

Did I finally fix ^:parallel MySQL tests?

bdd0938

camsaul changed the title ~~Search API namespace~~ metabase.search namespace May 10, 2024

Ok I give up just make the search tests single-threaded for MySQL/Mar…

e26b921

…iaDB.

crisptrutski approved these changes May 10, 2024

View reviewed changes

camsaul added 2 commits May 10, 2024 16:52

Merge branch 'master' into search-api-namespace

c5511d0

Revert the stuff to make paralell tests work for MySQL... for now.

26d40d0

camsaul removed the request for review from johnswanson May 10, 2024 16:53

camsaul added 2 commits May 10, 2024 16:54

Keep new name

69de518

Revert deps-graph changes

ad9d324

johnswanson reviewed May 10, 2024

View reviewed changes

tsmacdonald approved these changes May 13, 2024

View reviewed changes

camsaul added 2 commits May 21, 2024 21:48

Merge branch 'master' into search-api-namespace

7203b5b

Remove unused namespaces

635d152

camsaul commented May 21, 2024

View reviewed changes

camsaul added 2 commits May 22, 2024 01:21

Fix Kondo linter for log/info + format

3d17e2c

Misc fixes 🔧

d56fa4a

camsaul requested a review from noahmoss as a code owner May 22, 2024 01:23

Test fixes 🔧

c9c9a12

camsaul merged commit d5a32ec into master May 22, 2024
107 checks passed

camsaul deleted the search-api-namespace branch May 22, 2024 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`metabase.search` namespace #42487

`metabase.search` namespace #42487

camsaul commented May 9, 2024

camsaul May 9, 2024

crisptrutski May 10, 2024

camsaul May 9, 2024

camsaul May 9, 2024

camsaul May 9, 2024

camsaul May 9, 2024

replay-io bot commented May 9, 2024 •

edited

crisptrutski left a comment

crisptrutski May 10, 2024

crisptrutski May 10, 2024

crisptrutski May 10, 2024

crisptrutski May 10, 2024

crisptrutski May 10, 2024

crisptrutski May 10, 2024

crisptrutski commented May 10, 2024

johnswanson commented May 10, 2024 •

edited

johnswanson left a comment

piranha commented May 10, 2024

johnswanson commented May 10, 2024

tsmacdonald left a comment

camsaul May 21, 2024

github-actions bot commented May 22, 2024

		resources. This seems to be the default this is the default for Postgres, but MySQL/MariaDB seem to default to
		`HOLD_CURSORS_OVER_COMMIT`."

	resources. This seems to be the default this is the default for Postgres, but MySQL/MariaDB seem to default to
	`HOLD_CURSORS_OVER_COMMIT`."
	resources. This this is the default for Postgres, but MySQL/MariaDB seem to default to `HOLD_CURSORS_OVER_COMMIT`."

		;; otherwise we don't need our savepoint anymore and we can release it.
		(.releaseSavepoint connection savepoint))

metabase.search namespace #42487

metabase.search namespace #42487

Conversation

camsaul commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay-io bot commented May 9, 2024 • edited

crisptrutski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisptrutski commented May 10, 2024

johnswanson commented May 10, 2024 • edited

johnswanson left a comment

Choose a reason for hiding this comment

piranha commented May 10, 2024

johnswanson commented May 10, 2024

tsmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 22, 2024

`metabase.search` namespace #42487

`metabase.search` namespace #42487

replay-io bot commented May 9, 2024 •

edited

johnswanson commented May 10, 2024 •

edited