pykeepass icon indicating copy to clipboard operation
pykeepass copied to clipboard

create PyKeePassException

Open Evidlo opened this issue 1 year ago • 3 comments

There are lots of cases right now where PyKeePass allows internal exceptions to bubble up to the end-user. We should consider catching some of these where it makes sense and raising a PyKeePassException that is more descriptive.

e.g. A user tries to dereference a field when the reference doesn't exist

>>> kp.entries[0].username = '{REF:U@I:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA}'

>>> kp.deref(kp.entries[0].username)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[23], line 1
----> 1 kp.deref(kp.entries[0].username)

File ~/resources/pykeepass/pykeepass/pykeepass.py:703, in PyKeePass.deref(self, value)
    701         search_value = uuid.UUID(search_value)
    702     ref_entry = self.find_entries(first=True, **{search_in: search_value})
--> 703     value = value.replace(ref, getattr(ref_entry, wanted_field))
    704 return self.deref(value)

AttributeError: 'NoneType' object has no attribute 'username'

I'd like to collate a list of these cases here. Maybe @A6GibKm @FalkAlexander @firecat53 have some examples?

Evidlo avatar Mar 11 '24 20:03 Evidlo

I did a grep for except : in Secrets and the results I got (not including except GLib.Error ...) are

  • PyKeePass(...)
  • .save()
  • .atime, .mtime, .ctime, and .expiry_time,see https://gitlab.gnome.org/World/secrets/-/issues/439
  • entry.custom_properties.items(), see https://gitlab.gnome.org/World/secrets/-/issues/413

For me the most important thing would be to document what calls can fail. The secondary one would be to have descriptive errors in the library and only throw those.

For example, .save() can clearly fail due to IO and it would be nice to do except PyKeePassException.IO or something of the sort.

A6GibKm avatar Mar 11 '24 20:03 A6GibKm

I just stumbled into https://gitlab.gnome.org/World/secrets/-/issues/525. While its definitively my fault to not try to catch a broad exception (see related MR) it shows a bit the pitfalls of manually trying to handle exceptions that might come from outside the library.

A6GibKm avatar Mar 21 '24 17:03 A6GibKm

I'm not coming up with anything obvious other than the deref and IO examples you two listed already.

I also agree that getting too aggressive about catching too many exceptions could cause some debugging challenges when getting bug reports from people.

Do you think that the way @br-olf handles the deref issue in #386 (returning None if the referenced entry is missing) is reasonable or should it return an exception to be caught? I can handle either way (his fix is pretty simple to catch in my client)...just not sure if there's a "right" way in this case.

firecat53 avatar Mar 27 '24 02:03 firecat53