From c6fbc1ff677449c1c33de3240f043aa4727959a0 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Thu, 9 Mar 2023 22:59:08 +0700
Subject: [PATCH] Adds `metabase_field.database_auto_incremented` (#28942)
---
.../test/metabase/driver/snowflake_test.clj | 2 +
.../test/metabase/driver/sqlite_test.clj | 3 +-
resources/migrations/000_migrations.yaml | 16 +++++
.../driver/sql_jdbc/sync/describe_table.clj | 25 ++++---
src/metabase/models/field.clj | 1 -
src/metabase/sync/interface.clj | 29 ++++----
.../sync_metadata/fields/fetch_metadata.clj | 27 ++++----
.../sync_metadata/fields/sync_instances.clj | 33 ++++-----
.../sync_metadata/fields/sync_metadata.clj | 36 ++++++----
test/metabase/api/database_test.clj | 6 +-
test/metabase/api/field_test.clj | 4 +-
test/metabase/api/table_test.clj | 54 +++++++++------
test/metabase/driver/postgres_test.clj | 35 +++++-----
.../sql_jdbc/sync/describe_table_test.clj | 49 ++++++++++++--
test/metabase/driver/sql_jdbc_test.clj | 18 +++--
test/metabase/sync/sync_dynamic_test.clj | 6 +-
.../fields/fetch_metadata_test.clj | 22 ++++--
.../fields/sync_metadata_test.clj | 67 ++++++++++++++++---
test/metabase/sync_test.clj | 7 ++
test/metabase/test/mock/toucanery.clj | 20 +++++-
test/metabase/test/mock/util.clj | 1 +
21 files changed, 316 insertions(+), 145 deletions(-)
diff --git a/modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj b/modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj
index 1ea9689f1d2..de86971dab3 100644
--- a/modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj
+++ b/modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj
@@ -129,11 +129,13 @@
:base-type :type/Number
:pk? true
:database-position 0
+ :database-is-auto-increment true
:database-required false}
{:name "name"
:database-type "VARCHAR"
:base-type :type/Text
:database-position 1
+ :database-is-auto-increment false
:database-required true}}}
(driver/describe-table :snowflake (assoc (mt/db) :name "ABC") (db/select-one Table :id (mt/id :categories))))))))
diff --git a/modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj b/modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj
index fc3f37e3eaf..721ed950462 100644
--- a/modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj
+++ b/modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj
@@ -147,7 +147,8 @@
:database-type "TIMESTAMP"
:base-type :type/DateTime
:database-position 0
- :database-required false}}}
+ :database-required false
+ :database-is-auto-increment false}}}
(driver/describe-table driver db (db/select-one Table :id (mt/id :timestamp_table)))))))))))))
(deftest select-query-datetime
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index f0b44a3c74b..26abef3f221 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -14300,6 +14300,22 @@ databaseChangeLog:
constraintName: fk_action_model_id
onDelete: CASCADE
+ - changeSet:
+ id: v47.00-004
+ author: qnkhuat
+ comment: Added 0.47.0 - Add auto_incremented to metabase_field
+ changes:
+ - addColumn:
+ tableName: metabase_field
+ columns:
+ - column:
+ name: database_is_auto_increment
+ type: boolean
+ remarks: Indicates this field is auto incremented
+ defaultValueBoolean: false
+ constraints:
+ nullable: false
+
# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
########################################################################################################################
diff --git a/src/metabase/driver/sql_jdbc/sync/describe_table.clj b/src/metabase/driver/sql_jdbc/sync/describe_table.clj
index ce61f5d887c..49720daafdc 100644
--- a/src/metabase/driver/sql_jdbc/sync/describe_table.clj
+++ b/src/metabase/driver/sql_jdbc/sync/describe_table.clj
@@ -98,18 +98,21 @@
nil)
(fn [^ResultSet rs]
;; https://docs.oracle.com/javase/7/docs/api/java/sql/DatabaseMetaData.html#getColumns(java.lang.String,%20java.lang.String,%20java.lang.String,%20java.lang.String)
- #(let [default (.getString rs "COLUMN_DEF")
- no-default? (contains? #{nil "NULL" "null"} default)
- nullable (.getInt rs "NULLABLE")
- not-nullable? (= 0 nullable)
- auto-increment (.getString rs "IS_AUTOINCREMENT")
+ #(let [default (.getString rs "COLUMN_DEF")
+ no-default? (contains? #{nil "NULL" "null"} default)
+ nullable (.getInt rs "NULLABLE")
+ not-nullable? (= 0 nullable)
+ ;; IS_AUTOINCREMENT could return nil
+ auto-increment (.getString rs "IS_AUTOINCREMENT")
+ auto-increment? (= "YES" auto-increment)
no-auto-increment? (= "NO" auto-increment)
- column-name (.getString rs "COLUMN_NAME")
- required? (and no-default? not-nullable? no-auto-increment?)]
+ column-name (.getString rs "COLUMN_NAME")
+ required? (and no-default? not-nullable? no-auto-increment?)]
(merge
- {:name column-name
- :database-type (.getString rs "TYPE_NAME")
- :database-required required?}
+ {:name column-name
+ :database-type (.getString rs "TYPE_NAME")
+ :database-is-auto-increment auto-increment?
+ :database-required required?}
(when-let [remarks (.getString rs "REMARKS")]
(when-not (str/blank? remarks)
{:field-comment remarks})))))))
@@ -153,7 +156,7 @@
(map-indexed (fn [i {:keys [database-type], column-name :name, :as col}]
(let [semantic-type (calculated-semantic-type driver column-name database-type)]
(merge
- (u/select-non-nil-keys col [:name :database-type :field-comment :database-required])
+ (u/select-non-nil-keys col [:name :database-type :field-comment :database-required :database-is-auto-increment])
{:base-type (database-type->base-type-or-warn driver database-type)
:database-position i}
(when semantic-type
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index 28bcf773b3b..a835e451f13 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -183,7 +183,6 @@
:in mi/json-in
:out (comp update-semantic-numeric-values mi/json-out-with-keywordization))
-
(mi/define-methods
Field
{:hydration-keys (constantly [:destination :field :origin :human_readable_field])
diff --git a/src/metabase/sync/interface.clj b/src/metabase/sync/interface.clj
index 7146c6e7743..b9f1b5c6dad 100644
--- a/src/metabase/sync/interface.clj
+++ b/src/metabase/sync/interface.clj
@@ -22,21 +22,22 @@
(def TableMetadataField
"Schema for a given Field as provided in `describe-table`."
- {:name su/NonBlankString
- :database-type (s/maybe su/NonBlankString) ; blank if the Field is all NULL & untyped, i.e. in Mongo
- :base-type su/FieldType
- :database-position su/IntGreaterThanOrEqualToZero
- (s/optional-key :semantic-type) (s/maybe su/FieldSemanticOrRelationType)
- (s/optional-key :effective-type) (s/maybe su/FieldType)
- (s/optional-key :coercion-strategy) (s/maybe su/CoercionStrategy)
- (s/optional-key :field-comment) (s/maybe su/NonBlankString)
- (s/optional-key :pk?) s/Bool
- (s/optional-key :nested-fields) #{(s/recursive #'TableMetadataField)}
- (s/optional-key :nfc-path) [s/Any]
- (s/optional-key :custom) {s/Any s/Any}
- (s/optional-key :database-required) s/Bool
+ {:name su/NonBlankString
+ :database-type (s/maybe su/NonBlankString) ; blank if the Field is all NULL & untyped, i.e. in Mongo
+ :base-type su/FieldType
+ :database-position su/IntGreaterThanOrEqualToZero
+ (s/optional-key :semantic-type) (s/maybe su/FieldSemanticOrRelationType)
+ (s/optional-key :effective-type) (s/maybe su/FieldType)
+ (s/optional-key :coercion-strategy) (s/maybe su/CoercionStrategy)
+ (s/optional-key :field-comment) (s/maybe su/NonBlankString)
+ (s/optional-key :pk?) s/Bool
+ (s/optional-key :nested-fields) #{(s/recursive #'TableMetadataField)}
+ (s/optional-key :nfc-path) [s/Any]
+ (s/optional-key :custom) {s/Any s/Any}
+ (s/optional-key :database-is-auto-increment) s/Bool
+ (s/optional-key :database-required) s/Bool
;; for future backwards compatability, when adding things
- s/Keyword s/Any})
+ s/Keyword s/Any})
(def TableMetadata
"Schema for the expected output of `describe-table`."
diff --git a/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj b/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj
index cc4f34576ac..023106fc63b 100644
--- a/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj
+++ b/src/metabase/sync/sync_metadata/fields/fetch_metadata.clj
@@ -23,18 +23,19 @@
(s/defn ^:private fields->parent-id->fields :- {common/ParentID #{common/TableMetadataFieldWithID}}
[fields :- (s/maybe [i/FieldInstance])]
(->> (for [field fields]
- {:parent-id (:parent_id field)
- :id (:id field)
- :name (:name field)
- :database-type (:database_type field)
- :effective-type (:effective_type field)
- :coercion-strategy (:coercion_strategy field)
- :base-type (:base_type field)
- :semantic-type (:semantic_type field)
- :pk? (isa? (:semantic_type field) :type/PK)
- :field-comment (:description field)
- :database-position (:database_position field)
- :database-required (:database_required field)})
+ {:parent-id (:parent_id field)
+ :id (:id field)
+ :name (:name field)
+ :database-type (:database_type field)
+ :effective-type (:effective_type field)
+ :coercion-strategy (:coercion_strategy field)
+ :base-type (:base_type field)
+ :semantic-type (:semantic_type field)
+ :pk? (isa? (:semantic_type field) :type/PK)
+ :field-comment (:description field)
+ :database-is-auto-increment (:database_is_auto_increment field)
+ :database-position (:database_position field)
+ :database-required (:database_required field)})
;; make a map of parent-id -> set of child Fields
(group-by :parent-id)
;; remove the parent ID because the Metadata from `describe-table` won't have it. Save the results as a set
@@ -68,7 +69,7 @@
"Fetch active Fields from the Metabase application database for a given `table`."
[table :- i/TableInstance]
(db/select [Field :name :database_type :base_type :effective_type :coercion_strategy :semantic_type
- :parent_id :id :description :database_position :nfc_path :database_required]
+ :parent_id :id :description :database_position :nfc_path :database_is_auto_increment :database_required]
:table_id (u/the-id table)
:active true
{:order-by table/field-order-rule}))
diff --git a/src/metabase/sync/sync_metadata/fields/sync_instances.clj b/src/metabase/sync/sync_metadata/fields/sync_instances.clj
index 929c9db0f14..f92add2f16b 100644
--- a/src/metabase/sync/sync_metadata/fields/sync_instances.clj
+++ b/src/metabase/sync/sync_metadata/fields/sync_instances.clj
@@ -41,7 +41,7 @@
[table :- i/TableInstance, new-field-metadatas :- [i/TableMetadataField], parent-id :- common/ParentID]
(when (seq new-field-metadatas)
(db/insert-many! Field
- (for [{:keys [database-type database-required base-type effective-type coercion-strategy
+ (for [{:keys [database-type database-is-auto-increment database-required base-type effective-type coercion-strategy
field-comment database-position nfc-path visibility-type], field-name :name :as field} new-field-metadatas]
(do
(when (and effective-type
@@ -55,22 +55,23 @@
field-name
effective-type
base-type)))
- {:table_id (u/the-id table)
- :name field-name
- :display_name (humanization/name->human-readable-name field-name)
- :database_type (or database-type "NULL") ; placeholder for Fields w/ no type info (e.g. Mongo) & all NULL
- :base_type base-type
+ {:table_id (u/the-id table)
+ :name field-name
+ :display_name (humanization/name->human-readable-name field-name)
+ :database_type (or database-type "NULL") ; placeholder for Fields w/ no type info (e.g. Mongo) & all NULL
+ :base_type base-type
;; todo test this?
- :effective_type (if (and effective-type coercion-strategy) effective-type base-type)
- :coercion_strategy (when effective-type coercion-strategy)
- :semantic_type (common/semantic-type field)
- :parent_id parent-id
- :nfc_path nfc-path
- :description field-comment
- :position database-position
- :database_position database-position
- :database_required (or database-required false)
- :visibility_type (or visibility-type :normal)})))))
+ :effective_type (if (and effective-type coercion-strategy) effective-type base-type)
+ :coercion_strategy (when effective-type coercion-strategy)
+ :semantic_type (common/semantic-type field)
+ :parent_id parent-id
+ :nfc_path nfc-path
+ :description field-comment
+ :position database-position
+ :database_position database-position
+ :database_is_auto_increment (or database-is-auto-increment false)
+ :database_required (or database-required false)
+ :visibility_type (or visibility-type :normal)})))))
(s/defn ^:private create-or-reactivate-fields! :- (s/maybe [i/FieldInstance])
"Create (or reactivate) Metabase Field object(s) for any Fields in `new-field-metadatas`. Does *NOT* recursively
diff --git a/src/metabase/sync/sync_metadata/fields/sync_metadata.clj b/src/metabase/sync/sync_metadata/fields/sync_metadata.clj
index fdab400b4b5..540a5b7ed3c 100644
--- a/src/metabase/sync/sync_metadata/fields/sync_metadata.clj
+++ b/src/metabase/sync/sync_metadata/fields/sync_metadata.clj
@@ -19,19 +19,22 @@
"Update the metadata for a Metabase Field as needed if any of the info coming back from the DB has changed. Syncs
base type, database type, semantic type, and comments/remarks; returns `1` if the Field was updated; `0` otherwise."
[table :- i/TableInstance, field-metadata :- i/TableMetadataField, metabase-field :- common/TableMetadataFieldWithID]
- (let [{old-database-type :database-type
- old-base-type :base-type
- old-field-comment :field-comment
- old-semantic-type :semantic-type
- old-database-position :database-position
- old-database-name :name
- old-db-required :database-required} metabase-field
- {new-database-type :database-type
- new-base-type :base-type
- new-field-comment :field-comment
- new-database-position :database-position
- new-database-name :name
- new-db-required :database-required} field-metadata
+ (let [{old-database-type :database-type
+ old-base-type :base-type
+ old-field-comment :field-comment
+ old-semantic-type :semantic-type
+ old-database-position :database-position
+ old-database-name :name
+ old-database-is-auto-increment :database-is-auto-increment
+ old-db-required :database-required} metabase-field
+ {new-database-type :database-type
+ new-base-type :base-type
+ new-field-comment :field-comment
+ new-database-position :database-position
+ new-database-name :name
+ new-database-is-auto-increment :database-is-auto-increment
+ new-db-required :database-required} field-metadata
+ new-database-is-auto-increment (boolean new-database-is-auto-increment)
new-db-required (boolean new-db-required)
new-database-type (or new-database-type "NULL")
new-semantic-type (common/semantic-type field-metadata)
@@ -58,6 +61,7 @@
;; different they have the same canonical representation (lower-casing at the moment).
new-name? (not= old-database-name new-database-name)
+ new-db-auto-incremented? (not= old-database-is-auto-increment new-database-is-auto-increment)
new-db-required? (not= old-db-required new-db-required)
;; calculate combined updates
@@ -97,6 +101,12 @@
old-database-name
new-database-name))
{:name new-database-name})
+ (when new-db-auto-incremented?
+ (log/info (trs "Database auto incremented of {0} has changed from ''{1}'' to ''{2}''."
+ (common/field-metadata-name-for-logging table metabase-field)
+ old-database-is-auto-increment
+ new-database-is-auto-increment))
+ {:database_is_auto_increment new-database-is-auto-increment})
(when new-db-required?
(log/info (trs "Database required of {0} has changed from ''{1}'' to ''{2}''."
(common/field-metadata-name-for-logging table metabase-field)
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index 54f222658fb..615171a36db 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -369,7 +369,8 @@
:visibility_type "normal"
:has_field_values "none"
:database_position 0
- :database_required false})
+ :database_required false
+ :database_is_auto_increment true})
(merge
(field-details (db/select-one Field :id (mt/id :categories :name)))
{:table_id (mt/id :categories)
@@ -382,7 +383,8 @@
:visibility_type "normal"
:has_field_values "list"
:database_position 1
- :database_required true})]
+ :database_required true
+ :database_is_auto_increment false})]
:segments []
:metrics []
:id (mt/id :categories)
diff --git a/test/metabase/api/field_test.clj b/test/metabase/api/field_test.clj
index 071af85d35d..f9dd032eb7f 100644
--- a/test/metabase/api/field_test.clj
+++ b/test/metabase/api/field_test.clj
@@ -33,7 +33,8 @@
(testing "GET /api/field/:id"
(is (= (-> (merge
(mt/object-defaults Field)
- (db/select-one [Field :created_at :updated_at :last_analyzed :fingerprint :fingerprint_version :database_position :database_required]
+ (db/select-one [Field :created_at :updated_at :last_analyzed :fingerprint :fingerprint_version
+ :database_position :database_required :database_is_auto_increment]
:id (mt/id :users :name))
{:table_id (mt/id :users)
:table (merge
@@ -63,6 +64,7 @@
:effective_type "type/Text"
:has_field_values "list"
:database_required false
+ :database_is_auto_increment false
:dimensions []
:name_field nil})
(m/dissoc-in [:table :db :updated_at] [:table :db :created_at] [:table :db :timezone]))
diff --git a/test/metabase/api/table_test.clj b/test/metabase/api/table_test.clj
index 931d2e13a4e..32e4168392a 100644
--- a/test/metabase/api/table_test.clj
+++ b/test/metabase/api/table_test.clj
@@ -74,7 +74,8 @@
(field-defaults)
(select-keys
field
- [:created_at :fingerprint :fingerprint_version :fk_target_field_id :id :last_analyzed :updated_at :database_required])))
+ [:created_at :fingerprint :fingerprint_version :fk_target_field_id :id :last_analyzed :updated_at
+ :database_required :database_is_auto_increment])))
(defn- fk-field-details [field]
(-> (field-details field)
@@ -156,7 +157,8 @@
:effective_type "type/BigInteger"
:visibility_type "normal"
:has_field_values "none"
- :database_required false)
+ :database_required false
+ :database_is_auto_increment true)
(assoc (field-details (db/select-one Field :id (mt/id :users :name)))
:semantic_type "type/Name"
:table_id (mt/id :users)
@@ -171,7 +173,8 @@
:has_field_values "list"
:position 1
:database_position 1
- :database_required false)
+ :database_required false
+ :database_is_auto_increment false)
(assoc (field-details (db/select-one Field :id (mt/id :users :last_login)))
:table_id (mt/id :users)
:name "LAST_LOGIN"
@@ -185,7 +188,8 @@
:has_field_values "none"
:position 2
:database_position 2
- :database_required false)
+ :database_required false
+ :database_is_auto_increment false)
(assoc (field-details (db/select-one Field :table_id (mt/id :users), :name "PASSWORD"))
:semantic_type "type/Category"
:table_id (mt/id :users)
@@ -198,7 +202,8 @@
:has_field_values "list"
:position 3
:database_position 3
- :database_required false)]
+ :database_required false
+ :database_is_auto_increment false)]
:id (mt/id :users)})
(mt/user-http-request :rasta :get 200 (format "table/%d/query_metadata?include_sensitive_fields=true" (mt/id :users))))
"Make sure that getting the User table *does* include info about the password field, but not actual values themselves"))))
@@ -222,7 +227,8 @@
:base_type "type/BigInteger"
:effective_type "type/BigInteger"
:has_field_values "none"
- :database_required false)
+ :database_required false
+ :database_is_auto_increment true)
(assoc (field-details (db/select-one Field :id (mt/id :users :name)))
:table_id (mt/id :users)
:semantic_type "type/Name"
@@ -234,7 +240,8 @@
:has_field_values "list"
:position 1
:database_position 1
- :database_required false)
+ :database_required false
+ :database_is_auto_increment false)
(assoc (field-details (db/select-one Field :id (mt/id :users :last_login)))
:table_id (mt/id :users)
:name "LAST_LOGIN"
@@ -247,7 +254,8 @@
:has_field_values "none"
:position 2
:database_position 2
- :database_required false)]
+ :database_required false
+ :database_is_auto_increment false)]
:id (mt/id :users)})
(mt/user-http-request :rasta :get 200 (format "table/%d/query_metadata" (mt/id :users))))
"Make sure that getting the User table does *not* include password info"))))
@@ -447,22 +455,24 @@
:base_type "type/BigInteger"
:effective_type "type/BigInteger"
:has_field_values "none"
- :database_required false})
+ :database_required false
+ :database_is_auto_increment true})
(merge
(field-details (db/select-one Field :id (mt/id :categories :name)))
- {:table_id (mt/id :categories)
- :semantic_type "type/Name"
- :name "NAME"
- :display_name "Name"
- :database_type "CHARACTER VARYING"
- :base_type "type/Text"
- :effective_type "type/Text"
- :dimension_options []
- :default_dimension_option nil
- :has_field_values "list"
- :database_position 1
- :position 1
- :database_required true})]
+ {:table_id (mt/id :categories)
+ :semantic_type "type/Name"
+ :name "NAME"
+ :display_name "Name"
+ :database_type "CHARACTER VARYING"
+ :base_type "type/Text"
+ :effective_type "type/Text"
+ :dimension_options []
+ :default_dimension_option nil
+ :has_field_values "list"
+ :database_position 1
+ :position 1
+ :database_required true
+ :database_is_auto_increment false})]
:id (mt/id :categories)})
(mt/user-http-request :rasta :get 200 (format "table/%d/query_metadata" (mt/id :categories)))))))
diff --git a/test/metabase/driver/postgres_test.clj b/test/metabase/driver/postgres_test.clj
index 8c30a3fe173..afc9d19cc65 100644
--- a/test/metabase/driver/postgres_test.clj
+++ b/test/metabase/driver/postgres_test.clj
@@ -807,22 +807,25 @@
(testing "check that describe-table properly describes the database & base types of the enum fields"
(is (= {:name "birds"
- :fields #{{:name "name"
- :database-type "varchar"
- :base-type :type/Text
- :pk? true
- :database-position 0
- :database-required true}
- {:name "status"
- :database-type "bird_status"
- :base-type :type/PostgresEnum
- :database-position 1
- :database-required true}
- {:name "type"
- :database-type "bird type"
- :base-type :type/PostgresEnum
- :database-position 2
- :database-required true}}}
+ :fields #{{:name "name"
+ :database-type "varchar"
+ :base-type :type/Text
+ :pk? true
+ :database-position 0
+ :database-required true
+ :database-is-auto-increment false}
+ {:name "status"
+ :database-type "bird_status"
+ :base-type :type/PostgresEnum
+ :database-position 1
+ :database-required true
+ :database-is-auto-increment false}
+ {:name "type"
+ :database-type "bird type"
+ :base-type :type/PostgresEnum
+ :database-position 2
+ :database-required true
+ :database-is-auto-increment false}}}
(driver/describe-table :postgres db {:name "birds"}))))
(testing "check that when syncing the DB the enum types get recorded appropriately"
diff --git a/test/metabase/driver/sql_jdbc/sync/describe_table_test.clj b/test/metabase/driver/sql_jdbc/sync/describe_table_test.clj
index 63f66675916..d144c9ff772 100644
--- a/test/metabase/driver/sql_jdbc/sync/describe_table_test.clj
+++ b/test/metabase/driver/sql_jdbc/sync/describe_table_test.clj
@@ -7,7 +7,9 @@
:as sql-jdbc.describe-table]
[metabase.driver.sql-jdbc.sync.interface :as sql-jdbc.sync.interface]
[metabase.models.table :refer [Table]]
+ [metabase.sync :as sync]
[metabase.test :as mt]
+ [metabase.test.data.one-off-dbs :as one-off-dbs]
[metabase.util :as u]
[toucan.db :as db]))
@@ -23,14 +25,49 @@
(deftest describe-table-test
(is (= {:name "VENUES",
:fields
- #{{:name "ID", :database-type "BIGINT", :base-type :type/BigInteger, :database-position 0, :pk? true :database-required false}
- {:name "NAME", :database-type "CHARACTER VARYING", :base-type :type/Text, :database-position 1 :database-required false}
- {:name "CATEGORY_ID", :database-type "INTEGER", :base-type :type/Integer, :database-position 2 :database-required false}
- {:name "LATITUDE", :database-type "DOUBLE PRECISION", :base-type :type/Float, :database-position 3 :database-required false}
- {:name "LONGITUDE", :database-type "DOUBLE PRECISION", :base-type :type/Float, :database-position 4 :database-required false}
- {:name "PRICE", :database-type "INTEGER", :base-type :type/Integer, :database-position 5 :database-required false}}}
+ #{{:name "ID", :database-type "BIGINT", :base-type :type/BigInteger, :database-position 0, :pk? true :database-required false :database-is-auto-increment true}
+ {:name "NAME", :database-type "CHARACTER VARYING", :base-type :type/Text, :database-position 1 :database-required false :database-is-auto-increment false}
+ {:name "CATEGORY_ID", :database-type "INTEGER", :base-type :type/Integer, :database-position 2 :database-required false :database-is-auto-increment false}
+ {:name "LATITUDE", :database-type "DOUBLE PRECISION", :base-type :type/Float, :database-position 3 :database-required false :database-is-auto-increment false}
+ {:name "LONGITUDE", :database-type "DOUBLE PRECISION", :base-type :type/Float, :database-position 4 :database-required false :database-is-auto-increment false}
+ {:name "PRICE", :database-type "INTEGER", :base-type :type/Integer, :database-position 5 :database-required false :database-is-auto-increment false}}}
(sql-jdbc.describe-table/describe-table :h2 (mt/id) {:name "VENUES"}))))
+(deftest describe-auto-increment-on-non-pk-field-test
+ (testing "a non-pk field with auto-increment should be have metabase_field.database_is_auto_increment=true"
+ (one-off-dbs/with-blank-db
+ (doseq [statement [;; H2 needs that 'guest' user for QP purposes. Set that up
+ "CREATE USER IF NOT EXISTS GUEST PASSWORD 'guest';"
+ ;; Keep DB open until we say otherwise :)
+ "SET DB_CLOSE_DELAY -1;"
+ ;; create table & load data
+ "DROP TABLE IF EXISTS \"birds\";"
+ "CREATE TABLE \"employee_counter\" (\"id\" INTEGER AUTO_INCREMENT PRIMARY KEY, \"count\" INTEGER AUTO_INCREMENT, \"rank\" INTEGER NOT NULL)"
+ "GRANT ALL ON \"employee_counter\" TO GUEST;"]]
+ (jdbc/execute! one-off-dbs/*conn* [statement]))
+ (sync/sync-database! (mt/db))
+ (is (= {:fields #{{:base-type :type/Integer
+ :database-is-auto-increment true
+ :database-position 0
+ :database-required false
+ :database-type "INTEGER"
+ :name "id"
+ :pk? true}
+ {:base-type :type/Integer
+ :database-is-auto-increment true
+ :database-position 1
+ :database-required false
+ :database-type "INTEGER"
+ :name "count"}
+ {:base-type :type/Integer
+ :database-is-auto-increment false
+ :database-position 2
+ :database-required true
+ :database-type "INTEGER"
+ :name "rank"}}
+ :name "employee_counter"}
+ (sql-jdbc.describe-table/describe-table :h2 (mt/id) {:name "employee_counter"}))))))
+
(deftest describe-table-fks-test
(is (= #{{:fk-column-name "CATEGORY_ID"
:dest-table {:name "CATEGORIES", :schema "PUBLIC"}
diff --git a/test/metabase/driver/sql_jdbc_test.clj b/test/metabase/driver/sql_jdbc_test.clj
index d6457227404..c32201c9e75 100644
--- a/test/metabase/driver/sql_jdbc_test.clj
+++ b/test/metabase/driver/sql_jdbc_test.clj
@@ -26,32 +26,38 @@
:base-type :type/BigInteger
:pk? true
:database-position 0
- :database-required false}
+ :database-required false
+ :database-is-auto-increment true}
{:name "NAME"
:database-type "CHARACTER VARYING"
:base-type :type/Text
:database-position 1
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "CATEGORY_ID"
:database-type "INTEGER"
:base-type :type/Integer
:database-position 2
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "LATITUDE"
:database-type "DOUBLE PRECISION"
:base-type :type/Float
:database-position 3
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "LONGITUDE"
:database-type "DOUBLE PRECISION"
:base-type :type/Float
:database-position 4
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "PRICE"
:database-type "INTEGER"
:base-type :type/Integer
:database-position 5
- :database-required false}}}
+ :database-required false
+ :database-is-auto-increment false}}}
(driver/describe-table :h2 (mt/db) (db/select-one Table :id (mt/id :venues))))))
(deftest ^:parallel describe-table-fks-test
diff --git a/test/metabase/sync/sync_dynamic_test.clj b/test/metabase/sync/sync_dynamic_test.clj
index 28ca86e4c5f..97d89acb148 100644
--- a/test/metabase/sync/sync_dynamic_test.clj
+++ b/test/metabase/sync/sync_dynamic_test.clj
@@ -22,7 +22,7 @@
(for [field fields]
(u/select-non-nil-keys
field
- [:table_id :name :fk_target_field_id :parent_id :base_type :database_type]))))))))
+ [:table_id :name :fk_target_field_id :parent_id :base_type :database_type :database_is_auto_increment]))))))))
(defn- get-tables [database-or-id]
(->> (hydrate (db/select Table, :db_id (u/the-id database-or-id), {:order-by [:id]}) :fields)
@@ -32,5 +32,5 @@
(testing "basic test to make sure syncing nested fields works. This is sort of a higher-level test."
(mt/with-temp Database [db {:engine ::toucanery/toucanery}]
(sync/sync-database! db)
- (is (= (remove-nonsense toucanery/toucanery-tables-and-fields)
- (remove-nonsense (get-tables db)))))))
+ (is (= (remove-nonsense toucanery/toucanery-tables-and-fields)
+ (remove-nonsense (get-tables db)))))))
diff --git a/test/metabase/sync/sync_metadata/fields/fetch_metadata_test.clj b/test/metabase/sync/sync_metadata/fields/fetch_metadata_test.clj
index 8cde628a4b9..6924dc32717 100644
--- a/test/metabase/sync/sync_metadata/fields/fetch_metadata_test.clj
+++ b/test/metabase/sync/sync_metadata/fields/fetch_metadata_test.clj
@@ -22,31 +22,36 @@
:effective-type :type/Integer
:semantic-type :type/PK
:pk? true
- :database-required false}
+ :database-required false
+ :database-is-auto-increment true}
{:name "buyer"
:database-type "OBJECT"
:base-type :type/Dictionary
:effective-type :type/Dictionary
:pk? false
:database-required false
+ :database-is-auto-increment false
:nested-fields #{{:name "name"
:database-type "VARCHAR"
:base-type :type/Text
:effective-type :type/Text
:pk? false
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "cc"
:database-type "VARCHAR"
:base-type :type/Text
:effective-type :type/Text
:pk? false
- :database-required false}}}
+ :database-required false
+ :database-is-auto-increment false}}}
{:name "ts"
:database-type "BIGINT"
:base-type :type/BigInteger
:effective-type :type/DateTime
:coercion-strategy :Coercion/UNIXMilliSeconds->DateTime
:pk? false
+ :database-is-auto-increment false
:database-required false}
{:name "toucan"
:database-type "OBJECT"
@@ -54,31 +59,36 @@
:effective-type :type/Dictionary
:pk? false
:database-required false
+ :database-is-auto-increment false
:nested-fields #{{:name "name"
:database-type "VARCHAR"
:base-type :type/Text
:effective-type :type/Text
:pk? false
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "details"
:database-type "OBJECT"
:base-type :type/Dictionary
:effective-type :type/Dictionary
:pk? false
:database-required false
+ :database-is-auto-increment false
:nested-fields #{{:name "weight"
:database-type "DECIMAL"
:base-type :type/Decimal
:effective-type :type/Decimal
:semantic-type :type/Category
:pk? false
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "age"
:database-type "INT"
:base-type :type/Integer
:effective-type :type/Integer
:pk? false
- :database-required false}}}}}}
+ :database-required false
+ :database-is-auto-increment false}}}}}}
(let [transactions-table-id (u/the-id (db/select-one-id Table :db_id (u/the-id db), :name "transactions"))
remove-ids-and-nil-vals (partial walk/postwalk #(if-not (map? %)
diff --git a/test/metabase/sync/sync_metadata/fields/sync_metadata_test.clj b/test/metabase/sync/sync_metadata/fields/sync_metadata_test.clj
index c9bcde0f6fc..d1d7c3e7291 100644
--- a/test/metabase/sync/sync_metadata/fields/sync_metadata_test.clj
+++ b/test/metabase/sync/sync_metadata/fields/sync_metadata_test.clj
@@ -25,13 +25,15 @@
:database-type "Integer"
:base-type :type/Integer
:database-position 0
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "My Field"
:database-type "NULL"
:base-type :type/Integer
:id 1
:database-position 0
- :database-required false})))))
+ :database-required false
+ :database-is-auto-increment false})))))
(deftest database-required-changed-test
(testing "test that if database-required changes we will update it in the DB"
@@ -41,13 +43,48 @@
:database-type "Integer"
:base-type :type/Integer
:database-position 0
+ :database-required false
+ :database-is-auto-increment false}
+ {:name "My Field"
+ :database-type "Integer"
+ :base-type :type/Integer
+ :id 1
+ :database-position 0
+ :database-required true
+ :database-is-auto-increment false})))))
+
+(deftest database-is-auto-increment-changed-test
+ (testing "test that if database-required changes we will update it in the DB"
+ (is (= [["Field" 1 {:database_is_auto_increment true}]]
+ (updates-that-will-be-performed
+ {:name "My Field"
+ :database-type "Integer"
+ :base-type :type/Integer
+ :database-position 0
+ :database-required false
+ :database-is-auto-increment true}
+ {:name "My Field"
+ :database-type "Integer"
+ :base-type :type/Integer
+ :id 1
+ :database-position 0
+ :database-required false
+ :database-is-auto-increment false})))
+ (is (= [["Field" 1 {:database_is_auto_increment false}]]
+ (updates-that-will-be-performed
+ {:name "My Field"
+ :database-type "Integer"
+ :base-type :type/Integer
+ :database-position 0
+ ;; no :database-is-auto-increment key to test case where describe-table does not not return it
:database-required false}
{:name "My Field"
:database-type "Integer"
:base-type :type/Integer
:id 1
:database-position 0
- :database-required true})))))
+ :database-required false
+ :database-is-auto-increment true})))))
(deftest no-op-test
(testing "no changes should be made (i.e., no calls to `update!`) if nothing changes"
@@ -57,13 +94,15 @@
:database-type "Integer"
:base-type :type/Integer
:database-position 0
- :database-required false}
+ :database-required false
+ :database-is-auto-increment true}
{:name "My Field"
:database-type "Integer"
:base-type :type/Integer
:id 1
:database-position 0
- :database-required false})))))
+ :database-required false
+ :database-is-auto-increment true})))))
(deftest nil-database-type-test
(testing (str "test that if `database-type` comes back as `nil` in the metadata from the sync process, we won't try "
@@ -75,13 +114,15 @@
:database-type nil
:base-type :type/Integer
:database-position 0
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "My Field"
:database-type "Integer"
:base-type :type/Integer
:database-position 0
:id 1
- :database-required false}))))
+ :database-required false
+ :database-is-auto-increment false}))))
(testing (str "if `database-type` comes back as `nil` and was already saved in application DB as `NULL` no changes "
"should be made")
@@ -91,13 +132,15 @@
:database-type nil
:base-type :type/Integer
:database-position 0
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "My Field"
:database-type "NULL"
:base-type :type/Integer
:id 1
:database-position 0
- :database-required false})))))
+ :database-required false
+ :database-is-auto-increment false})))))
(deftest dont-overwrite-semantic-type-test
(testing "We should not override non-nil `semantic_type`s"
@@ -108,11 +151,13 @@
:base-type :type/Integer
:semantic-type nil
:database-position 0
- :database-required false}
+ :database-required false
+ :database-is-auto-increment false}
{:name "My Field"
:database-type "Integer"
:base-type :type/Integer
:semantic-type :type/Price
:id 1
:database-position 0
- :database-required false})))))
+ :database-required false
+ :database-is-auto-increment false})))))
diff --git a/test/metabase/sync_test.clj b/test/metabase/sync_test.clj
index a63bc30cfec..3162049afcf 100644
--- a/test/metabase/sync_test.clj
+++ b/test/metabase/sync_test.clj
@@ -31,15 +31,18 @@
:database-type "SERIAL"
:base-type :type/Integer
:semantic-type :type/PK
+ :database-is-auto-increment true
:database-position 0}
{:name "title"
:database-type "VARCHAR"
:base-type :type/Text
:semantic-type :type/Title
+ :database-is-auto-increment false
:database-position 1}
{:name "studio"
:database-type "VARCHAR"
:base-type :type/Text
+ :database-is-auto-increment false
:database-position 2}}
:description nil}
"studio" {:name "studio"
@@ -48,10 +51,12 @@
:database-type "VARCHAR"
:base-type :type/Text
:semantic-type :type/PK
+ :database-is-auto-increment false
:database-position 0}
{:name "name"
:database-type "VARCHAR"
:base-type :type/Text
+ :database-is-auto-increment false
:database-position 1}}
:description ""}})
@@ -110,6 +115,7 @@
:fingerprint false
:fingerprint_version false
:fk_target_field_id false
+ :database_is_auto_increment false
:id true
:last_analyzed false
:parent_id false
@@ -133,6 +139,7 @@
:effective_type :type/Integer
:semantic_type :type/PK
:database_position 0
+ :database_is_auto_increment true
:position 0}))
(defn- field:movie-studio []
diff --git a/test/metabase/test/mock/toucanery.clj b/test/metabase/test/mock/toucanery.clj
index 0aa475e4e6c..f45f5ab01ba 100644
--- a/test/metabase/test/mock/toucanery.clj
+++ b/test/metabase/test/mock/toucanery.clj
@@ -12,44 +12,56 @@
:fields #{{:name "id"
:pk? true
:database-type "SERIAL"
- :base-type :type/Integer}
+ :base-type :type/Integer
+ :database-is-auto-increment true}
{:name "ts"
:database-type "BIGINT"
:base-type :type/BigInteger
:effective-type :type/DateTime
- :coercion-strategy :Coercion/UNIXMilliSeconds->DateTime}
+ :coercion-strategy :Coercion/UNIXMilliSeconds->DateTime
+ :database-is-auto-increment false}
{:name "toucan"
:database-type "OBJECT"
:base-type :type/Dictionary
+ :database-is-auto-increment false
:nested-fields #{{:name "name"
:database-type "VARCHAR"
- :base-type :type/Text}
+ :base-type :type/Text
+ :database-is-auto-increment false}
{:name "details"
:database-type "OBJECT"
:base-type :type/Dictionary
+ :database-is-auto-increment false
:nested-fields #{{:name "age"
:database-type "INT"
+ :database-is-auto-increment false
:base-type :type/Integer}
{:name "weight"
:database-type "DECIMAL"
+ :database-is-auto-increment false
:semantic-type :type/Category
:base-type :type/Decimal}}}}}
{:name "buyer"
:database-type "OBJECT"
+ :database-is-auto-increment false
:base-type :type/Dictionary
:nested-fields #{{:name "name"
:database-type "VARCHAR"
+ :database-is-auto-increment false
:base-type :type/Text}
{:name "cc"
:database-type "VARCHAR"
+ :database-is-auto-increment false
:base-type :type/Text}}}}}
"employees" {:name "employees"
:schema nil
:fields #{{:name "id"
:database-type "SERIAL"
+ :database-is-auto-increment true
:base-type :type/Integer}
{:name "name"
:database-type "VARCHAR"
+ :database-is-auto-increment false
:base-type :type/Text}}}})
(driver/register! ::toucanery, :abstract? true)
@@ -105,6 +117,7 @@
:display_name "ID"
:database_type "SERIAL"
:base_type :type/Integer
+ :database_is_auto_increment true
:semantic_type :type/PK})]
:display_name "Employees"})
(merge mock.util/table-defaults
@@ -120,6 +133,7 @@
{:name "id"
:display_name "ID"
:database_type "SERIAL"
+ :database_is_auto_increment true
:base_type :type/Integer})
(merge mock.util/field-defaults
{:name "buyer"
diff --git a/test/metabase/test/mock/util.clj b/test/metabase/test/mock/util.clj
index 26701fa629f..309635b0d54 100644
--- a/test/metabase/test/mock/util.clj
+++ b/test/metabase/test/mock/util.clj
@@ -22,6 +22,7 @@
:caveats nil
:points_of_interest nil
:fk_target_field_id false
+ :database_is_auto_increment false
:updated_at true
:active true
:nfc_path nil
--
GitLab