Skip to content

Commit

Permalink
Add flag for raising errors on warnings
Browse files Browse the repository at this point in the history
fix strict mode
fix some strict mode asserts
add internal WARN_ERROR support and a warn_on_error function
make everything that says "warning" go through warn_or_error
add --warn-error flag to main.py
remove sql_where from tests
replace drop-existing with full-refresh in tests
(re-)add integration tests for malformed schema/source tests and strict mode
  • Loading branch information
Jacob Beck committed Feb 7, 2019
1 parent 314b453 commit 4a514fc
Show file tree
Hide file tree
Showing 25 changed files with 159 additions and 74 deletions.
2 changes: 1 addition & 1 deletion core/dbt/adapters/base/connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ def _rollback(self, connection):
operation does not require the lock.
"""
if dbt.flags.STRICT_MODE:
Connection(**connection)
assert isinstance(connection, Connection)

if connection.transaction_open is False:
raise dbt.exceptions.InternalException(
Expand Down
1 change: 1 addition & 0 deletions core/dbt/adapters/sql/connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import dbt.clients.agate_helper
import dbt.exceptions
from dbt.contracts.connection import Connection
from dbt.adapters.base import BaseConnectionManager
from dbt.compat import abstractclassmethod
from dbt.logger import GLOBAL_LOGGER as logger
Expand Down
5 changes: 2 additions & 3 deletions core/dbt/compilation.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ def recursively_prepend_ctes(model, manifest):
return (model, model.extra_ctes, manifest)

if dbt.flags.STRICT_MODE:
# ensure that all the nodes in this manifest are compiled
CompiledGraph(**manifest.to_flat_graph())
# ensure that the cte we're adding to is compiled
CompiledNode(**model.serialize())

prepended_ctes = []

Expand All @@ -81,7 +81,6 @@ def recursively_prepend_ctes(model, manifest):
cte_to_add = manifest.nodes.get(cte_id)
cte_to_add, new_prepended_ctes, manifest = recursively_prepend_ctes(
cte_to_add, manifest)

_extend_prepended_ctes(prepended_ctes, new_prepended_ctes)
new_cte_name = '__dbt__CTE__{}'.format(cte_to_add.get('name'))
sql = ' {} as (\n{}\n)'.format(new_cte_name, cte_to_add.compiled_sql)
Expand Down
3 changes: 2 additions & 1 deletion core/dbt/config/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from dbt.exceptions import RecursionException
from dbt.exceptions import SemverException
from dbt.exceptions import ValidationException
from dbt.exceptions import warn_or_error
from dbt.logger import GLOBAL_LOGGER as logger
from dbt.semver import VersionSpecifier
from dbt.semver import versions_compatible
Expand Down Expand Up @@ -417,7 +418,7 @@ def warn_for_unused_resource_config_paths(self, resource_fqns, disabled):
len(unused),
'\n'.join('- {}'.format('.'.join(u)) for u in unused)
)
logger.info(printer.yellow(msg))
warn_or_error(msg, log_fmt=printer.yellow('{}'))

def validate_version(self):
"""Ensure this package works with the installed version of dbt."""
Expand Down
5 changes: 4 additions & 1 deletion core/dbt/deprecations.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from dbt.logger import GLOBAL_LOGGER as logger
import dbt.links
import dbt.flags


class DBTDeprecation(object):
Expand All @@ -9,7 +10,9 @@ class DBTDeprecation(object):
def show(self, *args, **kwargs):
if self.name not in active_deprecations:
desc = self.description.format(**kwargs)
logger.info("* Deprecation Warning: {}\n".format(desc))
dbt.exceptions.warn_or_error(
"* Deprecation Warning: {}\n".format(desc)
)
active_deprecations.add(self.name)


Expand Down
10 changes: 10 additions & 0 deletions core/dbt/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from dbt.compat import basestring, builtins
from dbt.logger import GLOBAL_LOGGER as logger
import dbt.flags
import re


Expand Down Expand Up @@ -580,6 +581,15 @@ def raise_not_implemented(msg):
raise NotImplementedException(msg)


def warn_or_error(msg, node=None, log_fmt=None):
if dbt.flags.WARN_ERROR:
raise_compiler_error(msg, node)
else:
if log_fmt is not None:
msg = log_fmt.format(msg)
logger.warning(msg)


# Update this when a new function should be added to the
# dbt context's `exceptions` key!
CONTEXT_EXPORTS = {
Expand Down
4 changes: 3 additions & 1 deletion core/dbt/flags.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@
NON_DESTRUCTIVE = False
FULL_REFRESH = False
USE_CACHE = True
WARN_ERROR = False


def reset():
global STRICT_MODE, NON_DESTRUCTIVE, FULL_REFRESH
global STRICT_MODE, NON_DESTRUCTIVE, FULL_REFRESH, USE_CACHE, WARN_ERROR

STRICT_MODE = False
NON_DESTRUCTIVE = False
FULL_REFRESH = False
USE_CACHE = True
WARN_ERROR = False
5 changes: 3 additions & 2 deletions core/dbt/graph/selector.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,11 @@ def warn_if_useless_spec(spec, nodes):
if len(nodes) > 0:
return

logger.info(
"* Spec='{}' does not identify any models and was ignored\n"
msg = (
"* Spec='{}' does not identify any models"
.format(spec['raw'])
)
dbt.exceptions.warn_or_error(msg, log_fmt='{} and was ignored\n')


class NodeSelector(object):
Expand Down
16 changes: 15 additions & 1 deletion core/dbt/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,11 @@ def invoke_dbt(parsed):

arg_drop_existing = getattr(parsed, 'drop_existing', False)
arg_full_refresh = getattr(parsed, 'full_refresh', False)
flags.STRICT_MODE = getattr(parsed, 'strict', False)
flags.WARN_ERROR = (
flags.STRICT_MODE or
getattr(parsed, 'warn_error', False)
)

if arg_drop_existing:
dbt.deprecations.warn('drop-existing')
Expand Down Expand Up @@ -338,7 +343,16 @@ def parse_args(args):
'--strict',
action='store_true',
help='''Run schema validations at runtime. This will surface
bugs in dbt, but may incur a performance penalty.''')
bugs in dbt, but may incur a performance penalty. This flag implies
--warn-error''')

p.add_argument(
'--warn-error',
action='store_true',
help='''If dbt would normally warn, instead raise an exception.
Examples include --models that selects nothing, deprecations,
configurations with no associated models, invalid test configurations,
and missing sources/refs in tests''')

# if set, run dbt in single-threaded mode: thread count is ignored, and
# calls go through `map` instead of the thread pool. This is useful for
Expand Down
5 changes: 2 additions & 3 deletions core/dbt/parser/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,8 @@ def warn_invalid(filepath, key, value, explain):
msg = (
"Invalid test config given in {} @ {}: {} {}"
).format(filepath, key, value, explain)
if dbt.flags.STRICT_MODE:
dbt.exceptions.raise_compiler_error(msg, value)
dbt.utils.compiler_warning(value, msg)
dbt.exceptions.warn_or_error(msg, value,
log_fmt='Compilation warning: {}\n')


def _filter_validate(filepath, location, values, validate):
Expand Down
5 changes: 2 additions & 3 deletions core/dbt/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import os

import dbt.exceptions
import dbt.flags

from dbt.include.global_project import PACKAGES
from dbt.compat import basestring, DECIMALS
Expand Down Expand Up @@ -416,7 +415,7 @@ def invalid_ref_fail_unless_test(node, target_model_name,
if disabled:
logger.debug(msg)
else:
logger.warning(msg)
dbt.exceptions.warn_or_error(msg)

else:
dbt.exceptions.ref_target_not_found(
Expand All @@ -429,7 +428,7 @@ def invalid_source_fail_unless_test(node, target_name, target_table_name):
if node.get('resource_type') == NodeType.Test:
msg = dbt.exceptions.source_disabled_message(node, target_name,
target_table_name)
logger.warning('WARNING: {}'.format(msg))
dbt.exceptions.warn_or_error(msg, log_fmt='WARNING: {}')
else:
dbt.exceptions.source_target_not_found(node, target_name,
target_table_name)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{{
config(
materialized = "incremental",
sql_where = "TRUE",
unique_key = "id"
)
}}
Expand Down

This file was deleted.

67 changes: 37 additions & 30 deletions test/integration/001_simple_copy_test/test_simple_copy.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,17 @@ def test__postgres__simple_copy(self):
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["seed", "view_model", "incremental", "incremental_deprecated", "materialized"])
self.assertManyTablesEqual(["seed", "view_model", "incremental", "materialized"])

self.use_default_project({"data-paths": [self.dir("seed-update")]})
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["seed", "view_model", "incremental", "incremental_deprecated", "materialized"])
self.assertManyTablesEqual(["seed", "view_model", "incremental", "materialized"])

@use_profile("postgres")
def test__postgres__dbt_doesnt_run_empty_models(self):
Expand All @@ -44,7 +44,7 @@ def test__postgres__dbt_doesnt_run_empty_models(self):
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)

models = self.get_models_in_schema()

Expand All @@ -58,7 +58,7 @@ def test__presto__simple_copy(self):
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt(expect_pass=False)
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)
for result in results:
if 'incremental' in result.node.name:
self.assertIn('not implemented for presto', result.error)
Expand All @@ -69,16 +69,20 @@ def test__presto__simple_copy(self):
def test__snowflake__simple_copy(self):
self.use_default_project({"data-paths": [self.dir("seed-initial")]})

self.run_dbt(["seed"])
self.run_dbt()
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "INCREMENTAL_DEPRECATED", "MATERIALIZED"])
self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "MATERIALIZED"])

self.use_default_project({"data-paths": [self.dir("seed-update")]})
self.run_dbt(["seed"])
self.run_dbt()
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "INCREMENTAL_DEPRECATED", "MATERIALIZED"])
self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "MATERIALIZED"])

@use_profile("snowflake")
def test__snowflake__simple_copy__quoting_off(self):
Expand All @@ -90,9 +94,9 @@ def test__snowflake__simple_copy__quoting_off(self):
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "INCREMENTAL_DEPRECATED", "MATERIALIZED"])
self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "MATERIALIZED"])

self.use_default_project({
"data-paths": [self.dir("seed-update")],
Expand All @@ -101,9 +105,9 @@ def test__snowflake__simple_copy__quoting_off(self):
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "INCREMENTAL_DEPRECATED", "MATERIALIZED"])
self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "MATERIALIZED"])

@use_profile("snowflake")
def test__snowflake__seed__quoting_switch(self):
Expand All @@ -128,10 +132,9 @@ def test__bigquery__simple_copy(self):
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)

self.assertTablesEqual("seed","view_model")
self.assertTablesEqual("seed","incremental_deprecated")
self.assertTablesEqual("seed","incremental")
self.assertTablesEqual("seed","materialized")

Expand All @@ -140,10 +143,9 @@ def test__bigquery__simple_copy(self):
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)

self.assertTablesEqual("seed","view_model")
self.assertTablesEqual("seed","incremental_deprecated")
self.assertTablesEqual("seed","incremental")
self.assertTablesEqual("seed","materialized")

Expand All @@ -166,19 +168,19 @@ def test__snowflake__simple_copy__quoting_on(self):
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["seed", "view_model", "incremental", "incremental_deprecated", "materialized"])
self.assertManyTablesEqual(["seed", "view_model", "incremental", "materialized"])

self.use_default_project({
"data-paths": [self.dir("seed-update")],
})
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 7)
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["seed", "view_model", "incremental", "incremental_deprecated", "materialized"])
self.assertManyTablesEqual(["seed", "view_model", "incremental", "materialized"])


class BaseLowercasedSchemaTest(BaseTestSimpleCopy):
Expand All @@ -193,16 +195,21 @@ class TestSnowflakeSimpleLowercasedSchemaCopy(BaseLowercasedSchemaTest):
def test__snowflake__simple_copy(self):
self.use_default_project({"data-paths": [self.dir("seed-initial")]})

self.run_dbt(["seed"])
self.run_dbt()
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "INCREMENTAL_DEPRECATED", "MATERIALIZED"])
self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "MATERIALIZED"])

self.use_default_project({"data-paths": [self.dir("seed-update")]})
self.run_dbt(["seed"])
self.run_dbt()

self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "INCREMENTAL_DEPRECATED", "MATERIALIZED"])
results = self.run_dbt(["seed"])
self.assertEqual(len(results), 1)
results = self.run_dbt()
self.assertEqual(len(results), 6)

self.assertManyTablesEqual(["SEED", "VIEW_MODEL", "INCREMENTAL", "MATERIALIZED"])


class TestSnowflakeSimpleLowercasedSchemaQuoted(BaseLowercasedSchemaTest):
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
{{
config(
materialized = "incremental",
sql_where = "id>(select max(id) from {{this}})"
materialized = "incremental"
)
}}

select * from {{ this.schema }}.seed

{% if adapter.already_exists(this) %}
{% if is_incremental() %}

where id > (select max(id) from {{this}})

Expand Down
Loading

0 comments on commit 4a514fc

Please sign in to comment.