Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

log warnings on missing test refs [#968] #1169

Merged
merged 3 commits into from
Dec 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion dbt/compilation.py
Original file line number Diff line number Diff line change
Expand Up @@ -248,8 +248,9 @@ def compile(self):
self._check_resource_uniqueness(manifest)

resource_fqns = manifest.get_resource_fqns()
disabled_fqns = [n.fqn for n in manifest.disabled]
self.config.warn_for_unused_resource_config_paths(resource_fqns,
manifest.disabled)
disabled_fqns)

self.link_graph(linker, manifest)

Expand Down
6 changes: 3 additions & 3 deletions dbt/context/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import dbt.clients.jinja
import dbt.context.common
import dbt.flags
import dbt.parser
from dbt.parser import ParserUtils

from dbt.logger import GLOBAL_LOGGER as logger # noqa

Expand All @@ -26,14 +26,14 @@ def do_ref(*args):
else:
dbt.exceptions.ref_invalid_args(model, args)

target_model = dbt.parser.ParserUtils.resolve_ref(
target_model = ParserUtils.resolve_ref(
manifest,
target_model_name,
target_model_package,
current_project,
model.get('package_name'))

if target_model is None:
if target_model is None or target_model is ParserUtils.DISABLED:
dbt.exceptions.ref_target_not_found(
model,
target_model_name,
Expand Down
16 changes: 7 additions & 9 deletions dbt/contracts/graph/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,8 @@
'docs': PARSED_DOCUMENTATIONS_CONTRACT,
'disabled': {
'type': 'array',
'items': {
'type': 'array',
'items': {
'type': 'string',
},
'description': 'A disabled node FQN',
},
'description': 'An array of disabled node FQNs',
'items': PARSED_NODE_CONTRACT,
'description': 'An array of disabled nodes',
},
'generated_at': {
'type': 'string',
Expand Down Expand Up @@ -221,9 +215,13 @@ def serialize(self):
'child_map': forward_edges,
'generated_at': self.generated_at,
'metadata': self.metadata,
'disabled': self.disabled,
'disabled': [v.serialize() for v in self.disabled],
}

def find_disabled_by_name(self, name, package=None):
return dbt.utils.find_in_list_by_name(self.disabled, name, package,
NodeType.refable())

def _find_by_name(self, name, package, subgraph, nodetype):
"""
Expand Down
42 changes: 34 additions & 8 deletions dbt/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,21 +271,47 @@ def doc_target_not_found(model, target_doc_name, target_doc_package):
raise_compiler_error(msg, model)


def get_target_not_found_msg(model, target_model_name, target_model_package):
def _get_target_failure_msg(model, target_model_name, target_model_package,
include_path, reason):
target_package_string = ''

if target_model_package is not None:
target_package_string = "in package '{}' ".format(target_model_package)

return ("Model '{}' depends on model '{}' {}which was not found or is"
" disabled".format(model.get('unique_id'),
target_model_name,
target_package_string))
source_path_string = ''
if include_path:
source_path_string = ' ({})'.format(model.get('original_file_path'))

return ("{} '{}'{} depends on model '{}' {}which {}"
.format(model.get('resource_type').title(),
model.get('unique_id'),
source_path_string,
target_model_name,
target_package_string,
reason))


def get_target_disabled_msg(model, target_model_name, target_model_package):
return _get_target_failure_msg(model, target_model_name,
target_model_package, include_path=True,
reason='is disabled')


def get_target_not_found_msg(model, target_model_name, target_model_package):
return _get_target_failure_msg(model, target_model_name,
target_model_package, include_path=True,
reason='was not found')


def get_target_not_found_or_disabled_msg(model, target_model_name,
target_model_package):
return _get_target_failure_msg(model, target_model_name,
target_model_package, include_path=False,
reason='was not found or is disabled')


def ref_target_not_found(model, target_model_name, target_model_package):
msg = get_target_not_found_msg(model, target_model_name,
target_model_package)
msg = get_target_not_found_or_disabled_msg(model, target_model_name,
target_model_package)
raise_compiler_error(msg, model)


Expand Down
2 changes: 1 addition & 1 deletion dbt/parser/base_sql.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ def parse_sql_nodes(cls, nodes, root_project, projects,

# Ignore disabled nodes
if not node_parsed['config']['enabled']:
disabled.append(node_parsed['fqn'])
disabled.append(node_parsed)
continue

# Check for duplicate model names
Expand Down
18 changes: 16 additions & 2 deletions dbt/parser/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ def do_docs(*args):


class ParserUtils(object):
DISABLED = object()

@classmethod
def resolve_ref(cls, manifest, target_model_name, target_model_package,
current_project, node_package):
Expand All @@ -44,6 +46,7 @@ def resolve_ref(cls, manifest, target_model_name, target_model_package,
target_model_package)

target_model = None
disabled_target = None

# first pass: look for models in the current_project
# second pass: look for models in the node's package
Expand All @@ -58,6 +61,15 @@ def resolve_ref(cls, manifest, target_model_name, target_model_package,

if target_model is not None and dbt.utils.is_enabled(target_model):
return target_model

# it's possible that the node is disabled
if disabled_target is None:
disabled_target = manifest.find_disabled_by_name(
target_model_name, candidate
)

if disabled_target is not None:
return cls.DISABLED
return None

@classmethod
Expand Down Expand Up @@ -147,12 +159,14 @@ def process_refs(cls, manifest, current_project):
current_project,
node.get('package_name'))

if target_model is None:
if target_model is None or target_model is cls.DISABLED:
# This may raise. Even if it doesn't, we don't want to add
# this node to the graph b/c there is no destination node
node.config['enabled'] = False
dbt.utils.invalid_ref_fail_unless_test(
node, target_model_name, target_model_package)
node, target_model_name, target_model_package,
disabled=(target_model is cls.DISABLED)
)

continue

Expand Down
74 changes: 56 additions & 18 deletions dbt/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,30 @@ def find_by_name(flat_graph, target_name, target_package, subgraph,
nodetype)


def id_matches(unique_id, target_name, target_package, nodetypes, model):
"""Return True if the unique ID matches the given name, package, and type.

If package is None, any package is allowed.
nodetypes should be a container of NodeTypes that implements the 'in'
operator.
"""
node_parts = unique_id.split('.')
if len(node_parts) != 3:
node_type = model.get('resource_type', 'node')
msg = "{} names cannot contain '.' characters".format(node_type)
dbt.exceptions.raise_compiler_error(msg, model)

resource_type, package_name, node_name = node_parts

if resource_type not in nodetypes:
return False

if target_name != node_name:
return False

return target_package is None or target_package == package_name


def find_in_subgraph_by_name(subgraph, target_name, target_package, nodetype):
"""Find an entry in a subgraph by name. Any mapping that implements
.items() and maps unique id -> something can be used as the subgraph.
Expand All @@ -110,18 +134,17 @@ def find_in_subgraph_by_name(subgraph, target_name, target_package, nodetype):
You can use `None` for the package name as a wildcard.
"""
for name, model in subgraph.items():
node_parts = name.split('.')
if len(node_parts) != 3:
node_type = model.get('resource_type', 'node')
msg = "{} names cannot contain '.' characters".format(node_type)
dbt.exceptions.raise_compiler_error(msg, model)

resource_type, package_name, node_name = node_parts

if (resource_type in nodetype and
((target_name == node_name) and
(target_package is None or
target_package == package_name))):
if id_matches(name, target_name, target_package, nodetype, model):
return model

return None


def find_in_list_by_name(haystack, target_name, target_package, nodetype):
"""Find an entry in the given list by name."""
for model in haystack:
name = model.get('unique_id')
if id_matches(name, target_name, target_package, nodetype, model):
return model

return None
Expand Down Expand Up @@ -423,14 +446,29 @@ def __get__(self, obj, objtype):
return functools.partial(self.__call__, obj)


def invalid_ref_test_message(node, target_model_name, target_model_package,
disabled):
if disabled:
msg = dbt.exceptions.get_target_disabled_msg(
node, target_model_name, target_model_package
)
else:
msg = dbt.exceptions.get_target_not_found_msg(
node, target_model_name, target_model_package
)
return 'WARNING: {}'.format(msg)


def invalid_ref_fail_unless_test(node, target_model_name,
target_model_package):
target_model_package, disabled):
if node.get('resource_type') == NodeType.Test:
warning = dbt.exceptions.get_target_not_found_msg(
node,
target_model_name,
target_model_package)
logger.debug("WARNING: {}".format(warning))
msg = invalid_ref_test_message(node, target_model_name,
target_model_package, disabled)
if disabled:
logger.debug(msg)
else:
logger.warning(msg)

else:
dbt.exceptions.ref_target_not_found(
node,
Expand Down
49 changes: 48 additions & 1 deletion test/unit/test_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -1202,6 +1202,7 @@ def test__other_project_config(self):
'materialized': 'ephemeral'
})


sort_config = self.model_config.copy()
sort_config.update({
'enabled': False,
Expand Down Expand Up @@ -1319,7 +1320,53 @@ def test__other_project_config(self):
description='',
columns={}
),
}, [['snowplow', 'disabled'], ['snowplow', 'views', 'package']])
},
[
ParsedNode(
name='disabled',
resource_type='model',
package_name='snowplow',
path='disabled.sql',
original_file_path='disabled.sql',
root_path=get_os_path('/usr/src/app'),
raw_sql=("select * from events"),
schema='analytics',
refs=[],
depends_on={
'nodes': [],
'macros': []
},
config=disabled_config,
tags=[],
empty=False,
alias='disabled',
unique_id='model.snowplow.disabled',
fqn=['snowplow', 'disabled'],
columns={}
),
ParsedNode(
name='package',
resource_type='model',
package_name='snowplow',
path=get_os_path('views/package.sql'),
original_file_path=get_os_path('views/package.sql'),
root_path=get_os_path('/usr/src/app'),
raw_sql=("select * from events"),
schema='analytics',
refs=[],
depends_on={
'nodes': [],
'macros': []
},
config=sort_config,
tags=[],
empty=False,
alias='package',
unique_id='model.snowplow.package',
fqn=['snowplow', 'views', 'package'],
columns={}
)
])
)

def test__simple_schema_v1_test(self):
Expand Down