Implicitly delete obsolete FieldValues when fetching the current ones #38396
Conversation
|
|
Is it better to write a migration to delete duplicates then add a unique constraint on |
crisptrutski
force-pushed
the
dont-forget-full
branch
from
404c29f
to
7cc40ff
Compare
That's the end goal - see discussion on #37794. This is a short term idea. |
crisptrutski
force-pushed
the
dont-forget-full
branch
from
7cc40ff
to
efbc8e3
Compare
| @@ -493,7 +516,8 @@ | |||
| (defmethod serdes/load-find-local "FieldValues" [path] | |||
| ;; Delegate to finding the parent Field, then look up its corresponding FieldValues. | |||
| (let [field (serdes/load-find-local (pop path))] | |||
| (t2/select-one FieldValues :field_id (:id field)))) | |||
There was a problem hiding this comment.
Note: this fixes a subtle bug where we could lose a customer's custom field labels
crisptrutski
force-pushed
the
dont-forget-full
branch
from
b005c38
to
1dd0c8f
Compare
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
There was a problem hiding this comment.
Awesome. Just left some nits. This is strictly better than the current situation where there's unpredictable behaviour in the case of duplicate FieldValues instances. The downside of this approach is it's not particularly future-proof to stop the original problem happening again. We'll need the uniqueness constraint and a toucan hook to require :type in a select for that.
| then (t/plus now (t/millis 1))] | ||
| (mt/with-temp [Database {database-id :id} {} | ||
| Table {table-id :id} {:db_id database-id} | ||
| Field {field-id :id} {:table_id table-id} |
There was a problem hiding this comment.
| Field {field-id :id} {:table_id table-id} | |
| Field {field-id :id} {:table_id table-id} |
| (deftest implicit-deduplication-test | ||
| (let [now (t/zoned-date-time) | ||
| then (t/plus now (t/millis 1))] | ||
| (mt/with-temp [Database {database-id :id} {} |
There was a problem hiding this comment.
nit: technically vars for model identifiers are deprecated and apparently meant to be removed one day. We should honor that (even if I don't believe it will happen soon)
e.g.
| (mt/with-temp [Database {database-id :id} {} | |
| (mt/with-temp [:model/Database {database-id :id} {} |
|
The plan is indeed to follow with a db constraint and hooks, I just feel it's worth fixing the pain sooner than later, and the new "higher level" mutation methods will encapsulate the retries for concurrent inserts. |
|
@crisptrutski Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Relates to #668
Description
Before this change it was non-deterministic which FieldValues would be used in the presence of stale records. This change ensures that we always use the values that were last inserted, and delete the older rows in the process.
It also fixes a query that failed to specify the
:type, and so was totally non-deterministic.Checklist
Implement a Snowplow event to track when customer databases encounter duplicatesGoing to use idempotent operations for all inserts, which will make it safe to insert the constraint. Don't think events will be worth the faff.