From ea42908d4de15c41487470cba0fd7334a4d69fa7 Mon Sep 17 00:00:00 2001
From: Lars Kotthoff <lars@larsko.org>
Date: Wed, 5 Feb 2025 19:52:51 -0700
Subject: [PATCH] fix segfaults in Python cFFI API and add tests

---
 bindings/python-cffi/notmuch2/_base.py      |  3 +-
 bindings/python-cffi/notmuch2/_message.py   |  3 +-
 bindings/python-cffi/notmuch2/_query.py     |  2 +-
 bindings/python-cffi/tests/test_database.py | 31 +++++++++++++++++++++
 bindings/python-cffi/tests/test_message.py  | 14 ++++++++++
 5 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/bindings/python-cffi/notmuch2/_base.py b/bindings/python-cffi/notmuch2/_base.py
index 1cf03c88..c83abd01 100644
--- a/bindings/python-cffi/notmuch2/_base.py
+++ b/bindings/python-cffi/notmuch2/_base.py
@@ -167,7 +167,7 @@ class NotmuchIter(NotmuchObject, collections.abc.Iterator):
     It is tempting to use a generator function instead, but this would
     not correctly respect the :class:`NotmuchObject` memory handling
     protocol and in some unsuspecting cornercases cause memory
-    trouble.  You probably want to sublcass this in order to wrap the
+    trouble.  You probably want to subclass this in order to wrap the
     value returned by :meth:`__next__`.
 
     :param parent: The parent object.
@@ -223,7 +223,6 @@ class NotmuchIter(NotmuchObject, collections.abc.Iterator):
 
     def __next__(self):
         if not self._fn_valid(self._iter_p):
-            self._destroy()
             raise StopIteration()
         obj_p = self._fn_get(self._iter_p)
         self._fn_next(self._iter_p)
diff --git a/bindings/python-cffi/notmuch2/_message.py b/bindings/python-cffi/notmuch2/_message.py
index d4b34e91..abe37db6 100644
--- a/bindings/python-cffi/notmuch2/_message.py
+++ b/bindings/python-cffi/notmuch2/_message.py
@@ -610,7 +610,7 @@ class PropertiesMap(base.NotmuchObject, collections.abc.MutableMapping):
     def getall(self, prefix='', *, exact=False):
         """Return an iterator yielding all properties for a given key prefix.
 
-        The returned iterator yields all peroperties which start with
+        The returned iterator yields all properties which start with
         a given key prefix as ``(key, value)`` namedtuples.  If called
         with ``exact=True`` then only properties which exactly match
         the prefix are returned, those a key longer than the prefix
@@ -655,7 +655,6 @@ class PropertiesIter(base.NotmuchIter):
 
     def __next__(self):
         if not self._fn_valid(self._iter_p):
-            self._destroy()
             raise StopIteration
         key = capi.lib.notmuch_message_properties_key(self._iter_p)
         value = capi.lib.notmuch_message_properties_value(self._iter_p)
diff --git a/bindings/python-cffi/notmuch2/_query.py b/bindings/python-cffi/notmuch2/_query.py
index 1db6ec96..169fcf27 100644
--- a/bindings/python-cffi/notmuch2/_query.py
+++ b/bindings/python-cffi/notmuch2/_query.py
@@ -52,7 +52,7 @@ class Query(base.NotmuchObject):
         This executes the query and returns an iterator over the
         :class:`Message` objects found.
         """
-        msgs_pp = capi.ffi.new('notmuch_messages_t**')
+        msgs_pp = capi.ffi.new('notmuch_messages_t **')
         ret = capi.lib.notmuch_query_search_messages(self._query_p, msgs_pp)
         if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
             raise errors.NotmuchError(ret)
diff --git a/bindings/python-cffi/tests/test_database.py b/bindings/python-cffi/tests/test_database.py
index f1d12ea6..1557235d 100644
--- a/bindings/python-cffi/tests/test_database.py
+++ b/bindings/python-cffi/tests/test_database.py
@@ -303,6 +303,18 @@ class TestQuery:
         msgs = db.messages('*')
         assert isinstance(msgs, collections.abc.Iterator)
 
+    def test_messages_iterator(self, db):
+        for msg in db.messages('*'):
+            assert isinstance(msg, notmuch2.Message)
+            assert isinstance(msg.messageid, str)
+
+    def test_messages_iterator_list(self, db):
+        msgs = list(db.messages('*'))
+        assert len(msgs) == 3
+        for msg in msgs:
+            assert isinstance(msg, notmuch2.Message)
+            assert isinstance(msg.messageid, str)
+
     def test_message_no_results(self, db):
         msgs = db.messages('not_a_matching_query')
         with pytest.raises(StopIteration):
@@ -320,6 +332,25 @@ class TestQuery:
         threads = db.threads('*')
         assert isinstance(threads, collections.abc.Iterator)
 
+    def test_threads_iterator(self, db):
+        for t in db.threads('*'):
+            assert isinstance(t, notmuch2.Thread)
+            assert isinstance(t.threadid, str)
+            for msg in t:
+                assert isinstance(msg, notmuch2.Message)
+                assert isinstance(msg.messageid, str)
+
+    def test_threads_iterator_list(self, db):
+        threads = list(db.threads('*'))
+        assert len(threads) == 2
+        for t in threads:
+            assert isinstance(t, notmuch2.Thread)
+            assert isinstance(t.threadid, str)
+            msgs = list(t)
+            for msg in msgs:
+                assert isinstance(msg, notmuch2.Message)
+                assert isinstance(msg.messageid, str)
+
     def test_threads_no_match(self, db):
         threads = db.threads('not_a_matching_query')
         with pytest.raises(StopIteration):
diff --git a/bindings/python-cffi/tests/test_message.py b/bindings/python-cffi/tests/test_message.py
index 56701d05..3b103530 100644
--- a/bindings/python-cffi/tests/test_message.py
+++ b/bindings/python-cffi/tests/test_message.py
@@ -218,6 +218,20 @@ class TestProperties:
         props.add('foo', 'a')
         assert set(props.getall('foo')) == {('foo', 'a')}
 
+    def test_getall_iter(self, props):
+        props.add('foo', 'a')
+        props.add('foobar', 'b')
+        for prop in props.getall('foo'):
+            assert isinstance(prop.value, str)
+
+    def test_getall_iter_list(self, props):
+        props.add('foo', 'a')
+        props.add('foobar', 'b')
+        res = list(props.getall('foo'))
+        assert len(res) == 2
+        for prop in res:
+            assert isinstance(prop.value, str)
+
     def test_getall_prefix(self, props):
         props.add('foo', 'a')
         props.add('foobar', 'b')
-- 
2.48.1

