fix things

src/metabase/sync_database/analyze.clj

@@ -123,8 +123,8 @@
   "Mark FIELD as `:json` if it's textual, doesn't already have a special type, the majority of it's values are non-nil, and all of its non-nil values
    are valid serialized JSON dictionaries or arrays."
   [driver field field-stats]
-  (if-not (and (not (:special_type field))
-               (not (isa? (:base_type field) :type/Text)))
+  (if (or (:special_type field)
+          (not (isa? (:base_type field) :type/Text)))
     ;; this field isn't suited for this test
     field-stats
     ;; check for json values
@@ -134,14 +134,8 @@
         (log/debug (u/format-color 'green "Field '%s' looks like it contains valid JSON objects. Setting special_type to :type/JSON." (field/qualified-name field)))
         (assoc field-stats :special-type :type/JSON, :preview-display false)))))
 
-(defn valid-email?
-  [email]
-  (let [pattern #"[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?"]
-    (and (string? email) (re-matches pattern email))))
-
-
 (defn- values-are-valid-emails?
-  "`true` if at every item in VALUES is `nil` or a valid string-encoded JSON dictionary or array, and at least one of those is non-nil."
+  "`true` if at every item in VALUES is `nil` or a valid email, and at least one of those is non-nil."
   [values]
   (try
     (loop [at-least-one-non-nil-value? false, [val & more] values]
@@ -151,25 +145,25 @@
         (s/blank? val)         (recur at-least-one-non-nil-value? more)
         ;; If val is non-nil, check that it's a JSON dictionary or array. We don't want to mark Fields containing other
         ;; types of valid JSON values as :json (e.g. a string representation of a number or boolean)
-        :else                  (do (assert (valid-email? val))
+        :else                  (do (assert (u/is-email? val))
                                    (recur true more))))
     (catch Throwable _
       false)))
 
 (defn- test:email-special-type
-  "Mark FIELD as `:json` if it's textual, doesn't already have a special type, the majority of it's values are non-nil, and all of its non-nil values
-   are valid serialized JSON dictionaries or arrays."
+  "Mark FIELD as `:email` if it's textual, doesn't already have a special type, the majority of it's values are non-nil, and all of its non-nil values
+   are valid emails."
   [driver field field-stats]
-  (if-not (and (not (:special_type field))
-               (not (isa? (:base_type field) :type/Text)))
+  (if (or (:special_type field)
+          (not (isa? (:base_type field) :type/Text)))
     ;; this field isn't suited for this test
     field-stats
-    ;; check for json values
+    ;; check for emails
     (if-not (values-are-valid-emails? (take driver/max-sync-lazy-seq-results (driver/field-values-lazy-seq driver field)))
       field-stats
       (do
         (log/debug (u/format-color 'green "Field '%s' looks like it contains valid email addresses. Setting special_type to :type/Email." (field/qualified-name field)))
-        (assoc field-stats :special-type :type/Email, :preview-display false)))))
+        (assoc field-stats :special-type :type/Email, :preview-display true)))))
 
 
 
@@ -180,7 +174,8 @@
   (->> field-stats
        (test:no-preview-display driver field)
        (test:url-special-type   driver field)
-       (test:json-special-type  driver field)))
+       (test:json-special-type  driver field)
+       (test:email-special-type  driver field)))
 
 (defn make-analyze-table
   "Make a generic implementation of `analyze-table`."

test/metabase/sync_database/analyze_test.clj

@@ -23,6 +23,7 @@
 ;;; ## mark-json-field!
 
 (tu/resolve-private-vars metabase.sync-database.analyze values-are-valid-json?)
+(tu/resolve-private-vars metabase.sync-database.analyze values-are-valid-emails?)
 
 (def ^:const ^:private fake-values-seq-json
   "A sequence of values that should be marked is valid JSON.")
@@ -62,10 +63,12 @@
 (expect false (values-are-valid-json? ["false"]))
 
 ;; Check that things that are valid emails are marked as Emails
-(expect true (values-are-valid-email? ["helper@metabase.com"]))
-(expect true (values-are-valid-email? ["helper@metabase.com", "someone@here.com", "help@nope.com"]))
+(expect true (values-are-valid-emails? ["helper@metabase.com"]))
+(expect true (values-are-valid-emails? ["helper@metabase.com", "someone@here.com", "help@nope.com"]))
+(expect true (values-are-valid-emails? ["helper@metabase.com", nil, "help@nope.com"]))
 
-(expect false (values-are-valid-email? ["\"A string should not cause a Field to be marked as email\""]))
-(expect false (values-are-valid-email? [100]))
-(expect false (values-are-valid-email? ["true"]))
-(expect false (values-are-valid-email? ["false"]))
+(expect false (values-are-valid-emails? ["helper@metabase.com", "1111IsNot!An....email", "help@nope.com"]))
+(expect false (values-are-valid-emails? ["\"A string should not cause a Field to be marked as email\""]))
+(expect false (values-are-valid-emails? [100]))
+(expect false (values-are-valid-emails? ["true"]))
+(expect false (values-are-valid-emails? ["false"]))