[PATCH 1/2] test: uncaught exception when editing properties of a removed message

[PATCH 1/2] test: uncaught exception when editing properties of a removed message

[Kevin Boulain]

These two functions don't fail gracefully when editing a removed
message:
 BROKEN edit property on removed message without uncaught exception
        --- T610-message-property.20.EXPECTED   2023-02-27 11:33:25.792764376 +0000
        +++ T610-message-property.20.OUTPUT     2023-02-27 11:33:25.793764381 +0000
        @@ -1,2 +1,3 @@
         == stdout ==
         == stderr ==
        +terminate called after throwing an instance of 'Xapian::DocNotFoundError'

The other functions appear to be safe.
---
 test/T610-message-property.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 2685f3b5..944e1810 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -362,4 +362,28 @@ for (key,val) in msg.get_properties("testkey",True):
 EOF
 test_expect_equal_file /dev/null OUTPUT
 
+test_begin_subtest "edit property on removed message without uncaught exception"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
+EXPECT0(notmuch_message_remove_property (message, "example", "example"));
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "remove all properties on removed message without uncaught exception"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
+EXPECT0(notmuch_message_remove_all_properties_with_prefix (message, ""));
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done

[PATCH 2/2] lib/message-property: catch xapian exceptions

[Kevin Boulain]

Since libnotmuch exposes a C interface there's no way for clients to
catch this.
Inspired by what's done for tags (see notmuch_message_remove_tag).
---
 lib/message-property.cc       | 36 +++++++++++++++++++++++++++++------
 test/T610-message-property.sh |  4 ++--
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/lib/message-property.cc b/lib/message-property.cc
index d5afa30c..0d444bb8 100644
--- a/lib/message-property.cc
+++ b/lib/message-property.cc
@@ -25,6 +25,20 @@
 #include "database-private.h"
 #include "message-private.h"
 
