Consistent behavior change flag names + deprecation warnings (#10063)

* Rename flags

* Attempt refactor of flag require_resource_names_without_spaces

* Add deprecation warning for source_freshness_run_project_hooks

* Switch require_explicit_package_overrides_for_builtin_materializations default to True

* Add changelog

* fix core_types_pb2.py

* add playbook on introducing + maintaining behavoiur change flags

* be less canadian

* refactor: use hooks in FreshnessTask.get_hooks_by_type

* changelog entry for behaviour change

* Update docs link

---------

Co-authored-by: Michelle Ark <[email protected]>

.changes/unreleased/Breaking Changes-20240430-165247.yaml

@@ -0,0 +1,7 @@
+kind: Breaking Changes
+body: Update the default behaviour of require_explicit_package_overrides_for_builtin_materializations
+  to True.
+time: 2024-04-30T16:52:47.610025-04:00
+custom:
+  Author: jtcohen6
+  Issue: "10062"

.changes/unreleased/Under the Hood-20240429-142342.yaml

@@ -0,0 +1,6 @@
+kind: Under the Hood
+body: Consistent naming + deprecation warnings for "legacy behavior" flags
+time: 2024-04-29T14:23:42.804244+02:00
+custom:
+  Author: jtcohen6
+  Issue: "10062"

core/dbt/contracts/project.py

@@ -309,7 +309,6 @@ def validate(cls, data):
 
 @dataclass
 class ProjectFlags(ExtensibleDbtClassMixin):
-    allow_spaces_in_model_names: Optional[bool] = True
     cache_selected_only: Optional[bool] = None
     debug: Optional[bool] = None
     fail_fast: Optional[bool] = None
@@ -321,9 +320,7 @@ class ProjectFlags(ExtensibleDbtClassMixin):
     partial_parse: Optional[bool] = None
     populate_cache: Optional[bool] = None
     printer_width: Optional[int] = None
-    require_explicit_package_overrides_for_builtin_materializations: bool = False
     send_anonymous_usage_stats: bool = DEFAULT_SEND_ANONYMOUS_USAGE_STATS
-    source_freshness_run_project_hooks: bool = False
     static_parser: Optional[bool] = None
     use_colors: Optional[bool] = None
     use_colors_file: Optional[bool] = None
@@ -333,12 +330,17 @@ class ProjectFlags(ExtensibleDbtClassMixin):
     warn_error_options: Optional[Dict[str, Union[str, List[str]]]] = None
     write_json: Optional[bool] = None
 
+    # legacy behaviors
+    require_explicit_package_overrides_for_builtin_materializations: bool = True
+    require_resource_names_without_spaces: bool = False
+    source_freshness_run_project_hooks: bool = False
+
     @property
     def project_only_flags(self) -> Dict[str, Any]:
         return {
-            "source_freshness_run_project_hooks": self.source_freshness_run_project_hooks,
-            "allow_spaces_in_model_names": self.allow_spaces_in_model_names,
             "require_explicit_package_overrides_for_builtin_materializations": self.require_explicit_package_overrides_for_builtin_materializations,
+            "require_resource_names_without_spaces": self.require_resource_names_without_spaces,
+            "source_freshness_run_project_hooks": self.source_freshness_run_project_hooks,
         }
 
 

core/dbt/deprecations.py

@@ -123,6 +123,16 @@ class PackageMaterializationOverrideDeprecation(DBTDeprecation):
     _event = "PackageMaterializationOverrideDeprecation"
 
 
+class ResourceNamesWithSpacesDeprecation(DBTDeprecation):
+    _name = "resource-names-with-spaces"
+    _event = "ResourceNamesWithSpacesDeprecation"
+
+
+class SourceFreshnessProjectHooksNotRun(DBTDeprecation):
+    _name = "source-freshness-project-hooks"
+    _event = "SourceFreshnessProjectHooksNotRun"
+
+
 def renamed_env_var(old_name: str, new_name: str):
     class EnvironmentVariableRenamed(DBTDeprecation):
         _name = f"environment-variable-renamed:{old_name}"
@@ -163,6 +173,8 @@ def warn(name, *args, **kwargs):
     TestsConfigDeprecation(),
     ProjectFlagsMovedDeprecation(),
     PackageMaterializationOverrideDeprecation(),
+    ResourceNamesWithSpacesDeprecation(),
+    SourceFreshnessProjectHooksNotRun(),
 ]
 
 deprecations: Dict[str, DBTDeprecation] = {d.name: d for d in deprecations_list}

