From 8e7d87682cdc1305f5dcf13018b95f01ac427fd0 Mon Sep 17 00:00:00 2001
From: Stephan Hoyer <shoyer@google.com>
Date: Fri, 13 Sep 2024 23:45:04 -0700
Subject: [PATCH] Forbid modifying names of DataTree objects with parents
 (#9494)

---
 xarray/core/treenode.py       | 30 +++++++++++++++++++++---------
 xarray/tests/test_datatree.py | 20 +++++++++++++++++++-
 2 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/xarray/core/treenode.py b/xarray/core/treenode.py
index d74c82178ea..17accf74383 100644
--- a/xarray/core/treenode.py
+++ b/xarray/core/treenode.py
@@ -640,6 +640,14 @@ def same_tree(self, other: Tree) -> bool:
 AnyNamedNode = TypeVar("AnyNamedNode", bound="NamedNode")
 
 
+def _validate_name(name: str | None) -> None:
+    if name is not None:
+        if not isinstance(name, str):
+            raise TypeError("node name must be a string or None")
+        if "/" in name:
+            raise ValueError("node names cannot contain forward slashes")
+
+
 class NamedNode(TreeNode, Generic[Tree]):
     """
     A TreeNode which knows its own name.
@@ -653,8 +661,8 @@ class NamedNode(TreeNode, Generic[Tree]):
 
     def __init__(self, name=None, children=None):
         super().__init__(children=children)
-        self._name = None
-        self.name = name
+        _validate_name(name)
+        self._name = name
 
     @property
     def name(self) -> str | None:
@@ -663,11 +671,13 @@ def name(self) -> str | None:
 
     @name.setter
     def name(self, name: str | None) -> None:
-        if name is not None:
-            if not isinstance(name, str):
-                raise TypeError("node name must be a string or None")
-            if "/" in name:
-                raise ValueError("node names cannot contain forward slashes")
+        if self.parent is not None:
+            raise ValueError(
+                "cannot set the name of a node which already has a parent. "
+                "Consider creating a detached copy of this node via .copy() "
+                "on the parent node."
+            )
+        _validate_name(name)
         self._name = name
 
     def __repr__(self, level=0):
@@ -677,11 +687,13 @@ def __repr__(self, level=0):
         return repr_value
 
     def __str__(self) -> str:
-        return f"NamedNode('{self.name}')" if self.name else "NamedNode()"
+        name_repr = repr(self.name) if self.name is not None else ""
+        return f"NamedNode({name_repr})"
 
     def _post_attach(self: AnyNamedNode, parent: AnyNamedNode, name: str) -> None:
         """Ensures child has name attribute corresponding to key under which it has been stored."""
-        self.name = name
+        _validate_name(name)  # is this check redundant?
+        self._name = name
 
     def _copy_node(
         self: AnyNamedNode,
diff --git a/xarray/tests/test_datatree.py b/xarray/tests/test_datatree.py
index 0435b199b9b..22c2cae7d02 100644
--- a/xarray/tests/test_datatree.py
+++ b/xarray/tests/test_datatree.py
@@ -24,10 +24,28 @@ def test_empty(self):
         assert dt.children == {}
         assert_identical(dt.to_dataset(), xr.Dataset())
 
-    def test_unnamed(self):
+    def test_name(self):
         dt = DataTree()
         assert dt.name is None
 
+        dt = DataTree(name="foo")
+        assert dt.name == "foo"
+
+        dt.name = "bar"
+        assert dt.name == "bar"
+
+        dt = DataTree(children={"foo": DataTree()})
+        assert dt["/foo"].name == "foo"
+        with pytest.raises(
+            ValueError, match="cannot set the name of a node which already has a parent"
+        ):
+            dt["/foo"].name = "bar"
+
+        detached = dt["/foo"].copy()
+        assert detached.name == "foo"
+        detached.name = "bar"
+        assert detached.name == "bar"
+
     def test_bad_names(self):
         with pytest.raises(TypeError):
             DataTree(name=5)  # type: ignore[arg-type]