+#define LOG_XAPIAN_EXCEPTION(message, error) _log_xapian_exception (__location__, message, error)
+
+static void
+_log_xapian_exception (const char *where, notmuch_message_t *message,  const Xapian::Error error)
+{
+    notmuch_database_t *notmuch = notmuch_message_get_database (message);
+
+    _notmuch_database_log (notmuch,
+			   "A Xapian exception occurred at %s: %s\n",
+			   where,
+			   error.get_msg ().c_str ());
+    notmuch->exception_reported = true;
+}
+
 notmuch_status_t
 notmuch_message_get_property (notmuch_message_t *message, const char *key, const char **value)
 {
@@ -83,10 +97,15 @@ _notmuch_message_modify_property (notmuch_message_t *message, const char *key, c
 
     term = talloc_asprintf (message, "%s=%s", key, value);
 
-    if (delete_it)
-	private_status = _notmuch_message_remove_term (message, "property", term);
-    else
-	private_status = _notmuch_message_add_term (message, "property", term);
+    try {
+	if (delete_it)
+	    private_status = _notmuch_message_remove_term (message, "property", term);
+	else
+	    private_status = _notmuch_message_add_term (message, "property", term);
+    } catch (Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (message, error);
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
 
     if (private_status)
 	return COERCE_STATUS (private_status,
@@ -130,8 +149,13 @@ _notmuch_message_remove_all_properties (notmuch_message_t *message, const char *
     else
 	term_prefix = _find_prefix ("property");
 
-    /* XXX better error reporting ? */
-    _notmuch_message_remove_terms (message, term_prefix);
+    try {
+	/* XXX better error reporting ? */
+	_notmuch_message_remove_terms (message, term_prefix);
+    } catch (Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (message, error);
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
 
     return NOTMUCH_STATUS_SUCCESS;
 }
diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 944e1810..f7cabe4d 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -363,7 +363,6 @@ EOF
 test_expect_equal_file /dev/null OUTPUT
 
 test_begin_subtest "edit property on removed message without uncaught exception"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
 EXPECT0(notmuch_message_remove_property (message, "example", "example"));
@@ -371,11 +370,11 @@ EOF
 cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
+line 30: 3
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "remove all properties on removed message without uncaught exception"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
 EXPECT0(notmuch_message_remove_all_properties_with_prefix (message, ""));
@@ -383,6 +382,7 @@ EOF
 cat <<'EOF' >EXPECTED
 == stdout ==
 == stderr ==
+line 30: 3
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 

Re: [PATCH 2/2] lib/message-property: catch xapian exceptions

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> Since libnotmuch exposes a C interface there's no way for clients to
> diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
> index 944e1810..f7cabe4d 100755
> --- a/test/T610-message-property.sh
> +++ b/test/T610-message-property.sh
> @@ -363,7 +363,6 @@ EOF

Overall this looks good, but I think the tests are a little cryptic as written.

>  test_expect_equal_file /dev/null OUTPUT
>  
>  test_begin_subtest "edit property on removed message without uncaught exception"
> -test_subtest_known_broken
>  cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
>  EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
>  EXPECT0(notmuch_message_remove_property (message, "example", "example"));
> @@ -371,11 +370,11 @@ EOF
>  cat <<'EOF' >EXPECTED
>  == stdout ==
>  == stderr ==
> +line 30: 3
>  EOF
>  test_expect_equal_file EXPECTED OUTPUT

In general I think it's better if the output of a test does not change
when marked non-broken. More importantly, in this case it means that the
call to notmuch_message_remove_property is not actually returning 0 any
more. So that output is actually the EXPECT0 assertion failing. I agree
it should print something more helpful (and I understand these
assumptions are not documented). So you should probably test that the
status is not success, and ideally print the message. You can see some
examples in T560-lib-error.sh, in particular in the created file c_tail
which handles checking the error code and printing the message.

[PATCH v2 1/2] test: uncaught exception when editing properties of a removed message

[Kevin Boulain]

These two functions don't fail gracefully when editing a removed
message:
 BROKEN edit property on removed message without uncaught exception
        --- T610-message-property.20.EXPECTED   2023-02-27 11:33:25.792764376 +0000
        +++ T610-message-property.20.OUTPUT     2023-02-27 11:33:25.793764381 +0000
        @@ -1,2 +1,3 @@
         == stdout ==
         == stderr ==
        +terminate called after throwing an instance of 'Xapian::DocNotFoundError'

The other functions appear to be safe.
---
 test/T610-message-property.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 2685f3b5..57ef018e 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -362,4 +362,38 @@ for (key,val) in msg.get_properties("testkey",True):
 EOF
 test_expect_equal_file /dev/null OUTPUT
 
+test_begin_subtest "edit property on removed message without uncaught exception"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
+stat = notmuch_message_remove_property (message, "example", "example");
+const char *stat_str = notmuch_database_status_string (db);
+if (stat_str)
+    fputs (stat_str, stderr);
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+A Xapian exception occurred at message-property.cc:XXX: No termlist for document 54
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+add_email_corpus
+
+test_begin_subtest "remove all properties on removed message without uncaught exception"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
+notmuch_message_remove_all_properties_with_prefix (message, "");
+const char *stat_str = notmuch_database_status_string (db);
+if (stat_str)
+    fputs (stat_str, stderr);
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+A Xapian exception occurred at message-property.cc:XXX: No termlist for document 54
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done

[PATCH v2 2/2] lib/message-property: catch xapian exceptions

[Kevin Boulain]

Since libnotmuch exposes a C interface there's no way for clients to
catch this.
Inspired by what's done for tags (see notmuch_message_remove_tag).
---
 lib/message-property.cc       | 36 +++++++++++++++++++++++++++++------
 test/T610-message-property.sh |  2 --
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/lib/message-property.cc b/lib/message-property.cc
index d5afa30c..0d444bb8 100644
--- a/lib/message-property.cc
+++ b/lib/message-property.cc
@@ -25,6 +25,20 @@
 #include "database-private.h"
 #include "message-private.h"
 
+#define LOG_XAPIAN_EXCEPTION(message, error) _log_xapian_exception (__location__, message, error)
+
+static void
+_log_xapian_exception (const char *where, notmuch_message_t *message,  const Xapian::Error error)
+{
+    notmuch_database_t *notmuch = notmuch_message_get_database (message);
+
+    _notmuch_database_log (notmuch,
+			   "A Xapian exception occurred at %s: %s\n",
+			   where,
+			   error.get_msg ().c_str ());
+    notmuch->exception_reported = true;
+}
+
 notmuch_status_t
 notmuch_message_get_property (notmuch_message_t *message, const char *key, const char **value)
 {
@@ -83,10 +97,15 @@ _notmuch_message_modify_property (notmuch_message_t *message, const char *key, c
 
     term = talloc_asprintf (message, "%s=%s", key, value);
 
-    if (delete_it)
-	private_status = _notmuch_message_remove_term (message, "property", term);
-    else
-	private_status = _notmuch_message_add_term (message, "property", term);
+    try {
+	if (delete_it)
+	    private_status = _notmuch_message_remove_term (message, "property", term);
+	else
+	    private_status = _notmuch_message_add_term (message, "property", term);
+    } catch (Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (message, error);
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
 
     if (private_status)
 	return COERCE_STATUS (private_status,
@@ -130,8 +149,13 @@ _notmuch_message_remove_all_properties (notmuch_message_t *message, const char *
     else
 	term_prefix = _find_prefix ("property");
 
-    /* XXX better error reporting ? */
-    _notmuch_message_remove_terms (message, term_prefix);
+    try {
+	/* XXX better error reporting ? */
+	_notmuch_message_remove_terms (message, term_prefix);
+    } catch (Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (message, error);
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
 
     return NOTMUCH_STATUS_SUCCESS;
 }
diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 57ef018e..81112b05 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -363,7 +363,6 @@ EOF
 test_expect_equal_file /dev/null OUTPUT
 
 test_begin_subtest "edit property on removed message without uncaught exception"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
 stat = notmuch_message_remove_property (message, "example", "example");
@@ -381,7 +380,6 @@ test_expect_equal_file EXPECTED OUTPUT
 add_email_corpus
 
 test_begin_subtest "remove all properties on removed message without uncaught exception"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
 notmuch_message_remove_all_properties_with_prefix (message, "");

Re: [PATCH 2/2] lib/message-property: catch xapian exceptions

[Kevin Boulain]

On 2023-03-27 at 08:30 -03, David Bremner <david@tethera.net> wrote:
> So you should probably test that the status is not success, and
> ideally print the message. You can see some examples in
> T560-lib-error.sh, in particular in the created file c_tail which
> handles checking the error code and printing the message.

Good point, it has shown I was missing a add_email_corpus.

Re: [PATCH v2 1/2] test: uncaught exception when editing properties of a removed message

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> +== stderr ==
> +A Xapian exception occurred at message-property.cc:XXX: No termlist for document 54
> +EOF
> +test_expect_equal_file EXPECTED OUTPUT
> +
> +add_email_corpus

Hi Kevin;

Thanks for the speedy update. I hate to do this but I think the message
is now _too_ specific. Goldilocks and the three tests I guess.

In particular, is there some reason to think that the number 54 is
stable here? I'm worried about the test being flaky if there is some
Xapian related change. Maybe just sed the number away?

d

[PATCH v3 1/2] test: uncaught exception when editing properties of a removed message

[Kevin Boulain]

These two functions don't fail gracefully when editing a removed
message:
 BROKEN edit property on removed message without uncaught exception
        --- T610-message-property.20.EXPECTED   2023-02-27 11:33:25.792764376 +0000
        +++ T610-message-property.20.OUTPUT     2023-02-27 11:33:25.793764381 +0000
        @@ -1,2 +1,3 @@
         == stdout ==
         == stderr ==
        +terminate called after throwing an instance of 'Xapian::DocNotFoundError'

The other functions appear to be safe.
---
 test/T610-message-property.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 2685f3b5..402b4e9a 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -362,4 +362,36 @@ for (key,val) in msg.get_properties("testkey",True):
 EOF
 test_expect_equal_file /dev/null OUTPUT
 
+test_begin_subtest "edit property on removed message without uncaught exception"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
+stat = notmuch_message_remove_property (message, "example", "example");
+if (stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION)
+    fprintf (stderr, "unable to remove properties on message");
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+unable to remove properties on message
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+add_email_corpus
+
+test_begin_subtest "remove all properties on removed message without uncaught exception"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
+stat = notmuch_message_remove_all_properties_with_prefix (message, "");
+if (stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION)
+    fprintf (stderr, "unable to remove properties on message");
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+unable to remove properties on message
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done

[PATCH v3 2/2] lib/message-property: catch xapian exceptions

[Kevin Boulain]

Since libnotmuch exposes a C interface there's no way for clients to
catch this.
Inspired by what's done for tags (see notmuch_message_remove_tag).
---
 lib/message-property.cc       | 36 +++++++++++++++++++++++++++++------
 test/T610-message-property.sh |  2 --
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/lib/message-property.cc b/lib/message-property.cc
index d5afa30c..0d444bb8 100644
--- a/lib/message-property.cc
+++ b/lib/message-property.cc
@@ -25,6 +25,20 @@
 #include "database-private.h"
 #include "message-private.h"
 
+#define LOG_XAPIAN_EXCEPTION(message, error) _log_xapian_exception (__location__, message, error)
+
+static void
+_log_xapian_exception (const char *where, notmuch_message_t *message,  const Xapian::Error error)
+{
+    notmuch_database_t *notmuch = notmuch_message_get_database (message);
+
+    _notmuch_database_log (notmuch,
+			   "A Xapian exception occurred at %s: %s\n",
+			   where,
+			   error.get_msg ().c_str ());
+    notmuch->exception_reported = true;
+}
+
 notmuch_status_t
 notmuch_message_get_property (notmuch_message_t *message, const char *key, const char **value)
 {
@@ -83,10 +97,15 @@ _notmuch_message_modify_property (notmuch_message_t *message, const char *key, c
 
     term = talloc_asprintf (message, "%s=%s", key, value);
 
-    if (delete_it)
-	private_status = _notmuch_message_remove_term (message, "property", term);
-    else
-	private_status = _notmuch_message_add_term (message, "property", term);
+    try {
+	if (delete_it)
+	    private_status = _notmuch_message_remove_term (message, "property", term);
+	else
+	    private_status = _notmuch_message_add_term (message, "property", term);
+    } catch (Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (message, error);
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
 
     if (private_status)
 	return COERCE_STATUS (private_status,
@@ -130,8 +149,13 @@ _notmuch_message_remove_all_properties (notmuch_message_t *message, const char *
     else
 	term_prefix = _find_prefix ("property");
 
-    /* XXX better error reporting ? */
-    _notmuch_message_remove_terms (message, term_prefix);
+    try {
+	/* XXX better error reporting ? */
+	_notmuch_message_remove_terms (message, term_prefix);
+    } catch (Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (message, error);
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
 
     return NOTMUCH_STATUS_SUCCESS;
 }
diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 402b4e9a..13f8a3f9 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -363,7 +363,6 @@ EOF
 test_expect_equal_file /dev/null OUTPUT
 
 test_begin_subtest "edit property on removed message without uncaught exception"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
 stat = notmuch_message_remove_property (message, "example", "example");
@@ -380,7 +379,6 @@ test_expect_equal_file EXPECTED OUTPUT
 add_email_corpus
 
 test_begin_subtest "remove all properties on removed message without uncaught exception"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 EXPECT0(notmuch_database_remove_message (db, notmuch_message_get_filename (message)));
 stat = notmuch_message_remove_all_properties_with_prefix (message, "");

Re: [PATCH v2 1/2] test: uncaught exception when editing properties of a removed message

[Kevin Boulain]

On 2023-03-29 at 07:42 -03, David Bremner <david@tethera.net> wrote:
> Thanks for the speedy update. I hate to do this but I think the message
> is now _too_ specific. Goldilocks and the three tests I guess.
>
> In particular, is there some reason to think that the number 54 is
> stable here? I'm worried about the test being flaky if there is some
> Xapian related change. Maybe just sed the number away?

No worries, it's my fault. I've checked the expected status code and
printed a generic error message instead.

Re: [PATCH v3 1/2] test: uncaught exception when editing properties of a removed message

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> These two functions don't fail gracefully when editing a removed
> message:
>  BROKEN edit property on removed message without uncaught exception
>         --- T610-message-property.20.EXPECTED   2023-02-27 11:33:25.792764376 +0000
>         +++ T610-message-property.20.OUTPUT     2023-02-27 11:33:25.793764381 +0000
>         @@ -1,2 +1,3 @@
>          == stdout ==
>          == stderr ==
>         +terminate called after throwing an instance of 'Xapian::DocNotFoundError'
>

Series applied to master, thanks!

d