Merge pull request #270 from redknightlois/RDBC-1020

RDBC-1020 Fix infinite recursion in BeforeDelete and BeforeQuery event args

Author: poissoncorp

ravendb/documents/session/document_session_operations/in_memory_document_session_operations.py

@@ -327,7 +327,7 @@ def __init__(self, items=None, prepare_entities_deletes: bool = False):
         if items is None:
             items = []
         self.__deleted_entities = set(map(RefEq, items))
-        self.__prepare_entities_deleted = prepare_entities_deletes
+        self._prepare_entities_deleted = prepare_entities_deletes
         self.__on_before_deleted_entities: Union[set, None] = None
 
     def __getattribute__(self, item):
@@ -339,7 +339,7 @@ def __getattribute__(self, item):
             "discard",
             "DeletedEntitiesEnumeratorResult",
             "_DeletedEntitiesHolder__deleted_entities",
-            "_DeletedEntitiesHolder__prepare_entities_deleted",
+            "_prepare_entities_deleted",
             "_DeletedEntitiesHolder__on_before_deleted_entities",
         ]:
             return super().__getattribute__(item)
@@ -358,19 +358,17 @@ def __len__(self) -> int:
         )
 
     def __iter__(self):
-        deleted_transformed_iterator = (
-            self.DeletedEntitiesEnumeratorResult(item.ref, True) for item in self.__deleted_entities
-        )
-        if self.__on_before_deleted_entities is None:
-            return deleted_transformed_iterator
-
-        on_before_deleted_iterator = (
-            self.DeletedEntitiesEnumeratorResult(item.ref, False) for item in self.__on_before_deleted_entities
-        )
-        return itertools.chain(deleted_transformed_iterator, on_before_deleted_iterator)
+        # Snapshot the main set so that cascade deletes registered by BeforeDelete
+        # handlers do not raise "Set changed size during iteration".
+        yield from (self.DeletedEntitiesEnumeratorResult(item.ref, True) for item in list(self.__deleted_entities))
+        if self.__on_before_deleted_entities:
+            yield from (
+                self.DeletedEntitiesEnumeratorResult(item.ref, False)
+                for item in list(self.__on_before_deleted_entities)
+            )
 
     def add(self, element: object) -> None:
-        if self.__prepare_entities_deleted:
+        if self._prepare_entities_deleted:
             if self.__on_before_deleted_entities is None:
                 self.__on_before_deleted_entities = set()
             self.__on_before_deleted_entities.add(RefEq(element))
@@ -393,7 +391,7 @@ def clear(self) -> None:
             self.__on_before_deleted_entities.clear()
 
     def evict(self, entity) -> None:
-        if self.__prepare_entities_deleted:
+        if self._prepare_entities_deleted:
             raise RuntimeError("Cannot evict entity during OnBeforeDelete")
         self.__deleted_entities.discard(RefEq(entity))
 
@@ -1034,42 +1032,52 @@ def __prepare_for_creating_revisions_from_ids(self, result: SaveChangesData) ->
     def __prepare_for_entities_deletion(
         self, result: Union[None, SaveChangesData], changes: Union[None, Dict[str, List[DocumentsChanges]]]
     ) -> None:
-        for deleted_entity in self._deleted_entities:
-            document_info = self._documents_by_entity.get(deleted_entity.entity)
-            if document_info is None:
-                continue
-            if changes is not None:
-                doc_changes = []
-                change = DocumentsChanges("", "", DocumentsChanges.ChangeType.DOCUMENT_DELETED)
-                doc_changes.append(change)
-                changes[document_info.key] = doc_changes
-            else:
-                command = result.deferred_commands_map.get(
-                    IdTypeAndName.create(document_info.key, CommandType.CLIENT_ANY_COMMAND, None)
-                )
-                if command:
-                    self.__throw_invalid_deleted_document_with_deferred_command(command)
+        """Build delete commands for all entities in the deleted-entities set.
+
+        While iterating, BeforeDelete event handlers may call session.delete(),
+        which stages new deletions for a second pass via DeletedEntitiesHolder.
+        """
+        self._deleted_entities._prepare_entities_deleted = True
+        try:
+            for deleted_entity in self._deleted_entities:
+                document_info = self._documents_by_entity.get(deleted_entity.entity)
+                if document_info is None:
+                    continue
+                if changes is not None:
+                    doc_changes = []
+                    change = DocumentsChanges("", "", DocumentsChanges.ChangeType.DOCUMENT_DELETED)
+                    doc_changes.append(change)
+                    changes[document_info.key] = doc_changes
+                else:
+                    command = result.deferred_commands_map.get(
+                        IdTypeAndName.create(document_info.key, CommandType.CLIENT_ANY_COMMAND, None)
+                    )
+                    if command:
+                        self.__throw_invalid_deleted_document_with_deferred_command(command)
 
