Resolve ref to prefer package node over root node when parsing package, behind require_ref_searches_node_package_before_root flag

[MichelleArk]

Resolves #11351

Problem

• When there are nodes with duplicate names across packages, dbt currently resolves a ref from the package to the node in the running project. This can lead to unexpected cycles downstream

Solution

Behind a behavior change flag, fix the package search order to avoid this case.

Emit a warning when this behavior is currently relied on for tracking / rollout of behavior flag change.

Checklist

• [x] I have read the contributing guide and understand what's expected of me.
• [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 or relevant for this PR.
• [x] 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.
• [x] This PR includes type annotations for new and modified functions.

[github-actions[bot]]

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.

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 91.35%. Comparing base (fcd6870) to head (80ba7fa). :warning: Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11366      +/-   ##
==========================================
- Coverage   91.37%   91.35%   -0.02%     
==========================================
  Files         203      203              
  Lines       25030    25044      +14     
==========================================
+ Hits        22870    22878       +8     
- Misses       2160     2166       +6     

Flag	Coverage Δ
integration	88.15% <100.00%> (-0.08%)	:arrow_down:
unit	65.29% <73.33%> (+<0.01%)	:arrow_up:

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

Components	Coverage Δ
Unit Tests	65.29% <73.33%> (+<0.01%)	:arrow_up:
Integration Tests	88.15% <100.00%> (-0.08%)	:arrow_down:

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dbeatty10]

@MichelleArk What would be the next steps to determine if we can adopt the change in this PR or not?

Do we know of any other key assumptions that would this break if we adopt it?

[jtcohen6]

discussion with @zqureshi

This feels like a correct change to me - strongly agree with the rationale that @dbeatty10 @TomSteenbergen laid out in the other thread.

This change would negatively impact someone IFF:

• They have model_x defined in both their root project and an installed package
• They are counting on the fact that (single-argument) references to model_x within that package will resolve to the root project implementation, rather than the package installation

This was not possible prior to dbt-core v1.6 (released 1.5-2 years ago), when we added support for model namespacing (i.e. models by the same name in different projects/packages). The previous approach to accomplishing ^this use case, which we documented for many years, was to disable the package model_x and reimplement model_x in the root project.

So I think the risk here is relatively small, but it's also deeply nested logic, and we don't know exactly whom it might affect. (I did a little looking, and I did remember that this is a fairly common pattern for "template" models/packages that are reconfigured across multiple clients/regions/etc.)

Let's take a more-cautious approach and put this change behind a behavior flag, planned for True in v1.10. (I don't know if there's a good way for us to fire a log / deprecation warning without slowing down ref resolution, so we can track potential impact of this change, but that's worth some time-boxed investigation.)

And let's add a test, for the behavior before / after :)

[TomSteenbergen]

@MichelleArk Any update on this PR? IIUC, @jtcohen6's comment here still requires a follow-up

[QMalcolm]

Eeek I meant to "comment" instead of "request changes" 🤦🏻