BUG: Python's Message.header fails for empty headers

BUG: Python's Message.header fails for empty headers

[Vojtěch Káně]

Dear notmuch developers,
I'll start with a short description of how I encountered the bug, so my 
report is more understandable and also so that you can potentially 
direct me to other places where the bug actually resides (I am a bit 
skeptic I would find a bug in such a popular open source project as 
notmuch is).

I use Lieer https://github.com/gauteh/lieer to sync my maildir and nm 
database with Gmail. When I send a reply, Lieer checks whether the 
subject matches the original one and prints a warning otherwise (Google 
presumably knows better what a thread is and breaks it if the subject is 
different). The relevant snippet is:

>if 'In-Reply-To' in eml:
>  repl = eml['In-Reply-To'].strip().strip('<>')
>  self.vprint("looking for original message: %s" % repl)
>  with notmuch2.Database(mode = notmuch2.Database.MODE.READ_ONLY) as db:
>    try:
>      nmsg = db.find(repl)
>    except LookupError:
>      nmsg = None
>    if nmsg is not None:
>      (_, gids) = self.local.messages_to_gids([nmsg])
>      if nmsg.header('Subject') != eml['Subject']:
>        self.vprint ("warning: subject does not match, might not be able to associate with existing thread.")

But there is a catch: when the original message's subject is empty, the 
check fails with a LookupError. Stack trace follows. I use NixOS, so 
while the file paths are weird, they also guarantee reproducible 
package set.

>Traceback (most recent call last):
>  File "/nix/store/9l521wrzalkhqk46qn57z0wc5lnmdzwk-lieer-1.4/bin/.gmi-wrapped", line 25, in <module>
>    g.main ()
>  File "/nix/store/9l521wrzalkhqk46qn57z0wc5lnmdzwk-lieer-1.4/lib/python3.10/site-packages/lieer/gmailieer.py", line 230, in main
>    args.func (args)
>  File "/nix/store/9l521wrzalkhqk46qn57z0wc5lnmdzwk-lieer-1.4/lib/python3.10/site-packages/lieer/gmailieer.py", line 837, in send
>    if nmsg.header('Subject') != eml['Subject']:
>  File "/nix/store/y12m0r41lcwz1ksqs6gncra0djln92cj-python3.10-notmuch2-0.37/lib/python3.10/site-packages/notmuch2/_message.py", line 266, in header
>    raise LookupError

At first, this sounds reasonable: the subject is empty, so it is 
effectively missing. That would indicate a bug in Lieer itself and would 
be fixed by a try-catch block. Notmuch's source for Message.header, 
however, states:

>:returns: The header value, an empty string if the header is not present.
>:rtype: str

This makes an impression that no error should be raised and a harmless 
value (at least for the above-mentioned code) should be returned. Yet 
the docs continue with

>:raises LookupError: if the header is not present.

completely contradicting itself.

And so here the questions:
Is my confusion justified? What is the expected nm's behavior? Can we 
fix the docs and possible the implementation?

Thank you all for reading this long report and for your dedication to 
open source,
Vojta Káně

p.s. I am not subscribed to the mailing list, so please keep me in the 
replies.\r

Re: BUG: Python's Message.header fails for empty headers

[David Bremner]

Vojtěch Káně <vojta001@vkane.cz> writes:

> At first, this sounds reasonable: the subject is empty, so it is 
> effectively missing. That would indicate a bug in Lieer itself and would 
> be fixed by a try-catch block. Notmuch's source for Message.header, 
> however, states:
>
>>:returns: The header value, an empty string if the header is not present.
>>:rtype: str
>
> This makes an impression that no error should be raised and a harmless 
> value (at least for the above-mentioned code) should be returned. Yet 
> the docs continue with
>
>>:raises LookupError: if the header is not present.
>
> completely contradicting itself.
>
> And so here the questions:
> Is my confusion justified? What is the expected nm's behavior? Can we 
> fix the docs and possible the implementation?
>

I agree the bindings documentation does not make much sense.  I suspect
that the bindings should follow the underlying library and return "" if
the library does.  I don't use the bindings that much, so I am curious
what others think.\r

Re: BUG: Python's Message.header fails for empty headers

[Michael J Gruber]