-                change_vector = None
-                document_info = self._documents_by_id.get(document_info.key)
+                    change_vector = None
+                    document_info = self._documents_by_id.get(document_info.key)
 
-                if document_info:
-                    change_vector = document_info.change_vector
+                    if document_info:
+                        change_vector = document_info.change_vector
 
-                    if document_info.entity is not None:
-                        result.on_success.remove_document_by_entity(document_info.entity)
-                        result.entities.append(document_info.entity)
+                        if document_info.entity is not None:
+                            result.on_success.remove_document_by_entity(document_info.entity)
+                            result.entities.append(document_info.entity)
 
-                    result.on_success.remove_document_by_id(document_info.key)
+                        result.on_success.remove_document_by_id(document_info.key)
 
-                change_vector = change_vector if self._use_optimistic_concurrency else None
-                self.before_delete_invoke(BeforeDeleteEventArgs(self, document_info.key, document_info.entity))
-                result.session_commands.append(
-                    DeleteCommandData(document_info.key, change_vector, document_info.change_vector)
-                )
+                    change_vector = change_vector if self._use_optimistic_concurrency else None
+                    if deleted_entity.execute_on_before_delete:
+                        self.before_delete_invoke(BeforeDeleteEventArgs(self, document_info.key, document_info.entity))
+                    result.session_commands.append(
+                        DeleteCommandData(document_info.key, change_vector, document_info.change_vector)
+                    )
 
-            if changes is None:
-                result.on_success.clear_deleted_entities()
+                if changes is None:
+                    result.on_success.clear_deleted_entities()
+        finally:
+            self._deleted_entities._prepare_entities_deleted = False
 
     def __prepare_for_entities_puts(self, result: SaveChangesData) -> None:
         should_ignore_entity_changes = self.conventions.should_ignore_entity_changes

ravendb/documents/session/event_args.py