core/dbt/events/core_types.proto

@@ -404,24 +404,28 @@ message ProjectFlagsMovedDeprecationMsg {
 }
 
 // D014
-message SpacesInModelNameDeprecation {
-    string model_name = 1;
-    string model_version = 2;
-    string level = 3;
+message SpacesInResourceNameDeprecation {
+    string unique_id = 1;
+    string level = 2;
 }
 
-message SpacesInModelNameDeprecationMsg {
+message SpacesInResourceNameDeprecationMsg {
     CoreEventInfo info = 1;
-    SpacesInModelNameDeprecation data = 2;
+    SpacesInResourceNameDeprecation data = 2;
 }
 
 // D015
-message TotalModelNamesWithSpacesDeprecation {
+message ResourceNamesWithSpacesDeprecation {
     int32 count_invalid_names = 1;
     bool show_debug_hint = 2;
     string level = 3;
 }
 
+message ResourceNamesWithSpacesDeprecationMsg {
+    CoreEventInfo info = 1;
+    ResourceNamesWithSpacesDeprecation data = 2;
+}
+
 // D016
 message PackageMaterializationOverrideDeprecation {
     string package_name = 1;
@@ -433,9 +437,12 @@ message PackageMaterializationOverrideDeprecationMsg {
     PackageMaterializationOverrideDeprecation data = 2;
 }
 
-message TotalModelNamesWithSpacesDeprecationMsg {
+// D017
+message SourceFreshnessProjectHooksNotRun {}
+
+message SourceFreshnessProjectHooksNotRunMsg {
     CoreEventInfo info = 1;
-    TotalModelNamesWithSpacesDeprecation data = 2;
+    SourceFreshnessProjectHooksNotRun data = 2;
 }
 
 // I065

core/dbt/events/types.py

@@ -413,16 +413,12 @@ def message(self) -> str:
         return warning_tag(f"Deprecated functionality\n\n{description}")
 
 
-class SpacesInModelNameDeprecation(DynamicLevel):
+class SpacesInResourceNameDeprecation(DynamicLevel):
     def code(self) -> str:
         return "D014"
 
     def message(self) -> str:
-        version = ".v" + self.model_version if self.model_version else ""
-        description = (
-            f"Model `{self.model_name}{version}` has spaces in its name. This is deprecated and "
-            "may cause errors when using dbt."
-        )
+        description = f"Found spaces in the name of `{self.unique_id}`"
 
         if self.level == EventLevel.ERROR.value:
             description = error_tag(description)
@@ -432,22 +428,17 @@ def message(self) -> str:
         return line_wrap_message(description)
 
 
-class TotalModelNamesWithSpacesDeprecation(DynamicLevel):
+class ResourceNamesWithSpacesDeprecation(WarnLevel):
     def code(self) -> str:
         return "D015"
 
     def message(self) -> str:
-        description = f"Spaces in model names found in {self.count_invalid_names} model(s), which is deprecated."
+        description = f"Spaces found in {self.count_invalid_names} resource name(s). This is deprecated, and may lead to errors when using dbt. For more information: https://docs.getdbt.com/reference/global-configs/legacy-behaviors#require_resource_names_without_spaces"
 
         if self.show_debug_hint:
             description += " Run again with `--debug` to see them all."
 
-        if self.level == EventLevel.ERROR.value:
-            description = error_tag(description)
-        elif self.level == EventLevel.WARN.value:
-            description = warning_tag(description)
-
-        return line_wrap_message(description)
+        return line_wrap_message(warning_tag(description))
 
 
 class PackageMaterializationOverrideDeprecation(WarnLevel):
@@ -460,6 +451,16 @@ def message(self) -> str:
         return line_wrap_message(warning_tag(description))
 
 
+class SourceFreshnessProjectHooksNotRun(WarnLevel):
+    def code(self) -> str:
+        return "D017"
+
+    def message(self) -> str:
+        description = "In a future version of dbt, the `source freshness` command will start running `on-run-start` and `on-run-end` hooks by default. Please refer to https://docs.getdbt.com/reference/global-configs/legacy-behaviors#source_freshness_run_project_hooks for detailed documentation and suggested workarounds."
+
+        return line_wrap_message(warning_tag(description))
+
+
 # =======================================================
 # I - Project parsing
 # =======================================================

core/dbt/parser/manifest.py

@@ -29,6 +29,7 @@
 from dbt.mp_context import get_mp_context
 import msgpack
 
+import dbt.deprecations
 import dbt.exceptions
 import dbt.tracking
 import dbt.utils
@@ -64,9 +65,8 @@
     StateCheckVarsHash,
     DeprecatedModel,
     DeprecatedReference,
-    SpacesInModelNameDeprecation,
-    TotalModelNamesWithSpacesDeprecation,
     UpcomingReferenceDeprecation,
+    SpacesInResourceNameDeprecation,
 )
 from dbt.logger import DbtProcessState
 from dbt.node_types import NodeType, AccessType
@@ -525,7 +525,7 @@ def load(self) -> Manifest:
             self.write_manifest_for_partial_parse()
 
         self.check_for_model_deprecations()
-        self.check_for_spaces_in_model_names()
+        self.check_for_spaces_in_resource_names()
 
         return self.manifest
 
@@ -627,46 +627,43 @@ def check_for_model_deprecations(self):
                             )
                         )
 
-    def check_for_spaces_in_model_names(self):
-        """Validates that model names do not contain spaces
+    def check_for_spaces_in_resource_names(self):
+        """Validates that resource names do not contain spaces
 
         If `DEBUG` flag is `False`, logs only first bad model name
         If `DEBUG` flag is `True`, logs every bad model name
-        If `ALLOW_SPACES_IN_MODEL_NAMES` is `False`, logs are `ERROR` level and an exception is raised if any names are bad
-        If `ALLOW_SPACES_IN_MODEL_NAMES` is `True`, logs are `WARN` level
+        If `REQUIRE_RESOURCE_NAMES_WITHOUT_SPACES` is `True`, logs are `ERROR` level and an exception is raised if any names are bad
+        If `REQUIRE_RESOURCE_NAMES_WITHOUT_SPACES` is `False`, logs are `WARN` level
         """
-        improper_model_names = 0
+        improper_resource_names = 0
         level = (
-            EventLevel.WARN
-            if self.root_project.args.ALLOW_SPACES_IN_MODEL_NAMES
-            else EventLevel.ERROR
+            EventLevel.ERROR
+            if self.root_project.args.REQUIRE_RESOURCE_NAMES_WITHOUT_SPACES
+            else EventLevel.WARN
         )
 
         for node in self.manifest.nodes.values():
-            if isinstance(node, ModelNode) and " " in node.name:
-                if improper_model_names == 0 or self.root_project.args.DEBUG:
+            if " " in node.name:
+                if improper_resource_names == 0 or self.root_project.args.DEBUG:
                     fire_event(
-                        SpacesInModelNameDeprecation(
-                            model_name=node.name,
-                            model_version=version_to_str(node.version),
+                        SpacesInResourceNameDeprecation(
+                            unique_id=node.unique_id,
                             level=level.value,
                         ),
                         level=level,
                     )
-                improper_model_names += 1
-
-        if improper_model_names > 0:
-            fire_event(
-                TotalModelNamesWithSpacesDeprecation(
-                    count_invalid_names=improper_model_names,
-                    show_debug_hint=(not self.root_project.args.DEBUG),
-                    level=level.value,
-                ),
-                level=level,
-            )
-
-            if level == EventLevel.ERROR:
-                raise DbtValidationError("Model names cannot contain spaces")
+                improper_resource_names += 1
+
+        if improper_resource_names > 0:
+            if level == EventLevel.WARN:
+                flags = get_flags()
+                dbt.deprecations.warn(
+                    "resource-names-with-spaces",
+                    count_invalid_names=improper_resource_names,
+                    show_debug_hint=(not flags.DEBUG),
+                )
+            else:  # ERROR level
+                raise DbtValidationError("Resource names cannot contain spaces")
 
     def load_and_parse_macros(self, project_parser_files):
         for project in self.all_projects.values():

core/dbt/task/freshness.py

@@ -18,6 +18,7 @@
 from dbt_common.exceptions import DbtRuntimeError, DbtInternalError
 from dbt_common.events.functions import fire_event
 from dbt_common.events.types import Note
+from dbt import deprecations
 from dbt.events.types import (
     FreshnessCheckComplete,
     LogStartLine,
@@ -240,9 +241,12 @@ def task_end_messages(self, results):
         fire_event(FreshnessCheckComplete())
 
     def get_hooks_by_type(self, hook_type: RunHookType) -> List[HookNode]:
+        hooks = super().get_hooks_by_type(hook_type)
         if self.args.source_freshness_run_project_hooks:
-            return super().get_hooks_by_type(hook_type)
+            return hooks
         else:
+            if hooks:
+                deprecations.warn("source-freshness-project-hooks")
             return []
 
     def populate_metadata_freshness_cache(self, adapter, selected_uids: AbstractSet[str]) -> None:

docs/eli64/behavior-change-flags.md

@@ -0,0 +1,33 @@
+# Playbook: Behavior Change Flags
+
+User documentation: https://docs.getdbt.com/reference/global-configs/legacy-behaviors
+
+## Rules for introducing a new flag
+
+1. **Naming.** All behavior change flags should be named so that their default value changes from **False → True**. This makes it significantly easier to document, talk about, and understand.
+    * If the flag is prohibiting something that we previously allowed, use the verb “require.” Examples:
+        * `require_resource_names_without_spaces`
+        * `require_explicit_package_overrides_for_builtin_materializations`
+    * All flags should be of boolean type, and False by default when introduced: `bool = False`.
+2. **Documentation.** Start with the docs! What is the change? Who might be affected? What action will users need to take to mitigate this change? At this point, the dates for flag Introduction + Maturity are “TBD.”
+3. **Deprecation warnings**. As a general rule, **all** behavior changes should be accompanied by a deprecation warning.
+    * Always use our standard deprecations module: [https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/deprecations.py](https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/deprecations.py)
+    * This serves two purposes: Signalling the change to the user, and collecting telemetry so we can understand blast radius among users with telemtry enabled.
+    * These warning messages should link back to documentation: [https://docs.getdbt.com/reference/global-configs/legacy-behaviors](https://docs.getdbt.com/reference/global-configs/legacy-behaviors#deprecate_package_materialization_builtin_override)
+    * Even for additive behaviors that are not “breaking changes,” there is still an opportunity to signal these changes for users, and to gather an estimate of the impact. E.g. `source_freshness_run_project_hooks` should still include a proactive message any time someone runs the `source freshness` command in a project that has `on-run-*` hooks defined.
+    * The call site for these deprecation warnings should be as close as possible to the place where we’re evaluating conditional logic based on the project flag. Essentially, any time we check the flag value and it returns `False`, we should raise a deprecation warning while preserving the legacy behavior. (In the future, we might be able to streamline more of this boilerplate code.)
+    * If users want to silence these deprecation warnings, they can do so via `warn_error_options.silence`. Explicitly setting the flag to `False` in `dbt_project.yml` is not sufficient to silence the warning.
+4. **Exceptions.** If the behavior change is to raise an exception that prohibits behavior which was previously permitted (e.g. spaces in model names), the exception message should also link to the docs on legacy behaviors.
+5. **Backports.** Whenever possible, we should backport both the deprecation warning and the flag to the previous version of dbt Core.
+6. **Open a GitHub issue** in the dbt-core repository that is the implementation ticket for switching the default from `false` to `true`. Add the `behavior_change_flag` issue label, and add it to the GitHub milestone for the next minor version. (This is true in most cases, see below for exceptional considerations.) During planning, we will bundle up the “introduced” behavior changes into an epic/tasklist that schedules their maturation.
+
+## After introduction
+
+1. **Mature flag(s) by switching value from `False` → `True` in dbt-core `main`.**
+    * This should land in **the next minor (`1.X.0`) release of dbt-core**.
+    If the behavior change is mitigating a security vulnerability, and the next minor release is still planned for several months away, we still backport the fix + flag (off by default) to supported OSS versions, and we strongly advise all users to opt into the flag sooner.
+2. **Removing support for legacy behavior.**
+    * As a general rule, we will not entirely remove support for any legacy behaviors until dbt v2.0.
+        * We are not committing to supporting them forever (à la Rust editions). But we are also not taking them away willy-nilly.
+        * On a case-by-case basis, if there is a strong compelling reason to remove a legacy behavior and we see minimal in-the-wild usage (<1% of relevant projects), we can remove it entirely. This needs to be communicated well in advance — at least 2 minor versions after introduction in dbt Core.
+        * These are *project configurations*, not feature flags. While they add complexity to our codebase, such is the price of maintaining v1.* software.