Better exception handling reserved attribute (#5357)

* Better exception handling for reserved attribute
* Added unit test for Document

Co-authored-by: minhtuevo <[email protected]>
Co-authored-by: brimoor <[email protected]>

fiftyone/core/odm/__init__.py

@@ -63,6 +63,7 @@
 from .embedded_document import (
     BaseEmbeddedDocument,
     DynamicEmbeddedDocument,
+    DynamicEmbeddedDocumentException,
     EmbeddedDocument,
 )
 from .frame import (

fiftyone/core/odm/embedded_document.py

@@ -5,6 +5,7 @@
 | `voxel51.com <https://voxel51.com/>`_
 |
 """
+import re
 import mongoengine
 
 from .document import DynamicMixin, MongoEngineBaseDocument
@@ -30,6 +31,12 @@ def __init__(self, *args, **kwargs):
         self.validate()
 
 
+class DynamicEmbeddedDocumentException(Exception):
+    """Exception raised when an error occurs in a dynamic document operation."""
+
+    pass
+
+
 class DynamicEmbeddedDocument(
     DynamicMixin,
     BaseEmbeddedDocument,
@@ -45,12 +52,50 @@ class DynamicEmbeddedDocument(
     meta = {"abstract": True, "allow_inheritance": True}
 
     def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
+        try:
+            super().__init__(*args, **kwargs)
+        except AttributeError as e:
+            self._raise_reserved_attribute_exception(e)
+            raise e
+
         self.validate()
 
+    def __setattr__(self, name, value):
+        try:
+            super().__setattr__(name, value)
+        except AttributeError as e:
+            self._raise_reserved_attribute_exception(e)
+            raise e
+
     def __hash__(self):
         return hash(str(self))
 
+    def _raise_reserved_attribute_exception(self, e):
+        key = self._extract_attribute_from_exception(e)
+        if key is not None:
+            if isinstance(getattr(self.__class__, key, None), property):
+                raise DynamicEmbeddedDocumentException(
+                    f"Invalid attribute name '{key}'. '{key}' is a reserved keyword for {self.__class__.__name__} objects"
+                )
+
+        if "can't set attribute" in str(e):
+            raise DynamicEmbeddedDocumentException(
+                f"One or more attributes are reserved keywords for `{self.__class__.__name__}` objects"
+            )
+
+    @staticmethod
+    def _extract_attribute_from_exception(e):
+        pattern = (
+            r"(?:property '(?P<attribute1>\w+)' of '[^']+' object has no setter|"
+            r"can't set attribute '(?P<attribute2>\w+)')"
+        )
+        match = re.match(pattern, str(e))
+
+        if match:
+            return match.group("attribute1") or match.group("attribute2")
+
+        return None
+
     def _get_field(self, field_name, allow_missing=False):
         # pylint: disable=no-member
         chunks = field_name.split(".", 1)

tests/unittests/documents.py

@@ -9,12 +9,17 @@
 | `voxel51.com <https://voxel51.com/>`_
 |
 """
-
 import unittest
+
 from mongoengine import EmbeddedDocument
 from mongoengine.errors import ValidationError
+
+import fiftyone as fo
 from fiftyone.core.fields import EmbeddedDocumentListField
-from fiftyone.core.odm.dataset import SampleFieldDocument
+from fiftyone.core.odm import (
+    SampleFieldDocument,
+    DynamicEmbeddedDocumentException,
+)
 
 
 class MockEmbeddedDocument(EmbeddedDocument):
@@ -46,3 +51,64 @@ def test_validate(self):
         invalid_input = valid_input + [MockEmbeddedDocument()]
         with self.assertRaises(ValidationError):
             list_field.validate(invalid_input)
+
+
+class TestDetection(unittest.TestCase):
+    def test_valid_detection_creation(self):
+        detection = fo.Detection(
+            label="car",
+            bounding_box=[0.1, 0.1, 0.2, 0.2],
+            confidence=0.95,
+            custom_attr="test",
+            another_custom_attr=123,
+        )
+
+        self.assertEqual(detection.label, "car")
+        self.assertEqual(detection.bounding_box, [0.1, 0.1, 0.2, 0.2])
+        self.assertEqual(detection.confidence, 0.95)
+
+        # pylint: disable=no-member
+        self.assertEqual(detection.custom_attr, "test")
+        self.assertEqual(detection.another_custom_attr, 123)
+
+        detection.custom_attr = "value"
+        self.assertEqual(detection.custom_attr, "value")
+
+    def test_reserved_attribute_raises_exception(self):
+        # `has_mask` is a property of `Detection`
+
+        with self.assertRaises(DynamicEmbeddedDocumentException):
+            fo.Detection(has_mask="not allowed")
+
+        with self.assertRaises(DynamicEmbeddedDocumentException):
+            fo.Detection(
+                label="car",
+                bounding_box=[0.1, 0.1, 0.2, 0.2],
+                confidence=0.95,
+                has_mask="not allowed",
+                custom_attr="foo",
+                another_custom_attr=123,
+            )
+
+        detection = fo.Detection()
+
+        with self.assertRaises(DynamicEmbeddedDocumentException):
+            detection.has_mask = "not allowed"
+
+    def test_extract_attribute_from_exception(self):
+        # Test property setter pattern
+        exception1 = AttributeError(
+            "property 'name' of 'MyDoc' object has no setter"
+        )
+        attr1 = fo.Detection._extract_attribute_from_exception(exception1)
+        self.assertEqual(attr1, "name")
+
+        # Test can't set attribute pattern
+        exception2 = AttributeError("can't set attribute 'age'")
+        attr2 = fo.Detection._extract_attribute_from_exception(exception2)
+        self.assertEqual(attr2, "age")
+
+        # Test no match
+        exception3 = AttributeError("some other error")
+        attr3 = fo.Detection._extract_attribute_from_exception(exception3)
+        self.assertIsNone(attr3)