From 02feb69f0d3eadd572dd6a5200e270ff2f157711 Mon Sep 17 00:00:00 2001 From: Jacob Beck Date: Mon, 7 Oct 2019 08:20:46 -0400 Subject: [PATCH] PR feedback, also remove an extra manifest-building from "dbt docs generate" --- core/dbt/task/generate.py | 12 +- core/dbt/task/runnable.py | 8 +- .../test_docs_generate.py | 131 +++++++++++++++--- test/integration/048_rpc_test/test_rpc.py | 7 + 4 files changed, 134 insertions(+), 24 deletions(-) diff --git a/core/dbt/task/generate.py b/core/dbt/task/generate.py index d4c6d5eb48a..13a7a801dee 100644 --- a/core/dbt/task/generate.py +++ b/core/dbt/task/generate.py @@ -19,6 +19,7 @@ import dbt.exceptions from dbt.task.compile import CompileTask +from dbt.task.runnable import write_manifest CATALOG_FILENAME = 'catalog.json' @@ -178,8 +179,8 @@ def _coerce_decimal(value): class GenerateTask(CompileTask): def _get_manifest(self) -> Manifest: - manifest = dbt.loader.GraphLoader.load_all(self.config) - return manifest + # manifest = dbt.loader.GraphLoader.load_all(self.config) + return self.manifest def run(self): compile_results = None @@ -197,10 +198,8 @@ def run(self): adapter = get_adapter(self.config) with adapter.connection_named('generate_catalog'): - manifest = self._get_manifest() - dbt.ui.printer.print_timestamped_line("Building catalog") - catalog_table = adapter.get_catalog(manifest) + catalog_table = adapter.get_catalog(self.manifest) catalog_data: List[PrimitiveDict] = [ dict(zip(catalog_table.column_names, map(_coerce_decimal, row))) @@ -209,13 +208,14 @@ def run(self): catalog = Catalog(catalog_data) results = self.get_catalog_results( - nodes=catalog.make_unique_id_map(manifest), + nodes=catalog.make_unique_id_map(self.manifest), generated_at=datetime.utcnow(), compile_results=compile_results, ) path = os.path.join(self.config.target_path, CATALOG_FILENAME) results.write(path) + write_manifest(self.manifest, self.config) dbt.ui.printer.print_timestamped_line( 'Catalog written to {}'.format(os.path.abspath(path)) diff --git a/core/dbt/task/runnable.py b/core/dbt/task/runnable.py index f5763eb7604..131627d9b6a 100644 --- a/core/dbt/task/runnable.py +++ b/core/dbt/task/runnable.py @@ -21,6 +21,11 @@ MANIFEST_FILE_NAME = 'manifest.json' +def write_manifest(manifest, config): + if dbt.flags.WRITE_JSON: + manifest.write(os.path.join(config.target_path, MANIFEST_FILE_NAME)) + + def load_manifest(config): # performance trick: if the adapter has a manifest loaded, use that to # avoid parsing internal macros twice. Also, when loading the adapter's @@ -31,8 +36,7 @@ def load_manifest(config): internal = adapter.load_internal_manifest() manifest = GraphLoader.load_all(config, internal_manifest=internal) - if dbt.flags.WRITE_JSON: - manifest.write(os.path.join(config.target_path, MANIFEST_FILE_NAME)) + write_manifest(manifest, config) return manifest diff --git a/test/integration/029_docs_generate_tests/test_docs_generate.py b/test/integration/029_docs_generate_tests/test_docs_generate.py index 6c7c456f53b..687bd8f715b 100644 --- a/test/integration/029_docs_generate_tests/test_docs_generate.py +++ b/test/integration/029_docs_generate_tests/test_docs_generate.py @@ -821,7 +821,7 @@ def expected_seeded_manifest(self, model_database=None): return { 'nodes': { 'model.test.model': { - 'build_path': None, + 'build_path': 'target/compiled/test/model.sql', 'name': 'model', 'root_path': self.test_root_dir, 'resource_type': 'model', @@ -869,9 +869,17 @@ def expected_seeded_manifest(self, model_database=None): }, 'patch_path': schema_yml_path, 'docrefs': [], + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': ANY, + 'wrapped_sql': 'None', }, 'seed.test.seed': { 'build_path': None, + 'compiled': True, + 'compiled_sql': '', 'config': { 'enabled': True, 'materialized': 'seed', @@ -905,10 +913,16 @@ def expected_seeded_manifest(self, model_database=None): 'description': '', 'columns': {}, 'docrefs': [], + 'compiled': True, + 'compiled_sql': '', + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': '', + 'wrapped_sql': 'None', }, 'test.test.not_null_model_id': { 'alias': 'not_null_model_id', - 'build_path': None, + 'build_path': 'target/compiled/test/schema_test/not_null_model_id.sql', 'column_name': 'id', 'columns': {}, 'config': { @@ -941,10 +955,16 @@ def expected_seeded_manifest(self, model_database=None): 'tags': ['schema'], 'unique_id': 'test.test.not_null_model_id', 'docrefs': [], + 'compiled': True, + 'compiled_sql': AnyStringWith('count(*)'), + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': AnyStringWith('count(*)'), + 'wrapped_sql': AnyStringWith('count(*)'), }, 'test.test.test_nothing_model_': { 'alias': 'test_nothing_model_', - 'build_path': None, + 'build_path': 'target/compiled/test/schema_test/test_nothing_model_.sql', 'column_name': None, 'columns': {}, 'config': { @@ -977,10 +997,16 @@ def expected_seeded_manifest(self, model_database=None): 'tags': ['schema'], 'unique_id': 'test.test.test_nothing_model_', 'docrefs': [], + 'compiled': True, + 'compiled_sql': AnyStringWith('select 0'), + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': AnyStringWith('select 0'), + 'wrapped_sql': AnyStringWith('select 0'), }, 'test.test.unique_model_id': { 'alias': 'unique_model_id', - 'build_path': None, + 'build_path': 'target/compiled/test/schema_test/unique_model_id.sql', 'column_name': 'id', 'columns': {}, 'config': { @@ -1013,6 +1039,12 @@ def expected_seeded_manifest(self, model_database=None): 'tags': ['schema'], 'unique_id': 'test.test.unique_model_id', 'docrefs': [], + 'compiled': True, + 'compiled_sql': AnyStringWith('count(*)'), + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': AnyStringWith('count(*)'), + 'wrapped_sql': AnyStringWith('count(*)'), }, }, 'parent_map': { @@ -1123,7 +1155,7 @@ def expected_postgres_references_manifest(self, model_database=None): 'nodes': { 'model.test.ephemeral_copy': { 'alias': 'ephemeral_copy', - 'build_path': None, + 'build_path': 'target/compiled/test/ephemeral_copy.sql', 'columns': {}, 'config': { 'column_types': {}, @@ -1160,10 +1192,16 @@ def expected_postgres_references_manifest(self, model_database=None): 'database': self.default_database, 'tags': [], 'unique_id': 'model.test.ephemeral_copy', + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': ANY, + 'wrapped_sql': 'None', }, 'model.test.ephemeral_summary': { 'alias': 'ephemeral_summary', - 'build_path': None, + 'build_path': 'target/compiled/test/ephemeral_summary.sql', 'columns': { 'first_name': { 'description': 'The first name being summarized', @@ -1228,10 +1266,17 @@ def expected_postgres_references_manifest(self, model_database=None): 'schema': my_schema_name, 'database': self.default_database, 'tags': [], - 'unique_id': 'model.test.ephemeral_summary'}, + 'unique_id': 'model.test.ephemeral_summary', + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [ANY], + 'injected_sql': ANY, + 'wrapped_sql': 'None', + }, 'model.test.view_summary': { 'alias': 'view_summary', - 'build_path': None, + 'build_path': 'target/compiled/test/view_summary.sql', 'columns': { 'first_name': { 'description': 'The first name being summarized', @@ -1295,7 +1340,13 @@ def expected_postgres_references_manifest(self, model_database=None): 'schema': my_schema_name, 'sources': [], 'tags': [], - 'unique_id': 'model.test.view_summary' + 'unique_id': 'model.test.view_summary', + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': ANY, + 'wrapped_sql': 'None', }, 'seed.test.seed': { 'alias': 'seed', @@ -1330,7 +1381,13 @@ def expected_postgres_references_manifest(self, model_database=None): 'schema': my_schema_name, 'database': self.default_database, 'tags': [], - 'unique_id': 'seed.test.seed' + 'unique_id': 'seed.test.seed', + 'compiled': True, + 'compiled_sql': '', + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': '', + 'wrapped_sql': 'None', }, 'source.test.my_source.my_table': { 'columns': { @@ -1365,7 +1422,7 @@ def expected_postgres_references_manifest(self, model_database=None): }, ], 'external': { - 'file_format': None, 'location': None, 'partitions': None, + 'file_format': None, 'location': None, 'partitions': None, 'row_format': None, 'tbl_properties': None }, 'freshness': {'error_after': None, 'warn_after': None, 'filter': None}, @@ -1589,7 +1646,7 @@ def expected_bigquery_complex_manifest(self): 'sources': [], 'depends_on': {'macros': [], 'nodes': ['seed.test.seed']}, 'fqn': ['test', 'clustered'], - 'build_path': None, + 'build_path': 'target/compiled/test/clustered.sql', 'name': 'clustered', 'original_file_path': clustered_sql_path, 'package_name': 'test', @@ -1632,10 +1689,16 @@ def expected_bigquery_complex_manifest(self): 'description': 'A clustered and partitioned copy of the test model', 'patch_path': self.dir('bq_models/schema.yml'), 'docrefs': [], + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': ANY, + 'wrapped_sql': 'None', }, 'model.test.multi_clustered': { 'alias': 'multi_clustered', - 'build_path': None, + 'build_path': 'target/compiled/test/multi_clustered.sql', 'config': { 'cluster_by': ['first_name', 'email'], 'column_types': {}, @@ -1694,10 +1757,16 @@ def expected_bigquery_complex_manifest(self): 'description': 'A clustered and partitioned copy of the test model, clustered on multiple columns', 'patch_path': self.dir('bq_models/schema.yml'), 'docrefs': [], + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': ANY, + 'wrapped_sql': 'None', }, 'model.test.nested_view': { 'alias': 'nested_view', - 'build_path': None, + 'build_path': 'target/compiled/test/nested_view.sql', 'config': { 'column_types': {}, 'enabled': True, @@ -1757,10 +1826,16 @@ def expected_bigquery_complex_manifest(self): 'description': 'The test model', 'patch_path': self.dir('bq_models/schema.yml'), 'docrefs': [], + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': ANY, + 'wrapped_sql': 'None', }, 'model.test.nested_table': { 'alias': 'nested_table', - 'build_path': None, + 'build_path': 'target/compiled/test/nested_table.sql', 'config': { 'column_types': {}, 'enabled': True, @@ -1794,6 +1869,12 @@ def expected_bigquery_complex_manifest(self): 'columns': {}, 'description': '', 'docrefs': [], + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': ANY, + 'wrapped_sql': 'None', }, 'seed.test.seed': { 'build_path': None, @@ -1832,6 +1913,12 @@ def expected_bigquery_complex_manifest(self): 'columns': {}, 'description': '', 'docrefs': [], + 'compiled': True, + 'compiled_sql': '', + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': '', + 'wrapped_sql': 'None', }, }, 'child_map': { @@ -1960,7 +2047,7 @@ def expected_redshift_incremental_view_manifest(self): return { 'nodes': { 'model.test.model': { - 'build_path': None, + 'build_path': 'target/compiled/test/model.sql', 'name': 'model', 'root_path': self.test_root_dir, 'resource_type': 'model', @@ -2022,6 +2109,12 @@ def expected_redshift_incremental_view_manifest(self): }, 'patch_path': self.dir('rs_models/schema.yml'), 'docrefs': [], + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': ANY, + 'wrapped_sql': 'None', }, 'seed.test.seed': { 'build_path': None, @@ -2060,6 +2153,12 @@ def expected_redshift_incremental_view_manifest(self): 'columns': {}, 'description': '', 'docrefs': [], + 'compiled': True, + 'compiled_sql': ANY, + 'extra_ctes_injected': True, + 'extra_ctes': [], + 'injected_sql': ANY, + 'wrapped_sql': 'None', }, }, 'parent_map': { diff --git a/test/integration/048_rpc_test/test_rpc.py b/test/integration/048_rpc_test/test_rpc.py index 08d48dc1f24..3054007bd2c 100644 --- a/test/integration/048_rpc_test/test_rpc.py +++ b/test/integration/048_rpc_test/test_rpc.py @@ -927,6 +927,8 @@ def test_docs_generate_postgres(self): self.run_dbt_with_vars(['seed']) self.run_dbt_with_vars(['run']) self.assertFalse(os.path.exists('target/catalog.json')) + if os.path.exists('target/manifest.json'): + os.remove('target/manifest.json') result = self.async_query('docs.generate').json() dct = self.assertIsResult(result) self.assertTrue(os.path.exists('target/catalog.json')) @@ -949,6 +951,11 @@ def test_docs_generate_postgres(self): } for uid in expected: self.assertIn(uid, nodes) + self.assertTrue(os.path.exists('target/manifest.json')) + with open('target/manifest.json') as fp: + manifest = json.load(fp) + self.assertIn('nodes', manifest) + self.assertEqual(len(manifest['nodes']), 17) class FailedServerProcess(ServerProcess):