From 557d4c0333459dc976f0105bf7c40628f850576c Mon Sep 17 00:00:00 2001 From: Mike Gouline <1960272+gouline@users.noreply.github.com> Date: Mon, 8 Jan 2024 17:20:30 +1100 Subject: [PATCH] Fixes discovered in sandbox testing (#203) --- Makefile | 20 ++++++-- dbtmetabase/__main__.py | 13 +---- dbtmetabase/metabase.py | 49 ++++++++++++++----- sandbox/requirements.txt | 6 +-- setup.py | 4 +- .../fixtures/sample_project/models/schema.yml | 2 +- 6 files changed, 60 insertions(+), 34 deletions(-) diff --git a/Makefile b/Makefile index 1904d6e1..200e4904 100644 --- a/Makefile +++ b/Makefile @@ -59,6 +59,20 @@ dev-install: build && python3 -m pip install dist/dbt_metabase-*-py3-none-any.whl .PHONY: dev-install -dev-sandbox: - ( cd sandbox && docker-compose up --build ; docker-compose down ) -.PHONY: dev-sandbox +dev-sandbox-up: + ( cd sandbox && docker-compose up --build --detach --wait ) +.PHONY: dev-sandbox-up + +dev-sandbox-models: + ( source sandbox/.env && python3 -m dbtmetabase models \ + --dbt-manifest-path sandbox/target/manifest.json \ + --dbt-database $$POSTGRES_DB \ + --metabase-url http://localhost:$$MB_PORT \ + --metabase-username $$MB_USER \ + --metabase-password $$MB_PASSWORD \ + --metabase-database $$POSTGRES_DB ) +.PHONY: dev-sandbox-models + +dev-sandbox-down: + ( cd sandbox && docker-compose down ) +.PHONY: dev-sandbox-up diff --git a/dbtmetabase/__main__.py b/dbtmetabase/__main__.py index e4242c37..06040e4e 100644 --- a/dbtmetabase/__main__.py +++ b/dbtmetabase/__main__.py @@ -238,7 +238,7 @@ def wrapper( metabase_session_id: Optional[str], metabase_verify: bool, metabase_cert: Optional[str], - metabase_http_timeout: int, + metabase_timeout: int, verbose: bool, **kwargs, ): @@ -260,7 +260,7 @@ def wrapper( session_id=metabase_session_id, verify=metabase_verify, cert=metabase_cert, - http_timeout=metabase_http_timeout, + http_timeout=metabase_timeout, ), **kwargs, ) @@ -294,15 +294,6 @@ def wrapper( type=click.STRING, help="Target database name in Metabase.", ) -@click.option( - "--metabase-sync/--metabase-sync-skip", - "metabase_sync", - envvar="METABASE_SYNC", - show_envvar=True, - default=True, - show_default=True, - help="Attempt to synchronize Metabase schema with local models.", -) @click.option( "--metabase-sync-timeout", metavar="SECS", diff --git a/dbtmetabase/metabase.py b/dbtmetabase/metabase.py index 26cf859e..8ed373d9 100644 --- a/dbtmetabase/metabase.py +++ b/dbtmetabase/metabase.py @@ -125,9 +125,10 @@ def execute(self): json=update["body"], ) logger.info( - "Update to %s %s applied successfully", + "API %s/%s updated successfully: %s", update["kind"], update["id"], + ", ".join(update.get("body", {}).keys()), ) if not success: @@ -180,8 +181,10 @@ def _export_model(self, model: MetabaseModel) -> bool: body_table = {} # 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") + api_display_name = api_table.get("display_name") + if api_display_name != model_display_name and ( + model_display_name + or not self._friendly_equal(api_display_name, api_table.get("name")) ): body_table["display_name"] = model_display_name @@ -258,7 +261,7 @@ def _export_column( if not target_table or not target_field: logger.info( - "Passing on fk resolution for %s. Target field %s was not resolved during dbt model parsing.", + "Skipping FK resolution for %s table, %s field not resolved during dbt parsing", table_key, target_field, ) @@ -279,7 +282,7 @@ def _export_column( fk_target_field_id = fk_target_field.get("id") if fk_target_field.get(semantic_type_key) != "type/PK": logger.info( - "Target field %s will be set to PK for %s column FK", + "API field/%s will become PK (for %s column FK)", fk_target_field_id, column_name, ) @@ -291,13 +294,13 @@ def _export_column( ) else: logger.info( - "Target field %s is already PK, needed for %s column", + "API field/%s is already PK (for %s column FK)", fk_target_field_id, column_name, ) else: logger.error( - "Unable to find foreign key target %s.%s", + "Unable to find PK for %s.%s column FK", target_table, target_field, ) @@ -315,9 +318,10 @@ def _export_column( 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 ( + api_display_name = api_field.get("display_name") + if api_display_name != column_display_name and ( column_display_name - or api_field.get("display_name") != api_field.get("name") + or not self._friendly_equal(api_display_name, api_field.get("name")) ): body_field["display_name"] = column_display_name @@ -346,8 +350,9 @@ def _export_column( body_field["settings"] = settings # Allow explicit null type to override detected one - if api_field.get(semantic_type_key) != column.semantic_type and ( - column.semantic_type or column.semantic_type is NullValue + api_semantic_type = api_field.get(semantic_type_key) + if (column.semantic_type and api_semantic_type != column.semantic_type) or ( + column.semantic_type is NullValue and api_semantic_type ): body_field[semantic_type_key] = column.semantic_type or None @@ -365,7 +370,7 @@ def _load_tables(self, database_id: str) -> Mapping[str, MutableMapping]: metadata = self.client.api( "get", f"/api/database/{database_id}/metadata", - params={"include_hidden": True}, + params={"include_hidden": "true"}, ) bigquery_schema = metadata.get("details", {}).get("dataset-id") @@ -394,6 +399,24 @@ def _load_tables(self, database_id: str) -> Mapping[str, MutableMapping]: return tables + def _friendly_equal(self, a: Optional[str], b: Optional[str]) -> bool: + """Equality test for parameters susceptible to Metabase "friendly names". + + For example, "Some Name" is a friendly name for "some_name". + + Args: + a (Optional[str]): Possibly-friendly string. + b (Optional[str]): Possibly-friendly string. + + Returns: + bool: True if strings are equal normalization. + """ + + def normalize(x: Optional[str]) -> str: + return (x or "").replace(" ", "_").replace("-", "_").lower() + + return normalize(a) == normalize(b) + class _ExtractExposuresJob(_MetabaseClientJob): _RESOURCE_VERSION = 2 @@ -788,7 +811,7 @@ def __init__( self.session.mount( self.url, - http_adapter or HTTPAdapter(max_retries=Retry(total=3, backoff_factor=0.5)), + http_adapter or HTTPAdapter(max_retries=Retry(total=3, backoff_factor=1)), ) if not session_id: diff --git a/sandbox/requirements.txt b/sandbox/requirements.txt index 281f4f31..1c76a539 100644 --- a/sandbox/requirements.txt +++ b/sandbox/requirements.txt @@ -1,3 +1,3 @@ -molot -requests -dbt-postgres +molot>=1.0.0,<1.1.0 +requests>=2.26.0,<3.0.0 +dbt-postgres>=1.7.4,<2.0.0 diff --git a/setup.py b/setup.py index 9d19cedf..93f64c47 100755 --- a/setup.py +++ b/setup.py @@ -30,9 +30,7 @@ def requires_from_file(filename: str) -> list: packages=find_packages(exclude=["tests"]), test_suite="tests", install_requires=requires_from_file("requirements.txt"), - extras_require={ - "test": requires_from_file("requirements-test.txt"), - }, + tests_require=requires_from_file("requirements-test.txt"), classifiers=[ "Intended Audience :: Developers", "License :: OSI Approved :: MIT License", diff --git a/tests/fixtures/sample_project/models/schema.yml b/tests/fixtures/sample_project/models/schema.yml index 95ebf6d0..57486fe4 100644 --- a/tests/fixtures/sample_project/models/schema.yml +++ b/tests/fixtures/sample_project/models/schema.yml @@ -31,7 +31,7 @@ models: metabase.display_name: order_count metabase.semantic_type: null - - name: total_order_amount + - name: customer_lifetime_value description: Total value (AUD) of a customer's orders - name: orders