Annotation.id now gets/sets long form ID (with parent_view_id prefix)

Author: keighrim

mmif/serialize/annotation.py

@@ -116,17 +116,18 @@ def _add_prop_aliases(self, key_to_add, val_to_add):
         in `TimeFrame` and `BoundingBox` respectively.
         """
         prop_aliases = AnnotationTypes._prop_aliases.get(self._type.shortname, {})
-        for alias_reprep, alias_group in prop_aliases.items():
+        for alias_rep, alias_group in prop_aliases.items():
             if key_to_add in alias_group:
                 for alias in alias_group:
                     if alias != key_to_add:
                         self._props_ephemeral[alias] = val_to_add
                         if alias in self.properties.keys():
-                            warning_msg = f'Found both "{key_to_add}" and "{alias}" in the properties of "{self.id}" annotation in "{self.parent}" view. '
-                            if alias == alias_reprep:
-                                warning_msg += f'However "{key_to_add}" is an alias of "{alias_reprep}".'
+                            warning_msg = (f'Both "{key_to_add}" and "{alias}" are in the properties of "{self.id}", '
+                                           f'however ')
+                            if alias == alias_rep:
+                                warning_msg += f'"{key_to_add}" is an alias of "{alias_rep}".'
                             else:
-                                warning_msg += f'However "{key_to_add}" and "{alias}" are boath aliases of "{alias_reprep}".'
+                                warning_msg += f'"{key_to_add}" and "{alias}" are both aliases of "{alias_rep}".'
                             warning_msg += f'Having two synonyms in the same annotation can cause unexpected behavior. '
                             warnings.warn(warning_msg, UserWarning)
 
@@ -159,25 +160,39 @@ def parent(self, parent_view_id: str) -> None:
 
     @property
     def id(self) -> str:
-        return self.properties.id
+        return self.long_id
 
     @id.setter
     def id(self, aid: str) -> None:
-        self.properties.id = aid
+        self.long_id = aid
 
     @property
     def long_id(self) -> str:
-        if self.parent is not None and len(self.parent) > 0:
-            return f"{self.parent}{self.id_delimiter}{self.id}"
+        if len(self.parent) > 0:
+            return f"{self.parent}{self.id_delimiter}{self.properties.id}"
         else:
-            return self.id
+            return self.properties.id
 
     @long_id.setter
     def long_id(self, long_id: str) -> None:
         if self.id_delimiter in long_id:
-            self.parent, self.id = long_id.split(self.id_delimiter)
+            pid, aid = long_id.split(self.id_delimiter)
+            if self.parent == '' or self.parent == pid:
+                self.parent, self.properties.id = long_id.split(self.id_delimiter)
+            else:
+                raise ValueError(f'Parent view ID does not match, refusing the alter parent ID '
+                                 f'("{self.parent}" vs. "{pid}", given long ID "{long_id}"')
         else:
-            self.id = long_id
+            self.properties.id = long_id
+    
+    @property
+    def _short_id(self) -> str:
+        """
+        Method to directly get "short" form of the ID, not supposed to be used for general purpose, since 
+        we want to force usage of "long" form ID when recording and referring annotations. 
+        :return: 
+        """
+        return self.properties.id
 
     @staticmethod
     def check_prop_value_is_simple_enough(

mmif/serialize/mmif.py

@@ -239,14 +239,14 @@ def _deserialize(self, input_dict: dict) -> None:
                         for prop_key, prop_value in ann.properties.items():
                             self.get_document_by_id(doc_id)._props_ephemeral[prop_key] = prop_value
                     except KeyError:
-                        warnings.warn(f"Annotation {ann.id} (in view {view.id}) has a document ID {doc_id} that "
+                        warnings.warn(f"Annotation {ann.id} has a document ID {doc_id} that "
                                       f"does not exist in the MMIF object. Skipping.", RuntimeWarning)
 
                 ## caching start and end points for time-based annotations
                 # add quick access to `start` and `end` values if the annotation is using `targets` property
                 if 'targets' in ann.properties:
                     if 'start' in ann.properties or 'end' in ann.properties:
-                        raise ValueError(f"Annotation {ann.id} (in view {view.id}) has `targets` and `start`/`end/` "
+                        raise ValueError(f"Annotation {ann.id} has `targets` and `start`/`end` "
                                          f"properties at the same time. Annotation anchors are ambiguous.")
                     ann._props_ephemeral['start'] = self._get_linear_anchor_point(ann, start=True)
                     ann._props_ephemeral['end'] = self._get_linear_anchor_point(ann, start=False)
@@ -318,7 +318,9 @@ def generate_capital_annotations(self):
                         if doc_id is None:
                             doc_id = ann.get_property('document')
                         # only if we are sure that the document ID is unique across all views... (with v_id prefix)
-                        # TODO (krim @ 7/15/24): update id checking once https://github.com/clamsproject/mmif/issues/228 is resolved
+                        # since https://github.com/clamsproject/mmif/issues/228, all documents id should have 
+                        # v_id prefix. This code is here for backward compatibility 
+                        # TODO (krim @ 6/13/25): drop this checks with v2.0 release
                         if not any([doc_id == doc.id for doc in self.documents]) and Mmif.id_delimiter not in doc_id:
                             doc_id = f"{view.id}{Mmif.id_delimiter}{doc_id}"
                         existing_anns[doc_id].update(ann.properties)

mmif/serialize/view.py

@@ -439,7 +439,7 @@ def append(self, value: Union[Annotation, Document], overwrite=False) -> None:
                           in the list
         :return: None
         """