@@ -172,7 +172,7 @@ def __init__(self, session: "InMemoryDocumentSessionOperations", key: str, entit
 
     @property
     def session(self) -> "InMemoryDocumentSessionOperations":
-        return self.session
+        return self.__session
 
     @property
     def key(self) -> str:
@@ -190,7 +190,7 @@ def __init__(self, session: "InMemoryDocumentSessionOperations", query_customiza
 
     @property
     def session(self) -> "InMemoryDocumentSessionOperations":
-        return self.session
+        return self.__session
 
     @property
     def query_customization(self) -> DocumentQueryCustomization:

ravendb/tests/session_tests/test_session_events.py

@@ -0,0 +1,131 @@
+"""
+Session events: BeforeDeleteEventArgs and BeforeQueryEventArgs expose the current
+session without infinite recursion.
+
+C# reference: FastTests/Client/Events.cs
+  Before_Delete_Session_Listener_With_Delete_Inside
+"""
+
+from ravendb.documents.session.event_args import (
+    BeforeDeleteEventArgs,
+    BeforeQueryEventArgs,
+    BeforeStoreEventArgs,
+)
+from ravendb.tests.test_base import TestBase
+
+
+class User:
+    def __init__(self, name: str = ""):
+        self.name = name
+
+
+class TestEventArgsSessionProperty(TestBase):
+    def setUp(self):
+        super().setUp()
+
+    # ------------------------------------------------------------------ #
+    #  BeforeDeleteEventArgs.session returns the current session          #
+    # ------------------------------------------------------------------ #
+
+    def test_before_delete_event_args_session_returns_session(self):
+        """
+        C# spec: BeforeDeleteEventArgs.Session returns the current session so
+        handlers can perform additional session operations.
+        """
+        with self.store.open_session() as session:
+            session.store(User(name="Alice"), "users/1")
+            session.save_changes()
+
+        session_from_args = []
+
+        def handler(args: BeforeDeleteEventArgs):
+            session_from_args.append(args.session)
+
+        with self.store.open_session() as session:
+            session.add_before_delete(handler)
+            user = session.load("users/1", User)
+            session.delete(user)
+            session.save_changes()
+
+        self.assertEqual(1, len(session_from_args), "handler should have been called once")
+        self.assertIsNotNone(session_from_args[0], "args.session should return the session object")
+
+    def test_before_delete_handler_can_cascade_delete_via_args_session(self):
+        """
+        C# spec (Events.cs — Before_Delete_Session_Listener_With_Delete_Inside):
+          handler receives BeforeDeleteEventArgs, loads users/2 via args.session,
+          then calls args.session.delete(user2) to cascade the deletion.
+          After save_changes(), both users/1 and users/2 are deleted.
+        """
+        with self.store.open_session() as session:
+            session.store(User(name="Foo"), "users/1")
+            session.store(User(name="Bar"), "users/2")
+            session.save_changes()
+
+        with self.store.open_session() as session:
+
+            def handler(args: BeforeDeleteEventArgs):
+                user2 = args.session.load("users/2", User)
+                args.session.delete(user2)
+
+            session.add_before_delete(handler)
+            user1 = session.load("users/1", User)
+            session.delete(user1)
+            session.save_changes()
+
+        with self.store.open_session() as session:
+            user1 = session.load("users/1", User)
+            self.assertIsNone(user1, "users/1 should have been deleted")
+            user2 = session.load("users/2", User)
+            self.assertIsNone(user2, "users/2 should have been cascade-deleted via args.session.delete()")
+
+    # ------------------------------------------------------------------ #
+    #  BeforeQueryEventArgs.session returns the current session           #
+    # ------------------------------------------------------------------ #
+
+    def test_before_query_event_args_session_returns_session(self):
+        """
+        C# spec: BeforeQueryEventArgs.Session returns the current session.
+        """
+        with self.store.open_session() as session:
+            session.store(User(name="Carol"), "users/3")
+            session.save_changes()
+
+        session_from_args = []
+
+        def handler(args: BeforeQueryEventArgs):
+            session_from_args.append(args.session)
+
+        with self.store.open_session() as session:
+            session.add_before_query(handler)
+            _ = list(session.query(object_type=User))
+
+        self.assertEqual(1, len(session_from_args), "before_query handler should have been called once")
+        self.assertIsNotNone(session_from_args[0], "args.session should return the session object")
+
+    # ------------------------------------------------------------------ #
+    #  BeforeStoreEventArgs.session baseline                              #
+    # ------------------------------------------------------------------ #
+
+    def test_before_store_event_args_session_property_works(self):
+        """
+        Baseline: BeforeStoreEventArgs.session correctly returns the session.
+        Confirms the recursion bug is specific to BeforeDeleteEventArgs and
+        BeforeQueryEventArgs, not all event args.
+        """
+        with self.store.open_session() as session:
+            session.store(User(name="Dave"), "users/4")
+            session.save_changes()
+
+        session_from_args = []
+
+        def handler(args: BeforeStoreEventArgs):
+            session_from_args.append(args.session)
+
+        with self.store.open_session() as session:
+            session.add_before_store(handler)
+            session.store(User(name="Eve"), "users/5")
+            session.save_changes()
+
+        self.assertEqual(1, len(session_from_args), "before_store handler should be called")
+        self.assertIsNotNone(session_from_args[0], "BeforeStoreEventArgs.session should return the session object")