Merge pull request #6631 from metabase/fix-6630

GA sync fixes 

src/metabase/driver/googleanalytics.clj

@@ -50,12 +50,13 @@
          (.getId profile))))
 
 (defn- describe-database [database]
+  ;; Include a `_metabase_metadata` table in the list of Tables so we can provide custom metadata. See below.
   {:tables (set (for [table-id (cons "_metabase_metadata" (profile-ids database))]
                   {:name   table-id
                    :schema nil}))})
 
 
-;;; ---------------------------------------- describe-table ----------------------------------------
+;;; ------------------------------------------------- describe-table -------------------------------------------------
 
 (def ^:private ^:const redundant-date-fields
   "Set of column IDs covered by `unit->ga-dimension` in the GA QP.
@@ -101,11 +102,13 @@
           column))))
 
 (defn- describe-columns [database]
-  (set (for [^Column column (columns database)]
-         {:name      (.getId column)
-          :base-type (if (= (.getId column) "ga:date")
-                       :type/Date
-                       (qp/ga-type->base-type (column-attribute column :dataType)))})))
+  (set (for [^Column column (columns database)
+             :let [ga-type (column-attribute column :dataType)]]
+         {:name          (.getId column)
+          :base-type     (if (= (.getId column) "ga:date")
+                           :type/Date
+                           (qp/ga-type->base-type ga-type))
+          :database-type ga-type})))
 
 (defn- describe-table [database table]
   {:name   (:name table)
@@ -113,15 +116,48 @@
    :fields (describe-columns database)})
 
 
