Fix YAML parsing with anchors and duplicate keys in dbt_project.yml file

[jeremyyeo]

resolves #5268, #5331

Description

Just pondering on a fix for the linked issues.

1. Seems like safe_load is used in multiple places - so just revert to previous behaviour by default + option to use the newly introduced UniqueKeyLoader.
2. Looks like a call to flatten_mapping sorts out the YAML anchors.

This seems to work for:

# ~/.dbt/profiles.yml

postgres:
  outputs:
    dev: &profile
      type: postgres
      host: localhost
      user: root
      password: password
      schema: public
      database: postgres
      port: 5432
      threads: 8
    prod:
      <<: *profile
    uat: *profile
  target: dev

and

# dbt_project.yml

...
vars:
  foo: bar
  foo: bar
...

Checklist

• I have read the contributing guide and understand what's expected of me
• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
• I have run changie new to create a changelog entry

[github-actions]

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

[jtcohen6]

Thanks for catching the bugs and taking a swing at this @jeremyyeo! The approach you're taking here looks directionally correct. We want to try validating yaml with UniqueKeyLoader, raise a warning ONLY (which becomes an error if --warn-error), and then proceed to actually use the SafeLoader regardless.

It should be really simple to add unit test cases for the two cases we identified. Here's my go at doing that: f7705a3

@dbt-labs/core-language Let's prioritize review of this PR, since it resolves some known net-new regressions in v1.2, and blocks us from being able to put out a beta prerelease.

[jtcohen6]

👍

[jtcohen6]

Question for @dbt-labs/core-language on how to factor this code: Should we opt for one method with a boolean argument like this, or for two different methods: load_and_validate_unique_keys + safe_load?

[gshank]

Whether I prefer a 'unique' argument or a separate method name depends on why we're only actually using the 'unique' arg in 'load_yaml_text', but we call 'safe_load' in multiple other places. Do those other places not need the duplicate key validation? Why is it only called there?

[jeremyyeo]

That's a good point - I think we should be validating duplicate key everywhere.. so I reverted this to what it was before.

[jeremyyeo]

Not sure what's up with the unit test but standard run with dupe vars keys worked okay

Ah okay... because we raise a warning instead of cleanly parsing the dupe:

from core.dbt.clients.yaml_helper import load_yaml_text

text = """
vars:
  foo: bar
  foo: bar
"""

load_yaml_text(text)

$ python check.py  
06:12:47  [WARNING]: Compilation Error
  Duplicate 'foo' key found in yaml file

Change the assertion slightly.

Another thing to consider - a dupe with a different value:

vars:
  foo: bar
  foo: baz

What should that really resolve to (in this scenario it resolve to {'vars': {'foo': 'baz'}})? Perhaps it's not such a bad idea to error as it was doing in the first place?

[jtcohen6]

Perhaps it's not such a bad idea to error as it was doing in the first place?

This wasn't an error in previous versions, so we can't raise an error now. The implicit behavior is to "take the last"; that's what it used to do, and what it will keep doing. It's a huge net improvement that we're raising a warning, so that users can become aware of the duplication/discrepancy and fix it themselves!

One other small aesthetic thing. It's a bit confusing to see:

06:12:47  [WARNING]: Compilation Error

Rather than raising an exception, which we then catch and raise as a warning within load_yaml_text, could construct_mapping just use dbt.exceptions.warn_or_raise to raise the warning message directly? I'm not sure exactly how to make that interact with the try/except, but we should really be falling back to real SafeLoader in all cases, and just using the UniqueKeyLoader for validation/warning.

[jeremyyeo]

1. Use warn_or_raise in construct_mapping instead. warn_or_raise calls the exception - not sure if this is the right way to do this since I couldn't find any other use of warn_or_raise.
2. Tidied up confusing [WARNING]: Compilation Error print outs.
3. Not too sure how to get the path of the file to be included in the warning message as before (when we had the path in the load_yaml_text function).

[gshank]

We could add a path arg to 'safe_load', throw an error in 'construct_mapping', catch it and issue a warning in safe_load with the path. I did a quick scan to see how hard it would be to do that, and there's a number of places where it wouldn't really matter (loading samples, etc), so it might not be too hard to do that. Up to you whether you want to go to the trouble, though a warning without the file name would be kind of frustrating as a user.

[jtcohen6]

Is it fair to close this PR for now, since we ended up reverting the change in #5146?