Am Di., 9. Jan. 2024 um 00:09 Uhr schrieb David Bremner <david@tethera.net>:
>
> Vojtěch Káně <vojta001@vkane.cz> writes:
>
> > At first, this sounds reasonable: the subject is empty, so it is
> > effectively missing. That would indicate a bug in Lieer itself and would
> > be fixed by a try-catch block. Notmuch's source for Message.header,
> > however, states:
> >
> >>:returns: The header value, an empty string if the header is not present.
> >>:rtype: str
> >
> > This makes an impression that no error should be raised and a harmless
> > value (at least for the above-mentioned code) should be returned. Yet
> > the docs continue with
> >
> >>:raises LookupError: if the header is not present.
> >
> > completely contradicting itself.
> >
> > And so here the questions:
> > Is my confusion justified? What is the expected nm's behavior? Can we
> > fix the docs and possible the implementation?
> >
>
> I agree the bindings documentation does not make much sense.  I suspect
> that the bindings should follow the underlying library and return "" if
> the library does.  I don't use the bindings that much, so I am curious
> what others think.

I might be misunderstanding the OP,and I didn't check the RFC, but
isn't there a difference between a missing header and an empty header?

If there is, this may come down to the difference between testing for
an empty string, None or False in dynamically typed python ...
But it does make sense for the bindings to return an empty string or
None for an empty header and LookUpError for a missing header. I have
not checked whether our bindings in fact do.

Also, note that *sending* via lieer (i.e. via GMail API) provides more
pitfalls. For example, message IDs are rewritten, which makes it
unusable for patch series.

Cheers,
Michael\r

Re: BUG: Python's Message.header fails for empty headers

[David Bremner]

Michael J Gruber <michaeljgruber+grubix+git@gmail.com> writes:

>>
>> I agree the bindings documentation does not make much sense.  I suspect
>> that the bindings should follow the underlying library and return "" if
>> the library does.  I don't use the bindings that much, so I am curious
>> what others think.
>
> I might be misunderstanding the OP,and I didn't check the RFC, but
> isn't there a difference between a missing header and an empty header?

Are you suggesting the library should change as well?

> If there is, this may come down to the difference between testing for
> an empty string, None or False in dynamically typed python ...
> But it does make sense for the bindings to return an empty string or
> None for an empty header and LookUpError for a missing header. I have
> not checked whether our bindings in fact do.
>

AFAICT it checks explicitely for NULL, but then throws LookupError on
any falsy return from capi.ffi.string

        ret = capi.lib.notmuch_message_get_header(self._msg_p, name)
        if ret == capi.ffi.NULL:
            raise errors.NullPointerError()
        hdr = capi.ffi.string(ret)
        if not hdr:
            raise LookupError
        return hdr.decode(encoding='utf-8')

Re: BUG: Python's Message.header fails for empty headers

[Michael J Gruber]

Am Di., 9. Jan. 2024 um 13:38 Uhr schrieb David Bremner <david@tethera.net>:
>
> Michael J Gruber <michaeljgruber+grubix+git@gmail.com> writes:
>
> >>
> >> I agree the bindings documentation does not make much sense.  I suspect
> >> that the bindings should follow the underlying library and return "" if
> >> the library does.  I don't use the bindings that much, so I am curious
> >> what others think.
> >
> > I might be misunderstanding the OP,and I didn't check the RFC, but
> > isn't there a difference between a missing header and an empty header?
>
> Are you suggesting the library should change as well?

I don't know. Missing vs. empty are two different cases. It may be
that in terms of e-mail standards (RFC), a missing header is
equivalent to an empty one. Then one can argue whether an empty header
is a missing header (raise error) or a missing header is an empty
header (return None) ...
Or one may distinguish between mandatory headers and optional ones.
I'd really check the RFC.

In Python, there is a difference between a dictionary key with an
empty value and a key which is not present, of course. So, if I think
of the headers as a dictionary, I would expect to differentiate
between those cases, unless the data which the dict represents (e-mail
headers) treats them as equal by RFC.

> > If there is, this may come down to the difference between testing for
> > an empty string, None or False in dynamically typed python ...
> > But it does make sense for the bindings to return an empty string or
> > None for an empty header and LookUpError for a missing header. I have
> > not checked whether our bindings in fact do.
> >
>
> AFAICT it checks explicitely for NULL, but then throws LookupError on
> any falsy return from capi.ffi.string
>
>         ret = capi.lib.notmuch_message_get_header(self._msg_p, name)
>         if ret == capi.ffi.NULL:
>             raise errors.NullPointerError()
>         hdr = capi.ffi.string(ret)
>         if not hdr:
>             raise LookupError
>         return hdr.decode(encoding='utf-8')

That clarifies what OP observed :)