Merge pull request #3341 from metabase/support-postgres-inet-type

Fixes for new type system 

frontend/src/metabase/lib/query.js

@@ -550,14 +550,14 @@ var Query = {
         }
     },
 
-    getFieldOptions(fields, includeJoins = false, filterFn = (fields) => fields, usedFields = {}) {
+    getFieldOptions(fields, includeJoins = false, filterFn = _.identity, usedFields = {}) {
         var results = {
             count: 0,
             fields: null,
             fks: []
         };
         // filter based on filterFn, then remove fks if they'll be duplicated in the joins fields
-        results.fields = filterFn(fields).filter((f) => !usedFields[f.id] && (isFK(f.special_type) || !includeJoins));
+        results.fields = filterFn(fields).filter((f) => !usedFields[f.id] && (!isFK(f.special_type) || !includeJoins));
         results.count += results.fields.length;
         if (includeJoins) {
             results.fks = fields.filter((f) => isFK(f.special_type) && f.target).map((joinField) => {

frontend/src/metabase/lib/schema_metadata.js

@@ -5,6 +5,7 @@ import { isa, isFK, TYPE } from "metabase/lib/types";
 // primary field types used for picking operators, etc
 export const NUMBER = "NUMBER";
 export const STRING = "STRING";
+export const STRING_LIKE = "STRING_LIKE";
 export const BOOLEAN = "BOOLEAN";
 export const DATE_TIME = "DATE_TIME";
 export const LOCATION = "LOCATION";
@@ -33,6 +34,9 @@ const TYPES = {
         base: [TYPE.Text],
         special: [TYPE.Text]
     },
+    [STRING_LIKE]: {
+        base: [TYPE.TextLike]
+    },
     [BOOLEAN]: {
         base: [TYPE.Boolean]
     },
@@ -89,10 +93,8 @@ export function isFieldType(type, field) {
 
 export function getFieldType(field) {
     // try more specific types first, then more generic types
-    for (let type of [DATE_TIME, LOCATION, COORDINATE, NUMBER, STRING, BOOLEAN]) {
-        if (isFieldType(type, field)) {
-            return type;
-        }
+    for (const type of [DATE_TIME, LOCATION, COORDINATE, NUMBER, STRING, STRING_LIKE, BOOLEAN]) {
+        if (isFieldType(type, field)) return type;
     }
 }
 
@@ -270,6 +272,12 @@ const OPERATORS_BY_TYPE_ORDERED = {
         { name: "STARTS_WITH",      verboseName: "Starts with", advanced: true},
         { name: "ENDS_WITH",        verboseName: "Ends with", advanced: true}
     ],
+    [STRING_LIKE]: [
+        { name: "=",                verboseName: "Is" },
+        { name: "!=",               verboseName: "Is not" },
+        { name: "IS_NULL",          verboseName: "Is empty", advanced: true },
+        { name: "NOT_NULL",         verboseName: "Not empty", advanced: true }
+    ],
     [DATE_TIME]: [
         { name: "=",                verboseName: "Is" },
         { name: "<",                verboseName: "Before" },
@@ -315,10 +323,10 @@ const MORE_VERBOSE_NAMES = {
 }
 
 export function getOperators(field, table) {
-    let type = getFieldType(field) || UNKNOWN;
+    const type = getFieldType(field) || UNKNOWN;
     return OPERATORS_BY_TYPE_ORDERED[type].map(operatorForType => {
-        let operator = OPERATORS[operatorForType.name];
-        let verboseNameLower = operatorForType.verboseName.toLowerCase();
+        const operator = OPERATORS[operatorForType.name];
+        const verboseNameLower = operatorForType.verboseName.toLowerCase();
         return {
             ...operator,
             ...operatorForType,
@@ -428,11 +436,7 @@ function populateFields(aggregator, fields) {
 // TODO: unit test
 export function getAggregators(table) {
     const supportedAggregations = Aggregators.filter(function (agg) {
-        if (agg.requiredDriverFeature && table.db && !_.contains(table.db.features, agg.requiredDriverFeature)) {
-            return false;
-        } else {
-            return true;
-        }
+        return !(agg.requiredDriverFeature && table.db && !_.contains(table.db.features, agg.requiredDriverFeature));
     });
     return _.map(supportedAggregations, function(aggregator) {
         return populateFields(aggregator, table.fields);
@@ -469,7 +473,7 @@ export function addValidOperatorsToFields(table) {
 export function hasLatitudeAndLongitudeColumns(columnDefs) {
     let hasLatitude = false;
     let hasLongitude = false;
-    for (let col of columnDefs) {
+    for (const col of columnDefs) {
         if (isa(col.special_type, TYPE.Latitude)) {
             hasLatitude = true;
         }
@@ -503,6 +507,7 @@ export const ICON_MAPPING = {
     [LOCATION]: 'location',
     [COORDINATE]: 'location',
     [STRING]: 'string',
+    [STRING_LIKE]: 'string',
     [NUMBER]: 'int',
     [BOOLEAN]: 'io'
 };

frontend/test/metabase-bootstrap.js

@@ -31,6 +31,7 @@ window.MetabaseBootstrap = {
         "type/Dictionary":                ["type/Collection"],
         "type/FK":                        ["type/Special"],
         "type/Float":                     ["type/Number"],
+        "type/IPAddress":                 ["type/TextLike"],
         "type/ImageURL":                  ["type/URL"],
         "type/Integer":                   ["type/Number"],
         "type/Latitude":                  ["type/Coordinate"],
@@ -42,6 +43,7 @@ window.MetabaseBootstrap = {
         "type/Special":                   ["type/*"],
         "type/State":                     ["type/Category", "type/Address", "type/Text"],
         "type/Text":                      ["type/*"],
+        "type/TextLike":                  ["type/*"],
         "type/Time":                      ["type/DateTime"],
         "type/UNIXTimestamp":             ["type/Integer", "type/DateTime"],
         "type/UNIXTimestampMilliseconds": ["type/UNIXTimestamp"],

frontend/test/unit/lib/schema_metadata.spec.js

@@ -2,6 +2,7 @@ import {
     getFieldType,
     DATE_TIME,
     STRING,
+    STRING_LIKE,
     NUMBER,
     BOOLEAN,
     LOCATION,
@@ -40,6 +41,10 @@ describe('schema_metadata', () => {
             expect(getFieldType({ special_type: TYPE.Latitude })).toEqual(COORDINATE)
             expect(getFieldType({ special_type: TYPE.Longitude })).toEqual(COORDINATE)
         });
+        it('should know something that is string-like', () => {
+            expect(getFieldType({ base_type: TYPE.TextLike })).toEqual(STRING_LIKE);
+            expect(getFieldType({ base_type: TYPE.IPAddress })).toEqual(STRING_LIKE);
+        });
         it('should know what it doesn\'t know', () => {
             expect(getFieldType({ base_type: 'DERP DERP DERP' })).toEqual(undefined)
         });

sample_dataset/metabase/sample_dataset/generate.clj

 (ns metabase.sample-dataset.generate
+  "Logic for generating the sample dataset.
+   Run this with `lein generate-sample-dataset`."
   (:require (clojure.java [io :as io]
                           [jdbc :as jdbc])
             [clojure.math.numeric-tower :as math]
@@ -316,7 +318,7 @@
     :field        "PRODUCT_ID"
     :dest-table   "PRODUCTS"}])
 
-(def ^:private ^:const annotations
+(def ^:private ^:const metabase-metadata
   {:orders   {:description "This is a confirmed order for a product from a user."
               :columns     {:created_at {:description "The date and time an order was submitted."}
                             :id         {:description "This is a unique ID for the product. It is also called the “Invoice number” or “Confirmation number” in customer facing emails and screens."}
@@ -341,7 +343,7 @@
                           :source     {:description "The channel through which we acquired this user. Valid values include: Affiliate, Facebook, Google, Organic and Twitter"}
                           :state      {:description "The state or province of the account’s billing address"}
                           :zip        {:description  "The postal code of the account’s billing address"
-                                       :special_type :type/ZipCode}}}
+                                       :special_type "type/ZipCode"}}}
    :products {:description "This is our product catalog. It includes all products ever sold by the Sample Company."
               :columns      {:category   {:description "The type of product, valid values include: Doohicky, Gadget, Gizmo and Widget"}
                              :created_at {:description "The date the product was added to our catalog."}
@@ -353,7 +355,7 @@
                              :vendor     {:description "The source of the product."}}}
    :reviews  {:description "These are reviews our customers have left on products. Note that these are not tied to orders so it is possible people have reviewed products they did not purchase from us."
               :columns     {:body       {:description  "The review the user left. Limited to 2000 characters."
-                                         :special_type :type/Description}
+                                         :special_type "type/Description"}
                             :created_at {:description "The day and time a review was written by a user."}
                             :id         {:description "A unique internal identifier for the review. Should not be used externally."}
                             :product_id {:description "The product the review was for"}
@@ -396,12 +398,12 @@
      ;; Insert the _metabase_metadata table
      (println "Inserting _metabase_metadata...")
      (jdbc/execute! db ["CREATE TABLE \"_METABASE_METADATA\" (\"KEYPATH\" VARCHAR(255), \"VALUE\" VARCHAR(255), PRIMARY KEY (\"KEYPATH\"));"])
-     (jdbc/insert-multi! db "_METABASE_METADATA" (reduce concat (for [[table-name {table-description :description, columns :columns}] annotations]
+     (jdbc/insert-multi! db "_METABASE_METADATA" (reduce concat (for [[table-name {table-description :description, columns :columns}] metabase-metadata]
                                                                   (let [table-name (s/upper-case (name table-name))]
                                                                     (conj (for [[column-name kvs] columns
                                                                                 [k v]             kvs]
                                                                             {:keypath (format "%s.%s.%s" table-name (s/upper-case (name column-name)) (name k))
-                                                                             :value   (name v)})
+                                                                             :value   v})
                                                                           {:keypath (format "%s.description" table-name)
                                                                            :value table-description})))))
 
@@ -416,4 +418,5 @@
 (defn -main [& [filename]]
   (let [filename (or filename sample-dataset-filename)]
     (println (format "Writing sample dataset to %s..." filename))
-    (create-h2-db filename)))
+    (create-h2-db filename)
+    (System/exit 0)))

src/metabase/driver/postgres.clj

@@ -34,7 +34,7 @@
     :float4        :type/Float
     :float8        :type/Float
     :geometry      :type/*
-    :inet          :type/Text
+    :inet          :type/IPAddress
     :int           :type/Integer
     :int2          :type/Integer
     :int4          :type/Integer
@@ -81,8 +81,10 @@
   "Attempt to determine the special-type of a Field given its name and Postgres column type."
   [column-name column-type]
   ;; this is really, really simple right now.  if its postgres :json type then it's :type/SerializedJSON special-type
-  (when (= column-type :json)
-    :type/SerializedJSON))
+  (case column-type
+    :json :type/SerializedJSON
+    :inet :type/IPAddress
+    nil))
 
 (def ^:const ssl-params
   "Params to include in the JDBC connection spec for an SSL connection."
@@ -168,10 +170,12 @@
     message))
 
 (defn- prepare-value [{value :value, {:keys [base-type]} :field}]
-  (if (and (isa? base-type :type/UUID)
-           value)
-    (UUID/fromString value)
-    value))
+  (if-not value
+    value
+    (cond
+      (isa? base-type :type/UUID)      (UUID/fromString value)
+      (isa? base-type :type/IPAddress) (hx/cast :inet value)
+      :else                            value)))
 
 
 (defn- materialized-views

src/metabase/sync_database/sync.clj

@@ -47,22 +47,24 @@
   (doseq [{:keys [keypath value]} (driver/table-rows-seq driver database metabase-metadata-table)]
     ;; TODO: this does not support schemas in dbs :(
     (let [[_ table-name field-name k] (re-matches #"^([^.]+)\.(?:([^.]+)\.)?([^.]+)$" keypath)]
-      (try (when-not (if field-name
-                       (when-let [table-id (db/select-one-id Table
-                                             ;; TODO: this needs to support schemas
-                                             ;; TODO: eventually limit this to "core" schema tables
-                                             :db_id  (:id database)
-                                             :name   table-name
-                                             :active true)]
-                         (db/update-where! Field {:name     field-name
-                                                  :table_id table-id}
+      ;; ignore legacy entries that try to set field_type since it's no longer part of Field
+      (when-not (= (keyword k) :field_type)
+        (try (when-not (if field-name
+                         (when-let [table-id (db/select-one-id Table
+                                               ;; TODO: this needs to support schemas
+                                               ;; TODO: eventually limit this to "core" schema tables
+                                               :db_id  (:id database)
+                                               :name   table-name
+                                               :active true)]
+                           (db/update-where! Field {:name     field-name
+                                                    :table_id table-id}
+                             (keyword k) value))
+                         (db/update-where! Table {:name  table-name
+                                                  :db_id (:id database)}
                            (keyword k) value))
-                       (db/update-where! Table {:name  table-name
-                                                :db_id (:id database)}
-                         (keyword k) value))
-             (log/error (u/format-color "Error syncing _metabase_metadata: no matching keypath: %s" keypath)))
-           (catch Throwable e
-             (log/error (u/format-color 'red "Error in _metabase_metadata: %s" (.getMessage e))))))))
+               (log/error (u/format-color "Error syncing _metabase_metadata: no matching keypath: %s" keypath)))
+             (catch Throwable e
+               (log/error (u/format-color 'red "Error in _metabase_metadata: %s" (.getMessage e)))))))))
 
 
 (defn- save-table-fields!

src/metabase/types.clj

@@ -59,6 +59,11 @@
 
 (derive :type/Boolean :type/*)
 
+;;; Text-Like Types: Things that should be displayed as text for most purposes but that shouldn't support advanced filter options like starts with / contains
+
+(derive :type/TextLike :type/*)
+(derive :type/IPAddress :type/TextLike)
+
 ;;; "Virtual" Types
 
 (derive :type/Address :type/*)

test/metabase/driver/postgres_test.clj

@@ -62,7 +62,7 @@
 
 
 ;;; # UUID Support
-(i/def-database-definition ^:const ^:private with-uuid
+(i/def-database-definition ^:private with-uuid
   ["users"
    [{:field-name "user_id", :base-type :type/UUID}]
    [[#uuid "4f01dcfd-13f7-430c-8e6f-e505c0851027"]
@@ -99,7 +99,7 @@
 
 
 ;; Make sure that Tables / Fields with dots in their names get escaped properly
-(i/def-database-definition ^:const ^:private dots-in-names
+(i/def-database-definition ^:private dots-in-names
   ["objects.stuff"
    [{:field-name "dotted.name", :base-type :type/Text}]
    [["toucan_cage"]
@@ -117,7 +117,7 @@
 
 
 ;; Make sure that duplicate column names (e.g. caused by using a FK) still return both columns
-(i/def-database-definition ^:const ^:private duplicate-names
+(i/def-database-definition ^:private duplicate-names
   ["birds"
    [{:field-name "name", :base-type :type/Text}]
    [["Rasta"]
@@ -136,6 +136,22 @@
       :data (dissoc :cols :native_form)))
 
 
+;;; Check support for `inet` columns
+(i/def-database-definition ^:private ip-addresses
+  ["addresses"
+   [{:field-name "ip", :base-type {:native "inet"}}]
+   [[(hsql/raw "'192.168.1.1'::inet")]
+    [(hsql/raw "'10.4.4.15'::inet")]]])
+
+;; Filtering by inet columns should add the appropriate SQL cast, e.g. `cast('192.168.1.1' AS inet)` (otherwise this wouldn't work)
+(expect-with-engine :postgres
+  [[1]]
+  (rows (data/dataset metabase.driver.postgres-test/ip-addresses
+          (data/run-query addresses
+            (ql/aggregation (ql/count))
+            (ql/filter (ql/= $ip "192.168.1.1"))))))
+
+
 ;; Check that we properly fetch materialized views.
 ;; As discussed in #2355 they don't come back from JDBC `DatabaseMetadata` so we have to fetch them manually.
 (expect-with-engine :postgres

test/metabase/test/data/postgres.clj

@@ -16,6 +16,7 @@
    :type/Decimal    "DECIMAL"
    :type/Float      "FLOAT"
    :type/Integer    "INTEGER"
+   :type/IPAddress  "INET"
    :type/Text       "TEXT"
    :type/Time       "TIME"
    :type/UUID       "UUID"})