-        super()._append_with_key(value.id, value, overwrite)
+        super()._append_with_key(value._short_id, value, overwrite)
 
     def __getitem__(self, key: str):
         """

tests/test_serialize.py

@@ -511,22 +511,18 @@ def test_get_anchor_point(self):
         v1 = mmif.new_view()
         v2 = mmif.new_view()
         tps = []
-        tf1_targets_wo_vid = []
         tf2_targets_with_vid = []
         for timepoint in range(5):
             tp = v1.new_annotation(AnnotationTypes.TimePoint, timePoint=timepoint)
             tps.append(tp)
-            tf1_targets_wo_vid.append(f"{tp.id}")
-            tf2_targets_with_vid.append(f"{v1.id}{Mmif.id_delimiter}{tp.id}")
-        tf1 = v1.new_annotation(AnnotationTypes.TimeFrame, targets=tf1_targets_wo_vid)
+            tf2_targets_with_vid.append(f"{tp.id}")
+        # tf1 used to be here and was used when long_id was less forced
         tf2 = v2.new_annotation(AnnotationTypes.TimeFrame, targets=tf2_targets_with_vid)
         tf3 = v2.new_annotation(AnnotationTypes.TimeFrame, start=100, end=200)
         self.assertEqual(mmif.get_start(tf3), 100)
         self.assertEqual(mmif.get_end(tf3), 200)
         self.assertEqual(mmif._get_linear_anchor_point(tf3, start=True), 100)
         self.assertEqual(mmif._get_linear_anchor_point(tf3, start=False), 200)
-        self.assertEqual(mmif._get_linear_anchor_point(tf1, start=True), 0)
-        self.assertEqual(mmif._get_linear_anchor_point(tf1, start=False), 4)
         self.assertEqual(mmif._get_linear_anchor_point(tf2, start=True), 0)
         self.assertEqual(mmif._get_linear_anchor_point(tf2, start=False), 4)
         self.assertEqual(mmif._get_linear_anchor_point(mmif[tf2_targets_with_vid[0]], start=True), 0)
@@ -591,6 +587,7 @@ def test_mmif_getitem_document(self):
             self.fail("didn't get document 'm1'")
 
     def test_mmif_getitem_idconflict(self):
+        # TODO (krim @ 6/13/25): major re-write when addressing #295
         m = Mmif(validate=False)
         v1 = m.new_view()
         v1.id = 'v1'
@@ -607,11 +604,11 @@ def test_mmif_getitem_idconflict(self):
         self.assertIsNotNone(m[v1.id])
         self.assertIsNotNone(m[v2.id])
         # conflict short IDs
-        self.assertEqual(v1a.id, v2a.id)
+        self.assertEqual(v1a._short_id, v2a._short_id)
         with pytest.raises(KeyError):
-            _ = m[v1a.id]
-        self.assertIsNotNone(m[v1a.long_id])
-        self.assertIsNotNone(m[v2a.long_id])
+            _ = m[v1a._short_id]
+        self.assertIsNotNone(m[v1a.id])
+        self.assertIsNotNone(m[v2a.id])
 
     def test_mmif_getitem_view(self):
         try:
@@ -1018,14 +1015,17 @@ def test_get_property_with_alias(self):
     def test_id(self):
         anno_obj: Annotation = self.data['everything']['mmif']['v5:bb1']
 
-        old_id = anno_obj.id
+        old_id = anno_obj._short_id
         self.assertEqual('bb1', old_id)
 
     def test_change_id(self):
         anno_obj: Annotation = self.data['everything']['mmif']['v5:bb1']
 
-        anno_obj.id = 'bb200'
-        self.assertEqual('bb200', anno_obj.id)
+        anno_obj.id = 'v5:bb200'
+        self.assertEqual('v5:bb200', anno_obj.id)
+        self.assertEqual('bb200', anno_obj._short_id)
+        with self.assertRaises(ValueError):
+            anno_obj.id = 'v4:bb200'
 
         serialized = json.loads(anno_obj.serialize())
         new_id = serialized['properties']['id']
@@ -1074,7 +1074,11 @@ def test_get_property(self):
         d.long_id = 'v1:d1'
         self.assertEqual('v1:d1', d.long_id)
         self.assertEqual('v1', d.parent)
-        self.assertEqual('d1', d.id)
+        self.assertEqual('d1', d._short_id)
+        d.id = 'v1:d1'
+        self.assertEqual('v1:d1', d.long_id)
+        self.assertEqual('v1', d.parent)
+        self.assertEqual('d1', d._short_id)
 
 
     def test_nontext_document(self):
@@ -1261,7 +1265,7 @@ def test_capital_annotation_generation_viewfinder(self):
             v = mmif.new_view()
             v.id = f'v{i}'
             v.metadata.app = tester_appname
-            v.new_annotation(AnnotationTypes.Region, document=f'doc{i}')
+            v.new_annotation(AnnotationTypes.Region, document=doc.id)
             mmif.add_view(v)
         authors = ['me', 'you']
         for i in range(2):
@@ -1283,7 +1287,7 @@ def test_capital_annotation_nongeneration_for_writable_documents(self):
         v = mmif.new_view()
         v.metadata.app = tester_appname
         vid = v.id
-        new_td_id = mmif[vid].new_textdocument(text='new text', document='doc0', origin='transformation').id
+        new_td_id = mmif[vid].new_textdocument(text='new text', document=doc.id, origin='transformation').id
         doc.add_property('author', 'me')
 
         mmif_roundtrip = Mmif(mmif.serialize())

tests/test_utils.py

@@ -51,22 +51,22 @@ def setUp(self):
         self.mmif_obj.add_document(self.video_doc)
 
     def test_extract_mid_frame(self):
-        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=100, end=200, timeUnit='frame', document='d1')
+        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=100, end=200, timeUnit='frame', document=self.video_doc.id)
         self.assertEqual(150, vdh.get_mid_framenum(self.mmif_obj, tf))
-        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=0, end=200, timeUnit='frame', document='d1')
+        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=0, end=200, timeUnit='frame', document=self.video_doc.id)
         self.assertEqual(100, vdh.get_mid_framenum(self.mmif_obj, tf))
-        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=0, end=3, timeUnit='seconds', document='d1')
+        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=0, end=3, timeUnit='seconds', document=self.video_doc.id)
         self.assertEqual(vdh.convert(1.5, 's', 'f', self.fps), vdh.get_mid_framenum(self.mmif_obj, tf))
 
     def test_extract_representative_frame(self):
-        tp = self.a_view.new_annotation(AnnotationTypes.TimePoint, timePoint=1500, timeUnit='milliseconds', document='d1')
-        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=1000, end=2000, timeUnit='milliseconds', document='d1')
+        tp = self.a_view.new_annotation(AnnotationTypes.TimePoint, timePoint=1500, timeUnit='milliseconds', document=self.video_doc.id)
+        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=1000, end=2000, timeUnit='milliseconds', document=self.video_doc.id)
         tf.add_property('representatives', [tp.id])
         rep_frame_num = vdh.get_representative_framenum(self.mmif_obj, tf)
         expected_frame_num = vdh.millisecond_to_framenum(self.video_doc, tp.get_property('timePoint'))
         self.assertEqual(expected_frame_num, rep_frame_num)
         # and should work even if no representatives are provided
-        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=1000, end=2000, timeUnit='milliseconds', document='d1')
+        tf = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=1000, end=2000, timeUnit='milliseconds', document=self.video_doc.id)
         self.assertEqual(vdh.get_representative_framenum(self.mmif_obj, tf), vdh.get_mid_framenum(self.mmif_obj, tf))
         # check there is an error if there is a representative referencing a timepoint that does not exist
         tf.add_property('representatives', ['fake_tp_id'])
@@ -111,7 +111,7 @@ def test_convert_timepoint(self):
         self.assertEqual(vdh.convert(3, 's', 'f', self.fps), vdh.convert_timepoint(self.mmif_obj, timepoint_ann, 'f'))
 
     def test_convert_timeframe(self):
-        self.a_view.metadata.new_contain(AnnotationTypes.TimeFrame, timeUnit='frame', document='d1')
+        self.a_view.metadata.new_contain(AnnotationTypes.TimeFrame, timeUnit='frame', document=self.video_doc.id)
         timeframe_ann = self.a_view.new_annotation(AnnotationTypes.TimeFrame, start=100, end=200)
         for times in zip((3.337, 6.674), vdh.convert_timeframe(self.mmif_obj, timeframe_ann, 's')):
             self.assertAlmostEqual(*times, places=0)