Merge pull request #7624 from metabase/sync-dont-overwrite-special-types

Don't overwrite PKs and FKs when infering special_type

src/metabase/sync/analyze/classifiers/name.clj

@@ -119,11 +119,14 @@
 (s/defn infer-special-type :- (s/maybe i/FieldInstance)
   "Classifer that infers the special type of a FIELD based on its name and base type."
   [field :- i/FieldInstance, _ :- (s/maybe i/Fingerprint)]
-  (when-let [inferred-special-type (special-type-for-name-and-base-type (:name field) (:base_type field))]
-    (log/debug (format "Based on the name of %s, we're giving it a special type of %s."
-                       (sync-util/name-for-logging field)
-                       inferred-special-type))
-    (assoc field :special_type inferred-special-type)))
+  ;; Don't overwrite keys, else we're ok with overwriting as a new more precise type might have
+  ;; been added.
+  (when (not-any? (partial isa? (:special_type field)) [:type/PK :type/FK])
+    (when-let [inferred-special-type (special-type-for-name-and-base-type (:name field) (:base_type field))]
+      (log/debug (format "Based on the name of %s, we're giving it a special type of %s."
+                         (sync-util/name-for-logging field)
+                         inferred-special-type))
+      (assoc field :special_type inferred-special-type))))
 
 (defn- prefix-or-postfix
   [s]

test/metabase/sync/analyze/classifiers/name_test.clj

 (ns metabase.sync.analyze.classifiers.name-test
   (:require [expectations :refer :all]
-            [metabase.models.table :as table]
-            [metabase.sync.analyze.classifiers.name :refer :all]))
+            [metabase.models
+             [field :refer [Field]]
+             [table :as table :refer [Table]]]
+            [metabase.sync.analyze.classifiers.name :refer :all]
+            [metabase.test.data :as data]
+            [metabase.util :as u]
+            [toucan.util.test :as tt]))
 
 ;; Postfix + pluralization
 (expect
@@ -22,3 +27,24 @@
 (expect
   :entity/GenericTable
   (-> {:name "foo"} table/map->TableInstance infer-entity-type :entity_type))
+
+
+;; Don't overwrite PK/FK `special_type`s.
+(expect
+  nil
+  (tt/with-temp* [Table [{table-id :id}]
+                  Field [{field-id :id} {:table_id     table-id
+                                         :special_type :type/FK
+                                         :name         "City"
+                                         :base_type    :type/Text}]]
+    (-> field-id Field (infer-special-type nil) :special_type)))
+
+;; ... but overwrite other types to alow evolution of our type system
+(expect
+  :type/City
+  (tt/with-temp* [Table [{table-id :id}]
+                  Field [{field-id :id} {:table_id     table-id
+                                         :special_type :type/Category
+                                         :name         "City"
+                                         :base_type    :type/Text}]]
+    (-> field-id Field (infer-special-type nil) :special_type)))