Partial parsing bug with empty schema file - ensure None is not passed to load_yaml_text #6494
Conversation
core/dbt/parser/schemas.py
Outdated
| @@ -100,8 +100,11 @@ | |||
| def yaml_from_file(source_file: SchemaSourceFile) -> Dict[str, Any]: | |||
| """If loading the yaml fails, raise an exception.""" | |||
| path = source_file.path.relative_path | |||
| # File contents filed is Optional, and is sometimes None, so ensure | |||
| # we are not passing in None to load_yaml_text. | |||
| contents = source_file.contents if source_file.contents else "" | |||
There was a problem hiding this comment.
I can go either way on this, but this could be:
contents = source_file.contents or ""
Taken further, this function could be:
def yaml_from_file(source_file: SchemaSourceFile) -> Dict[str, Any]:
try:
return load_yaml_text(source_file.contents or "", source_file.path)
except ValidationException as e:
raise YamlLoadFailure(source_file.project_name, source_file.path.relative_path, e)
The initial variables of path and contents are only used once, so the only benefit is readability. However, in both cases I need to refer to the variable definition to see what's being passed in; i.e. contents is forced to have a value, hence is not quite the same as source_file.contents, and path is actually the relative path, and not the absolute path. If these are moved into the function call, it's more readable to me (though that may differ by user). Also, the docstring is basically the code at that point, so it's no longer needed.
As an alternative for the None vs "" issue with load_yaml_text(), should we consider overriding None with "" in load_yaml_text() itself? In other words, will this happen elsewhere? Will we always want to pass in an empty string when we have a None? Or are there valid scenarios where we want to pass in a None for this argument and generate the resulting error?
There was a problem hiding this comment.
I changed yaml_from_file as per your suggestion. I'd rather not change load_yaml_text at this point because we're going to backport this and we want to make backport changes as minimal as possible.
| @@ -477,6 +477,8 @@ def test_postgres_skip_macros(self): | |||
| # initial run so we have a msgpack file | |||
| self.setup_directories() | |||
| self.copy_file('test-files/model_one.sql', 'models/model_one.sql') | |||
| # use empty_schema file for bug #4850 | |||
| self.copy_file('test-files/empty_schema.yml', 'models/eschema.yml') | |||
There was a problem hiding this comment.
This test used to run always without this file being present, but now runs always with this file being. So we effectively lost the integration test scenario where this file doesn't exist at all. Do we care about that? We may not; I don't know enough about this functionality.
There was a problem hiding this comment.
No, we don't care about that.
gshank
force-pushed
the
ct-343-bug_partial_parsing_empty_schema_file
branch
from
396dedd
to
79423ce
Compare
| @@ -1,7 +1,9 @@ | |||
| import os | |||
| # Do not import the os package because we expose this package in jinja | |||
There was a problem hiding this comment.
I don't understand this comment. I believe the os package is still running, even if only a few objects are pulled in from that namespace. In other words, I don't think this is doing anything different than the original version.
leahwicz
added
backport 1.0.latest
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.1.latest 1.1.latest
# Navigate to the new working tree
cd .worktrees/backport-1.1.latest
# Create a new branch
git switch --create backport-6494-to-1.1.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6ef3fbbf7608955a8828bb99d03ce6aee29ffd79
# Push it to GitHub
git push --set-upstream origin backport-6494-to-1.1.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.1.latestThen, create a pull request where the |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.3.latest 1.3.latest
# Navigate to the new working tree
cd .worktrees/backport-1.3.latest
# Create a new branch
git switch --create backport-6494-to-1.3.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6ef3fbbf7608955a8828bb99d03ce6aee29ffd79
# Push it to GitHub
git push --set-upstream origin backport-6494-to-1.3.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3.latestThen, create a pull request where the |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.2.latest 1.2.latest
# Navigate to the new working tree
cd .worktrees/backport-1.2.latest
# Create a new branch
git switch --create backport-6494-to-1.2.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6ef3fbbf7608955a8828bb99d03ce6aee29ffd79
# Push it to GitHub
git push --set-upstream origin backport-6494-to-1.2.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.2.latestThen, create a pull request where the |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.0.latest 1.0.latest
# Navigate to the new working tree
cd .worktrees/backport-1.0.latest
# Create a new branch
git switch --create backport-6494-to-1.0.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6ef3fbbf7608955a8828bb99d03ce6aee29ffd79
# Push it to GitHub
git push --set-upstream origin backport-6494-to-1.0.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.0.latestThen, create a pull request where the |
resolves #4850
Description
The "contents" of a file object is defined as Optional[str] but load_yaml_text does not support passing in None from contents. Ensure that if field is set to None, an empty string is passed.
Checklist
changie newto create a changelog entry