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

Fix parsing error for projects using the new YAML format for snapshots #11362

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

amardatar
Copy link

Resolves #11164

Problem

See #11164 - dbt projects using the new YAML format for Snapshots fail to parse, due to the schema used during parsing having raw_code = None while this is required by schema validation to be a string.

Solution

This manually sets block.file.contents to an empty string to allow validation to pass while parsing.

Checklist

  • I have read the contributing guide and understand what's expected of me.
  • I have run this code in development, and it appears to resolve the stated issue.
  • This PR includes tests, or tests are not required or relevant for this PR.
  • This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX.
  • This PR includes type annotations for new and modified functions.

Copy link

codecov bot commented Mar 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.87%. Comparing base (77d8e32) to head (2e7ebfa).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11362      +/-   ##
==========================================
- Coverage   88.92%   88.87%   -0.05%     
==========================================
  Files         190      190              
  Lines       24197    24197              
==========================================
- Hits        21517    21505      -12     
- Misses       2680     2692      +12     
Flag Coverage Δ
integration 86.08% <100.00%> (-0.13%) ⬇️
unit 62.59% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Unit Tests 62.59% <100.00%> (ø)
Integration Tests 86.08% <100.00%> (-0.13%) ⬇️
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MichelleArk
Copy link
Contributor

Thank you for putting together this PR and laying out the potential approaches so clearly! This implementation looks great to me in terms of limiting the blast radius and mitigating the root cause. Let's put together a small test case, perhaps by extending the existing TestYamlSnapshotPartialParsing functional test.

Would be great to get this contribution in quickly since it's affecting many people! Let me know if you want to take a stab at the test or I can take it over the finish line (keeping your changelog cred of course!) 🏁

@amardatar
Copy link
Author

Hey @MichelleArk thanks for picking this up! I've had a go at adding some tests - but totally happy for you to take this over as well if it's easier to get it through!

While adding the tests, I did a bit more digging, and found that there were some KeyErrors in a call to ManifestLoader.safe_update_project_parser_files_partially - which were handled but in a way that resulted in short-circuiting and not updating all the files. I'm assuming these were unintentional and just a result of iteration, and are safe to handle in this manner.

I'm just having a look now to see if I can update any unit tests to match the changes - but have pushed the changes first in case you've got any feedback.

@amardatar
Copy link
Author

Added some tests in, although the conditions that trigger this issue are quite specific, so I ended up having to change the unit tests a bit more than I was expecting (including changing some parts I wasn't planning to change). Happy to take on any feedback around what I have modified if we want it in a better state before merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes community This PR is from a community member
Projects
None yet
2 participants