-
-;;;---------------------------------------- can-connect?----------------------------------------
+;;; ----------------------------------------------- _metabase_metadata -----------------------------------------------
+
+;; The following is provided so we can specify custom display_names for Tables and Fields since there's not yet a way
+;; to do it directly in `describe-database` or `describe-table`. Just fake results for the Table in `table-rows-seq`
+;; (rows in `_metabase_metadata` are just property -> value, e.g. `<table>.display_name` -> `<display-name>`.)
+
+(defn- property+profile->display-name
+  "Format a table name for a GA property and GA profile"
+  [^Webproperty property, ^Profile profile]
+  (let [property-name (s/replace (.getName property) #"^https?://" "")
+        profile-name  (s/replace (.getName profile)  #"^https?://" "")]
+    ;; don't include the profile if it's the same as property-name or is the default "All Web Site Data"
+    (if (or (.contains property-name profile-name)
+            (= profile-name "All Web Site Data"))
+      property-name
+      (str property-name " (" profile-name ")"))))
+
+(defn- table-rows-seq [database table]
+  ;; this method is only supposed to be called for _metabase_metadata, make sure that's the case
+  {:pre [(= (:name table) "_metabase_metadata")]}
+  ;; now build a giant sequence of all the things we want to set
+  (apply concat
+         ;; set display_name for all the tables
+         (for [[^Webproperty property, ^Profile profile] (properties+profiles database)]
+           (cons {:keypath (str (.getId profile) ".display_name")
+                  :value   (property+profile->display-name property profile)}
+                 ;; set display_name and description for each column for this table
+                 (apply concat (for [^Column column (columns database)]
+                                 [{:keypath (str (.getId profile) \. (.getId column) ".display_name")
+                                   :value   (column-attribute column :uiName)}
+                                  {:keypath (str (.getId profile) \. (.getId column) ".description")
+                                   :value   (column-attribute column :description)}]))))))
+
+
+;;; -------------------------------------------------- can-connect? --------------------------------------------------
 
 (defn- can-connect? [details-map]
   {:pre [(map? details-map)]}
   (boolean (profile-ids {:details details-map})))
 
 
-;;;---------------------------------------- execute-query----------------------------------------
+;;; ------------------------------------------------- execute-query --------------------------------------------------
 
 (defn- column-with-name ^Column [database-or-id column-name]
   (some (fn [^Column column]
@@ -185,7 +221,7 @@
   (google/execute (mbql-query->request query)))
 
 
-;;; ---------------------------------------- Driver ----------------------------------------
+;;; ----------------------------------------------------- Driver -----------------------------------------------------
 
 (defrecord GoogleAnalyticsDriver []
   clojure.lang.Named
@@ -215,7 +251,8 @@
                                                   :required     true}])
           :execute-query            (u/drop-first-arg (partial qp/execute-query do-query))
           :process-query-in-context (u/drop-first-arg process-query-in-context)
-          :mbql->native             (u/drop-first-arg qp/mbql->native)}))
+          :mbql->native             (u/drop-first-arg qp/mbql->native)
+          :table-rows-seq           (u/drop-first-arg table-rows-seq)}))
 
 (defn -init-driver
   "Register the Google Analytics driver"

src/metabase/sync/sync_metadata/metabase_metadata.clj

 (ns metabase.sync.sync-metadata.metabase-metadata
-  "Logic for syncing the special `_metabase_metadata` table, which is a way for datasets
-   such as the Sample Dataset to specific properties such as special types that should
-   be applied during sync.
+  "Logic for syncing the special `_metabase_metadata` table, which is a way for datasets such as the Sample Dataset to
+  specific properties such as special types that should be applied during sync.
 
-   Currently, this is only used by the Sample Dataset, but theoretically in the future we could
-   add additional sample datasets and preconfigure them by populating this Table; or 3rd-party
-   applications or users can add this table to their database for an enhanced Metabase experience
-   out-of-the box."
+  Currently, this is only used by the Sample Dataset, but theoretically in the future we could add additional sample
+  datasets and preconfigure them by populating this Table; or 3rd-party applications or users can add this table to
+  their database for an enhanced Metabase experience out-of-the box."
   (:require [clojure.string :as str]
             [clojure.tools.logging :as log]
             [metabase
@@ -59,20 +57,23 @@
            k value))))))
 
 (s/defn ^:private sync-metabase-metadata-table!
-  "Databases may include a table named `_metabase_metadata` (case-insentive) which includes descriptions or other metadata about the `Tables` and `Fields`
-   it contains. This table is *not* synced normally, i.e. a Metabase `Table` is not created for it. Instead, *this* function is called, which reads the data it
-   contains and updates the relevant Metabase objects.
+  "Databases may include a table named `_metabase_metadata` (case-insentive) which includes descriptions or other
+  metadata about the `Tables` and `Fields` it contains. This table is *not* synced normally, i.e. a Metabase `Table`
+  is not created for it. Instead, *this* function is called, which reads the data it contains and updates the relevant
+  Metabase objects.
 
-   The table should have the following schema:
+  The table should have the following schema:
 
-     column  | type    | example
-     --------+---------+-------------------------------------------------
-     keypath | varchar | \"products.created_at.description\"
-     value   | varchar | \"The date the product was added to our catalog.\"
+    column  | type    | example
+    --------+---------+-------------------------------------------------
+    keypath | varchar | \"products.created_at.description\"
+    value   | varchar | \"The date the product was added to our catalog.\"
 
-   `keypath` is of the form `table-name.key` or `table-name.field-name.key`, where `key` is the name of some property of `Table` or `Field`.
+  `keypath` is of the form `table-name.key` or `table-name.field-name.key`, where `key` is the name of some property
+  of `Table` or `Field`.
 
-   This functionality is currently only used by the Sample Dataset. In order to use this functionality, drivers must implement optional fn `:table-rows-seq`."
+  This functionality is currently only used by the Sample Dataset. In order to use this functionality, drivers *must*
+  implement optional fn `:table-rows-seq`."
   [driver, database :- i/DatabaseInstance, metabase-metadata-table :- i/DatabaseMetadataTable]
   (doseq [{:keys [keypath value]} (driver/table-rows-seq driver database metabase-metadata-table)]
     (sync-util/with-error-handling (format "Error handling metabase metadata entry: set %s -> %s" keypath value)
@@ -90,7 +91,8 @@
    This table contains information about type information, descriptions, and other properties that
    should be set for Metabase objects like Tables and Fields."
   [database :- i/DatabaseInstance]
-  (sync-util/with-error-handling (format "Error syncing _metabase_metadata table for %s" (sync-util/name-for-logging database))
+  (sync-util/with-error-handling (format "Error syncing _metabase_metadata table for %s"
+                                         (sync-util/name-for-logging database))
     ;; If there's more than one metabase metadata table (in different schemas) we'll sync each one in turn.
     ;; Hopefully this is never the case.
     (doseq [table (:tables (fetch-metadata/db-metadata database))]

src/metabase/sync/sync_metadata/tables.clj

@@ -16,7 +16,7 @@
             [schema.core :as s]
             [toucan.db :as db]))
 
-;;; ------------------------------------------------------------  "Crufty" Tables ------------------------------------------------------------
+;;; ------------------------------------------------ "Crufty" Tables -------------------------------------------------
 
 ;; Crufty tables are ones we know are from frameworks like Rails or Django and thus automatically mark as `:cruft`
 
@@ -78,7 +78,7 @@
   (boolean (some #(re-find % (str/lower-case (:name table))) crufty-table-patterns)))
 
 
-;;; ------------------------------------------------------------ Syncing ------------------------------------------------------------
+;;; ---------------------------------------------------- Syncing -----------------------------------------------------
 
 ;; TODO - should we make this logic case-insensitive like it is for fields?
 
@@ -137,7 +137,8 @@
               :active true))))
 
 (s/defn sync-tables!
-  "Sync the Tables recorded in the Metabase application database with the ones obtained by calling DATABASE's driver's implementation of `describe-database`."
+  "Sync the Tables recorded in the Metabase application database with the ones obtained by calling DATABASE's driver's
+  implementation of `describe-database`."
   [database :- i/DatabaseInstance]
   ;; determine what's changed between what info we have and what's in the DB
   (let [db-metadata             (db-metadata database)
@@ -145,7 +146,8 @@
         [new-tables old-tables] (data/diff db-metadata our-metadata)]
     ;; create new tables as needed or mark them as active again
     (when (seq new-tables)
-      (sync-util/with-error-handling (format "Error creating/reactivating tables for %s" (sync-util/name-for-logging database))
+      (sync-util/with-error-handling (format "Error creating/reactivating tables for %s"
+                                             (sync-util/name-for-logging database))
         (create-or-reactivate-tables! database new-tables)))
     ;; mark old tables as inactive
     (when (seq old-tables)

test/metabase/driver/google_analytics_test.clj → test/metabase/driver/googleanalytics_test.clj

-(ns metabase.driver.google-analytics-test
+(ns metabase.driver.googleanalytics-test
   "Tests for the Google Analytics driver and query processor."
   (:require [expectations :refer :all]
             [metabase.driver.googleanalytics.query-processor :as qp]
             [metabase.query-processor.interface :as qpi]
             [metabase.util :as u]))
 
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; |                                                 QUERY "TRANSFORMATION                                                  |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                             QUERY "TRANSFORMATION"                                             |
+;;; +----------------------------------------------------------------------------------------------------------------+
 
 ;; check that a built-in Metric gets removed from the query and put in `:ga`
 (expect
@@ -46,9 +46,9 @@
                                              [:< 100 200]]}}))
 
 
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; |                                       MBQL->NATIVE (EXPANDED QUERY -> GA QUERY)                                        |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                   MBQL->NATIVE (EXPANDED QUERY -> GA QUERY)                                    |
+;;; +----------------------------------------------------------------------------------------------------------------+
 
 (defn- ga-query [inner-query]
   {:query (merge {:ids                "ga:0123456"