From 98bbb00151ce0c2d232e220f6ae57e0082b28f8d Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Wed, 10 Aug 2022 13:24:28 -0600
Subject: [PATCH] [Actions] Simplify emitter schema model (#24570)
* Move writeback migrations to 45
* Empty commit to trigger GitHub Actions
* [Actions] Simplify emitter schema model
emitter_action was dropped since emitters just have a singular action
and the join table was unecessary.
emitter_action.action_id columns moved onto emitter table.
Dropped CardEmitter and DashboardEmitter pre-insert, pre-update,
pre-delete since they were used in tests only and normal operation
would see the emitter inserted first.
Since previous code may have 'orphaned' emitters without an action, we
delete emitters without action to be safe.
* Handle flakiness with geojson java.net.UnknownHostException errors (#24523)
* Handle flakiness with geojson java.net.UnknownHostException errors
In CI seems like we are getting errant errors:
```clojure
geojson.clj:62
It validates URLs and files appropriately
http://0xc0000200
expected: (valid? geojson)
actual: #error {
:cause "Invalid IP address literal: 0xc0000200"
:via
[{:type clojure.lang.ExceptionInfo
:message "Invalid GeoJSON file location: must either start with http:// or https:// or be a relative path to a file on the classpath. URLs referring to hosts that supply internal hosting metadata are prohibited."
:data {:status-code 400, :url "http://0xc0000200"}
:at [metabase.api.geojson$valid_url_QMARK_ invokeStatic "geojson.clj" 62]}
{:type java.net.UnknownHostException
:message "0xc0000200"
:at [java.net.InetAddress getAllByName "InetAddress.java" 1340]}
{:type java.lang.IllegalArgumentException
:message "Invalid IP address literal: 0xc0000200"
:at [sun.net.util.IPAddressUtil validateNumericFormatV4 "IPAddressUtil.java" 150]}]
```
Not clear if this change has a hope of fixing it: if it doesn't resolve
once its possible it is cached somewhere in the network stack, or it
won't resolve if you ask again.
But gonna give it a shot.
Set the property `"networkaddress.cache.negative.ttl"` to `"0"`
> networkaddress.cache.negative.ttl (default: 10)
> Indicates the caching policy for un-successful name lookups from the name service. The value is specified as an integer to indicate the number of seconds to cache the failure for un-successful lookups.
> A value of 0 indicates "never cache". A value of -1 indicates "cache forever".
From
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/net/InetAddress.html
in the hopes that we can try multiple times. Restores the original value
after the test completes so we don't inadvertently change behavior
elsewhere.
If we get an error of java.net.UnknownHostException we try again if we
have attempts remaining. If we get a boolean it means the ip resolution
worked so we can rely on the response (checking if it resolves locally
or not)
* add a delay
* comment out test
Co-authored-by: Cam Saul <github@camsaul.com>
Co-authored-by: dpsutton <dan@dpsutton.com>
---
.../models/entity_id_test.clj | 1 -
resources/migrations/000_migrations.yaml | 63 +++++++++++++++++++
src/metabase/api/emitter.clj | 12 ++--
src/metabase/models.clj | 1 -
src/metabase/models/action.clj | 8 +--
src/metabase/models/emitter.clj | 53 +---------------
test/metabase/actions/test_util.clj | 7 +--
test/metabase/api/card_test.clj | 6 +-
test/metabase/api/dashboard_test.clj | 6 +-
test/metabase/cmd/copy_test.clj | 2 +-
10 files changed, 86 insertions(+), 73 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj b/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj
index 03c5d50f27e..f8f5aaaee50 100644
--- a/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/models/entity_id_test.clj
@@ -45,7 +45,6 @@
metabase.models.emitter.CardEmitterInstance
metabase.models.emitter.DashboardEmitterInstance
metabase.models.emitter.EmitterInstance
- metabase.models.emitter.EmitterActionInstance
metabase.models.field_values.FieldValuesInstance
metabase.models.login_history.LoginHistoryInstance
metabase.models.metric_important_field.MetricImportantFieldInstance
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index 36999093f56..06d9a06d371 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -12542,6 +12542,69 @@ databaseChangeLog:
columnNames: action_id
constraintName: pk_http_action
+ - changeSet:
+ id: v45.00-014
+ author: snoe
+ comment: Added 0.45.0 - writeback add action_id to emitter
+ changes:
+ - addColumn:
+ columns:
+ - column:
+ name: action_id
+ type: int
+ remarks: The related action
+ tableName: emitter
+
+ # FK constraint is added separately because deleteCascade doesn't work in addColumn -- see #14321
+ - changeSet:
+ id: v45.00-015
+ author: snoe
+ comment: Added 0.45.0 - writeback add fk for action_id to emitter
+ changes:
+ - addForeignKeyConstraint:
+ baseTableName: emitter
+ baseColumnNames: action_id
+ referencedTableName: action
+ referencedColumnNames: id
+ constraintName: fk_emitter_ref_action_id
+ onDelete: CASCADE
+
+ - changeSet:
+ id: v45.00-016
+ author: snoe
+ comment: Added 0.45.0 - writeback Move foreign keys from emitter_action and dashboard_emitter
+ changes:
+ - sql:
+ sql: >-
+ update emitter set
+ action_id = (select action_id from emitter_action where emitter_id = id);
+
+ - changeSet:
+ id: v45.00-017
+ author: snoe
+ comment: Added 0.45.0 - writeback remove any emitters that were orphaned to be safe.
+ changes:
+ - sql:
+ sql: delete from emitter where action_id is null;
+
+ - changeSet:
+ id: v45.00-018
+ author: snoe
+ comment: Added 0.45.0 - writeback Make emitter action_id not nullable
+ changes:
+ - addNotNullConstraint:
+ columnDataType: int
+ tableName: emitter
+ columnName: action_id
+
+ - changeSet:
+ id: v45.00-019
+ author: snoe
+ comment: Added 0.45.0 - writeback drop emitter_action
+ changes:
+ - dropTable:
+ tableName: emitter_action
+
# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
########################################################################################################################
diff --git a/src/metabase/api/emitter.clj b/src/metabase/api/emitter.clj
index 1bbc5e752ed..abdc5f670a5 100644
--- a/src/metabase/api/emitter.clj
+++ b/src/metabase/api/emitter.clj
@@ -4,8 +4,7 @@
[metabase.actions.http-action :as http-action]
[metabase.api.common :as api]
[metabase.mbql.schema :as mbql.s]
- [metabase.models
- :refer [CardEmitter DashboardEmitter Emitter EmitterAction]]
+ [metabase.models :refer [CardEmitter DashboardEmitter Emitter]]
[metabase.query-processor.writeback :as qp.writeback]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
@@ -19,9 +18,7 @@
[emitter-id]
(when-let [emitter (->> (db/query {:select [:*]
:from [[Emitter :emitter]]
- :left-join [[EmitterAction :emitter_action]
- [:= :emitter_action.emitter_id :emitter.id]
- ;; TODO -- not 100% sure we need CardEmitter and DashboardEmitter, we'd
+ :left-join [;; TODO -- not 100% sure we need CardEmitter and DashboardEmitter, we'd
;; only need that if we wanted to hydrate the Card or Dashboard they
;; came from.
[CardEmitter :card_emitter]
@@ -44,8 +41,9 @@
;; Create stuff the hard way by hand because H2 doesn't return the Emitter ID if you have Toucan `pre-insert` create
;; them for you because of some issue with INSERT RETURNING ROWS or something like that. See
;; [[metabase.models.emitter/pre-insert]] for more discussion.
- (let [emitter-id (u/the-id (db/insert! Emitter {:options options, :parameter_mappings parameter_mappings}))]
- (db/insert! EmitterAction {:emitter_id emitter-id, :action_id action_id})
+ (let [emitter-id (u/the-id (db/insert! Emitter {:options options,
+ :parameter_mappings parameter_mappings
+ :action_id action_id}))]
(cond
dashboard_id
(db/insert! DashboardEmitter {:emitter_id emitter-id, :dashboard_id dashboard_id})
diff --git a/src/metabase/models.clj b/src/metabase/models.clj
index 8497ebb1eb9..28b184c6999 100644
--- a/src/metabase/models.clj
+++ b/src/metabase/models.clj
@@ -105,7 +105,6 @@
[emitter CardEmitter]
[emitter DashboardEmitter]
[emitter Emitter]
- [emitter EmitterAction]
[field Field]
[field-values FieldValues]
[login-history LoginHistory]
diff --git a/src/metabase/models/action.clj b/src/metabase/models/action.clj
index 5546f0af655..6db464fb695 100644
--- a/src/metabase/models/action.clj
+++ b/src/metabase/models/action.clj
@@ -132,10 +132,8 @@
[emitters]
;; emitters apparently might actually be `[nil]` (not 100% sure why) so just make sure we're not doing anything dumb
;; if this is the case.
- (if-let [emitter-ids (seq (filter some? (map :id emitters)))]
- (let [emitter-actions (db/select 'EmitterAction :emitter_id [:in emitter-ids])
- action-id-by-emitter-id (into {} (map (juxt :emitter_id :action_id) emitter-actions))
- actions (m/index-by :id (select-actions :id [:in (map :action_id emitter-actions)]))]
+ (if-let [action-id-by-emitter-id (not-empty (into {} (map (juxt :id :action_id) (filter :id emitters))))]
+ (let [actions-by-id (m/index-by :id (select-actions :id [:in (map val action-id-by-emitter-id)]))]
(for [{emitter-id :id, :as emitter} emitters]
- (some-> emitter (assoc :action (get actions (get action-id-by-emitter-id emitter-id))))))
+ (some-> emitter (assoc :action (get actions-by-id (get action-id-by-emitter-id emitter-id))))))
emitters))
diff --git a/src/metabase/models/emitter.clj b/src/metabase/models/emitter.clj
index 867f56be756..3cc604b4fc0 100644
--- a/src/metabase/models/emitter.clj
+++ b/src/metabase/models/emitter.clj
@@ -12,7 +12,6 @@
(models/defmodel CardEmitter :card_emitter)
(models/defmodel DashboardEmitter :dashboard_emitter)
(models/defmodel Emitter :emitter)
-(models/defmodel EmitterAction :emitter_action)
(defn- normalize-parameter-mappings
[parameter-mappings]
@@ -33,58 +32,11 @@
:options :json})
:properties (constantly {:timestamped? true})}))
-(u/strict-extend
- (class EmitterAction)
- models/IModel
- (merge models/IModelDefaults
- ;; This is ok as long as we're 1:1
- {:primary-key (constantly :emitter_id)}))
-
-(defn- pre-insert
- "We currently support two ways of creating a CardEmitter or DashboardEmitter:
-
- 1. Automatically create the base `Emitter` and `EmitterAction` at the same time. Pass in `:action_id` but not
- `:emitter_id`. This was the original way we coded this but it actually causes a few issues since H2 doesn't seem
- to return the newly created object when you do things this way
-
- 2. Create the base `Emitter` and `EmitterAction` by hand. Pass in `:emitter_id` but not `:action_id`. This is
- probably preferable going forward because H2 does the right thing when you do it this way."
- [{action-id :action_id, emitter-id :emitter_id, :as emitter}]
- ;; if `emitter_id` was passed then assume the Base `Emitter` was created manually. Otherwise, create one
- ;; automatically.
- (let [base-emitter (if emitter-id
- (db/select-one Emitter :id emitter-id)
- (db/insert! Emitter (select-keys emitter [:parameter_mappings :options])))]
- ;; if `action_id` was passed then automatically create an `EmitterAction`. Otherwise assume it was created manually.
- (when action-id
- (db/insert! EmitterAction {:action_id action-id, :emitter_id (u/the-id base-emitter)}))
- ;; ok now Toucan should be able to create the `CardEmitter`/`DashboardEmitter` for us.
- (-> emitter
- (select-keys [:dashboard_id :card_id])
- (assoc :emitter_id (u/the-id base-emitter)))))
-
-(defn- pre-update
- [emitter]
- (when-some [base-emitter (not-empty (select-keys emitter [:parameter_mappings :options]))]
- (db/update! Emitter (:emitter_id emitter) base-emitter))
- (when-some [emitter-action (-> emitter
- (select-keys [:action_id])
- not-empty)]
- (db/update! EmitterAction (:emitter_id emitter) emitter-action))
- (not-empty (select-keys emitter [:emitter_id :dashboard_id :card_id])))
-
-(defn- pre-delete
- [emitter]
- (db/delete! Emitter :id (:emitter_id emitter))
- emitter)
-
(def ^:private Emitter-subtype-IModel-impl
"[[models/IModel]] impl for `DashboardEmitter` and `CardEmitter`"
(merge models/IModelDefaults
- {:primary-key (constantly :emitter_id) ; This is ok as long as we're 1:1
- :pre-delete pre-delete
- :pre-update pre-update
- :pre-insert pre-insert}))
+ ; This is ok as long as we're 1:1
+ {:primary-key (constantly :emitter_id)}))
(u/strict-extend (class DashboardEmitter)
models/IModel
@@ -107,6 +59,7 @@
(let [qualified-join-column (keyword (str "typed_emitter." (name join-column)))
emitters (->> {:select [qualified-join-column
:emitter.id
+ :emitter.action_id
:emitter.options
:emitter.parameter_mappings
:emitter.created_at
diff --git a/test/metabase/actions/test_util.clj b/test/metabase/actions/test_util.clj
index e2058cf17b9..f8860662d66 100644
--- a/test/metabase/actions/test_util.clj
+++ b/test/metabase/actions/test_util.clj
@@ -5,7 +5,7 @@
[metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
[metabase.http-client :as client]
[metabase.models :refer [Action Card CardEmitter Dashboard DashboardEmitter
- Database Emitter EmitterAction QueryAction]]
+ Database Emitter QueryAction]]
[metabase.models.action :as action]
[metabase.test :as mt]
[metabase.test.data.dataset-definitions :as defs]
@@ -168,7 +168,8 @@
(let [parent-model (db/resolve-model card-or-dashboard-model)]
(mt/with-temp* [parent-model [{emitter-parent-id :id}]
Emitter [{emitter-id :id} {:parameter_mappings {"my_id" [:variable [:template-tag "id"]]
- "my_fail" [:variable [:template-tag "fail"]]}}]]
+ "my_fail" [:variable [:template-tag "fail"]]}
+ :action_id action-id}]]
(testing "Sanity check: emitter-id should be non-nil"
(is (integer? emitter-id)))
(testing "Sanity check: make sure parameter mappings were defined the way we'd expect"
@@ -177,8 +178,6 @@
(db/select-one-field :parameter_mappings Emitter :id emitter-id))))
;; these are tied to the Card or Dashboad and Emitter above and will get cascade deleted. We can't use `with-temp*` for them
;; because it doesn't seem to work with tables with compound PKs
- (db/insert! EmitterAction {:emitter_id emitter-id
- :action_id action-id})
(condp = (type parent-model)
(type Card)
(db/insert! CardEmitter {:card_id emitter-parent-id
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 59259b34900..ff3880933dc 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -19,6 +19,7 @@
Collection
Dashboard
Database
+ Emitter
ModerationReview
PersistedInfo
Pulse
@@ -754,8 +755,9 @@
(testing "GET /api/card/:id"
(testing "Fetch card with an emitter"
(mt/with-temp* [Card [read-card {:name "Test Read Card"}]
- Card [write-card {:is_write true :name "Test Write Card"}]]
- (db/insert! CardEmitter {:action_id (u/the-id (db/select-one-field :action_id QueryAction :card_id (u/the-id write-card)))
+ Card [write-card {:is_write true :name "Test Write Card"}]
+ Emitter [{emitter-id :id} {:action_id (u/the-id (db/select-one-field :action_id QueryAction :card_id (u/the-id write-card)))}]]
+ (db/insert! CardEmitter {:emitter_id emitter-id
:card_id (u/the-id read-card)})
(testing "admin sees emitters"
(is (partial=
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index b7008af020c..2159615ba90 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -15,6 +15,7 @@
DashboardCard
DashboardCardSeries
DashboardEmitter
+ Emitter
Field
FieldValues
Pulse
@@ -345,8 +346,9 @@
(testing "GET /api/dashboard/:id"
(testing "Fetch dashboard with an emitter"
(mt/with-temp* [Dashboard [dashboard {:name "Test Dashboard"}]
- Card [write-card {:is_write true :name "Test Write Card"}]]
- (db/insert! DashboardEmitter {:action_id (u/the-id (db/select-one-field :action_id QueryAction :card_id (u/the-id write-card)))
+ Card [write-card {:is_write true :name "Test Write Card"}]
+ Emitter [{emitter-id :id} {:action_id (u/the-id (db/select-one-field :action_id QueryAction :card_id (u/the-id write-card)))}]]
+ (db/insert! DashboardEmitter {:emitter_id emitter-id
:dashboard_id (u/the-id dashboard)})
(testing "admin sees emitters"
(is (partial=
diff --git a/test/metabase/cmd/copy_test.clj b/test/metabase/cmd/copy_test.clj
index 2173f8a3c94..f420566e167 100644
--- a/test/metabase/cmd/copy_test.clj
+++ b/test/metabase/cmd/copy_test.clj
@@ -26,7 +26,7 @@
(let [migrated-model-names (set (map :name copy/entities))
;; Models that should *not* be migrated in `load-from-h2`.
models-to-exclude #{"TaskHistory" "Query" "QueryCache" "QueryExecution" "CardFavorite" "DashboardFavorite"
- "Action" "HTTPAction" "QueryAction" "CardEmitter" "DashboardEmitter" "EmitterAction"}
+ "Action" "HTTPAction" "QueryAction" "CardEmitter" "DashboardEmitter"}
all-model-names (set (for [ns u/metabase-namespace-symbols
:when (or (re-find #"^metabase\.models\." (name ns))
(= (name ns) "metabase.db.data-migrations"))
--
GitLab