Repository: dbt-labs/dbt-core
Pull request: Running a snapshot with missing required configurations results in un...
…caught Python exception (#3381)

* Running a snapshot with missing required configurations results in uncaught Python exception

resolves #3381 

<!---
  Include the number of the issue addressed by this PR above if applicable.
  PRs for code changes without an associated issue *will not be merged*.
  See CONTRIBUTING.md for more information.

  Example:
    resolves #1234
-->


### Description

<!--- Describe the Pull Request here -->
Adds additional validation logic for snapshots when required configurations are not provided


### Checklist
 - [X] I have signed the [CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
 - [X] I have run this code in development and it appears to resolve the stated issue
 - [X] This PR includes tests, or tests are not required/relevant for this PR
 - [X] I have updated the `CHANGELOG.md` and added information about my change to the "dbt next" section.


Problem statement:
Running a snapshot with missing required configurations results in uncaught Python exception
### Describe the bug
In an attempt to educate myself on how snapshots work, I created a snapshot to test out, but neglected to realize that the configuration object was required. When I ran `dbt snapshot`, it failed with a fairly non-actionable error message:

```
2021-05-20T22:41:00.291112Z: uncaught python exception
'NoneType' object has no attribute 'lower' Code: -32000
```

### Steps To Reproduce
1. Create a snapshot without a config object, i.e.
```
{% snapshot snap_demo %}

select * from {{ ref('snapshot_demo_source') }}

{% endsnapshot %}
```
2. Run `dbt snapshot`

### Expected behavior
It would be helpful to get an error message that indicates what actually went wrong, and guides the user to provide required configurations

### Screenshots and log output
If applicable, add screenshots or log output to help explain your problem.

### System information
**Which database are you using dbt with?**
- [ ] postgres
- [ ] redshift
- [ ] bigquery
- [X] snowflake
- [ ] other (specify: ____________)


**The output of `dbt --version`:**
```
0.19.1
```

**The operating system you're using:**
MacOS

**The output of `python --version`:**

### Additional context
Add any other context about the problem here.

Hints from issue discussion:
@jnatkins Such a good issue, and a great catch, thank you for opening!

I think there's a pretty straightforward resolution here, which is adding a check to the `validate` method of `SnapshotConfig`:

https://github.com/fishtown-analytics/dbt/blob/e775f2b38e52e8b9f25f8ac98c942d22b2c8c55d/core/dbt/contracts/graph/model_config.py#L448-L485

This method already exists to ensure that snapshots with certain strategies (`timestamp`, `check`) also contain the required associated configurations. We should first just check to make sure the [three required configurations](https://docs.getdbt.com/docs/building-a-dbt-project/snapshots#snapshot-configurations) are present. Something like:
```python
        if not data.get('strategy') or not data.get('unique_key') or not data.get('target_schema'):
            raise ValidationError(
                "Snapshots must be configured with a 'strategy', 'unique_key', and 'target_schema'.")
```

So instead of `'NoneType' object has no attribute 'lower'`, you'd get:
```
Compilation Error in snapshot my_snap (snapshots/my_snap.sql)
  at path []: Snapshots must be configured with a 'strategy', 'unique_key', and 'target_schema'.
```

Whether those configs are set via in-file `config()` blocks, or in `dbt_project.yml`, doesn't much matter. They just need to be set one way or another.

I'm going to tag this as a `good first issue`. Is this a fix you (or anyone else) might be interested in contributing? :)
@jtcohen6 I'd definitely be interested in contributing a fix here. I don't have a dev environment set up for dbt core yet, but I can probably scrape something together pretty quickly.

Commit message: Merge branch 'develop' of github.com:jnatkins/dbt into dbt-3381

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a961c370613..f1fea0c233b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,7 @@
 - Add a better error messages for undefined macros and when there are less packages installed than specified in `packages.yml`. ([#2999](https://github.com/fishtown-analytics/dbt/issues/2999))
 - Separate `compiled_path` from `build_path`, and print the former alongside node error messages ([#1985](https://github.com/fishtown-analytics/dbt/issues/1985), [#3327](https://github.com/fishtown-analytics/dbt/pull/3327))
 - Fix exception caused when running `dbt debug` with BigQuery connections ([#3314](https://github.com/fishtown-analytics/dbt/issues/3314), [#3351](https://github.com/fishtown-analytics/dbt/pull/3351))
+- Running a snapshot with missing required configurations results in uncaught Python exception ([#3381](https://github.com/fishtown-analytics/dbt/issues/3381), [#3385](https://github.com/fishtown-analytics/dbt/pull/3385))
 
 ### Under the hood
 - Added logic for registry requests to raise a timeout error after a response hangs out for 30 seconds and 5 attempts have been made to reach the endpoint ([#3177](https://github.com/fishtown-analytics/dbt/issues/3177), [#3275](https://github.com/fishtown-analytics/dbt/pull/3275))
diff --git a/core/dbt/contracts/graph/model_config.py b/core/dbt/contracts/graph/model_config.py
index 580b24f8f03..e3566e2fff7 100644
--- a/core/dbt/contracts/graph/model_config.py
+++ b/core/dbt/contracts/graph/model_config.py
@@ -457,6 +457,11 @@ class SnapshotConfig(EmptySnapshotConfig):
     @classmethod
     def validate(cls, data):
         super().validate(data)
+        if not data.get('strategy') or not data.get('unique_key') or not \
+                data.get('target_schema'):
+            raise ValidationError(
+                "Snapshots must be configured with a 'strategy', 'unique_key', "
+                "and 'target_schema'.")
         if data.get('strategy') == 'check':
             if not data.get('check_cols'):
                 raise ValidationError(
diff --git a/test/integration/004_simple_snapshot_test/test_simple_snapshot.py b/test/integration/004_simple_snapshot_test/test_simple_snapshot.py
index ffd8dbf2028..7bcb1437eb9 100644
--- a/test/integration/004_simple_snapshot_test/test_simple_snapshot.py
+++ b/test/integration/004_simple_snapshot_test/test_simple_snapshot.py
@@ -531,7 +531,7 @@ def test__postgres__invalid(self):
         with self.assertRaises(dbt.exceptions.CompilationException) as exc:
             self.run_dbt(['compile'], expect_pass=False)
 
-        self.assertIn('Compilation Error in model ref_snapshot', str(exc.exception))
+        self.assertIn('Snapshots must be configured with a \'strategy\'', str(exc.exception))
 
 
 class TestCheckCols(TestSimpleSnapshotFiles):
diff --git a/test/unit/test_contracts_graph_parsed.py b/test/unit/test_contracts_graph_parsed.py
index f2fb11b3abd..92d1568af06 100644
--- a/test/unit/test_contracts_graph_parsed.py
+++ b/test/unit/test_contracts_graph_parsed.py
@@ -1343,6 +1343,22 @@ def test_invalid_missing_check_cols(basic_check_snapshot_config_dict):
     del wrong_fields['check_cols']
     with pytest.raises(ValidationError, match=r"A snapshot configured with the check strategy"):
         SnapshotConfig.validate(wrong_fields)
+        
+def test_missing_snapshot_configs(basic_check_snapshot_config_dict):
+    wrong_fields = basic_check_snapshot_config_dict
+    del wrong_fields['strategy']
+    with pytest.raises(ValidationError, match=r"Snapshots must be configured with a 'strategy'"):
+        SnapshotConfig.validate(wrong_fields)
+    
+    wrong_fields['strategy'] = 'timestamp'
+    del wrong_fields['unique_key']
+    with pytest.raises(ValidationError, match=r"Snapshots must be configured with a 'strategy'"):
+        SnapshotConfig.validate(wrong_fields)
+        
+    wrong_fields['unique_key'] = 'id'
+    del wrong_fields['target_schema']
+    with pytest.raises(ValidationError, match=r"Snapshots must be configured with a 'strategy'"):
+        SnapshotConfig.validate(wrong_fields)
 
 
 def test_invalid_check_value(basic_check_snapshot_config_dict):


Code review comments:
diff_hunk: @@ -15,6 +15,7 @@
 - Add a better error messages for undefined macros and when there are less packages installed than specified in `packages.yml`. ([#2999](https://github.com/fishtown-analytics/dbt/issues/2999))
 - Separate `compiled_path` from `build_path`, and print the former alongside node error messages ([#1985](https://github.com/fishtown-analytics/dbt/issues/1985), [#3327](https://github.com/fishtown-analytics/dbt/pull/3327))
 - Fix exception caused when running `dbt debug` with BigQuery connections ([#3314](https://github.com/fishtown-analytics/dbt/issues/3314), [#3351](https://github.com/fishtown-analytics/dbt/pull/3351))
+- Running a snapshot with missing required configurations results in uncaught Python exception ([#3381](https://github.com/fishtown-analytics/dbt/issues/3381), [#3385](https://github.com/fishtown-analytics/dbt/pull/3385))
path: CHANGELOG.md
text: @user1:
I'd prefer to have the changelog entry describe the change being made, rather than the (now-solved) problem:
```suggestion
- Raise better error if snapshot is missing required configurations ([#3381](https://github.com/fishtown-analytics/dbt/issues/3381), [#3385](https://github.com/fishtown-analytics/dbt/pull/3385))
```

Could you also add yourself to the list of Contributors below? You definitely deserve to be on there (: