-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add minimal validation of schema file yaml prior to partial parsing #3460
Conversation
ee905b1
to
e0fc1a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before:
Encountered an error:
string indices must be integers
After:
Encountered an error:
Compilation Error
The schema file at models/schema.yml is invalid because the value of 'models' is not a list
Definitely an improvement!
@@ -3,7 +3,8 @@ | |||
FilePath, ParseFileType, SourceFile, FileHash, AnySourceFile, SchemaSourceFile | |||
) | |||
|
|||
from dbt.parser.schemas import yaml_from_file | |||
from dbt.parser.schemas import yaml_from_file, schema_file_keys, check_format_version | |||
from dbt.exceptions import CompilationException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For later, not now: We may want a cleaner distinction between compile-time exceptions (Jinja rendering, SQL templating) and validation/parse-time exceptions. This would be one of the latter.
return source_file | ||
|
||
|
||
# Do some minimal validation of the yaml in a schema file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we doing this validation on all yaml files, or just the ones we've identified to be new/changed during partial parsing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this for a while, and in theory we could get away with just doing it on changed and new files, but then the validation would have to be done in the partial parsing code, which felt wrong, and also yaml files with these errors wouldn't get any benefit from the better errors. Also we'd have different error messages depending on whether or not somebody was doing partial parsing.
e0fc1a7
to
eb47b85
Compare
resolves #3246
Description
Partial parsing expects a few things from the dictionary contents of a schema yaml file, but validation hasn't been performed yet, so certain types of errors in the yaml would result in unhelpful errors, such as "key error". This adds validation that the value of the expected yaml keys is a list, that the list contains dictionaries, and that the dictionaries contain a 'name' key.
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.