fix(harness): don't error out when attempting attaching storage before begin

ops/charm.py

@@ -1540,7 +1540,9 @@ def __init__(self, name: str, raw: '_StorageMetaDict'):
         self.multiple_range = None
         if 'multiple' in raw:
             range = raw['multiple']['range']
-            if '-' not in range:
+            if range[-1] == '+':
+                self.multiple_range = (int(range[:-1]), None)
+            elif '-' not in range:
                 self.multiple_range = (int(range), int(range))
             else:
                 range = range.split('-')

ops/testing.py

@@ -405,7 +405,11 @@ def begin_with_initial_hooks(self) -> None:
         for storage_name in self._meta.storages:
             for storage_index in self._backend.storage_list(storage_name, include_detached=True):
                 s = model.Storage(storage_name, storage_index, self._backend)
-                self.attach_storage(s.full_id)
+                if self._backend._storage_is_attached(storage_name, storage_index):
+                    # Attaching was done already, but we still need the event to be emitted.
+                    self.charm.on[storage_name].storage_attached.emit(s)
+                else:
+                    self.attach_storage(s.full_id)
         # Storage done, emit install event
         charm.on.install.emit()
 
@@ -690,8 +694,8 @@ def add_storage(self, storage_name: str, count: int = 1,
         Args:
             storage_name: The storage backend name on the Charm
             count: Number of disks being added
-            attach: True to also attach the storage mount and emit storage-attached if
-                harness.begin() has been called.
+            attach: True to also attach the storage mount; if :meth:`begin`
+                has been called a True value will also emit storage-attached
 
         Return:
             A list of storage IDs, e.g. ["my-storage/1", "my-storage/2"].
@@ -739,12 +743,12 @@ def attach_storage(self, storage_id: str) -> None:
         """Attach a storage device.
 
         The intent of this function is to simulate a ``juju attach-storage`` call.
-        It will trigger a storage-attached hook if the storage unit in question exists
+        If called after :meth:`begin` and hooks are not disabled, it will trigger
+        a storage-attached hook if the storage unit in question exists
         and is presently marked as detached.
 
         The test harness uses symbolic links to imitate storage mounts, which may lead to some
-        inconsistencies compared to the actual charm. Users should be cognizant of
-        this potential discrepancy.
+        inconsistencies compared to the actual charm.
 
         Args:
             storage_id: The full storage ID of the storage unit being attached, including the
@@ -2339,7 +2343,17 @@ def _storage_attach(self, storage_id: str):
                     mounting_dir.parent.mkdir(parents=True, exist_ok=True)
                     target_dir = pathlib.Path(store["location"])
                     target_dir.mkdir(parents=True, exist_ok=True)
-                    mounting_dir.symlink_to(target_dir)
+                    try:
+                        mounting_dir.symlink_to(target_dir, target_is_directory=True)
+                    except FileExistsError:
+                        # If the symlink is already the one we want, then we
+                        # don't need to do anything here.
+                        # NOTE: In Python 3.9, this can use `mounting_dir.readlink()`
+                        if (
+                            not mounting_dir.is_symlink()
+                            or os.readlink(mounting_dir) != str(target_dir)
+                        ):
+                            raise
 
         index = int(index)
         if not self._storage_is_attached(name, index):

test/test_charm.py

@@ -282,6 +282,10 @@ def _on_stor4_attach(self, event: ops.StorageAttachedEvent):
     multiple:
       range: 2-
     type: filesystem
+  stor-plus:
+    multiple:
+      range: 10+
+    type: filesystem
 ''')
 
         fake_script(
@@ -329,6 +333,7 @@ def _on_stor4_attach(self, event: ops.StorageAttachedEvent):
         self.assertEqual(self.meta.storages['stor2'].multiple_range, (2, 2))
         self.assertEqual(self.meta.storages['stor3'].multiple_range, (2, None))
         self.assertEqual(self.meta.storages['stor-4'].multiple_range, (2, 4))
+        self.assertEqual(self.meta.storages['stor-plus'].multiple_range, (10, None))
 
         charm = MyCharm(self.create_framework())
 

test/test_testing.py

@@ -4749,6 +4749,96 @@ def test_storage_mount(self):
         self.harness.attach_storage(storage_id)
         self.assertTrue((self.root / "mounts/foo/bar").read_text(), "foobar")
 
+    def _make_storage_attach_harness(self, meta: typing.Optional[str] = None):
+        class MyCharm(ops.CharmBase):
+            def __init__(self, framework: ops.Framework):
+                super().__init__(framework)
+                self.attached: typing.List[str] = []
+                self.locations: typing.List[pathlib.Path] = []
+                framework.observe(self.on['test-storage'].storage_attached, self._on_attach)
+
+            def _on_attach(self, event: ops.StorageAttachedEvent):
+                self.attached.append(event.storage.full_id)
+                self.locations.append(event.storage.location)
+
+        if meta is None:
+            meta = '''
+                name: test
+                containers:
+                    test-container:
+                        mounts:
+                            - storage: test-storage
+                              location: /mounts/foo
+                storage:
+                    test-storage:
+                        type: filesystem
+                '''
+        harness = ops.testing.Harness(MyCharm, meta=meta)
+        self.addCleanup(harness.cleanup)
+        return harness
+
+    def test_storage_attach_begin_no_emit(self):
+        """If `begin()` hasn't been called, `attach` does not emit storage-attached."""
+        harness = self._make_storage_attach_harness()
+        harness.add_storage('test-storage', attach=True)
+        harness.begin()
+        self.assertNotIn('test-storage/0', harness.charm.attached)
+
+    def test_storage_attach_begin_with_hooks_emits(self):
+        """`attach` doesn't emit storage-attached before `begin_with_initial_hooks`."""
+        harness = self._make_storage_attach_harness()
+        harness.add_storage('test-storage', attach=True)
+        harness.begin_with_initial_hooks()
+        self.assertIn('test-storage/0', harness.charm.attached)
+        self.assertTrue(harness.charm.locations[0])
+
+    def test_storage_add_with_later_attach(self):
+        harness = self._make_storage_attach_harness()
+        harness.begin()
+        storage_ids = harness.add_storage('test-storage', attach=False)
+        self.assertNotIn('test-storage/0', harness.charm.attached)
+        for s_id in storage_ids:
+            harness.attach_storage(s_id)
+            # It's safe to call `attach_storage` more than once, and this will
+            # only trigger the event once - this is the same as executing
+            # `juju attach-storage <unit> <storage>` more than once.
+            harness.attach_storage(s_id)
+        self.assertEqual(harness.charm.attached.count('test-storage/0'), 1)
+
+    def test_storage_machine_charm_metadata(self):
+        meta = '''
+            name: test
+            storage:
+                test-storage:
+                    type: filesystem
+                    mount: /mounts/foo
+            '''
+        harness = self._make_storage_attach_harness(meta)
+        harness.begin()
+        harness.add_storage('test-storage', attach=True)
+        self.assertIn('test-storage/0', harness.charm.attached)
+
+    def test_storage_multiple_storage_instances(self):
+        meta = '''
+            name: test
+            storage:
+                test-storage:
+                    type: filesystem
+                    mount: /mounts/foo
+                    multiple:
+                        range: 2-4
+            '''
+        harness = self._make_storage_attach_harness(meta)
+        harness.begin()
+        harness.add_storage('test-storage', 2, attach=True)
+        self.assertEqual(harness.charm.attached, ['test-storage/0', 'test-storage/1'])
+        self.assertNotEqual(harness.charm.locations[0], harness.charm.locations[1])
+        harness.add_storage('test-storage', 2, attach=True)
+        self.assertEqual(
+            harness.charm.attached, [
+                'test-storage/0', 'test-storage/1', 'test-storage/2', 'test-storage/3'])
+        self.assertEqual(len(set(harness.charm.locations)), 4)
+
 
 class TestSecrets(unittest.TestCase):
     def test_add_model_secret_by_app_name_str(self):