detect cycles (#848)

Co-authored-by: David Baraff <nosuch@nowhere>

Author: davidbaraff

src/opentimelineio/errorStatus.h

@@ -36,7 +36,8 @@ struct ErrorStatus {
         CANNOT_COMPUTE_AVAILABLE_RANGE,
         INVALID_TIME_RANGE,
         OBJECT_WITHOUT_DURATION,
-        CANNOT_TRIM_TRANSITION
+        CANNOT_TRIM_TRANSITION,
+        OBJECT_CYCLE
     };
 
     ErrorStatus()

src/opentimelineio/serialization.cpp

@@ -556,16 +556,26 @@ void SerializableObject::Writer::write(std::string const& key, SerializableObjec
         return;
     }
 
-#ifdef OTIO_INSTANCING_SUPPORT
     auto e = _id_for_object.find(value);
     if (e != _id_for_object.end()) {
+#ifdef OTIO_INSTANCING_SUPPORT
         /*
          * We've already written this value.
          */
         _encoder.write_value(SerializableObject::ReferenceId { e->second });
+#else
+        /*
+         * We're encountering the same object while it is still
+         * in the map, meaning we're in the middle of writing it out.
+         * That's a cycle, as opposed to mere instancing, which we
+         * allow so as not to break old allowed behavior.
+         */
+        std::string s = string_printf("cyclically encountered object has schema %s",
+                                      value->schema_name().c_str());
+        _encoder._error(ErrorStatus(ErrorStatus::OBJECT_CYCLE, s));
+#endif        
         return;
     }
-#endif
 
     std::string const& schema_type_name = value->_schema_name_for_reference();
     if (_next_id_for_type.find(schema_type_name) == _next_id_for_type.end()) {
@@ -590,10 +600,16 @@ void SerializableObject::Writer::write(std::string const& key, SerializableObjec
     _encoder.write_key("OTIO_REF_ID");
     _encoder.write_value(next_id);
 #endif
-
     value->write_to(*this);
 
     _encoder.end_object();
+
+#ifndef OTIO_INSTANCING_SUPPORT
+    auto valueEntry = _id_for_object.find(value);
+    if (valueEntry != _id_for_object.end()) {
+        _id_for_object.erase(valueEntry);
+    }
+#endif    
 }
 
 void SerializableObject::Writer::write(std::string const& key, AnyDictionary const& value) {

src/py-opentimelineio/opentimelineio-bindings/otio_errorStatusHandler.cpp

@@ -58,6 +58,8 @@ ErrorStatusHandler::~ErrorStatusHandler() noexcept(false) {
         throw _NotAChildException(full_details());
     case ErrorStatus::CANNOT_COMPUTE_AVAILABLE_RANGE:
         throw _CannotComputeAvailableRangeException(full_details());
+    case ErrorStatus::OBJECT_CYCLE:
+        throw py::value_error("Detected SerializableObject cycle while copying/serializing: " + details());
     default:
         throw py::value_error(full_details());
     }

tests/test_serializable_object.py

@@ -173,6 +173,29 @@ def test_truthiness(self):
         o = otio.core.SerializableObject()
         self.assertTrue(o)
 
+    def test_instancing_without_instancing_support(self):
+        o = otio.core.SerializableObjectWithMetadata()
+        c = otio.core.SerializableObjectWithMetadata()
+        o.metadata["child1"] = c
+        o.metadata["child2"] = c
+        self.assertTrue(o.metadata["child1"] is o.metadata["child2"])
+
+        oCopy = o.clone()
+        # Note: If we ever enable INSTANCING_SUPPORT in the C++ code,
+        # then this will (and should) fail
+        self.assertTrue(oCopy.metadata["child1"] is not oCopy.metadata["child2"])
+
+    def test_cycle_detection(self):
+        o = otio.core.SerializableObjectWithMetadata()
+        o.metadata["myself"] = o
+
+        # Note: If we ever enable INSTANCING_SUPPORT in the C++ code,
+        # then modify the code below to be:
+        #   oCopy = o.clone()
+        #   self.assertTrue(oCopy is oCopy.metadata["myself"])
+        with self.assertRaises(ValueError):
+            o.clone()
+
 
 if __name__ == '__main__':
     unittest.main()