fix: Unnecessary updates because of display_name and settings (#149)

* Fix unnecessary field updates because of display_name and settings * Same display_name fix for models * Correct allowed meta fields for models and columns * Type check and lint fixes
gouline · Nov 7, 2022 · cceb1b3 · cceb1b3
1 parent 0df4084
commit cceb1b3
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 23 deletions.
diff --git a/dbtmetabase/metabase.py b/dbtmetabase/metabase.py
@@ -3,6 +3,7 @@
 import re
 import time
 from typing import (
+    Any,
     Iterable,
     List,
     Mapping,
@@ -253,8 +254,13 @@ def export_model(
         model_visibility = model.visibility_type or None
 
         body_table = {}
-        if api_table.get("display_name") != model_display_name:
+
+        # Update if specified, otherwise reset one that had been set
+        if api_table.get("display_name") != model_display_name and (
+            model_display_name or api_table.get("display_name") != api_table.get("name")
+        ):
             body_table["display_name"] = model_display_name
+
         if api_table.get("description") != model_description:
             body_table["description"] = model_description
         if api_table.get("points_of_interest") != model_points_of_interest:
@@ -389,12 +395,15 @@ def export_column(
         if api_field["fk_target_field_id"] and not fk_target_field_id:
             fk_target_field_id = api_field["fk_target_field_id"]
 
-        api_settings = api_field.get("settings") or {}
-        body_field = {
-            "settings": api_settings.copy(),
-        }
-        if api_field.get("display_name") != column_display_name:
+        body_field: MutableMapping[str, Optional[Any]] = {}
+
+        # Update if specified, otherwise reset one that had been set
+        if api_field.get("display_name") != column_display_name and (
+            column_display_name
+            or api_field.get("display_name") != api_field.get("name")
+        ):
             body_field["display_name"] = column_display_name
+
         if api_field.get("description") != column_description:
             body_field["description"] = column_description
         if api_field.get("visibility_type") != column_visibility:
@@ -411,11 +420,13 @@ def export_column(
             and column.coercion_strategy
         ):
             body_field["coercion_strategy"] = column.coercion_strategy
-        if (
-            api_settings.get("number_style") != column.number_style
-            and column.number_style
-        ):
-            body_field["settings"]["number_style"] = column.number_style
+
+        settings = api_field.get("settings") or {}
+        if settings.get("number_style") != column.number_style and column.number_style:
+            settings["number_style"] = column.number_style
+
+        if settings:
+            body_field["settings"] = settings
 
         # Allow explicit null type to override detected one
         if api_field.get(semantic_type_key) != column.semantic_type and (
@@ -895,7 +906,11 @@ def api(
             headers["X-Metabase-Session"] = self.session_id
 
         response = requests.request(
-            method, f"{self.protocol}://{self.host}{path}", verify=self.verify, **kwargs
+            method,
+            f"{self.protocol}://{self.host}{path}",
+            verify=self.verify,
+            timeout=30,
+            **kwargs,
         )
 
         if critical:

diff --git a/dbtmetabase/models/metabase.py b/dbtmetabase/models/metabase.py
@@ -3,20 +3,22 @@
 from typing import MutableMapping, Optional, Sequence
 
 # Allowed metabase.* fields
-# Must be covered by MetabaseModel attributes
-METABASE_MODEL_META_FIELDS = [
+_METABASE_COMMON_META_FIELDS = [
     "display_name",
     "visibility_type",
-    "points_of_interest",
-    "caveats",
 ]
 # Must be covered by MetabaseColumn attributes
-METABASE_COLUMN_META_FIELDS = METABASE_MODEL_META_FIELDS + [
+METABASE_COLUMN_META_FIELDS = _METABASE_COMMON_META_FIELDS + [
     "semantic_type",
     "has_field_values",
     "coercion_strategy",
     "number_style",
 ]
+# Must be covered by MetabaseModel attributes
+METABASE_MODEL_META_FIELDS = _METABASE_COMMON_META_FIELDS + [
+    "points_of_interest",
+    "caveats",
+]
 
 # Default model schema (only schema in BigQuery)
 METABASE_MODEL_DEFAULT_SCHEMA = "PUBLIC"
@@ -31,26 +33,28 @@ class ModelType(str, Enum):
 class MetabaseColumn:
     name: str
     description: Optional[str] = None
-    display_name: Optional[str] = None
-
-    meta_fields: MutableMapping = field(default_factory=dict)
 
-    semantic_type: Optional[str] = None
+    display_name: Optional[str] = None
     visibility_type: Optional[str] = None
+    semantic_type: Optional[str] = None
     has_field_values: Optional[str] = None
     coercion_strategy: Optional[str] = None
     number_style: Optional[str] = None
 
     fk_target_table: Optional[str] = None
     fk_target_field: Optional[str] = None
 
+    meta_fields: MutableMapping = field(default_factory=dict)
+
 
 @dataclass
 class MetabaseModel:
     name: str
     schema: str
     description: str = ""
+
     display_name: Optional[str] = None
+    visibility_type: Optional[str] = None
     points_of_interest: Optional[str] = None
     caveats: Optional[str] = None
 
@@ -59,8 +63,6 @@ class MetabaseModel:
     source: Optional[str] = None
     unique_id: Optional[str] = None
 
-    visibility_type: Optional[str] = None
-
     @property
     def ref(self) -> Optional[str]:
         if self.model_type == ModelType.nodes: