[Gramps-devel] cause for 'NoneType' object has no attribute

[Enno Borgsteede]

Devs,

In the report I mentioned earlier, the reporter pasted a trace with
'NoneType' object has no attribute, but when I tested with his database,
my Gramps, same 4.1.1 AIO download, would never crash, but rather hang,
i.e. loop in a place function.

On Mantis, I see loads of reports with similar 'Nonetype' object errors,
which are often hard to reproduce, and now I wonder whether they may
have similar causes, i.e. loops that may run very long on PCs with large
memory, and crash with a 'Nonetype' object error when less (heap) memory
is available.

Right now it looks like a wild speculation, for which I've found no
support on the web yet, but I can imagine that in Python, combined with
all the libs that we use, programs run out of (heap) memory when in a
loop, and functions return None, and thus a 'Nonetype' object, where
such is not expected by us.

In https://gramps-project.org/bugs/view.php?id=7965, which never raised
an exception here, I found that that Gramps 4.1.1 AIO looped in the
exact location where it crashed on reporters PC, i.e. while looking up a
place name for an event to be exported to GEDCOM, where the place was
embedded in another place, which was embedded by itself. The event and
two places are exactly the same object types that show up in reporters
trace, in three different stacks for what I found to be nested function
calls in the analysis that I made yesterday night. That can't be a
coincidence, can it?

Reporter has a German Windows 8.1, I tested on 2 PCs with Dutch Windows
8.1, and WindowsXP running in VirtualBox, with the exact same 4.1.1 AIO.
I have intel i7 and i3 here, and don't know what hardware reporter has.
Can hardware cause such a difference with Gramps looping here, and
crashing on his, with 3 separate stack traces that reveal the exact
calls where I found the loop, apparently in different threads? I'm quite
confused.

regards,

Enno

[John Ralls]

On Aug 21, 2014, at 2:31 PM, Enno Borgsteede <***@gmail.com> wrote:

> Devs,
>
> In the report I mentioned earlier, the reporter pasted a trace with
> 'NoneType' object has no attribute, but when I tested with his database,
> my Gramps, same 4.1.1 AIO download, would never crash, but rather hang,
> i.e. loop in a place function.
>
> On Mantis, I see loads of reports with similar 'Nonetype' object errors,
> which are often hard to reproduce, and now I wonder whether they may
> have similar causes, i.e. loops that may run very long on PCs with large
> memory, and crash with a 'Nonetype' object error when less (heap) memory
> is available.
>
> Right now it looks like a wild speculation, for which I've found no
> support on the web yet, but I can imagine that in Python, combined with
> all the libs that we use, programs run out of (heap) memory when in a
> loop, and functions return None, and thus a 'Nonetype' object, where
> such is not expected by us.
>
> In https://gramps-project.org/bugs/view.php?id=7965, which never raised
> an exception here, I found that that Gramps 4.1.1 AIO looped in the
> exact location where it crashed on reporters PC, i.e. while looking up a
> place name for an event to be exported to GEDCOM, where the place was
> embedded in another place, which was embedded by itself. The event and
> two places are exactly the same object types that show up in reporters
> trace, in three different stacks for what I found to be nested function
> calls in the analysis that I made yesterday night. That can't be a
> coincidence, can it?
>
> Reporter has a German Windows 8.1, I tested on 2 PCs with Dutch Windows
> 8.1, and WindowsXP running in VirtualBox, with the exact same 4.1.1 AIO.
> I have intel i7 and i3 here, and don't know what hardware reporter has.
> Can hardware cause such a difference with Gramps looping here, and
> crashing on his, with 3 separate stack traces that reveal the exact
> calls where I found the loop, apparently in different threads? I'm quite
> confused.

That seems unlikely. Python is supposed to raise a MemoryError exception when that happens, not return a None-type instead of the correct object. The other type of memory exhaustion, running out of stack (which might happen in the case of infinite recursion) will just crash the interpreter. On M$Win that will result in a "quit unexpectedly" message box.

In #7965, the first exception comes from
for (objclass, handle) in self.dbase.find_backlink_handles(
event.handle, ['Person']):
person = self.dbase.get_person_from_handle(handle)
for ref in person.get_event_ref_list():

In the second and third exceptions, the cause is
while len(place.get_placeref_list()) > 0:
place = db.get_place_from_handle(place.get_placeref_list()[0].ref)
items[int(place.get_type())] = place.name

with the last line being the one raising the exception in each case. Both of those are due to failing to check that the data returned from the database is valid, or even that the key is. No memory magic required.

Regards,
John Ralls

[Enno Borgsteede]

John,
> That seems unlikely. Python is supposed to raise a MemoryError exception when that happens, not return a None-type instead of the correct object. The other type of memory exhaustion, running out of stack (which might happen in the case of infinite recursion) will just crash the interpreter. On M$Win that will result in a "quit unexpectedly" message box.
OK, got that. I've seen such exceptions in other reports. On my systems,
being 8.1 with 8 GB RAM, and a virtual XP with 1 GB, this Gramps AIO
4.1.1-something might take hours to run out of memory in a location
loop, and I didn't wait that long.
> In #7965, the first exception comes from
> for (objclass, handle) in self.dbase.find_backlink_handles(
> event.handle, ['Person']):
> person = self.dbase.get_person_from_handle(handle)
> for ref in person.get_event_ref_list():
>
> In the second and third exceptions, the cause is
> while len(place.get_placeref_list()) > 0:
> place = db.get_place_from_handle(place.get_placeref_list()[0].ref)
> items[int(place.get_type())] = place.name
>
> with the last line being the one raising the exception in each case. Both of those are due to failing to check that the data returned from the database is valid, or even that the key is. No memory magic required.
I read your words, and know what you mean, but they don't take my
worries away. And that's because where the report has an privatized
export attached, I got the real database by mail, and ran the same
export with the same Gramps AIO 4.1.1-something and the same GEDCOM
extensions plug-in. So, if the problem were related to not checking
data, I would see the same exceptions, right? I mean, I have the same
data, same Gramps code and libraries, pretty much the same Windows, and
so forth.

When I search for nonetype in the bug tracker, I see more than a hundred
results, more than a dozen for August alone, and we still have 9 days
left. And a significant portion seems to be quite random, like on
opening or closing/saving a database, not on specific actions like
merging persons/places, etc. And we see complaints on the user list too.
How come? Did we add sloppy code? I can't believe that, because if we
did, I would see similar errors here whenever I run 4.x. And that is not
the case. I don't use 4.x for production, for a couple of reasons, one
of which is the new GTK+ 3, but I have no complaints about its stability
on my PCs.

There's another thing that I don't understand, which is that if there
were a Nonetype person in reporters database, I would expect that export
would quit right on that, and not throw two other exceptions for places.
Am I misreading things?

regards,

Enno

[John Ralls]

On Aug 22, 2014, at 4:59 AM, Enno Borgsteede <***@gmail.com> wrote:

> John,
>> That seems unlikely. Python is supposed to raise a MemoryError exception when that happens, not return a None-type instead of the correct object. The other type of memory exhaustion, running out of stack (which might happen in the case of infinite recursion) will just crash the interpreter. On M$Win that will result in a "quit unexpectedly" message box.
> OK, got that. I've seen such exceptions in other reports. On my systems, being 8.1 with 8 GB RAM, and a virtual XP with 1 GB, this Gramps AIO 4.1.1-something might take hours to run out of memory in a location loop, and I didn't wait that long.
>> In #7965, the first exception comes from
>> for (objclass, handle) in self.dbase.find_backlink_handles(
>> event.handle, ['Person']):
>> person = self.dbase.get_person_from_handle(handle)
>> for ref in person.get_event_ref_list():
>>
>> In the second and third exceptions, the cause is
>> while len(place.get_placeref_list()) > 0:
>> place = db.get_place_from_handle(place.get_placeref_list()[0].ref)
>> items[int(place.get_type())] = place.name
>>
>> with the last line being the one raising the exception in each case. Both of those are due to failing to check that the data returned from the database is valid, or even that the key is. No memory magic required.
> I read your words, and know what you mean, but they don't take my worries away. And that's because where the report has an privatized export attached, I got the real database by mail, and ran the same export with the same Gramps AIO 4.1.1-something and the same GEDCOM extensions plug-in. So, if the problem were related to not checking data, I would see the same exceptions, right? I mean, I have the same data, same Gramps code and libraries, pretty much the same Windows, and so forth.
>
> When I search for nonetype in the bug tracker, I see more than a hundred results, more than a dozen for August alone, and we still have 9 days left. And a significant portion seems to be quite random, like on opening or closing/saving a database, not on specific actions like merging persons/places, etc. And we see complaints on the user list too. How come? Did we add sloppy code? I can't believe that, because if we did, I would see similar errors here whenever I run 4.x. And that is not the case. I don't use 4.x for production, for a couple of reasons, one of which is the new GTK+ 3, but I have no complaints about its stability on my PCs.
>
> There's another thing that I don't understand, which is that if there were a Nonetype person in reporters database, I would

Enno,

I don't know how the database comes to have corrupted pointers. It could be happening anywhere in the software stack, and the only way to find it is with a binary debugger like gdb or lldb, perhaps along with a memory tool like Valgrind. That's exceedingly tedious work especially under MinGW where the available tools are quite limited.

Note, BTW, that the code causing this particular bug isn't likely to suffer memory exhaustion: It's a loop, not recursion, so there's no growth in the stack; any allocated memory should get garbage-collected on each pass through the loop, so while the memory footprint will fluctuate a bit it shouldn't grow without limit. You can test that with the dataset by tripping the infinite loop and watching the memory allocation in Task Manager.

Sloppy code? You bet. Here's the eventual destination of the get_foo_from_handle() convenience functions in db/read.py:
def get_from_handle(self, handle, class_type, data_map):
if isinstance(handle, UNITYPE):
handle = handle.encode('utf-8')
data = data_map.get(handle)
if data:
newobj = class_type()
newobj.unserialize(data)
return newobj
return None

That means that every caller needs to check that it's getting something back. It would be much better to raise an exception here to force calling code to deal with passing a bad handle. Note, though, that there are 6 implementations of get_place_from_handle() and 8 of get_person_from_handle() not including the declaration (which raises a NotImplementedError) in db/base.py. That's still just symptom detection: The real problem is the bad handle. I'll leave it as an exercise to track that down and work out a more robust way of ensuring that either a valid handle is returned or a useful exception is raised so that calling code can respond appropriately.

Regards,
John Ralls

[Enno Borgsteede]

John,
> I don't know how the database comes to have corrupted pointers. It could be happening anywhere in the software stack, and the only way to find it is with a binary debugger like gdb or lldb, perhaps along with a memory tool like Valgrind. That's exceedingly tedious work especially under MinGW where the available tools are quite limited.
IF the user's database had corrupted pointers, and the reported error
message suggests that such is the case for a person, I would get an
error when I export all to GEDCOM right? The real deal is, that on my
PCs, all persons ARE retrieved from the database, and the only problem I
see is a loop.

So what I'm saying is that there are NO bad pointers in the database. I
have a copy of that, and apart from the location loop, it works fine.
The user ran a database repair on my request too, and found no errors at
all.

I just switched my XP setup to German, ran Gramps AIO 4.1.1-unstable
with the offending database, and got the same results: No errors, just a
loop. Check and repair returned nothing except missing media, which is
what I expected, because I only got the database that he sent BEFORE his
repair.

So, to me it looks like we're back to square one. There is NO error in
the database that I got, but NoneType objects DO seem to be read on his
PC. How come? I can't reproduce this, so it makes no sense to run any
debugger here.
> Note, BTW, that the code causing this particular bug isn't likely to suffer memory exhaustion: It's a loop, not recursion, so there's no growth in the stack; any allocated memory should get garbage-collected on each pass through the loop, so while the memory footprint will fluctuate a bit it shouldn't grow without limit. You can test that with the dataset by tripping the infinite loop and watching the memory allocation in Task Manager.
OK, my fault. I assumed that it was a recursive call.
> Sloppy code? You bet. Here's the eventual destination of the get_foo_from_handle() convenience functions in db/read.py:
> def get_from_handle(self, handle, class_type, data_map):
> if isinstance(handle, UNITYPE):
> handle = handle.encode('utf-8')
> data = data_map.get(handle)
> if data:
> newobj = class_type()
> newobj.unserialize(data)
> return newobj
> return None
>
> That means that every caller needs to check that it's getting something back. It would be much better to raise an exception here to force calling code to deal with passing a bad handle. Note, though, that there are 6 implementations of get_place_from_handle() and 8 of get_person_from_handle() not including the declaration (which raises a NotImplementedError) in db/base.py. That's still just symptom detection: The real problem is the bad handle. I'll leave it as an exercise to track that down and work out a more robust way of ensuring that either a valid handle is returned or a useful exception is raised so that calling code can respond appropriately.
True. It would be interesting to implement such exceptions just to see
whether there is any corruption on reading on particular PCs, which is
what my experience with the user's database suggests. I ran a test with
German locale today, just to make sure that above code is not influenced
by that.

Any ideas, given that there's no corruption in the database itself?

regards,

Enno

[John Ralls]

On Aug 23, 2014, at 4:28 AM, Enno Borgsteede <***@gmail.com> wrote:

> John,
>> I don't know how the database comes to have corrupted pointers. It could be happening anywhere in the software stack, and the only way to find it is with a binary debugger like gdb or lldb, perhaps along with a memory tool like Valgrind. That's exceedingly tedious work especially under MinGW where the available tools are quite limited.
> IF the user's database had corrupted pointers, and the reported error message suggests that such is the case for a person, I would get an error when I export all to GEDCOM right? The real deal is, that on my PCs, all persons ARE retrieved from the database, and the only problem I see is a loop.
>
> So what I'm saying is that there are NO bad pointers in the database. I have a copy of that, and apart from the location loop, it works fine. The user ran a database repair on my request too, and found no errors at all.
>
> I just switched my XP setup to German, ran Gramps AIO 4.1.1-unstable with the offending database, and got the same results: No errors, just a loop. Check and repair returned nothing except missing media, which is what I expected, because I only got the database that he sent BEFORE his repair.
>
> So, to me it looks like we're back to square one. There is NO error in the database that I got, but NoneType objects DO seem to be read on his PC. How come? I can't reproduce this, so it makes no sense to run any debugger here.
>> Note, BTW, that the code causing this particular bug isn't likely to suffer memory exhaustion: It's a loop, not recursion, so there's no growth in the stack; any allocated memory should get garbage-collected on each pass through the loop, so while the memory footprint will fluctuate a bit it shouldn't grow without limit. You can test that with the dataset by tripping the infinite loop and watching the memory allocation in Task Manager.
> OK, my fault. I assumed that it was a recursive call.
>> Sloppy code? You bet. Here's the eventual destination of the get_foo_from_handle() convenience functions in db/read.py:
>> def get_from_handle(self, handle, class_type, data_map):
>> if isinstance(handle, UNITYPE):
>> handle = handle.encode('utf-8')
>> data = data_map.get(handle)
>> if data:
>> newobj = class_type()
>> newobj.unserialize(data)
>> return newobj
>> return None
>>
>> That means that every caller needs to check that it's getting something back. It would be much better to raise an exception here to force calling code to deal with passing a bad handle. Note, though, that there are 6 implementations of get_place_from_handle() and 8 of get_person_from_handle() not including the declaration (which raises a NotImplementedError) in db/base.py. That's still just symptom detection: The real problem is the bad handle. I'll leave it as an exercise to track that down and work out a more robust way of ensuring that either a valid handle is returned or a useful exception is raised so that calling code can respond appropriately.
> True. It would be interesting to implement such exceptions just to see whether there is any corruption on reading on particular PCs, which is what my experience with the user's database suggests. I ran a test with German locale today, just to make sure that above code is not influenced by that.
>
> Any ideas, given that there's no corruption in the database itself?

First, eliminate add-ons: Get the user to move aside his ~/.gramps/gramps41/plugins directory and run it again to make sure that he gets the same exceptions in a clean environment.

As I said, the real problem is the bad handle, so you have to track down where that came from. You might get the user to change get_main_location from
items = {int(place.get_type()): place.name}
while len(place.get_placeref_list()) > 0:
place = db.get_place_from_handle(place.get_placeref_list()[0].ref)
items[int(place.get_type())] = place.name
return items

to
items = {int(place.get_type()): place.name}
while len(place.get_placeref_list()) > 0:
parent_handle = place.get_placeref_list()[0].ref
parent_place = db.get_place_from_handle(parent_handle)
if parent_handle == None:
raise RuntimeError('Bad handle %s from Place %s returned None' % (repr(parent_handle), place.name))
place = parent_place
items[int(place.get_type())] = place.name
return items

That will at least give you the place that's the source of the bad handle and maybe what's wrong with it, which you can examine in the database copy that you have. You can do something similar in GedcomExtensions._process_family_event() for the other None-type exception.

Regards,
John Ralls

[Nick Hall]

On 23/08/14 16:06, John Ralls wrote:
>> John,
>>> >>I don't know how the database comes to have corrupted pointers. It could be happening anywhere in the software stack, and the only way to find it is with a binary debugger like gdb or lldb, perhaps along with a memory tool like Valgrind. That's exceedingly tedious work especially under MinGW where the available tools are quite limited.
>> >IF the user's database had corrupted pointers, and the reported error message suggests that such is the case for a person, I would get an error when I export all to GEDCOM right? The real deal is, that on my PCs, all persons ARE retrieved from the database, and the only problem I see is a loop.
>> >
>> >So what I'm saying is that there are NO bad pointers in the database. I have a copy of that, and apart from the location loop, it works fine. The user ran a database repair on my request too, and found no errors at all.
>> >
>> >I just switched my XP setup to German, ran Gramps AIO 4.1.1-unstable with the offending database, and got the same results: No errors, just a loop. Check and repair returned nothing except missing media, which is what I expected, because I only got the database that he sent BEFORE his repair.
>> >
>> >So, to me it looks like we're back to square one. There is NO error in the database that I got, but NoneType objects DO seem to be read on his PC. How come? I can't reproduce this, so it makes no sense to run any debugger here.
>>> >>Note, BTW, that the code causing this particular bug isn't likely to suffer memory exhaustion: It's a loop, not recursion, so there's no growth in the stack; any allocated memory should get garbage-collected on each pass through the loop, so while the memory footprint will fluctuate a bit it shouldn't grow without limit. You can test that with the dataset by tripping the infinite loop and watching the memory allocation in Task Manager.
>> >OK, my fault. I assumed that it was a recursive call.
>>> >>Sloppy code? You bet. Here's the eventual destination of the get_foo_from_handle() convenience functions in db/read.py:
>>> >> def get_from_handle(self, handle, class_type, data_map):
>>> >> if isinstance(handle, UNITYPE):
>>> >> handle = handle.encode('utf-8')
>>> >> data = data_map.get(handle)
>>> >> if data:
>>> >> newobj = class_type()
>>> >> newobj.unserialize(data)
>>> >> return newobj
>>> >> return None
>>> >>
>>> >>That means that every caller needs to check that it's getting something back. It would be much better to raise an exception here to force calling code to deal with passing a bad handle. Note, though, that there are 6 implementations of get_place_from_handle() and 8 of get_person_from_handle() not including the declaration (which raises a NotImplementedError) in db/base.py. That's still just symptom detection: The real problem is the bad handle. I'll leave it as an exercise to track that down and work out a more robust way of ensuring that either a valid handle is returned or a useful exception is raised so that calling code can respond appropriately.
>> >True. It would be interesting to implement such exceptions just to see whether there is any corruption on reading on particular PCs, which is what my experience with the user's database suggests. I ran a test with German locale today, just to make sure that above code is not influenced by that.
>> >
>> >Any ideas, given that there's no corruption in the database itself?
> First, eliminate add-ons: Get the user to move aside his ~/.gramps/gramps41/plugins directory and run it again to make sure that he gets the same exceptions in a clean environment.
>
> As I said, the real problem is the bad handle, so you have to track down where that came from. You might get the user to change get_main_location from
> items = {int(place.get_type()): place.name}
> while len(place.get_placeref_list()) > 0:
> place = db.get_place_from_handle(place.get_placeref_list()[0].ref)
> items[int(place.get_type())] = place.name
> return items
>
> to
> items = {int(place.get_type()): place.name}
> while len(place.get_placeref_list()) > 0:
> parent_handle = place.get_placeref_list()[0].ref
> parent_place = db.get_place_from_handle(parent_handle)
> if parent_handle == None:
> raise RuntimeError('Bad handle %s from Place %s returned None' % (repr(parent_handle), place.name))
> place = parent_place
> items[int(place.get_type())] = place.name
> return items
>
> That will at least give you the place that's the source of the bad handle and maybe what's wrong with it, which you can examine in the database copy that you have. You can do something similar in GedcomExtensions._process_family_event() for the other None-type exception.

I expect that the code is being run through the private proxy, not that
the pointers are corrupted. The location name is built by walking the
place hierarchy. If a private place is encountered, then None is
returned instead of a place object.

It is quite common for code that is never run with proxies to avoid this
check. Unfortunately, I forgot that this code is used in the export,
which can use proxies. The fix is easy though.

Enno - Shall I assign bug #7965 to myself and fix it for you?

Regards,

Nick.

[Enno Borgsteede]

Nick
> I expect that the code is being run through the private proxy, not that
> the pointers are corrupted. The location name is built by walking the
> place hierarchy. If a private place is encountered, then None is
> returned instead of a place object.
That's good to know, but if there were a private location in my copy of
the database, I would sure get that exception too, wouldn't I? Truth is,
I don't see it, because there is no such thing in the database. Once I
make one private, the exception shows, but in the original, it's just
not there. Besides, in the report, the first error is a NoneType person,
not a place.
> It is quite common for code that is never run with proxies to avoid this
> check. Unfortunately, I forgot that this code is used in the export,
> which can use proxies. The fix is easy though.
>
> Enno - Shall I assign bug #7965 to myself and fix it for you?
Your help is much appreciated, and the error indeed shows when I create
that private location. I fear it's not the user's problem though,
although I agree that it needs repair.

I would be much obliged if you can define a scenario with a private
person or event, whatever, that would explain that first error. Can you?

thanks,

Enno

[Nick Hall]

On 23/08/14 21:17, Enno Borgsteede wrote:
> I would be much obliged if you can define a scenario with a private
> person or event, whatever, that would explain that first error. Can you?
>

Yes. The code is trying to find witnesses for a family event. The
error will occur if a witness is returned as a backlink, but is excluded
by the proxy. Although the backlink handle will be valid, the proxy
will return None. The private proxy has code to avoid this, but the
filter proxy has a FIXME comment in the find_backlink_handles method.

There is also a possible privacy issue with this code. Suppose that
both the person and event are public, but the event reference is
private. The person will be returned as a backlink, but would not be
reachable in the forward direction.

Nick.

[Enno Borgsteede]

Nick,
> On 23/08/14 21:17, Enno Borgsteede wrote:
>> I would be much obliged if you can define a scenario with a private
>> person or event, whatever, that would explain that first error. Can you?
>>
> Yes. The code is trying to find witnesses for a family event. The
> error will occur if a witness is returned as a backlink, but is excluded
> by the proxy. Although the backlink handle will be valid, the proxy
> will return None. The private proxy has code to avoid this, but the
> filter proxy has a FIXME comment in the find_backlink_handles method.
OK, thanks. I checked reporter's database again, and found no error when
I export to GEDCOM with the privacy filter checked. I also tried the
gramplet mentioned by Jerome in another thread, changing its version
check to 4.1, and saw no private objects in his database.
> There is also a possible privacy issue with this code. Suppose that
> both the person and event are public, but the event reference is
> private. The person will be returned as a backlink, but would not be
> reachable in the forward direction.
Hehe, nice. Can I interpret that a design error, or could there be a
genuine use case behind this? Is it something like not wanting to
display that a person attended an event?

Anyway, I'm glad that you found the error with the private location,
which I can reproduce indeed, but w.r.t. the NoneType errors, I am still
at square one. There is no corruption in reporter's database, and he
just confirmed that the database he sent is the exact one that caused
the errors mentioned in the report. And if that's true, we still have an
unreliable part of Gramps or associated libraries that causes NoneType
errors there, not here.

In other words, my worries still exist. I'm a Python amateur, but I have
more than 25 years experience with C, about 10 with C++, so I know what
null pointers can do, and it looks like None objects behave quite the
same. And from what I've read here, I assume that in Gramps:

1. Whenever an object list is retrieved, all objects in it should be valid,
2. If the privacy proxy is in effect, private object are skipped, so
rule 1 is still true,
3. If all objects are private, the resulting list is empty, so that rule
1 still applies.

If they are true, and this looks like good programming practice to me,
any iterator that meets a None and thus NoneType object indicates a
serious bug in lower software layers being either Gramps or any library
used. And given that I know that the integrity of the database that I
got is OK, since it does not trigger errors here, shows no errors in
check and repair here, and not on reporter's PC either, the situation
that I see has not changed.

Since there are lots of reports where 'NoneType' errors look quite
random, and he got those where I didn't get any, ever, it looks like we
need some low level code that asserts the validity of objects read, so
that iteration will be safe again.

regards,

Enno

[John Ralls]

On Aug 24, 2014, at 9:22 AM, Enno Borgsteede <***@gmail.com> wrote:

> Nick,
>> On 23/08/14 21:17, Enno Borgsteede wrote:
>>> I would be much obliged if you can define a scenario with a private
>>> person or event, whatever, that would explain that first error. Can you?
>>>
>> Yes. The code is trying to find witnesses for a family event. The
>> error will occur if a witness is returned as a backlink, but is excluded
>> by the proxy. Although the backlink handle will be valid, the proxy
>> will return None. The private proxy has code to avoid this, but the
>> filter proxy has a FIXME comment in the find_backlink_handles method.
> OK, thanks. I checked reporter's database again, and found no error when
> I export to GEDCOM with the privacy filter checked. I also tried the
> gramplet mentioned by Jerome in another thread, changing its version
> check to 4.1, and saw no private objects in his database.
>> There is also a possible privacy issue with this code. Suppose that
>> both the person and event are public, but the event reference is
>> private. The person will be returned as a backlink, but would not be
>> reachable in the forward direction.
> Hehe, nice. Can I interpret that a design error, or could there be a
> genuine use case behind this? Is it something like not wanting to
> display that a person attended an event?
>
> Anyway, I'm glad that you found the error with the private location,
> which I can reproduce indeed, but w.r.t. the NoneType errors, I am still
> at square one. There is no corruption in reporter's database, and he
> just confirmed that the database he sent is the exact one that caused
> the errors mentioned in the report. And if that's true, we still have an
> unreliable part of Gramps or associated libraries that causes NoneType
> errors there, not here.
>
> In other words, my worries still exist. I'm a Python amateur, but I have
> more than 25 years experience with C, about 10 with C++, so I know what
> null pointers can do, and it looks like None objects behave quite the
> same. And from what I've read here, I assume that in Gramps:
>
> 1. Whenever an object list is retrieved, all objects in it should be valid,
> 2. If the privacy proxy is in effect, private object are skipped, so
> rule 1 is still true,
> 3. If all objects are private, the resulting list is empty, so that rule
> 1 still applies.

No, private objects are not skipped, they're returned as None. If all objects are private, the resulting list is filled with None. When iterating over the list each object should be tested for being None and skipped if it is.

>
> If they are true, and this looks like good programming practice to me,
> any iterator that meets a None and thus NoneType object indicates a
> serious bug in lower software layers being either Gramps or any library
> used. And given that I know that the integrity of the database that I
> got is OK, since it does not trigger errors here, shows no errors in
> check and repair here, and not on reporter's PC either, the situation
> that I see has not changed.
>
> Since there are lots of reports where 'NoneType' errors look quite
> random, and he got those where I didn't get any, ever, it looks like we
> need some low level code that asserts the validity of objects read, so
> that iteration will be safe again.
>

I dunno about "low level", but IMO it would be better if get_foo_from_handle raised exceptions instead of returning None. That would at least point to the direct cause of the problem instead of producing mysterious dereferencing errors; in cases where we expect that some handles might be filtered it's simple enough to catch the exception and continue the loop.

Has he tested with no addons yet?

Regards,
John Ralls

[Enno Borgsteede]

John,
> No, private objects are not skipped, they're returned as None. If all
> objects are private, the resulting list is filled with None. When
> iterating over the list each object should be tested for being None
> and skipped if it is.
OK, I would probably do that different, but for this particular case
that is irrelevant, because I get no exception here. All objects seem to
be valid here.
> I dunno about "low level", but IMO it would be better if
> get_foo_from_handle raised exceptions instead of returning None. That
> would at least point to the direct cause of the problem instead of
> producing mysterious dereferencing errors; in cases where we expect
> that some handles might be filtered it's simple enough to catch the
> exception and continue the loop.
I agree. It's a change that may prevent a lot of searching later.
> Has he tested with no addons yet?
No. I sent a new question about that, also asking about fancy virus
scanners, debuggers, and similar things. I did do the reverse myself,
i.e. install all available add-ons, with no adverse effect.

I'll report back when more info is available.

regards,

Enno

[John Ralls]

On Aug 24, 2014, at 9:50 AM, John Ralls <***@ceridwen.us> wrote:

>
> On Aug 24, 2014, at 9:22 AM, Enno Borgsteede <***@gmail.com> wrote:
>
>> Nick,
>>> On 23/08/14 21:17, Enno Borgsteede wrote:
>>>> I would be much obliged if you can define a scenario with a private
>>>> person or event, whatever, that would explain that first error. Can you?
>>>>
>>> Yes. The code is trying to find witnesses for a family event. The
>>> error will occur if a witness is returned as a backlink, but is excluded
>>> by the proxy. Although the backlink handle will be valid, the proxy
>>> will return None. The private proxy has code to avoid this, but the
>>> filter proxy has a FIXME comment in the find_backlink_handles method.
>> OK, thanks. I checked reporter's database again, and found no error when
>> I export to GEDCOM with the privacy filter checked. I also tried the
>> gramplet mentioned by Jerome in another thread, changing its version
>> check to 4.1, and saw no private objects in his database.
>>> There is also a possible privacy issue with this code. Suppose that
>>> both the person and event are public, but the event reference is
>>> private. The person will be returned as a backlink, but would not be
>>> reachable in the forward direction.
>> Hehe, nice. Can I interpret that a design error, or could there be a
>> genuine use case behind this? Is it something like not wanting to
>> display that a person attended an event?
>>
>> Anyway, I'm glad that you found the error with the private location,
>> which I can reproduce indeed, but w.r.t. the NoneType errors, I am still
>> at square one. There is no corruption in reporter's database, and he
>> just confirmed that the database he sent is the exact one that caused
>> the errors mentioned in the report. And if that's true, we still have an
>> unreliable part of Gramps or associated libraries that causes NoneType
>> errors there, not here.
>>
>> In other words, my worries still exist. I'm a Python amateur, but I have
>> more than 25 years experience with C, about 10 with C++, so I know what
>> null pointers can do, and it looks like None objects behave quite the
>> same. And from what I've read here, I assume that in Gramps:
>>
>> 1. Whenever an object list is retrieved, all objects in it should be valid,
>> 2. If the privacy proxy is in effect, private object are skipped, so
>> rule 1 is still true,
>> 3. If all objects are private, the resulting list is empty, so that rule
>> 1 still applies.
>
> No, private objects are not skipped, they're returned as None. If all objects are private, the resulting list is filled with None. When iterating over the list each object should be tested for being None and skipped if it is.

Oops, should have looked at the code instead of taking Nick's word for it. Private objects are indeed skipped; just as important, they're tested for validity (necessary before attempting to dereference them to check the privacy). The filter proxy doesn't do anything, just wraps the database find_backlink_handles. Living checks that Person handles are valid (i.e., get_person_from_handle() doesn't return None) but doesn't check if the person is living; all other handles are passed through unchecked. ReferencedBySelection checks for the handle being in its dictionary of references, but doesn't check that it's valid in the database.

>
>>
>> If they are true, and this looks like good programming practice to me,
>> any iterator that meets a None and thus NoneType object indicates a
>> serious bug in lower software layers being either Gramps or any library
>> used. And given that I know that the integrity of the database that I
>> got is OK, since it does not trigger errors here, shows no errors in
>> check and repair here, and not on reporter's PC either, the situation
>> that I see has not changed.
>>
>> Since there are lots of reports where 'NoneType' errors look quite
>> random, and he got those where I didn't get any, ever, it looks like we
>> need some low level code that asserts the validity of objects read, so
>> that iteration will be safe again.
>>
>
> I dunno about "low level", but IMO it would be better if get_foo_from_handle raised exceptions instead of returning None. That would at least point to the direct cause of the problem instead of producing mysterious dereferencing errors; in cases where we expect that some handles might be filtered it's simple enough to catch the exception and continue the loop.

Make that get_from_handle(). Get_*_from_handle() are just convenience functions for it.

Another source of None-type errors is find_backlink_handles(handle, include_classes=None). Many calls don't specify the list of classes they're interested in, but make assumptions about what they'll get. Passing a valid handle to the wrong get_foo_from_handle() will return None. A well-written exception would make the problem obvious; a silent and unchecked None produces a mystery.

GedcomExtensions._process_family_event happens to be one of the calls that *does* set include_classes, so that's not the issue here, but I think is likely that it is in https://gramps-project.org/bugs/view.php?id=8011 and probably in many others as well.

Regards,
John Ralls

[Nick Hall]

On 24/08/14 20:07, John Ralls wrote:
>> No, private objects are not skipped, they're returned as None. If all objects are private, the resulting list is filled with None. When iterating over the list each object should be tested for being None and skipped if it is.
> Oops, should have looked at the code instead of taking Nick's word for it. Private objects are indeed skipped; just as important, they're tested for validity (necessary before attempting to dereference them to check the privacy). The filter proxy doesn't do anything, just wraps the database find_backlink_handles. Living checks that Person handles are valid (i.e., get_person_from_handle() doesn't return None) but doesn't check if the person is living; all other handles are passed through unchecked. ReferencedBySelection checks for the handle being in its dictionary of references, but doesn't check that it's valid in the database.
>

Yes. Sorry if I gave the wrong impression. The private proxy has some
checks in the backlinks - it was the filter proxy that had the FIXME
comment.

Nick.

[Nick Hall]

On 24/08/14 20:07, John Ralls wrote:
>> I dunno about "low level", but IMO it would be better if get_foo_from_handle raised exceptions instead of returning None. That would at least point to the direct cause of the problem instead of producing mysterious dereferencing errors; in cases where we expect that some handles might be filtered it's simple enough to catch the exception and continue the loop.
> Make that get_from_handle(). Get_*_from_handle() are just convenience functions for it.
>
> Another source of None-type errors is find_backlink_handles(handle, include_classes=None). Many calls don't specify the list of classes they're interested in, but make assumptions about what they'll get. Passing a valid handle to the wrong get_foo_from_handle() will return None. A well-written exception would make the problem obvious; a silent and unchecked None produces a mystery.

Are you suggesting that the proxies raise an exception, or just the
database layer?

Nick.

[John Ralls]

On Aug 24, 2014, at 12:17 PM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 24/08/14 20:07, John Ralls wrote:
>>> I dunno about "low level", but IMO it would be better if get_foo_from_handle raised exceptions instead of returning None. That would at least point to the direct cause of the problem instead of producing mysterious dereferencing errors; in cases where we expect that some handles might be filtered it's simple enough to catch the exception and continue the loop.
>> Make that get_from_handle(). Get_*_from_handle() are just convenience functions for it.
>>
>> Another source of None-type errors is find_backlink_handles(handle, include_classes=None). Many calls don't specify the list of classes they're interested in, but make assumptions about what they'll get. Passing a valid handle to the wrong get_foo_from_handle() will return None. A well-written exception would make the problem obvious; a silent and unchecked None produces a mystery.
>
> Are you suggesting that the proxies raise an exception, or just the
> database layer?

get_from_handle() is where I’d raise the exception, and I’d make three of them:

* handle == None
* handle.class != class_type
* data == None

Whether they should be separate classes so that they can be easily discriminated by the calling code is worth discussing, but for the moment they could simply be RuntimeError with different messages.

The only issue I see with the proxies is Filter not filtering.

Regards,
John Ralls

[Nick Hall]

On 24/08/14 21:01, John Ralls wrote:
>>>> I dunno about "low level", but IMO it would be better if get_foo_from_handle raised exceptions instead of returning None. That would at least point to the direct cause of the problem instead of producing mysterious dereferencing errors; in cases where we expect that some handles might be filtered it's simple enough to catch the exception and continue the loop.
>>> >>Make that get_from_handle(). Get_*_from_handle() are just convenience functions for it.
>>> >>
>>> >>Another source of None-type errors is find_backlink_handles(handle, include_classes=None). Many calls don't specify the list of classes they're interested in, but make assumptions about what they'll get. Passing a valid handle to the wrong get_foo_from_handle() will return None. A well-written exception would make the problem obvious; a silent and unchecked None produces a mystery.
>> >
>> >Are you suggesting that the proxies raise an exception, or just the
>> >database layer?
> get_from_handle() is where I’d raise the exception, and I’d make three of them:
>
> * handle == None
> * handle.class != class_type
> * data == None
>
> Whether they should be separate classes so that they can be easily discriminated by the calling code is worth discussing, but for the moment they could simply be RuntimeError with different messages.
>

I'm happy with that. The gen/errors.py file already defines a DbError
and DatabaseError class.

Nick.

[John Ralls]

On Aug 24, 2014, at 4:39 PM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 24/08/14 21:01, John Ralls wrote:
>>>>> I dunno about "low level", but IMO it would be better if get_foo_from_handle raised exceptions instead of returning None. That would at least point to the direct cause of the problem instead of producing mysterious dereferencing errors; in cases where we expect that some handles might be filtered it's simple enough to catch the exception and continue the loop.
>>>> >>Make that get_from_handle(). Get_*_from_handle() are just convenience functions for it.
>>>> >>
>>>> >>Another source of None-type errors is find_backlink_handles(handle, include_classes=None). Many calls don't specify the list of classes they're interested in, but make assumptions about what they'll get. Passing a valid handle to the wrong get_foo_from_handle() will return None. A well-written exception would make the problem obvious; a silent and unchecked None produces a mystery.
>>> >
>>> >Are you suggesting that the proxies raise an exception, or just the
>>> >database layer?
>> get_from_handle() is where I’d raise the exception, and I’d make three of them:
>>
>> * handle == None
>> * handle.class != class_type
>> * data == None
>>
>> Whether they should be separate classes so that they can be easily discriminated by the calling code is worth discussing, but for the moment they could simply be RuntimeError with different messages.
>>
>
> I'm happy with that. The gen/errors.py file already defines a DbError and DatabaseError class.

I’m not sure either of them are the right one to use, though. DbError says it’s for Berkeley DB errors. Unrecognized handles aren’t likely to be BDB’s fault, so that’s out IMO. DatabaseError says “database errors”. I’m not sure that it qualifies either; there isn’t necessarily anything wrong with the database. I like HandleError, "Error used to report wrong database handle errors”. That seems perfect. Proposed patch for master attached.

How far back should I do this? All the way to 3.4?

Regards,
John Ralls

[John Ralls]

On Aug 25, 2014, at 11:05 AM, John Ralls <***@ceridwen.us> wrote:

>
> On Aug 24, 2014, at 4:39 PM, Nick Hall <nick-***@gramps-project.org> wrote:
>
>> On 24/08/14 21:01, John Ralls wrote:
>>>>>> I dunno about "low level", but IMO it would be better if get_foo_from_handle raised exceptions instead of returning None. That would at least point to the direct cause of the problem instead of producing mysterious dereferencing errors; in cases where we expect that some handles might be filtered it's simple enough to catch the exception and continue the loop.
>>>>>>> Make that get_from_handle(). Get_*_from_handle() are just convenience functions for it.
>>>>>>>
>>>>>>> Another source of None-type errors is find_backlink_handles(handle, include_classes=None). Many calls don't specify the list of classes they're interested in, but make assumptions about what they'll get. Passing a valid handle to the wrong get_foo_from_handle() will return None. A well-written exception would make the problem obvious; a silent and unchecked None produces a mystery.
>>>>>
>>>>> Are you suggesting that the proxies raise an exception, or just the
>>>>> database layer?
>>> get_from_handle() is where I’d raise the exception, and I’d make three of them:
>>>
>>> * handle == None
>>> * handle.class != class_type
>>> * data == None
>>>
>>> Whether they should be separate classes so that they can be easily discriminated by the calling code is worth discussing, but for the moment they could simply be RuntimeError with different messages.
>>>
>>
>> I'm happy with that. The gen/errors.py file already defines a DbError and DatabaseError class.
>
> I’m not sure either of them are the right one to use, though. DbError says it’s for Berkeley DB errors. Unrecognized handles aren’t likely to be BDB’s fault, so that’s out IMO. DatabaseError says “database errors”. I’m not sure that it qualifies either; there isn’t necessarily anything wrong with the database. I like HandleError, "Error used to report wrong database handle errors”. That seems perfect. Proposed patch for master attached.
>
> How far back should I do this? All the way to 3.4?

Looking through the uses of get.*from_handle to add try/except for cases where a None value is actually handled (there aren’t many so far…) I find that there’s another group of functions that can return None, get.*from_gramps_id(). I’m inclined to define a new Exception GrampsIdError and raise that instead of return None from the root of them, __get_obj_from_gramps_id in read.py for all of the same reasons.

Any reason not to?

Regards,
John Ralls

[Tim Lyons]

Is changing all the code to raise exceptions instead of returning None really
a good idea?

It seems to me that there are a huge number of places to change - all the
get functions should be changed, and all the places where they are called.
This includes plug-ins etc, not just the main-line code.

Just wondering.



--
View this message in context: http://gramps.1791082.n4.nabble.com/cause-for-NoneType-object-has-no-attribute-tp4666879p4666954.html
Sent from the GRAMPS - Dev mailing list archive at Nabble.com.

[Nick Hall]

On 25/08/14 22:25, Tim Lyons wrote:
> Is changing all the code to raise exceptions instead of returning None really
> a good idea?
>
> It seems to me that there are a huge number of places to change - all the
> get functions should be changed, and all the places where they are called.
> This includes plug-ins etc, not just the main-line code.
>
> Just wondering.

At the moment, code that uses proxies checks for None. This indicates
that the object is hidden and the code skips the object. There is no way
to distinguish between a hidden object and an invalid handle.

Code that doesn't use proxies generally doesn't check for None. An
invalid handle produces an unclear error message.

The main effect of this change would be to make the error message clearer.

Do we need to change any code outside of the database code?

Nick.

[John Ralls]

On Aug 25, 2014, at 3:29 PM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 25/08/14 22:25, Tim Lyons wrote:
>> Is changing all the code to raise exceptions instead of returning None really
>> a good idea?
>>
>> It seems to me that there are a huge number of places to change - all the
>> get functions should be changed, and all the places where they are called.
>> This includes plug-ins etc, not just the main-line code.
>>
>> Just wondering.
>
> At the moment, code that uses proxies checks for None. This indicates
> that the object is hidden and the code skips the object. There is no way
> to distinguish between a hidden object and an invalid handle.
>
> Code that doesn't use proxies generally doesn't check for None. An
> invalid handle produces an unclear error message.
>
> The main effect of this change would be to make the error message clearer.
>
> Do we need to change any code outside of the database code?
>
Nick,

Yes. For example, from gramps/plugins/views/citationtreeview.py:
for handle in self.selected_handles():
# The handle will either be a Source handle or a Citation handle
source = self.dbstate.db.get_source_from_handle(handle)
citation = self.dbstate.db.get_citation_from_handle(handle)
if (not source and not citation) or (source and citation):
raise ValueError("selection must be either source or citation”)

This will break, because the handle might not be either and certainly can’t be both, so one of the two calls will raise a HandleError. Nothing special about that one, it just happened to be the one I was working on at the moment.

Regards,
John Ralls

[Nick Hall]

On 25/08/14 23:36, John Ralls wrote:
>> Do we need to change any code outside of the database code?
>> >
> Nick,
>
> Yes. For example, from gramps/plugins/views/citationtreeview.py:
> for handle in self.selected_handles():
> # The handle will either be a Source handle or a Citation handle
> source = self.dbstate.db.get_source_from_handle(handle)
> citation = self.dbstate.db.get_citation_from_handle(handle)
> if (not source and not citation) or (source and citation):
> raise ValueError("selection must be either source or citation”)
>
> This will break, because the handle might not be either and certainly can’t be both, so one of the two calls will raise a HandleError. Nothing special about that one, it just happened to be the one I was working on at the moment.

Quite right. I forgot about that case. It was the proxy code that I
didn't think needed changing.

Of course it would be nice to display an error dialog to the user when
an exception occurs. This might be too much work for now though.

Nick.

[John Ralls]

On Aug 25, 2014, at 3:41 PM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 25/08/14 23:36, John Ralls wrote:
>>> Do we need to change any code outside of the database code?
>>> >
>> Nick,
>>
>> Yes. For example, from gramps/plugins/views/citationtreeview.py:
>> for handle in self.selected_handles():
>> # The handle will either be a Source handle or a Citation handle
>> source = self.dbstate.db.get_source_from_handle(handle)
>> citation = self.dbstate.db.get_citation_from_handle(handle)
>> if (not source and not citation) or (source and citation):
>> raise ValueError("selection must be either source or citation”)
>>
>> This will break, because the handle might not be either and certainly can’t be both, so one of the two calls will raise a HandleError. Nothing special about that one, it just happened to be the one I was working on at the moment.
>
> Quite right. I forgot about that case. It was the proxy code that I didn't think needed changing.
>
> Of course it would be nice to display an error dialog to the user when an exception occurs. This might be too much work for now though.

Nick,

Oh, the user will get an error dialog all right. The “unhandled exception” one, with a nice traceback with a lot more info about where the bad handle came from than we’re getting now.

I’ve decided to change the proxy functions as well because they can be called in outside code; while they wrap the get_from_handle/get_obj_from_gramps_id in write.py and read.py, they also impose their own conditions and could return None. For now I’ve got HandleError, GrampsIdError, PrivateError, and FilterError, all derived from LookupError to make generic exception handling easy and to avoid having to import the others all over the place. I’m tempted to replace the lot of them with KeyError.

Regards,
John Ralls

[Nick Hall]

On 26/08/14 00:05, John Ralls wrote:
> Oh, the user will get an error dialog all right. The “unhandled exception” one, with a nice traceback with a lot more info about where the bad handle came from than we’re getting now.

OK. The full error report assistant does seem the correct outcome
here. I was considering something more user-friendly, but it wouldn't
be appropriate here.

>
> I’ve decided to change the proxy functions as well because they can be called in outside code; while they wrap the get_from_handle/get_obj_from_gramps_id in write.py and read.py, they also impose their own conditions and could return None. For now I’ve got HandleError, GrampsIdError, PrivateError, and FilterError, all derived from LookupError to make generic exception handling easy and to avoid having to import the others all over the place. I’m tempted to replace the lot of them with KeyError.

Yes. The proxy functions return None when an object should be hidden
from the user. This is an intended return code, and doesn't indicate an
error.

Failure to handle the None return in proxy code is a common bug.

Why are you raising a PrivateError?

Nick.

[John Ralls]

On Aug 25, 2014, at 4:17 PM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 26/08/14 00:05, John Ralls wrote:
>> Oh, the user will get an error dialog all right. The “unhandled exception” one, with a nice traceback with a lot more info about where the bad handle came from than we’re getting now.
>
> OK. The full error report assistant does seem the correct outcome here. I was considering something more user-friendly, but it wouldn't be appropriate here.
>
>>
>> I’ve decided to change the proxy functions as well because they can be called in outside code; while they wrap the get_from_handle/get_obj_from_gramps_id in write.py and read.py, they also impose their own conditions and could return None. For now I’ve got HandleError, GrampsIdError, PrivateError, and FilterError, all derived from LookupError to make generic exception handling easy and to avoid having to import the others all over the place. I’m tempted to replace the lot of them with KeyError.
>
> Yes. The proxy functions return None when an object should be hidden from the user. This is an intended return code, and doesn't indicate an error.
>
> Failure to handle the None return in proxy code is a common bug.
>
> Why are you raising a PrivateError?

Nick,

Sorry, are you objecting to switching private.py to raising exceptions or to the name of the exception?

Regards,
John Ralls

[Nick Hall]

On 26/08/14 02:11, John Ralls wrote:
> Sorry, are you objecting to switching private.py to raising exceptions or to the name of the exception?

It depends on which branches we want to modify.

If we want to make the change in the maintenance branches, then we
shouldn't change the API at all. However, if we keep the changes to the
database layer, we are mainly improving the error reporting. I don't
think that much code uses the return code of None, although you have
already identified one such case.

If we change the proxy layer then third-party reports that use proxies
will need updating.

I don't dislike the idea of the proxy code raising exceptions. Perhaps a
single ProxyException class would be sufficient for all proxies?

Nick.

[John Ralls]

On Aug 26, 2014, at 3:16 PM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 26/08/14 02:11, John Ralls wrote:
>> Sorry, are you objecting to switching private.py to raising exceptions or to the name of the exception?
>
> It depends on which branches we want to modify.
>
> If we want to make the change in the maintenance branches, then we shouldn't change the API at all. However, if we keep the changes to the database layer, we are mainly improving the error reporting. I don't think that much code uses the return code of None, although you have already identified one such case.
>
> If we change the proxy layer then third-party reports that use proxies will need updating.
>
> I don't dislike the idea of the proxy code raising exceptions. Perhaps a single ProxyException class would be sufficient for all proxies?

I’ve actually identified dozens of such cases so far. Here’s what’s in my index:
# modified: gramps/gen/db/read.py
# modified: gramps/gen/db/write.py
# modified: gramps/gen/errors.py
# modified: gramps/gen/filters/rules/person/_isspouseoffiltermatch.py
# modified: gramps/gen/merge/diff.py
# modified: gramps/gen/proxy/filter.py
# modified: gramps/gen/proxy/living.py
# modified: gramps/gen/proxy/private.py
# modified: gramps/gui/editors/editprimary.py
# modified: gramps/gui/plug/report/_bookdialog.py
# modified: gramps/gui/selectors/baseselector.py
# modified: gramps/gui/views/bookmarks.py
# modified: gramps/gui/views/navigationview.py
# modified: gramps/gui/views/treemodels/flatbasemodel.py
# modified: gramps/plugins/importer/importxml.py
# modified: gramps/plugins/lib/libpersonview.py
# modified: gramps/plugins/lib/libplaceview.py
# modified: gramps/plugins/view/citationlistview.py
# modified: gramps/plugins/view/citationtreeview.py
# modified: gramps/plugins/view/eventview.py
# modified: gramps/plugins/view/mediaview.py
# modified: gramps/plugins/view/noteview.py
# modified: gramps/plugins/view/repoview.py
# modified: gramps/plugins/view/sourceview.py
#

That’s the easy stuff: Files that overload get_.*_from_(handle|gramps_id) and files containing the regex ‘if.*get.*from_(handle|gramps_id)’. I can’t think of a good regex that will check for cases that assign with the result of get.*from_(handle|gramps_id) and then test the result, so I guess I’ll have to look at every instance. It will take a while; gramps-addons needs to be checked too.

If we don’t change the API then nothing changes, and we keep getting None-type bug reports until 4.2.0. On the other hand, this is turning into a pretty massive change, so maybe it doesn’t belong in maintenance branches.

As for the exception name, sure, we can have a single ProxyError instead of separate PrivateError and FilterError. Might make sense to call it ProxyWarning, since it’s really a notification rather than an actual error.

Regards,
John Ralls

[Nick Hall]

On 27/08/14 00:36, John Ralls wrote:
>> On 26/08/14 02:11, John Ralls wrote:
>>> >>Sorry, are you objecting to switching private.py to raising exceptions or to the name of the exception?
>> >
>> >It depends on which branches we want to modify.
>> >
>> >If we want to make the change in the maintenance branches, then we shouldn't change the API at all. However, if we keep the changes to the database layer, we are mainly improving the error reporting. I don't think that much code uses the return code of None, although you have already identified one such case.
>> >
>> >If we change the proxy layer then third-party reports that use proxies will need updating.
>> >
>> >I don't dislike the idea of the proxy code raising exceptions. Perhaps a single ProxyException class would be sufficient for all proxies?
> I’ve actually identified dozens of such cases so far. Here’s what’s in my index:
> # modified: gramps/gen/db/read.py
> # modified: gramps/gen/db/write.py
> # modified: gramps/gen/errors.py
> # modified: gramps/gen/filters/rules/person/_isspouseoffiltermatch.py
> # modified: gramps/gen/merge/diff.py
> # modified: gramps/gen/proxy/filter.py
> # modified: gramps/gen/proxy/living.py
> # modified: gramps/gen/proxy/private.py
> # modified: gramps/gui/editors/editprimary.py
> # modified: gramps/gui/plug/report/_bookdialog.py
> # modified: gramps/gui/selectors/baseselector.py
> # modified: gramps/gui/views/bookmarks.py
> # modified: gramps/gui/views/navigationview.py
> # modified: gramps/gui/views/treemodels/flatbasemodel.py
> # modified: gramps/plugins/importer/importxml.py
> # modified: gramps/plugins/lib/libpersonview.py
> # modified: gramps/plugins/lib/libplaceview.py
> # modified: gramps/plugins/view/citationlistview.py
> # modified: gramps/plugins/view/citationtreeview.py
> # modified: gramps/plugins/view/eventview.py
> # modified: gramps/plugins/view/mediaview.py
> # modified: gramps/plugins/view/noteview.py
> # modified: gramps/plugins/view/repoview.py
> # modified: gramps/plugins/view/sourceview.py
> #
>
> That’s the easy stuff: Files that overload get_.*_from_(handle|gramps_id) and files containing the regex ‘if.*get.*from_(handle|gramps_id)’. I can’t think of a good regex that will check for cases that assign with the result of get.*from_(handle|gramps_id) and then test the result, so I guess I’ll have to look at every instance. It will take a while; gramps-addons needs to be checked too.
>
> If we don’t change the API then nothing changes, and we keep getting None-type bug reports until 4.2.0. On the other hand, this is turning into a pretty massive change, so maybe it doesn’t belong in maintenance branches.
>
> As for the exception name, sure, we can have a single ProxyError instead of separate PrivateError and FilterError. Might make sense to call it ProxyWarning, since it’s really a notification rather than an actual error.

Perhaps we are doing too much here?

There is nothing wrong with returning None. In fact, because the
database tables look like dictionaries, it is quite a pythonic way of
doing things. We would expect the following to throw an exception:

db.person[handle]

but this to return None:

db.person.get(handle)

The convenience method, get_person_from_handle, is just an extension of
the latter.

We want to achieve the following:

1. Clearer error reports for bad handles.
2. The ability to distinguish between a bad handle and an object not
being visible through a proxy.

Doesn't raising an exception only in the database layer achieve this?
It would also have less impact on the code. We might even decide that
such a minor change could be made in the maintenance branches.

Nick.

[John Ralls]

On Aug 27, 2014, at 6:41 AM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 27/08/14 00:36, John Ralls wrote:
>>> On 26/08/14 02:11, John Ralls wrote:
>>>> >>Sorry, are you objecting to switching private.py to raising exceptions or to the name of the exception?
>>> >
>>> >It depends on which branches we want to modify.
>>> >
>>> >If we want to make the change in the maintenance branches, then we shouldn't change the API at all. However, if we keep the changes to the database layer, we are mainly improving the error reporting. I don't think that much code uses the return code of None, although you have already identified one such case.
>>> >
>>> >If we change the proxy layer then third-party reports that use proxies will need updating.
>>> >
>>> >I don't dislike the idea of the proxy code raising exceptions. Perhaps a single ProxyException class would be sufficient for all proxies?
>> I’ve actually identified dozens of such cases so far. Here’s what’s in my index:
>> # modified: gramps/gen/db/read.py
>> # modified: gramps/gen/db/write.py
>> # modified: gramps/gen/errors.py
>> # modified: gramps/gen/filters/rules/person/_isspouseoffiltermatch.py
>> # modified: gramps/gen/merge/diff.py
>> # modified: gramps/gen/proxy/filter.py
>> # modified: gramps/gen/proxy/living.py
>> # modified: gramps/gen/proxy/private.py
>> # modified: gramps/gui/editors/editprimary.py
>> # modified: gramps/gui/plug/report/_bookdialog.py
>> # modified: gramps/gui/selectors/baseselector.py
>> # modified: gramps/gui/views/bookmarks.py
>> # modified: gramps/gui/views/navigationview.py
>> # modified: gramps/gui/views/treemodels/flatbasemodel.py
>> # modified: gramps/plugins/importer/importxml.py
>> # modified: gramps/plugins/lib/libpersonview.py
>> # modified: gramps/plugins/lib/libplaceview.py
>> # modified: gramps/plugins/view/citationlistview.py
>> # modified: gramps/plugins/view/citationtreeview.py
>> # modified: gramps/plugins/view/eventview.py
>> # modified: gramps/plugins/view/mediaview.py
>> # modified: gramps/plugins/view/noteview.py
>> # modified: gramps/plugins/view/repoview.py
>> # modified: gramps/plugins/view/sourceview.py
>> #
>>
>> That’s the easy stuff: Files that overload get_.*_from_(handle|gramps_id) and files containing the regex ‘if.*get.*from_(handle|gramps_id)’. I can’t think of a good regex that will check for cases that assign with the result of get.*from_(handle|gramps_id) and then test the result, so I guess I’ll have to look at every instance. It will take a while; gramps-addons needs to be checked too.
>>
>> If we don’t change the API then nothing changes, and we keep getting None-type bug reports until 4.2.0. On the other hand, this is turning into a pretty massive change, so maybe it doesn’t belong in maintenance branches.
>>
>> As for the exception name, sure, we can have a single ProxyError instead of separate PrivateError and FilterError. Might make sense to call it ProxyWarning, since it’s really a notification rather than an actual error.
>
> Perhaps we are doing too much here?
>
> There is nothing wrong with returning None. In fact, because the database tables look like dictionaries, it is quite a pythonic way of doing things. We would expect the following to throw an exception:
>
> db.person[handle]
>
> but this to return None:
>
> db.person.get(handle)
>
> The convenience method, get_person_from_handle, is just an extension of the latter.
>
> We want to achieve the following:
>
> 1. Clearer error reports for bad handles.
> 2. The ability to distinguish between a bad handle and an object not being visible through a proxy.
>
> Doesn't raising an exception only in the database layer achieve this? It would also have less impact on the code. We might even decide that such a minor change could be made in the maintenance branches.

Umm, raising an exception doesn't return anything. It passes control to the first 'except Foo:' it finds as it unwinds the stack, where Foo matches the type of the exception or a parent. That means that every instance where the return value is checked has to be changed to handle the exception instead. For example:
if get_person_from_handle(handle):

is converted to
try:
get_person_from_handle(handle)
except LookupError:
#Do whatever the "else" clause of the if statement said
#Do whatever the branch did

Otherwise, the first except: it's going to encounter is the top-level exception handler, the one that puts up the "Your data is safe, but..." dialog. That will break a huge swath of Gramps.

The less invasive alternative is to log an info message (the way we use get_foo_from_handle() to check if the handle points to a foo means that it's not really an error, and people seem to object when we log errors or warnings routinely). The problem with that is that log messages seldom make it into bug reports.

Regards,
John Ralls

[Nick Hall]

On 27/08/14 15:27, John Ralls wrote:
> Umm, raising an exception doesn't return anything. It passes control to the first 'except Foo:' it finds as it unwinds the stack, where Foo matches the type of the exception or a parent. That means that every instance where the return value is checked has to be changed to handle the exception instead. For example:
> if get_person_from_handle(handle):
>
> is converted to
> try:
> get_person_from_handle(handle)
> except LookupError:
> #Do whatever the "else" clause of the if statement said
> #Do whatever the branch did

This is why I'm suggesting that proxies shouldn't raise exceptions, they
continue to return None instead.

I am assuming that most of the checks for None are proxy checks. If a
HandleError is raised then there is no problem letting the top-level
exception handle deal with it. Launching the error report assistant is
probably the correct action in such cases.

There will be some non-proxy code that needs changing. It may better to
re-write the source/citation selector code as:

if db.has_source_handle(handle):
obj = db.get_source_from_handle(handle)
else:
obj = db.get_citation_from_handle(handle)

Returning None for the get_.*_from_gramps_id methods is probably best
too. These methods could well be used to check that a particular Gramps
ID exists in the database. There is not much benefit to be gained by
changing this code.


> Otherwise, the first except: it's going to encounter is the top-level exception handler, the one that puts up the "Your data is safe, but..." dialog. That will break a huge swath of Gramps.

The RunDatabaseRepair dialog is something that will need to be looked
at. I don't think that it is called much. Is it still useful?


> The less invasive alternative is to log an info message (the way we use get_foo_from_handle() to check if the handle points to a foo means that it's not really an error, and people seem to object when we log errors or warnings routinely). The problem with that is that log messages seldom make it into bug reports.
>

We could just put in some debug statements and ask the user to turn on
debugging output for such bugs.


Nick.

[Enno Borgsteede]

Nick,
> The RunDatabaseRepair dialog is something that will need to be looked
> at. I don't think that it is called much. Is it still useful?
Huh? Is that the dialog that's shown when an error is detected? Looks
like it's called quite often, or ... ?

Working on #8001 where Check.py is called and runs into an exception,
I'm quite paranoid.

regards,

Enno

[Nick Hall]

On 27/08/14 22:12, Enno Borgsteede wrote:
>> The RunDatabaseRepair dialog is something that will need to be looked
>> >at. I don't think that it is called much. Is it still useful?
> Huh? Is that the dialog that's shown when an error is detected? Looks
> like it's called quite often, or ... ?
>

I can only see seven calls to the RunDatabaseRepair dialog. It is the
dialog that says:

"Gramps has detected an error in the database. This can usually be
resolved by running the "Check and Repair Database" tool.

If this problem continues to exist after running this tool, please file
a bug report at http://bugs.gramps-project.org"

Normal error handling uses the error report assistant, which is called
for uncaught top-level exceptions.

Nick.

[Enno Borgsteede]

John,
> Umm, raising an exception doesn't return anything. It passes control to the first 'except Foo:' it finds as it unwinds the stack, where Foo matches the type of the exception or a parent. That means that every instance where the return value is checked has to be changed to handle the exception instead. For example:
> if get_person_from_handle(handle):
>
> is converted to
> try:
> get_person_from_handle(handle)
> except LookupError:
> #Do whatever the "else" clause of the if statement said
> #Do whatever the branch did
>
> Otherwise, the first except: it's going to encounter is the top-level exception handler, the one that puts up the "Your data is safe, but..." dialog. That will break a huge swath of Gramps.
Huh? When above call raises as exception, and the handle is not None, it
looks like something's rotten in the state of Denmark, and if that's the
case, I would be very much obliged to be informed. Or am I paranoid again?

regards,

Enno

[Nick Hall]

On 27/08/14 22:32, Enno Borgsteede wrote:
>> For example:
>> > if get_person_from_handle(handle):
>> >
>> >is converted to
>> > try:
>> > get_person_from_handle(handle)
>> > except LookupError:
>> > #Do whatever the "else" clause of the if statement said
>> > #Do whatever the branch did
>> >
>> >Otherwise, the first except: it's going to encounter is the top-level exception handler, the one that puts up the "Your data is safe, but..." dialog. That will break a huge swath of Gramps.
> Huh? When above call raises as exception, and the handle is not None, it
> looks like something's rotten in the state of Denmark, and if that's the
> case, I would be very much obliged to be informed. Or am I paranoid again?

The proxy code will return None if the handle is valid but the object
should not be visible to the user. Code that is intended to run with
proxies should always check the return value. Code that does run with
proxies tends not to check the return code.

At the moment there is no way to tell the difference between a bad
handle and an object that should be hidden from the user. John is
suggesting raising a HandleError for a bad handle and a LookupError for
an object that should be hidden from the user.

I suggested that proxies still return None instead of raising an
exception. This might be worth considering if most of the current
checking is related to proxies.

Nick.

[Enno Borgsteede]

Nick,
> At the moment there is no way to tell the difference between a bad
> handle and an object that should be hidden from the user. John is
> suggesting raising a HandleError for a bad handle and a LookupError for
> an object that should be hidden from the user.
>
> I suggested that proxies still return None instead of raising an
> exception. This might be worth considering if most of the current
> checking is related to proxies.
Aha, ok, I get that. I prefer to use exceptions for real errors only,
which means that I support your suggestion. Hidden objects are not
errors, and should therefore either be omitted from a list where
appropriate, or returned as None when retrieved through a handle that is
not bad by itself.

regards,

Enno

[Nick Hall]

On 27/08/14 23:35, Enno Borgsteede wrote:
> Hidden objects are not
> errors, and should therefore either be omitted from a list where
> appropriate, or returned as None when retrieved through a handle that is
> not bad by itself.

I think that the lists of reference objects are not pruned for
performance reasons. For example, if a person is retrieved from the
database using the private proxy, it still contains references to
private events. Pruning the event reference list it would require
retrieving each event to see if it was private. This is why there are
going to be requests to retrieve hidden objects.

Nick.

[Enno Borgsteede]

Nick,
> I think that the lists of reference objects are not pruned for
> performance reasons. For example, if a person is retrieved from the
> database using the private proxy, it still contains references to
> private events. Pruning the event reference list it would require
> retrieving each event to see if it was private. This is why there are
> going to be requests to retrieve hidden objects.
That's clear. Thanks.

Enno

[Tim Lyons]

Nick Hall wrote
> The proxy code will return None if the handle is valid but the object
> should not be visible to the user. Code that is intended to run with
> proxies should always check the return value. Code that does run with
> proxies tends not to check the return code.
>
> At the moment there is no way to tell the difference between a bad
> handle and an object that should be hidden from the user. John is
> suggesting raising a HandleError for a bad handle and a LookupError for
> an object that should be hidden from the user.
>
> I suggested that proxies still return None instead of raising an
> exception. This might be worth considering if most of the current
> checking is related to proxies.

And I think Nick is suggesting that an exception should be raised for a bad
handle. Do I understand you correctly?

I think two issues can be distinguished.

(1) What should the API be?

(2) How to track down where the bad handle came from?

As far as (1) is concerned, I am persuaded by:

"There is nothing wrong with returning None. In fact, because the
database tables look like dictionaries, it is quite a pythonic way of
doing things. We would expect the following to throw an exception:

db.person[handle]

but this to return None:

db.person.get(handle)

The convenience method, get_person_from_handle, is just an extension of
the latter.
"

As far as (2) is concerned, John said "IMO it would be better if
get_foo_from_handle raised exceptions instead of returning None. That would
at least point to the direct cause of the problem instead of producing
mysterious dereferencing errors; in cases where we expect that some handles
might be filtered it's simple enough to catch the exception and continue the
loop."

"An unhandled exception about a bad handle removes the mystery, tells us why
the exception was raised (handle was None, wrong type, wasn’t found or was
private/filtered), and tells us what the handle’s repr was. That’s a lot
more clues for debugging the problem and may point straight at the fix."

The problem with this is that it is not pythonic, as said at (1), and it
entails a lot of change of code. Raising an exception only in the case where
the handle is bad maybe reduces the amount of code that needs to be changed
(John has already pointed out the source/citation code, but maybe there are
many others). But it doesn't seem to solve the problem of tracking down
where bad handles come from.

Would it be possible for the get_*_for_handle/get_*_from_gramps_id and
related functions to save the information about the error (including the
handle's repr) when they are about to return None (possibly including a
stack trace if that is not too expensive). Then when the general
AttributeError exception about Nonetype is raised, we could insert extra
code to retrieve the stored diagnostic information, and output it after the
normal stack trace?


This could be combined either with leaving the API unchanged, or changing it
to raise an exception just for a bad handle.



--
View this message in context: http://gramps.1791082.n4.nabble.com/cause-for-NoneType-object-has-no-attribute-tp4666879p4667012.html
Sent from the GRAMPS - Dev mailing list archive at Nabble.com.

[John Ralls]

On Aug 27, 2014, at 4:06 PM, Tim Lyons <***@gmail.com> wrote:

> Nick Hall wrote
>> The proxy code will return None if the handle is valid but the object
>> should not be visible to the user. Code that is intended to run with
>> proxies should always check the return value. Code that does run with
>> proxies tends not to check the return code.
>>
>> At the moment there is no way to tell the difference between a bad
>> handle and an object that should be hidden from the user. John is
>> suggesting raising a HandleError for a bad handle and a LookupError for
>> an object that should be hidden from the user.
>>
>> I suggested that proxies still return None instead of raising an
>> exception. This might be worth considering if most of the current
>> checking is related to proxies.
>
> And I think Nick is suggesting that an exception should be raised for a bad
> handle. Do I understand you correctly?
>
> I think two issues can be distinguished.
>
> (1) What should the API be?
>
> (2) How to track down where the bad handle came from?
>
> As far as (1) is concerned, I am persuaded by:
>
> "There is nothing wrong with returning None. In fact, because the
> database tables look like dictionaries, it is quite a pythonic way of
> doing things. We would expect the following to throw an exception:
>
> db.person[handle]
>
> but this to return None:
>
> db.person.get(handle)
>
> The convenience method, get_person_from_handle, is just an extension of
> the latter.
> "
>
> As far as (2) is concerned, John said "IMO it would be better if
> get_foo_from_handle raised exceptions instead of returning None. That would
> at least point to the direct cause of the problem instead of producing
> mysterious dereferencing errors; in cases where we expect that some handles
> might be filtered it's simple enough to catch the exception and continue the
> loop."
>
> "An unhandled exception about a bad handle removes the mystery, tells us why
> the exception was raised (handle was None, wrong type, wasn’t found or was
> private/filtered), and tells us what the handle’s repr was. That’s a lot
> more clues for debugging the problem and may point straight at the fix."
>
> The problem with this is that it is not pythonic, as said at (1), and it
> entails a lot of change of code. Raising an exception only in the case where
> the handle is bad maybe reduces the amount of code that needs to be changed
> (John has already pointed out the source/citation code, but maybe there are
> many others). But it doesn't seem to solve the problem of tracking down
> where bad handles come from.
>
> Would it be possible for the get_*_for_handle/get_*_from_gramps_id and
> related functions to save the information about the error (including the
> handle's repr) when they are about to return None (possibly including a
> stack trace if that is not too expensive). Then when the general
> AttributeError exception about Nonetype is raised, we could insert extra
> code to retrieve the stored diagnostic information, and output it after the
> normal stack trace?
>
>
> This could be combined either with leaving the API unchanged, or changing it
> to raise an exception just for a bad handle.

Tim, Nick,

What does "bad handle" mean?

There are hundreds of cases where get_foo_from_handle() is used to determine if handle points to a foo. There are hundreds more cases where bar=get_foo_from_handle() and bar is immediately dereferenced in the next line. In the first case the handle isn't "bad"; the calling code is just using an expensive way to find out what kind of handle it is*. In the second case, the calling code expects that a "good" handle will return an object of the right type.

I also don't see how to differentiate code that can use proxies from code that can't, nor do I see how a particular class can know whether the db object passed to it is querying via a proxy or not.

IMO both returning None and raising an exception are equally valid and equally pythonic for indicating that there is no handle for a query. The difference is that code that doesn't check for a None return will raise an misleading AttributeError, while code that doesn't handle the exception raises the right exception that points out the real error. I don't agree that database tables look like dictionaries, at least not outside of gen/db. If there's a e.g. Person[handle] API I haven't seen it.

I get that you don't like exceptions for routine signalling, so I'll drop that.

How about this variation on Tim's suggestion: Add logger.debug() statements including a traceback to both the DbBsddb and proxies, and add code to append the log buffer to the traceback in its error message?

Perhaps we can also agree that failing to handle a None return from any get is itself a bug and that the first thing to fix for any None-type AttributeError is to add such a check to the offending call.

Regards,
John Ralls

* The handle's class is encoded with the handle in the handle constructor, so it should be cheaper to check that than to query the database. Is it not reliable, or is it a recent addition? If the latter, we might get a nice speedup by checking it in get_foo_from_handle() before passing the query to the database.

[Nick Hall]

On 28/08/14 01:04, John Ralls wrote:
> What does "bad handle" mean?

I was using the term "bad handle" to mean a broken reference. This
should result in an error.


> There are hundreds of cases where get_foo_from_handle() is used to determine if handle points to a foo. There are hundreds more cases where bar=get_foo_from_handle() and bar is immediately dereferenced in the next line. In the first case the handle isn't "bad"; the calling code is just using an expensive way to find out what kind of handle it is*. In the second case, the calling code expects that a "good" handle will return an object of the right type.

I didn't realise that these methods were used so much to determine the
type of object returned by a handle. I expected the majority of checks
to be proxy related.


> I also don't see how to differentiate code that can use proxies from code that can't, nor do I see how a particular class can know whether the db object passed to it is querying via a proxy or not.

The code doesn't normally know if the database instance is actually a
proxy. As a general rule, report and exporter plugins should be written
to handle proxies. Most other code would expect a None return code to
be an error. The RunDatabaseRepair dialog is sometimes presented to the
user in such cases.


> IMO both returning None and raising an exception are equally valid and equally pythonic for indicating that there is no handle for a query. The difference is that code that doesn't check for a None return will raise an misleading AttributeError, while code that doesn't handle the exception raises the right exception that points out the real error. I don't agree that database tables look like dictionaries, at least not outside of gen/db. If there's a e.g. Person[handle] API I haven't seen it.

See:

https://docs.python.org/2/library/bsddb.html

The get_from_handle method could have used:

data = data_map[handle]

which would throw a KeyError if the handle is not a valid key.

>
> I get that you don't like exceptions for routine signalling, so I'll drop that.

I don't mind using exceptions in this way, but I don't really want to
change the API.

>
> How about this variation on Tim's suggestion: Add logger.debug() statements including a traceback to both the DbBsddb and proxies, and add code to append the log buffer to the traceback in its error message?

I've never had a problem with this type of error message myself. If the
bug is proxy-related then it is quite easy to fix. If it points to a
broken reference then improving the error message doesn't really help.
The big problem is working out what caused the broken reference.

Adding extra logger.debug() statements is a good idea.

> Perhaps we can also agree that failing to handle a None return from any get is itself a bug and that the first thing to fix for any None-type AttributeError is to add such a check to the offending call.

What do we do when we encounter a None return? We could make the code
compatible with proxies and ignore the object, or we could raise a more
user-friendly error such as the RunDatabaseRepair dialog.

I suggest that we just add some extra logger.debug() statements to the
maintenance branches.

In the long-term, do we want all code to support proxies? If this is
the case, we have no way to distinguish between a bad handle and a proxy
hiding an object. Perhaps we need to raise a HandleError in the
database layer only. What impact would this have on the code?


>
> Regards,
> John Ralls
>
> * The handle's class is encoded with the handle in the handle constructor, so it should be cheaper to check that than to query the database. Is it not reliable, or is it a recent addition? If the latter, we might get a nice speedup by checking it in get_foo_from_handle() before passing the query to the database.
>

The Handle class is fairly new. It is used by Gramps Connect and the
database differences report.

It would be easy to modify the source-citation selector to return a
tuple containing a class and handle. I coded it the current way to be
consistent with the other selectors.


Regards,

Nick.

[John Ralls]

On Aug 28, 2014, at 11:57 AM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 28/08/14 01:04, John Ralls wrote:
>> What does "bad handle" mean?
>
> I was using the term "bad handle" to mean a broken reference. This should result in an error.

But only the calling code knows whether it thinks that the handle it passed in should get a valid return value. There’s no way for the function being called to tell.
>
>
>> There are hundreds of cases where get_foo_from_handle() is used to determine if handle points to a foo. There are hundreds more cases where bar=get_foo_from_handle() and bar is immediately dereferenced in the next line. In the first case the handle isn't "bad"; the calling code is just using an expensive way to find out what kind of handle it is*. In the second case, the calling code expects that a "good" handle will return an object of the right type.
>
> I didn't realise that these methods were used so much to determine the type of object returned by a handle. I expected the majority of checks to be proxy related.
>
>
>> I also don't see how to differentiate code that can use proxies from code that can't, nor do I see how a particular class can know whether the db object passed to it is querying via a proxy or not.
>
> The code doesn't normally know if the database instance is actually a proxy. As a general rule, report and exporter plugins should be written to handle proxies. Most other code would expect a None return code to be an error. The RunDatabaseRepair dialog is sometimes presented to the user in such cases.
>
>
>> IMO both returning None and raising an exception are equally valid and equally pythonic for indicating that there is no handle for a query. The difference is that code that doesn't check for a None return will raise an misleading AttributeError, while code that doesn't handle the exception raises the right exception that points out the real error. I don't agree that database tables look like dictionaries, at least not outside of gen/db. If there's a e.g. Person[handle] API I haven't seen it.
>
> See:
>
> https://docs.python.org/2/library/bsddb.html
>
> The get_from_handle method could have used:
>
> data = data_map[handle]
>
> which would throw a KeyError if the handle is not a valid key.

But it doesn’t, presumably because the original design was for None to be a legitimate return value. Unfortunately a lot of contributors didn’t get that and blithely expect the return to be a valid object. We’re talking about the Gramps database API here, not the underlying BDB one. There’s no way for any Gramps code to use
person_table[handle]
instead of
get_person_from_handle(handle)
and get an exception if the handle isn’t valid. I think that’s good use of data-hiding. Should the Python wrapper for BDB go away for some reason it will be helpful to be able to switch to another backend without having to rewrite the whole application.

>
>>
>> I get that you don't like exceptions for routine signalling, so I'll drop that.
>
> I don't mind using exceptions in this way, but I don't really want to change the API.
>
>>
>> How about this variation on Tim's suggestion: Add logger.debug() statements including a traceback to both the DbBsddb and proxies, and add code to append the log buffer to the traceback in its error message?
>
> I've never had a problem with this type of error message myself. If the bug is proxy-related then it is quite easy to fix. If it points to a broken reference then improving the error message doesn't really help. The big problem is working out what caused the broken reference.
>
> Adding extra logger.debug() statements is a good idea.
>
>> Perhaps we can also agree that failing to handle a None return from any get is itself a bug and that the first thing to fix for any None-type AttributeError is to add such a check to the offending call.
>
> What do we do when we encounter a None return? We could make the code compatible with proxies and ignore the object, or we could raise a more user-friendly error such as the RunDatabaseRepair dialog.

That depends upon what the function in question is doing, where the handle came from, and how sure we are that the handle represents database corruption or is a bug in the code. Consider, for example, bug 8011, where the code called find_backlink_handles() without specifying the class list and then assuming that the result set contained only Person or Family, while under 4.1 it can contain Person, Family, or Place. Firing off the RunDatabaseRepair dialog in that case isn’t user-friendly because it makes the user do something futile and casts doubt on Gramps’s ability to properly care for the user’s research.

Of course while reviewing the function one would add the class_list parameter (‘Person’, ‘Family’) and test the returned handle for type before attempting to retrieve the object. Since it’s a report it is presumably OK for it to get None and it should just ignore the object and move on.

>
> I suggest that we just add some extra logger.debug() statements to the maintenance branches.
>
> In the long-term, do we want all code to support proxies? If this is the case, we have no way to distinguish between a bad handle and a proxy hiding an object. Perhaps we need to raise a HandleError in the database layer only. What impact would this have on the code?

Either it gets us back to where we are now or we have to differentiate at the client-class level between classes that are designed to use proxies (which return None if the handle isn’t found in the table) and classes that are designed to use DbBsddb and DbBsddbRead directly, which would raise a HandleError; the proxy classes would catch the HandleError and return None. In that case we have the potentially harder job of figuring out which classes are proxy users and which aren’t, and coding the latter to handle the exception.

>>
>> * The handle's class is encoded with the handle in the handle constructor, so it should be cheaper to check that than to query the database. Is it not reliable, or is it a recent addition? If the latter, we might get a nice speedup by checking it in get_foo_from_handle() before passing the query to the database.
>>
>
> The Handle class is fairly new. It is used by Gramps Connect and the database differences report.
>
> It would be easy to modify the source-citation selector to return a tuple containing a class and handle. I coded it the current way to be consistent with the other selectors.

OK, but that leaves all of the other selectors still doing an expensive query to determine the type from the handle.

Regards,
John Ralls

[John Ralls]

On Aug 28, 2014, at 11:57 AM, Nick Hall <nick-***@gramps-project.org> wrote:

> Adding extra logger.debug() statements is a good idea.

How does this look?

Regards,
John Ralls

[Tim Lyons]

I agree with most of what is said. I don't know whether I would like
exceptions for routine signalling in this case if we were talking about a
completely new API, but like Nick, my main worry is not wanting to change
the API.

I haven't read all of the code (yet), but just a few of points:

(1) I think Python evaluates the parameters before calling the function, so
all the traceback.print_stack will be created before calling LOG.debug, and
discovering that debug is off. Will this be too large a performance hit? (I
think it will).

(2) Is it worth implementing the speed up: "the handle's class is encoded
with the handle in the handle constructor, so it should be cheaper to check
that than to query the database. Is it not reliable, or is it a recent
addition? If the latter, we might get a nice speedup by checking it in
get_foo_from_handle() before passing the query to the database. " at the
same time?

(3) This is going to produce a huge amount of output, when only the last
three or four lines will be of any relevance.

(4) Are the problems you are trying to resolve reproducible enough to allow
the user to re-run the problem with debug on - will they be able to do that?

My suggestion was that the diagnostic information would be output the first
time the fault occurred, without the user having to re-run the thing he was
doing with debug turned on. (And when a get_* works OK you would clear the
buffer of the diagnostic information).

Just so I understand this, isn't it the case that this will just raise the
error a couple of lines before the line at which the error is raised at the
moment. So what extra information does this actually gain? (I understand
that if the handle or the proposed dereference had been passed as a
parameter, then the place where the error occurred may be some way away from
where it is raised at the moment, but is that often the problem?)

Is it just the case that this will enable the developers to distinguish
privacy errors from wrong handle type etc. in other words, the main thing
will be the handle repr and the path in the get-* code, rather than actually
the stacktrace?

Tim.


P.S. Nick says:
"I think that the lists of reference objects are not pruned for
performance reasons. For example, if a person is retrieved from the
database using the private proxy, it still contains references to
private events. Pruning the event reference list it would require
retrieving each event to see if it was private. This is why there are
going to be requests to retrieve hidden objects. "

When I had enhanced some of the proxy code (for citations), I thought that
there was generally quite a lot of code to examine reference lists to and to
re-write then with hidden things removed. I did find some places where this
had not been done and where this was near the citation code, I fixed them.
So I think that the lack of pruning may be a bug rather than intentional.



--
View this message in context: http://gramps.1791082.n4.nabble.com/cause-for-NoneType-object-has-no-attribute-tp4666879p4667033.html
Sent from the GRAMPS - Dev mailing list archive at Nabble.com.

[John Ralls]

On Aug 28, 2014, at 3:41 PM, Tim Lyons <***@gmail.com> wrote:

> I agree with most of what is said. I don't know whether I would like
> exceptions for routine signalling in this case if we were talking about a
> completely new API, but like Nick, my main worry is not wanting to change
> the API.
>
> I haven't read all of the code (yet), but just a few of points:
>
> (1) I think Python evaluates the parameters before calling the function, so
> all the traceback.print_stack will be created before calling LOG.debug, and
> discovering that debug is off. Will this be too large a performance hit? (I
> think it will).

It might be. Simple to fix, though: Just protect the call with LOG.isEnabledFor(logger.DEBUG). Python 3 adds a nice stack_info parameter to the output functions so the call to print_stack isn't needed there. Doesn't help us much for Py2, unfortunately.

>
> (2) Is it worth implementing the speed up: "the handle's class is encoded
> with the handle in the handle constructor, so it should be cheaper to check
> that than to query the database. Is it not reliable, or is it a recent
> addition? If the latter, we might get a nice speedup by checking it in
> get_foo_from_handle() before passing the query to the database. " at the
> same time?

Perhaps, but not in the same commit.

>
> (3) This is going to produce a huge amount of output, when only the last
> three or four lines will be of any relevance.

For what value of "huge"? I've set it to print only three frames, starting with the calling one. That's because if it's coming through a proxy the stack is frame_we're_interested_in->proxy.get_foo_for_handle->DbBsddbRead.get_foo_for_handle->DbBsdDb.get_for_handle. The last one is the one in which the info is logged.

>
> (4) Are the problems you are trying to resolve reproducible enough to allow
> the user to re-run the problem with debug on - will they be able to do that?

If there's an actual error in the code or if their db is truly whacked it should reproduce consistently. If it doesn't reproduce it's likely to be a memory-management problem that we don't have any control over.

>
> My suggestion was that the diagnostic information would be output the first
> time the fault occurred, without the user having to re-run the thing he was
> doing with debug turned on. (And when a get_* works OK you would clear the
> buffer of the diagnostic information).

Isn't that the way the logging buffer works already, a ring buffer that keeps only the last n lines of logging output? That's my understanding from the wiki's Logging System page; I haven't yet looked at the code.

>
> Just so I understand this, isn't it the case that this will just raise the
> error a couple of lines before the line at which the error is raised at the
> moment. So what extra information does this actually gain? (I understand
> that if the handle or the proposed dereference had been passed as a
> parameter, then the place where the error occurred may be some way away from
> where it is raised at the moment, but is that often the problem?)

It's not raising an error at all.

>
> Is it just the case that this will enable the developers to distinguish
> privacy errors from wrong handle type etc. in other words, the main thing
> will be the handle repr and the path in the get-* code, rather than actually
> the stacktrace?

My motive for the stack trace is to check that the function calling get_foo_from_handle() is the one raising the AttributeError, or the source of the None object if it's getting passed as a parameter.

Regards,
John Ralls

[Nick Hall]

There does seem to be a large amount of output at the moment.

The get_from_handle method is called with both None and empty string
handles. I don't mind a small stack trace if we can eliminate most of
these unnecessary calls. I'll look into this a bit further.

Nick.


On 29/08/14 04:22, John Ralls wrote:
> On Aug 28, 2014, at 3:41 PM, Tim Lyons <***@gmail.com> wrote:
>
>> I agree with most of what is said. I don't know whether I would like
>> exceptions for routine signalling in this case if we were talking about a
>> completely new API, but like Nick, my main worry is not wanting to change
>> the API.
>>
>> I haven't read all of the code (yet), but just a few of points:
>>
>> (1) I think Python evaluates the parameters before calling the function, so
>> all the traceback.print_stack will be created before calling LOG.debug, and
>> discovering that debug is off. Will this be too large a performance hit? (I
>> think it will).
> It might be. Simple to fix, though: Just protect the call with LOG.isEnabledFor(logger.DEBUG). Python 3 adds a nice stack_info parameter to the output functions so the call to print_stack isn't needed there. Doesn't help us much for Py2, unfortunately.
>
>> (2) Is it worth implementing the speed up: "the handle's class is encoded
>> with the handle in the handle constructor, so it should be cheaper to check
>> that than to query the database. Is it not reliable, or is it a recent
>> addition? If the latter, we might get a nice speedup by checking it in
>> get_foo_from_handle() before passing the query to the database. " at the
>> same time?
> Perhaps, but not in the same commit.
>
>> (3) This is going to produce a huge amount of output, when only the last
>> three or four lines will be of any relevance.
> For what value of "huge"? I've set it to print only three frames, starting with the calling one. That's because if it's coming through a proxy the stack is frame_we're_interested_in->proxy.get_foo_for_handle->DbBsddbRead.get_foo_for_handle->DbBsdDb.get_for_handle. The last one is the one in which the info is logged.
>
>> (4) Are the problems you are trying to resolve reproducible enough to allow
>> the user to re-run the problem with debug on - will they be able to do that?
> If there's an actual error in the code or if their db is truly whacked it should reproduce consistently. If it doesn't reproduce it's likely to be a memory-management problem that we don't have any control over.
>
>> My suggestion was that the diagnostic information would be output the first
>> time the fault occurred, without the user having to re-run the thing he was
>> doing with debug turned on. (And when a get_* works OK you would clear the
>> buffer of the diagnostic information).
> Isn't that the way the logging buffer works already, a ring buffer that keeps only the last n lines of logging output? That's my understanding from the wiki's Logging System page; I haven't yet looked at the code.
>
>> Just so I understand this, isn't it the case that this will just raise the
>> error a couple of lines before the line at which the error is raised at the
>> moment. So what extra information does this actually gain? (I understand
>> that if the handle or the proposed dereference had been passed as a
>> parameter, then the place where the error occurred may be some way away from
>> where it is raised at the moment, but is that often the problem?)
> It's not raising an error at all.
>
>> Is it just the case that this will enable the developers to distinguish
>> privacy errors from wrong handle type etc. in other words, the main thing
>> will be the handle repr and the path in the get-* code, rather than actually
>> the stacktrace?
> My motive for the stack trace is to check that the function calling get_foo_from_handle() is the one raising the AttributeError, or the source of the None object if it's getting passed as a parameter.
>
> Regards,
> John Ralls
>
>
> ------------------------------------------------------------------------------
> Slashdot TV.
> Video for Nerds. Stuff that matters.
> http://tv.slashdot.org/
> _______________________________________________
> Gramps-devel mailing list
> Gramps-***@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/gramps-devel
>
>

[Tim Lyons]

On 29 Aug 2014, at 04:22, John Ralls wrote:

>
> On Aug 28, 2014, at 3:41 PM, Tim Lyons <***@gmail.com> wrote:
>
>> (3) This is going to produce a huge amount of output, when only the
>> last
>> three or four lines will be of any relevance.
>
> For what value of "huge"? I've set it to print only three frames,
> starting with the calling one. That's because if it's coming through
> a proxy the stack is frame_we're_interested_in-
> >proxy.get_foo_for_handle->DbBsddbRead.get_foo_for_handle-
> >DbBsdDb.get_for_handle. The last one is the one in which the info
> is logged.
>
>> My suggestion was that the diagnostic information would be output
>> the first
>> time the fault occurred, without the user having to re-run the
>> thing he was
>> doing with debug turned on. (And when a get_* works OK you would
>> clear the
>> buffer of the diagnostic information).
>
> Isn't that the way the logging buffer works already, a ring buffer
> that keeps only the last n lines of logging output? That's my
> understanding from the wiki's Logging System page; I haven't yet
> looked at the code.

I thought you were using the normal debug logger. If debug logging is
turned on (https://gramps-project.org/wiki/index.php?title=Logging_system
#If_debugging_is_enabled) then the debug output is simply streamed to
the console. so if there are a thousand calls where
get_foo_from_handle() is used to determine if handle points to a foo
(and it doesn't), before the one that is troublesome, then there will
be one thousand times three frames.

I agree that a ring buffer would avoid that, but I am not sure that is
what you are doing (and then there is the additional problem of
outputting the buffer).

>>
>> Just so I understand this, isn't it the case that this will just
>> raise the
>> error a couple of lines before the line at which the error is
>> raised at the
>> moment. So what extra information does this actually gain? (I
>> understand
>> that if the handle or the proposed dereference had been passed as a
>> parameter, then the place where the error occurred may be some way
>> away from
>> where it is raised at the moment, but is that often the problem?)
>
> It's not raising an error at all.

Sorry, I did understand what you were doing, I meant log a diagnostic,
not raise an error.
>
>>
>> Is it just the case that this will enable the developers to
>> distinguish
>> privacy errors from wrong handle type etc. in other words, the main
>> thing
>> will be the handle repr and the path in the get-* code, rather than
>> actually
>> the stacktrace?
>
> My motive for the stack trace is to check that the function calling
> get_foo_from_handle() is the one raising the AttributeError, or the
> source of the None object if it's getting passed as a parameter.
>

so in:
In #7965, the first exception comes from
line 65 for (objclass, handle) in
self.dbase.find_backlink_handles(
line 66 event.handle, ['Person']):
line 67 person =
self.dbase.get_person_from_handle(handle)
line 68 for ref in person.get_event_ref_list():

instead of
line 68, in _process_family_event
for ref in person.get_event_ref_list():
AttributeError: 'NoneType' object has no attribute 'get_event_ref_list'

you would get
line 68, in _process_family_event
for ref in person.get_event_ref_list():
AttributeError: 'NoneType' object has no attribute 'get_event_ref_list'
---additional debug info
line 67, in _process_family_event
person = self.dbase.get_person_from_handle(handle)
LivingProxyDB.get_person_from_handle(0xabcdef) excluded person

(lots of irrelevant stacktrace lines omitted)

The point I was trying to make is that given the AttributeError is on
line 68, you know that the 'None' Person comes from line 67. However,
I agree that the important point is that it tells you why the None
person was returned from get_*_from* - the extra stack trace from the
get_*_from* is mostly not especially useful.

Regards,
Tim.


> Regards,
> John Ralls
>

[John Ralls]

On Aug 29, 2014, at 2:58 PM, Tim Lyons <***@gmail.com> wrote:

>
> On 29 Aug 2014, at 04:22, John Ralls wrote:
>
>>
>> On Aug 28, 2014, at 3:41 PM, Tim Lyons <***@gmail.com> wrote:
>>
>>> (3) This is going to produce a huge amount of output, when only the last
>>> three or four lines will be of any relevance.
>>
>> For what value of "huge"? I've set it to print only three frames, starting with the calling one. That's because if it's coming through a proxy the stack is frame_we're_interested_in->proxy.get_foo_for_handle->DbBsddbRead.get_foo_for_handle->DbBsdDb.get_for_handle. The last one is the one in which the info is logged.
>>
>>> My suggestion was that the diagnostic information would be output the first
>>> time the fault occurred, without the user having to re-run the thing he was
>>> doing with debug turned on. (And when a get_* works OK you would clear the
>>> buffer of the diagnostic information).
>>
>> Isn't that the way the logging buffer works already, a ring buffer that keeps only the last n lines of logging output? That's my understanding from the wiki's Logging System page; I haven't yet looked at the code.
>
> I thought you were using the normal debug logger. If debug logging is turned on (https://gramps-project.org/wiki/index.php?title=Logging_system#If_debugging_is_enabled) then the debug output is simply streamed to the console. so if there are a thousand calls where get_foo_from_handle() is used to determine if handle points to a foo (and it doesn't), before the one that is troublesome, then there will be one thousand times three frames.
>
> I agree that a ring buffer would avoid that, but I am not sure that is what you are doing (and then there is the additional problem of outputting the buffer).


Ah, you're right. I didn't correctly understand the description of the RotateHandler in Logging System. If I explicitly use that at the right level then if I understand correctly (and mind, I still haven't looked at the code) it will do what you asked, display the log messages attached to the error dialog in the original bug report. Am I getting close?



>
>>>
>>> Just so I understand this, isn't it the case that this will just raise the
>>> error a couple of lines before the line at which the error is raised at the
>>> moment. So what extra information does this actually gain? (I understand
>>> that if the handle or the proposed dereference had been passed as a
>>> parameter, then the place where the error occurred may be some way away from
>>> where it is raised at the moment, but is that often the problem?)
>>
>> It's not raising an error at all.
>
> Sorry, I did understand what you were doing, I meant log a diagnostic, not raise an error.
>>
>>>
>>> Is it just the case that this will enable the developers to distinguish
>>> privacy errors from wrong handle type etc. in other words, the main thing
>>> will be the handle repr and the path in the get-* code, rather than actually
>>> the stacktrace?
>>
>> My motive for the stack trace is to check that the function calling get_foo_from_handle() is the one raising the AttributeError, or the source of the None object if it's getting passed as a parameter.
>>
>
> so in:
> In #7965, the first exception comes from
> line 65 for (objclass, handle) in self.dbase.find_backlink_handles(
> line 66 event.handle, ['Person']):
> line 67 person = self.dbase.get_person_from_handle(handle)
> line 68 for ref in person.get_event_ref_list():
>
> instead of
> line 68, in _process_family_event
> for ref in person.get_event_ref_list():
> AttributeError: 'NoneType' object has no attribute 'get_event_ref_list'
>
> you would get
> line 68, in _process_family_event
> for ref in person.get_event_ref_list():
> AttributeError: 'NoneType' object has no attribute 'get_event_ref_list'
> ---additional debug info
> line 67, in _process_family_event
> person = self.dbase.get_person_from_handle(handle)
> LivingProxyDB.get_person_from_handle(0xabcdef) excluded person
>
> (lots of irrelevant stacktrace lines omitted)
>
> The point I was trying to make is that given the AttributeError is on line 68, you know that the 'None' Person comes from line 67. However, I agree that the important point is that it tells you why the None person was returned from get_*_from* - the extra stack trace from the get_*_from* is mostly not especially useful.

Yeah, if the reason for the None is an exclusion. If it's because the handle doesn't point to a person, the message will be
DbBsddb.get_from_handle() passed Person handle 0xabcdef and found no data.
Suppose the person in question wasn't the most recent call to get_person_from_handle. Which one caused the message? Having the line number for the call might help to disambiguate the message as long as it isn't in a loop. But maybe that's rare or nonexistant, so the traceback is just extra overhead. No problem, I can take it out, and if it proves necessary we can put it back in later.

Regards,
John Ralls

[Tim Lyons]

On 30 Aug 2014, at 05:00, John Ralls wrote:

>
> On Aug 29, 2014, at 2:58 PM, Tim Lyons <***@gmail.com> wrote:
>
>>
>> On 29 Aug 2014, at 04:22, John Ralls wrote:
>>
>>>
>>> On Aug 28, 2014, at 3:41 PM, Tim Lyons <***@gmail.com> wrote:
>>>
>>>> (3) This is going to produce a huge amount of output, when only
>>>> the last
>>>> three or four lines will be of any relevance.
>>>
>>> For what value of "huge"? I've set it to print only three frames,
>>> starting with the calling one. That's because if it's coming
>>> through a proxy the stack is frame_we're_interested_in-
>>> >proxy.get_foo_for_handle->DbBsddbRead.get_foo_for_handle-
>>> >DbBsdDb.get_for_handle. The last one is the one in which the info
>>> is logged.
>>>
>>>> My suggestion was that the diagnostic information would be output
>>>> the first
>>>> time the fault occurred, without the user having to re-run the
>>>> thing he was
>>>> doing with debug turned on. (And when a get_* works OK you would
>>>> clear the
>>>> buffer of the diagnostic information).
>>>
>>> Isn't that the way the logging buffer works already, a ring buffer
>>> that keeps only the last n lines of logging output? That's my
>>> understanding from the wiki's Logging System page; I haven't yet
>>> looked at the code.
>>
>> I thought you were using the normal debug logger. If debug logging
>> is turned on (https://gramps-project.org/wiki/index.php?title=Logging_system
>> #If_debugging_is_enabled) then the debug output is simply streamed
>> to the console. so if there are a thousand calls where
>> get_foo_from_handle() is used to determine if handle points to a
>> foo (and it doesn't), before the one that is troublesome, then
>> there will be one thousand times three frames.
>>
>> I agree that a ring buffer would avoid that, but I am not sure that
>> is what you are doing (and then there is the additional problem of
>> outputting the buffer).
>
>
> Ah, you're right. I didn't correctly understand the description of
> the RotateHandler in Logging System. If I explicitly use that at the
> right level then if I understand correctly (and mind, I still
> haven't looked at the code) it will do what you asked, display the
> log messages attached to the error dialog in the original bug
> report. Am I getting close?

Well, you could do that, but the RotateHandler logging that has been
instantiated/implemented outputs its buffer to a dialogue box when the
'Warning button' is clicked, see https://gramps-project.org/wiki/index.php?title=Gramps_3.4_Wiki_Manual_-_Main_Window
#Introduction

That is definitely not a useful way to get the diagnostic information
from the user's computer to the developer. It is extremely unlikely
that users will click the Warning button at all, still less at the
right moment and that they would record the information.

You could of course instantiate a different RotateHandler, but that
seems like a very complicated solution. (See below)


>>>> Just so I understand this, isn't it the case that this will just
>>>> raise the
>>>> error a couple of lines before the line at which the error is
>>>> raised at the
>>>> moment. So what extra information does this actually gain? (I
>>>> understand
>>>> that if the handle or the proposed dereference had been passed as a
>>>> parameter, then the place where the error occurred may be some
>>>> way away from
>>>> where it is raised at the moment, but is that often the problem?)
>>>
>>> It's not raising an error at all.
>>
>> Sorry, I did understand what you were doing, I meant log a
>> diagnostic, not raise an error.
>>>
>>>>
>>>> Is it just the case that this will enable the developers to
>>>> distinguish
>>>> privacy errors from wrong handle type etc. in other words, the
>>>> main thing
>>>> will be the handle repr and the path in the get-* code, rather
>>>> than actually
>>>> the stacktrace?
>>>
>>> My motive for the stack trace is to check that the function
>>> calling get_foo_from_handle() is the one raising the
>>> AttributeError, or the source of the None object if it's getting
>>> passed as a parameter.
>>>
>>
>> so in:
>> In #7965, the first exception comes from
>> line 65 for (objclass, handle) in
>> self.dbase.find_backlink_handles(
>> line 66 event.handle, ['Person']):
>> line 67 person =
>> self.dbase.get_person_from_handle(handle)
>> line 68 for ref in person.get_event_ref_list():
>>
>> instead of
>> line 68, in _process_family_event
>> for ref in person.get_event_ref_list():
>> AttributeError: 'NoneType' object has no attribute
>> 'get_event_ref_list'
>>
>> you would get
>> line 68, in _process_family_event
>> for ref in person.get_event_ref_list():
>> AttributeError: 'NoneType' object has no attribute
>> 'get_event_ref_list'
>> ---additional debug info
>> line 67, in _process_family_event
>> person = self.dbase.get_person_from_handle(handle)
>> LivingProxyDB.get_person_from_handle(0xabcdef) excluded person
>>
>> (lots of irrelevant stacktrace lines omitted)
>>
>> The point I was trying to make is that given the AttributeError is
>> on line 68, you know that the 'None' Person comes from line 67.
>> However, I agree that the important point is that it tells you why
>> the None person was returned from get_*_from* - the extra stack
>> trace from the get_*_from* is mostly not especially useful.
>
> Yeah, if the reason for the None is an exclusion. If it's because
> the handle doesn't point to a person, the message will be
> DbBsddb.get_from_handle() passed Person handle 0xabcdef and found
> no data.
> Suppose the person in question wasn't the most recent call to
> get_person_from_handle. Which one caused the message? Having the
> line number for the call might help to disambiguate the message as
> long as it isn't in a loop. But maybe that's rare or nonexistant, so
> the traceback is just extra overhead. No problem, I can take it out,
> and if it proves necessary we can put it back in later.
>

I think that it's probably going to be rare or non-existent, so I
think the traceback is just extra overhead.

I still suggest writing the message:
LivingProxyDB.get_person_from_handle(0xabcdef) excluded person
or
DbBsddb.get_from_handle() passed Person handle 0xabcdef and found no
data.
etc.

to a buffer (unconditionally), and then modifying the exception
handler for AttributeError, so that as well as the normal stacktrace
and message, it also output the message from the buffer.

That way, the diagnostics would always be output, and would not rely
on the user being willing and able to reproduce the problem.

I suppose the buffer only needs to hold the latest message!

Regards,
Tim.

[John Ralls]

On Aug 30, 2014, at 3:47 PM, Tim Lyons <***@gmail.com> wrote:

>
> On 30 Aug 2014, at 05:00, John Ralls wrote:
>
>> Ah, you're right. I didn't correctly understand the description of the RotateHandler in Logging System. If I explicitly use that at the right level then if I understand correctly (and mind, I still haven't looked at the code) it will do what you asked, display the log messages attached to the error dialog in the original bug report. Am I getting close?
>
> Well, you could do that, but the RotateHandler logging that has been instantiated/implemented outputs its buffer to a dialogue box when the 'Warning button' is clicked, see https://gramps-project.org/wiki/index.php?title=Gramps_3.4_Wiki_Manual_-_Main_Window#Introduction
>
> That is definitely not a useful way to get the diagnostic information from the user's computer to the developer. It is extremely unlikely that users will click the Warning button at all, still less at the right moment and that they would record the information.
>
> You could of course instantiate a different RotateHandler, but that seems like a very complicated solution. (See below)

That's one model. Another would be that the unhandled exception handler grabs the buffer and appends it to the traceback on the grounds that there might be something useful in the warnings to help with the debugging, no button-pushing required.

>>
>> Yeah, if the reason for the None is an exclusion. If it's because the handle doesn't point to a person, the message will be
>> DbBsddb.get_from_handle() passed Person handle 0xabcdef and found no data.
>> Suppose the person in question wasn't the most recent call to get_person_from_handle. Which one caused the message? Having the line number for the call might help to disambiguate the message as long as it isn't in a loop. But maybe that's rare or nonexistant, so the traceback is just extra overhead. No problem, I can take it out, and if it proves necessary we can put it back in later.
>>
>
> I think that it's probably going to be rare or non-existent, so I think the traceback is just extra overhead.
>
> I still suggest writing the message:
> LivingProxyDB.get_person_from_handle(0xabcdef) excluded person
> or
> DbBsddb.get_from_handle() passed Person handle 0xabcdef and found no data.
> etc.
>
> to a buffer (unconditionally), and then modifying the exception handler for AttributeError, so that as well as the normal stacktrace and message, it also output the message from the buffer.
>
> That way, the diagnostics would always be output, and would not rely on the user being willing and able to reproduce the problem.
>
> I suppose the buffer only needs to hold the latest message!

Maybe if we further filter it on None type errors. A quick trip through the bug list shows AttributeErrors for dict and DbBsddbRead as well as None. But what if there are None type errors that are from some other cause? Then that one-line error will be from the last call of get_foo_from_handle where the result was correctly handled. Of course that would be immediately apparent if there's a stack trace attached to it.

If the unhandled exception report *always* contains the exception traceback and the last 20 lines of logging output then the dev reviewing the bug knows that it's nothing special, but might contain some useful clues. A bit noisier perhaps, but better communicates the context, and besides it might be more generally useful.

Regards,
John Ralls

[Tim Lyons]

Yes, I think you are getting closer!


John Ralls-2 wrote
> On Aug 30, 2014, at 3:47 PM, Tim Lyons &lt;

> guy.linton@

> &gt; wrote:
>
>>
>> On 30 Aug 2014, at 05:00, John Ralls wrote:
>>
>>> Ah, you're right. I didn't correctly understand the description of the
>>> RotateHandler in Logging System. If I explicitly use that at the right
>>> level then if I understand correctly (and mind, I still haven't looked
>>> at the code) it will do what you asked, display the log messages
>>> attached to the error dialog in the original bug report. Am I getting
>>> close?
>>
>> Well, you could do that, but the RotateHandler logging that has been
>> instantiated/implemented outputs its buffer to a dialogue box when the
>> 'Warning button' is clicked, see
>> https://gramps-project.org/wiki/index.php?title=Gramps_3.4_Wiki_Manual_-_Main_Window#Introduction
>>
>> That is definitely not a useful way to get the diagnostic information
>> from the user's computer to the developer. It is extremely unlikely that
>> users will click the Warning button at all, still less at the right
>> moment and that they would record the information.
>>
>> You could of course instantiate a different RotateHandler, but that seems
>> like a very complicated solution. (See below)
>
> That's one model. Another would be that the unhandled exception handler
> grabs the buffer and appends it to the traceback on the grounds that there
> might be something useful in the warnings to help with the debugging, no
> button-pushing required.

The trouble with that is that using the existing RotateHandler would cause
the 'Warning button" to appear every time the code used the None return
legitimately. So if the user presses the 'Warning button' at any random
time, he is likely to see a complete buffer full of messages about None
returns from get_*_from... even though there is nothing wrong.


John Ralls-2 wrote
>>>
>>> Yeah, if the reason for the None is an exclusion. If it's because the
>>> handle doesn't point to a person, the message will be
>>> DbBsddb.get_from_handle() passed Person handle 0xabcdef and found no
>>> data.
>>> Suppose the person in question wasn't the most recent call to
>>> get_person_from_handle. Which one caused the message? Having the line
>>> number for the call might help to disambiguate the message as long as it
>>> isn't in a loop. But maybe that's rare or nonexistant, so the traceback
>>> is just extra overhead. No problem, I can take it out, and if it proves
>>> necessary we can put it back in later.
>>>
>>
>> I think that it's probably going to be rare or non-existent, so I think
>> the traceback is just extra overhead.
>>
>> I still suggest writing the message:
>> LivingProxyDB.get_person_from_handle(0xabcdef) excluded person
>> or
>> DbBsddb.get_from_handle() passed Person handle 0xabcdef and found no
>> data.
>> etc.
>>
>> to a buffer (unconditionally), and then modifying the exception handler
>> for AttributeError, so that as well as the normal stacktrace and message,
>> it also output the message from the buffer.
>>
>> That way, the diagnostics would always be output, and would not rely on
>> the user being willing and able to reproduce the problem.
>>
>> I suppose the buffer only needs to hold the latest message!
>
> Maybe if we further filter it on None type errors. A quick trip through
> the bug list shows AttributeErrors for dict and DbBsddbRead as well as
> None. But what if there are None type errors that are from some other
> cause? Then that one-line error will be from the last call of
> get_foo_from_handle where the result was correctly handled. Of course that
> would be immediately apparent if there's a stack trace attached to it.
>
> If the unhandled exception report *always* contains the exception
> traceback and the last 20 lines of logging output then the dev reviewing
> the bug knows that it's nothing special, but might contain some useful
> clues. A bit noisier perhaps, but better communicates the context, and
> besides it might be more generally useful.

Yes, I think that would be good, as you say, the last 20 lines of loging
output might be useful, and it might not, but no harm in including it for
the dev to check. Certainly much better to always generate and dump this
data, rather than rely on the user being able to reproduce the error, and
rerun the situation with some obscure diagnostic turned on! (Except for the
problem I mentioned above).

Regards,
Tim.



--
View this message in context: http://gramps.1791082.n4.nabble.com/cause-for-NoneType-object-has-no-attribute-tp4666879p4667070.html
Sent from the GRAMPS - Dev mailing list archive at Nabble.com.

[John Ralls]

On Aug 31, 2014, at 3:19 PM, Tim Lyons <***@gmail.com> wrote:

> Yes, I think you are getting closer!
>
>
> John Ralls-2 wrote
>> On Aug 30, 2014, at 3:47 PM, Tim Lyons &lt;
>
>> guy.linton@
>
>> &gt; wrote:
>>
>>>
>>> On 30 Aug 2014, at 05:00, John Ralls wrote:
>>>
>>>> Ah, you're right. I didn't correctly understand the description of the
>>>> RotateHandler in Logging System. If I explicitly use that at the right
>>>> level then if I understand correctly (and mind, I still haven't looked
>>>> at the code) it will do what you asked, display the log messages
>>>> attached to the error dialog in the original bug report. Am I getting
>>>> close?
>>>
>>> Well, you could do that, but the RotateHandler logging that has been
>>> instantiated/implemented outputs its buffer to a dialogue box when the
>>> 'Warning button' is clicked, see
>>> https://gramps-project.org/wiki/index.php?title=Gramps_3.4_Wiki_Manual_-_Main_Window#Introduction
>>>
>>> That is definitely not a useful way to get the diagnostic information
>>> from the user's computer to the developer. It is extremely unlikely that
>>> users will click the Warning button at all, still less at the right
>>> moment and that they would record the information.
>>>
>>> You could of course instantiate a different RotateHandler, but that seems
>>> like a very complicated solution. (See below)
>>
>> That's one model. Another would be that the unhandled exception handler
>> grabs the buffer and appends it to the traceback on the grounds that there
>> might be something useful in the warnings to help with the debugging, no
>> button-pushing required.
>
> The trouble with that is that using the existing RotateHandler would cause
> the 'Warning button" to appear every time the code used the None return
> legitimately. So if the user presses the 'Warning button' at any random
> time, he is likely to see a complete buffer full of messages about None
> returns from get_*_from... even though there is nothing wrong.

Hmm, true. Besides, it's not really appropriate to issue a warning about something that might be intentional, like using get_for_from_bar() for type determination. In other languages I'd say that Hone and '' handlers do deserve a warning, but with Python's "ask forgiveness rather than permission" paradigm I'm not so sure.

So I could use logger.Info for the messagss. For context, here are the counts for logging calls at various levels:
Critical: 0
Error: 62
Warning: 183
Info: 70
Debug: 339

You might think the following is too complicated, but I don't think it would be particularly hard to implement:
The standard levels are defined as integers, 50, 40, ..., 10 respectively, so we could define a new level 25 and assign it to the get_foo_from_bar() None returns. Those messages could be sent to the RotateHandler, with the latter modified if necessary to not light up the warning icon if it contains only messages at level 25, or there could be a special Rotate25Handler that collects only them, and that also added to the unhandled exception handler's output.

If you don't like having a special level, we could do pretty much the same thing without it, but that would add all of the existing info messages to the buffer. I haven't looked at those in detail yet, so I don't know if they'd be useful for routine bug analysis. I suppose that any that aren't useful could be demoted to debug messages.

Regards,
John Ralls

[Tim Lyons]

DIVERSION

I think that generally get_foo-from_bar() should unquestionably*
either return a good foo, or raise an exception, regardless of the
language:
- you've passed in None, you should have passed a bar,
- that looks like a bar, but it isn't a real one - something has gone
badly wrong,
- that kind of bar doesn't have a foo, you should have called
get_<kind>_from_bar() first,

If you named it check_and_get_foo_from_bar() there might be an
argument for returning None instead of an exception, but such a
function would not meet any criteria of modularity (it would be a
single function doing two different things).

There should be a get_object_type_from_handle() function for type
determination so we don't have to misuse get_*_from_handle().

But, 'we are where we are', and there is masses of code that has been
written on the assumption that these particular get_foo_from_bar() do
not return an exception. Perhaps some of it dates back to a time when
you could not tell the object_type from the handle, perhaps some of it
derives from a performance improvement of not sanitising the ref lists
in the proxy code. Whatever, we have a lot of code, including third
party add-ons, that work the way they do, and I don't think it is
worth changing the API at this time.

Is it worth changing the API for Gramps 4.2, or 5 or something -
possibly not. Is it worth implementing get_object_type_from_handle()
and gradually replacing all the places where the old function is used
purely for type determination - possibly yes (certainly for new code).
Perhaps those should be enhancement proposals, but I suspect that with
the shortage of resources, there are probably other things that are
more worthwhile to do.




* Unless you come up with some corner case I hadn't thought of!

END_DIVERSION




On 1 Sep 2014, at 06:49, John Ralls wrote:

>
> On Aug 31, 2014, at 3:19 PM, Tim Lyons <***@gmail.com> wrote:
>
>> Yes, I think you are getting closer!
>>
>>
>> John Ralls-2 wrote
>>> On Aug 30, 2014, at 3:47 PM, Tim Lyons &lt;guy.linton@&gt; wrote:
>>>> On 30 Aug 2014, at 05:00, John Ralls wrote:
>>>>
>>>>> Ah, you're right. I didn't correctly understand the description
>>>>> of the
>>>>> RotateHandler in Logging System. If I explicitly use that at the
>>>>> right
>>>>> level then if I understand correctly (and mind, I still haven't
>>>>> looked
>>>>> at the code) it will do what you asked, display the log messages
>>>>> attached to the error dialog in the original bug report. Am I
>>>>> getting
>>>>> close?
>>>>
>>>> Well, you could do that, but the RotateHandler logging that has
>>>> been
>>>> instantiated/implemented outputs its buffer to a dialogue box
>>>> when the
>>>> 'Warning button' is clicked, see
>>>> https://gramps-project.org/wiki/index.php?title=Gramps_3.4_Wiki_Manual_-_Main_Window
>>>> #Introduction
>>>>
>>>> That is definitely not a useful way to get the diagnostic
>>>> information
>>>> from the user's computer to the developer. It is extremely
>>>> unlikely that
>>>> users will click the Warning button at all, still less at the right
>>>> moment and that they would record the information.
>>>>
>>>> You could of course instantiate a different RotateHandler, but
>>>> that seems
>>>> like a very complicated solution. (See below)
>>>
>>> That's one model. Another would be that the unhandled exception
>>> handler
>>> grabs the buffer and appends it to the traceback on the grounds
>>> that there
>>> might be something useful in the warnings to help with the
>>> debugging, no
>>> button-pushing required.
>>
>> The trouble with that is that using the existing RotateHandler
>> would cause
>> the 'Warning button" to appear every time the code used the None
>> return
>> legitimately. So if the user presses the 'Warning button' at any
>> random
>> time, he is likely to see a complete buffer full of messages about
>> None
>> returns from get_*_from... even though there is nothing wrong.
>
> Hmm, true. Besides, it's not really appropriate to issue a warning
> about something that might be intentional, like using
> get_for_from_bar() for type determination. In other languages I'd
> say that Hone and '' handlers do deserve a warning, but with
> Python's "ask forgiveness rather than permission" paradigm I'm not
> so sure.
>
> So I could use logger.Info for the messagss. For context, here are
> the counts for logging calls at various levels:
> Critical: 0
> Error: 62
> Warning: 183
> Info: 70
> Debug: 339
>
> You might think the following is too complicated, but I don't think
> it would be particularly hard to implement:
> The standard levels are defined as integers, 50, 40, ..., 10
> respectively, so we could define a new level 25 and assign it to the
> get_foo_from_bar() None returns. Those messages could be sent to the
> RotateHandler, with the latter modified if necessary to not light up
> the warning icon if it contains only messages at level 25, or there
> could be a special Rotate25Handler that collects only them, and that
> also added to the unhandled exception handler's output.
>
> If you don't like having a special level, we could do pretty much
> the same thing without it, but that would add all of the existing
> info messages to the buffer. I haven't looked at those in detail
> yet, so I don't know if they'd be useful for routine bug analysis. I
> suppose that any that aren't useful could be demoted to debug
> messages.


I quite like the idea of a separate level and a special RotateHandler
to collect them.

I did wonder whether it should be a level between Error and Warning (I
think that is 35 not 25), because as discussed above, it is really a
bug in the code to be using get_foo_from_bar() for purposes it is not
really supposed to be used for. (On the other hand, I understand that
the user doesn't want to know about it in almost all circumstances, so
maybe it should be between Warning and Info).

Note that I think much of the code that tests log levels uses greater-
then or less-than tests, so wherever the new level is set you need to
check the effect of the existing log handlers on the new level.

By the way, I do think it is worthwhile trying to enable the diagnosis
of these Nonetype errors. They are probably mainly bugs in our code,
so we should do as much as possible to avoid the user having to
collect additional information (or re-run the thing he did) to allow
us to fix them.

Regards,
Tim.

[John Ralls]

On Sep 1, 2014, at 4:54 AM, Tim Lyons <***@gmail.com> wrote:

> DIVERSION
>
> I think that generally get_foo-from_bar() should unquestionably* either return a good foo, or raise an exception, regardless of the language:
> - you've passed in None, you should have passed a bar,
> - that looks like a bar, but it isn't a real one - something has gone badly wrong,
> - that kind of bar doesn't have a foo, you should have called get_<kind>_from_bar() first,
>
> If you named it check_and_get_foo_from_bar() there might be an argument for returning None instead of an exception, but such a function would not meet any criteria of modularity (it would be a single function doing two different things).
>
> There should be a get_object_type_from_handle() function for type determination so we don't have to misuse get_*_from_handle().
>
> But, 'we are where we are', and there is masses of code that has been written on the assumption that these particular get_foo_from_bar() do not return an exception. Perhaps some of it dates back to a time when you could not tell the object_type from the handle, perhaps some of it derives from a performance improvement of not sanitising the ref lists in the proxy code. Whatever, we have a lot of code, including third party add-ons, that work the way they do, and I don't think it is worth changing the API at this time.
>
> Is it worth changing the API for Gramps 4.2, or 5 or something - possibly not. Is it worth implementing get_object_type_from_handle() and gradually replacing all the places where the old function is used purely for type determination - possibly yes (certainly for new code). Perhaps those should be enhancement proposals, but I suspect that with the shortage of resources, there are probably other things that are more worthwhile to do.
>
>
>
>
> * Unless you come up with some corner case I hadn't thought of!
>

Tim,

<Resume_Diversion>
I guess you've forgotten about Nick's "DbBsddb is like a dict, so foo[bar] raises and foo.get(bar) returns None". You agreed at the time (I didn't and still don't) that get_foo_from_bar() somehow reflects that behavior.


Nick explained that the Handle class with that type indicator is a relatively new feature and that the use of get_foo_from_handle for type determination is legacy.

get_object_type_from_handle()? Seriously? This isn't C. The interface should be handle.getClass() and handle.isClass(classname).

Profiling is always necessary when doing optimization, and I haven't done any for this, but intuitively it seems that replacing get_foo_from_handle with checking the handle type directly would be a pretty good one. Querying the database is an expensive operation, and each handle carries its type along with it. If profiling confirms my intuition it would indeed be worthwhile to do this.

</Resume_Diversion>

Regards,
John Ralls

[Tim Lyons]

On 1 Sep 2014, at 17:09, John Ralls wrote:

>
> On Sep 1, 2014, at 4:54 AM, Tim Lyons <***@gmail.com> wrote:
>
>> DIVERSION
>>
>> I think that generally get_foo-from_bar() should unquestionably*
>> either return a good foo, or raise an exception, regardless of the
>> language:
>> - you've passed in None, you should have passed a bar,
>> - that looks like a bar, but it isn't a real one - something has
>> gone badly wrong,
>> - that kind of bar doesn't have a foo, you should have called
>> get_<kind>_from_bar() first,
>>
>> If you named it check_and_get_foo_from_bar() there might be an
>> argument for returning None instead of an exception, but such a
>> function would not meet any criteria of modularity (it would be a
>> single function doing two different things).
>>
>> There should be a get_object_type_from_handle() function for type
>> determination so we don't have to misuse get_*_from_handle().
>>
>> But, 'we are where we are', and there is masses of code that has
>> been written on the assumption that these particular
>> get_foo_from_bar() do not return an exception. Perhaps some of it
>> dates back to a time when you could not tell the object_type from
>> the handle, perhaps some of it derives from a performance
>> improvement of not sanitising the ref lists in the proxy code.
>> Whatever, we have a lot of code, including third party add-ons,
>> that work the way they do, and I don't think it is worth changing
>> the API at this time.
>>
>> Is it worth changing the API for Gramps 4.2, or 5 or something -
>> possibly not. Is it worth implementing
>> get_object_type_from_handle() and gradually replacing all the
>> places where the old function is used purely for type determination
>> - possibly yes (certainly for new code). Perhaps those should be
>> enhancement proposals, but I suspect that with the shortage of
>> resources, there are probably other things that are more worthwhile
>> to do.
>>
>>
>>
>>
>> * Unless you come up with some corner case I hadn't thought of!
>>
>
> Tim,
>
> <Resume_Diversion>
> I guess you've forgotten about Nick's "DbBsddb is like a dict, so
> foo[bar] raises and foo.get(bar) returns None". You agreed at the
> time (I didn't and still don't) that get_foo_from_bar() somehow
> reflects that behavior.

OK, well, either I've changed my mind, or there was a mis-speak or mis-
hear before. I'm (now) still strongly of the opinion that
get_foo_from_bar() should raise, and I'm not really convinced by the
analogy.

> Nick explained that the Handle class with that type indicator is a
> relatively new feature and that the use of get_foo_from_handle for
> type determination is legacy.
>
> get_object_type_from_handle()? Seriously? This isn't C. The
> interface should be handle.getClass() and handle.isClass(classname).

Whatever, I'm prepared to be convinced about the best interface for
the purpose. What would handle.getClass() return? would it return e.g.
Person? If it returns Person.handle, or something like that, I would
like a function that returned Person. (I'm maybe not understanding
whether handle.getClass() is a standard built-in function, or a
special purpose function).

> Profiling is always necessary when doing optimization, and I haven't
> done any for this, but intuitively it seems that replacing
> get_foo_from_handle with checking the handle type directly would be
> a pretty good one. Querying the database is an expensive operation,
> and each handle carries its type along with it. If profiling
> confirms my intuition it would indeed be worthwhile to do this.

Well, if you have time to do the profiling? But irrespective, it seems
to me that it would be better (in the long run) to replace the
get_foo_from_handle, even just for clarity of code.

Tim.
>
> </Resume_Diversion>
>
> Regards,
> John Ralls
>
>

[Nick Hall]

On 01/09/14 17:09, John Ralls wrote:
> I guess you've forgotten about Nick's "DbBsddb is like a dict, so foo[bar] raises and foo.get(bar) returns None". You agreed at the time (I didn't and still don't) that get_foo_from_bar() somehow reflects that behavior.

Although the database tables look like dictionaries, this doesn't mean
that our database methods shouldn't raise exceptions. However, we need
a good reason to change the API, and certainly can't do so in the
maintenance branches.


>
> Nick explained that the Handle class with that type indicator is a relatively new feature and that the use of get_foo_from_handle for type determination is legacy.
>
> get_object_type_from_handle()? Seriously? This isn't C. The interface should be handle.getClass() and handle.isClass(classname).
>
> Profiling is always necessary when doing optimization, and I haven't done any for this, but intuitively it seems that replacing get_foo_from_handle with checking the handle type directly would be a pretty good one. Querying the database is an expensive operation, and each handle carries its type along with it. If profiling confirms my intuition it would indeed be worthwhile to do this.

No. Database handles don't carry there type with them. They are just
strings.

When following forward references we know the type, and backlink handles
are provided in a tuple with a type, so most of the time we don't need
to determine the type of a handle.

One exception in the source/citation code (which I seem to remember
writing) could be re-written as:

if db.has_source_handle(handle):
obj = db.get_source_from_handle(handle)
else:
obj = db.get_citation_from_handle(handle)

Another alternative would be to return a (type, handle) tuple from the
selectors.


Nick.

[John Ralls]

On Sep 1, 2014, at 1:52 PM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 01/09/14 17:09, John Ralls wrote:
>> I guess you've forgotten about Nick's "DbBsddb is like a dict, so foo[bar] raises and foo.get(bar) returns None". You agreed at the time (I didn't and still don't) that get_foo_from_bar() somehow reflects that behavior.
>
> Although the database tables look like dictionaries, this doesn't mean
> that our database methods shouldn't raise exceptions. However, we need
> a good reason to change the API, and certainly can't do so in the
> maintenance branches.
>
>
>>
>> Nick explained that the Handle class with that type indicator is a relatively new feature and that the use of get_foo_from_handle for type determination is legacy.
>>
>> get_object_type_from_handle()? Seriously? This isn't C. The interface should be handle.getClass() and handle.isClass(classname).
>>
>> Profiling is always necessary when doing optimization, and I haven't done any for this, but intuitively it seems that replacing get_foo_from_handle with checking the handle type directly would be a pretty good one. Querying the database is an expensive operation, and each handle carries its type along with it. If profiling confirms my intuition it would indeed be worthwhile to do this.
>
> No. Database handles don't carry there type with them. They are just
> strings.
>
> When following forward references we know the type, and backlink handles
> are provided in a tuple with a type, so most of the time we don't need
> to determine the type of a handle.
>
> One exception in the source/citation code (which I seem to remember
> writing) could be re-written as:
>
> if db.has_source_handle(handle):
> obj = db.get_source_from_handle(handle)
> else:
> obj = db.get_citation_from_handle(handle)
>
> Another alternative would be to return a (type, handle) tuple from the
> selectors.

Nick,

So there's still a lot of work to do to integrate the Handle class into the older code. We always know the type of the handle when it's inserted into a variable, so we should just create a Handle instead of returning just a string. This shouldn't break any existing code:
>>> from gramps.gen.lib.handle import Handle
>>> handle = Handle('Person', 'a1b2c3d4e5f6')
>>> handle
'a1b2c3d4e5f6'
>>> handle.classname
'Person'

Functions returning tuples and their callers can be pretty easily converted to return and use Handles.

We may not need to check handles very often, but I saw a lot of examples when I was working on the exception approach.

I don't think it's necessary to serialize the classname into the database since the type is constrained by its place in the object hierarchy.

It occurred to me after sending that message that getClass() and isClass() might be confusing; it would be quite reasonable to expect getClass() to return Handle, the value of handle.__class__, or "Handle", the value of handle.__class__.__name___, so instead I think we want getRefClassName() and hasRefClassName(classname); the first would return return handle.classname, the second would return True if the classname parameter matched the member classname and False otherwise.

Regards,
John Ralls

[John Ralls]

On Sep 1, 2014, at 4:54 AM, Tim Lyons <***@gmail.com> wrote:
>
>
> I quite like the idea of a separate level and a special RotateHandler to collect them.
>
> I did wonder whether it should be a level between Error and Warning (I think that is 35 not 25), because as discussed above, it is really a bug in the code to be using get_foo_from_bar() for purposes it is not really supposed to be used for. (On the other hand, I understand that the user doesn't want to know about it in almost all circumstances, so maybe it should be between Warning and Info).
>
> Note that I think much of the code that tests log levels uses greater-then or less-than tests, so wherever the new level is set you need to check the effect of the existing log handlers on the new level.
>
> By the way, I do think it is worthwhile trying to enable the diagnosis of these Nonetype errors. They are probably mainly bugs in our code, so we should do as much as possible to avoid the user having to collect additional information (or re-run the thing he did) to allow us to fix them.

Tim,

I finally found and examined the gui logging code.

It turns out the wiki is correct, and you're confusing the RotateHandler, which collects all messages, and the WarningHandler, which collects only warnings and above. It's the latter which lights up the warning button.

Regardless of what you and I think about the "right" way to use get_foo_from_bar(), setting the special log level at 35 will make it visible to the Warning handler, which isn't what we want. Having it at 25 will get it captured by the RotateHandler only.

It looks like the RotateHandler's contents are added only in the error report assistant. Many reports I see don't seem to have been made with the assistant. The reporters seem to have just copied the traceback from the GtkHandler's window and pasted it into the bugtracker's "create" page. I propose to attach the RotateHandler's buffer to the window to increase the odds of it getting into the bug report.

Regards,
John Ralls

[Tim Lyons]

On 1 Sep 2014, at 20:20, John Ralls wrote:

>
> On Sep 1, 2014, at 4:54 AM, Tim Lyons <***@gmail.com> wrote:
>>
>>
>> I quite like the idea of a separate level and a special
>> RotateHandler to collect them.
>>
>> I did wonder whether it should be a level between Error and Warning
>> (I think that is 35 not 25), because as discussed above, it is
>> really a bug in the code to be using get_foo_from_bar() for
>> purposes it is not really supposed to be used for. (On the other
>> hand, I understand that the user doesn't want to know about it in
>> almost all circumstances, so maybe it should be between Warning and
>> Info).
>>
>> Note that I think much of the code that tests log levels uses
>> greater-then or less-than tests, so wherever the new level is set
>> you need to check the effect of the existing log handlers on the
>> new level.
>>
>> By the way, I do think it is worthwhile trying to enable the
>> diagnosis of these Nonetype errors. They are probably mainly bugs
>> in our code, so we should do as much as possible to avoid the user
>> having to collect additional information (or re-run the thing he
>> did) to allow us to fix them.
>
> Tim,
>
> I finally found and examined the gui logging code.
>
> It turns out the wiki is correct, and you're confusing the
> RotateHandler, which collects all messages, and the WarningHandler,
> which collects only warnings and above. It's the latter which lights
> up the warning button.
>
> Regardless of what you and I think about the "right" way to use
> get_foo_from_bar(), setting the special log level at 35 will make it
> visible to the Warning handler, which isn't what we want. Having it
> at 25 will get it captured by the RotateHandler only.
>
> It looks like the RotateHandler's contents are added only in the
> error report assistant. Many reports I see don't seem to have been
> made with the assistant. The reporters seem to have just copied the
> traceback from the GtkHandler's window and pasted it into the
> bugtracker's "create" page. I propose to attach the RotateHandler's
> buffer to the window to increase the odds of it getting into the bug
> report.

Interesting!

Out of interest, can you give an example (or a grep that will reveal
them) of messages that would get into the RotateHandler's contents,
but would not be in the GtkHandler's window?

Note, the messages from get_foo_from__bar() should be generated and
put into the RotateHandler at the default logging level - will this
happen if they are at level 25?

Tim.

[John Ralls]

On Sep 1, 2014, at 1:06 PM, Tim Lyons <***@gmail.com> wrote:

>
> On 1 Sep 2014, at 20:20, John Ralls wrote:
>
>>
>> On Sep 1, 2014, at 4:54 AM, Tim Lyons <***@gmail.com> wrote:
>>>
>>>
>>> I quite like the idea of a separate level and a special RotateHandler to collect them.
>>>
>>> I did wonder whether it should be a level between Error and Warning (I think that is 35 not 25), because as discussed above, it is really a bug in the code to be using get_foo_from_bar() for purposes it is not really supposed to be used for. (On the other hand, I understand that the user doesn't want to know about it in almost all circumstances, so maybe it should be between Warning and Info).
>>>
>>> Note that I think much of the code that tests log levels uses greater-then or less-than tests, so wherever the new level is set you need to check the effect of the existing log handlers on the new level.
>>>
>>> By the way, I do think it is worthwhile trying to enable the diagnosis of these Nonetype errors. They are probably mainly bugs in our code, so we should do as much as possible to avoid the user having to collect additional information (or re-run the thing he did) to allow us to fix them.
>>
>> Tim,
>>
>> I finally found and examined the gui logging code.
>>
>> It turns out the wiki is correct, and you're confusing the RotateHandler, which collects all messages, and the WarningHandler, which collects only warnings and above. It's the latter which lights up the warning button.
>>
>> Regardless of what you and I think about the "right" way to use get_foo_from_bar(), setting the special log level at 35 will make it visible to the Warning handler, which isn't what we want. Having it at 25 will get it captured by the RotateHandler only.
>>
>> It looks like the RotateHandler's contents are added only in the error report assistant. Many reports I see don't seem to have been made with the assistant. The reporters seem to have just copied the traceback from the GtkHandler's window and pasted it into the bugtracker's "create" page. I propose to attach the RotateHandler's buffer to the window to increase the odds of it getting into the bug report.
>
> Interesting!
>
> Out of interest, can you give an example (or a grep that will reveal them) of messages that would get into the RotateHandler's contents, but would not be in the GtkHandler's window?

Sorry, I don't understand the question. At present GtkHandler displays a window on an error, and the default log level is set to logging.WARNING, so only warnings would be in RotateHandler's buffer. If you start Gramps with --debug, then all of the INFO and DEBUG level messages will be handled by the RotateHandler; it will keep the last 20. At present none of them will be displayed in the GtkHandler window, but they'll be added to the bug report as part of "additional information" if you use the assistant to create the bug report. Since WARNING messages are pretty rare

You can find all of the messages at e.g. the INFO level with
egrep -rI '(logging|LOG)\.Info' gramps

>
> Note, the messages from get_foo_from__bar() should be generated and put into the RotateHandler at the default logging level - will this happen if they are at level 25?

I'd change the default logging level to 25 (we need to come up with a name for the level, do you like ALERT?) from logging.WARNING. Since the RotateHandler seems not to have its level set, it should handle all emitted log messages.

Regards,
John Ralls

[Tim Lyons]

On 1 Sep 2014, at 22:18, John Ralls wrote:

>
> On Sep 1, 2014, at 1:06 PM, Tim Lyons <***@gmail.com> wrote:
>> Out of interest, can you give an example (or a grep that will
>> reveal them) of messages that would get into the RotateHandler's
>> contents, but would not be in the GtkHandler's window?
>
> Sorry, I don't understand the question. At present GtkHandler
> displays a window on an error, and the default log level is set to
> logging.WARNING, so only warnings would be in RotateHandler's
> buffer. If you start Gramps with --debug, then all of the INFO and
> DEBUG level messages will be handled by the RotateHandler; it will
> keep the last 20. At present none of them will be displayed in the
> GtkHandler window, but they'll be added to the bug report as part of
> "additional information" if you use the assistant to create the bug
> report. Since WARNING messages are pretty rare
>
> You can find all of the messages at e.g. the INFO level with
> egrep -rI '(logging|LOG)\.Info' gramps

I was trying to find out which messages appeared in the 'additional
information' part of the bug report assistant, and did not appear in
the GtkHandler window.

I think the answer is 'all the Warning messages' (and INFO and DEBUG
messages if you start Gramps with --debug).

I think it will be useful for debug purposes if both the RotateHandler
and the Rotete25Handler messages were in the GtkHandler window and the
'additional information' of the bug report assistant. Some of the
warnings (IIRC) include missing modules (like the geography one, I
think) and anomalies about locale setup - who knows when and whether
these might be useful.

>> Note, the messages from get_foo_from__bar() should be generated and
>> put into the RotateHandler at the default logging level - will this
>> happen if they are at level 25?
>
> I'd change the default logging level to 25 (we need to come up with
> a name for the level, do you like ALERT?) from logging.WARNING.
> Since the RotateHandler seems not to have its level set, it should
> handle all emitted log messages.

ALERT seems fine. I prefer separate RotateHandler and Rotate25Handler,
so that the missing module and locale setup etc. which are rare and
not overwritten by the None messages (which will be very common). [But
this may be too complicated]

Tim.

[John Ralls]

On Sep 1, 2014, at 2:45 PM, Tim Lyons <***@gmail.com> wrote:

>
> On 1 Sep 2014, at 22:18, John Ralls wrote:
>
>>
>> On Sep 1, 2014, at 1:06 PM, Tim Lyons <***@gmail.com> wrote:
>>> Out of interest, can you give an example (or a grep that will reveal them) of messages that would get into the RotateHandler's contents, but would not be in the GtkHandler's window?
>>
>> Sorry, I don't understand the question. At present GtkHandler displays a window on an error, and the default log level is set to logging.WARNING, so only warnings would be in RotateHandler's buffer. If you start Gramps with --debug, then all of the INFO and DEBUG level messages will be handled by the RotateHandler; it will keep the last 20. At present none of them will be displayed in the GtkHandler window, but they'll be added to the bug report as part of "additional information" if you use the assistant to create the bug report. Since WARNING messages are pretty rare
>>
>> You can find all of the messages at e.g. the INFO level with
>> egrep -rI '(logging|LOG)\.Info' gramps
>
> I was trying to find out which messages appeared in the 'additional information' part of the bug report assistant, and did not appear in the GtkHandler window.
>
> I think the answer is 'all the Warning messages' (and INFO and DEBUG messages if you start Gramps with --debug).
>
> I think it will be useful for debug purposes if both the RotateHandler and the Rotete25Handler messages were in the GtkHandler window and the 'additional information' of the bug report assistant. Some of the warnings (IIRC) include missing modules (like the geography one, I think) and anomalies about locale setup - who knows when and whether these might be useful.
>
>>> Note, the messages from get_foo_from__bar() should be generated and put into the RotateHandler at the default logging level - will this happen if they are at level 25?
>>
>> I'd change the default logging level to 25 (we need to come up with a name for the level, do you like ALERT?) from logging.WARNING. Since the RotateHandler seems not to have its level set, it should handle all emitted log messages.
>
> ALERT seems fine. I prefer separate RotateHandler and Rotate25Handler, so that the missing module and locale setup etc. which are rare and not overwritten by the None messages (which will be very common). [But this may be too complicated]

Tim,

No, not *all* the messages, only the last 20. That's the "Rotate" part. Missing module messages may have been pushed off the top.

I'm not sure yet that I correctly understand what messages get passed to the default logger, but the RotateHandler and WarnHandler are set only on it so it's possible that they miss some messages given that many are passed to other loggers. If all messages are passed to the default logger regardless then it's not possible to keep ALERT messages out of the RotateHandler; if they aren't then it's easy to have a separate ALERT logger with a separate RotateHandler instance, but we also have some cleanup work to make sure that all of the other messages are getting to RotateHandler.

Regards,
John Ralls

[John Ralls]

On Sep 1, 2014, at 3:47 PM, John Ralls <***@ceridwen.us> wrote:

>
> On Sep 1, 2014, at 2:45 PM, Tim Lyons <***@gmail.com> wrote:
>
>>
>> On 1 Sep 2014, at 22:18, John Ralls wrote:
>>
>>>
>>> On Sep 1, 2014, at 1:06 PM, Tim Lyons <***@gmail.com> wrote:
>>>> Out of interest, can you give an example (or a grep that will reveal them) of messages that would get into the RotateHandler's contents, but would not be in the GtkHandler's window?
>>>
>>> Sorry, I don't understand the question. At present GtkHandler displays a window on an error, and the default log level is set to logging.WARNING, so only warnings would be in RotateHandler's buffer. If you start Gramps with --debug, then all of the INFO and DEBUG level messages will be handled by the RotateHandler; it will keep the last 20. At present none of them will be displayed in the GtkHandler window, but they'll be added to the bug report as part of "additional information" if you use the assistant to create the bug report. Since WARNING messages are pretty rare
>>>
>>> You can find all of the messages at e.g. the INFO level with
>>> egrep -rI '(logging|LOG)\.Info' gramps
>>
>> I was trying to find out which messages appeared in the 'additional information' part of the bug report assistant, and did not appear in the GtkHandler window.
>>
>> I think the answer is 'all the Warning messages' (and INFO and DEBUG messages if you start Gramps with --debug).
>>
>> I think it will be useful for debug purposes if both the RotateHandler and the Rotete25Handler messages were in the GtkHandler window and the 'additional information' of the bug report assistant. Some of the warnings (IIRC) include missing modules (like the geography one, I think) and anomalies about locale setup - who knows when and whether these might be useful.
>>
>>>> Note, the messages from get_foo_from__bar() should be generated and put into the RotateHandler at the default logging level - will this happen if they are at level 25?
>>>
>>> I'd change the default logging level to 25 (we need to come up with a name for the level, do you like ALERT?) from logging.WARNING. Since the RotateHandler seems not to have its level set, it should handle all emitted log messages.
>>
>> ALERT seems fine. I prefer separate RotateHandler and Rotate25Handler, so that the missing module and locale setup etc. which are rare and not overwritten by the None messages (which will be very common). [But this may be too complicated]
>
> Tim,
>
> No, not *all* the messages, only the last 20. That's the "Rotate" part. Missing module messages may have been pushed off the top.
>
> I'm not sure yet that I correctly understand what messages get passed to the default logger, but the RotateHandler and WarnHandler are set only on it so it's possible that they miss some messages given that many are passed to other loggers. If all messages are passed to the default logger regardless then it's not possible to keep ALERT messages out of the RotateHandler; if they aren't then it's easy to have a separate ALERT logger with a separate RotateHandler instance, but we also have some cleanup work to make sure that all of the other messages are getting to RotateHandler.

I’ve studied the logging docs a bit to refresh my memory of it all works, and the Gramps code to see what it’s doing.
Unfortunately all of the existing handlers are installed on the root handler, which means that every message goes to them. The WarnHandler sees only logging.WARN and higher, but the RotateHandler sees everything DEBUG and above; the only way to separate the get_foo_from_bar() messages is to set their level < 10 so that the rest of the logging system ignores them.

I’ve attached a patch that makes a start at that. It compiles, but I haven’t yet figured out how to test it to make sure that it works.

Regards,
John Ralls

[John Ralls]

On Sep 3, 2014, at 5:37 PM, John Ralls <***@ceridwen.us> wrote:

>
> On Sep 1, 2014, at 3:47 PM, John Ralls <***@ceridwen.us> wrote:
>
>>
>> On Sep 1, 2014, at 2:45 PM, Tim Lyons <***@gmail.com> wrote:
>>
>>>
>>> On 1 Sep 2014, at 22:18, John Ralls wrote:
>>>
>>>>
>>>> On Sep 1, 2014, at 1:06 PM, Tim Lyons <***@gmail.com> wrote:
>>>>> Out of interest, can you give an example (or a grep that will reveal them) of messages that would get into the RotateHandler's contents, but would not be in the GtkHandler's window?
>>>>
>>>> Sorry, I don't understand the question. At present GtkHandler displays a window on an error, and the default log level is set to logging.WARNING, so only warnings would be in RotateHandler's buffer. If you start Gramps with --debug, then all of the INFO and DEBUG level messages will be handled by the RotateHandler; it will keep the last 20. At present none of them will be displayed in the GtkHandler window, but they'll be added to the bug report as part of "additional information" if you use the assistant to create the bug report. Since WARNING messages are pretty rare
>>>>
>>>> You can find all of the messages at e.g. the INFO level with
>>>> egrep -rI '(logging|LOG)\.Info' gramps
>>>
>>> I was trying to find out which messages appeared in the 'additional information' part of the bug report assistant, and did not appear in the GtkHandler window.
>>>
>>> I think the answer is 'all the Warning messages' (and INFO and DEBUG messages if you start Gramps with --debug).
>>>
>>> I think it will be useful for debug purposes if both the RotateHandler and the Rotete25Handler messages were in the GtkHandler window and the 'additional information' of the bug report assistant. Some of the warnings (IIRC) include missing modules (like the geography one, I think) and anomalies about locale setup - who knows when and whether these might be useful.
>>>
>>>>> Note, the messages from get_foo_from__bar() should be generated and put into the RotateHandler at the default logging level - will this happen if they are at level 25?
>>>>
>>>> I'd change the default logging level to 25 (we need to come up with a name for the level, do you like ALERT?) from logging.WARNING. Since the RotateHandler seems not to have its level set, it should handle all emitted log messages.
>>>
>>> ALERT seems fine. I prefer separate RotateHandler and Rotate25Handler, so that the missing module and locale setup etc. which are rare and not overwritten by the None messages (which will be very common). [But this may be too complicated]
>>
>> Tim,
>>
>> No, not *all* the messages, only the last 20. That's the "Rotate" part. Missing module messages may have been pushed off the top.
>>
>> I'm not sure yet that I correctly understand what messages get passed to the default logger, but the RotateHandler and WarnHandler are set only on it so it's possible that they miss some messages given that many are passed to other loggers. If all messages are passed to the default logger regardless then it's not possible to keep ALERT messages out of the RotateHandler; if they aren't then it's easy to have a separate ALERT logger with a separate RotateHandler instance, but we also have some cleanup work to make sure that all of the other messages are getting to RotateHandler.
>
> I’ve studied the logging docs a bit to refresh my memory of it all works, and the Gramps code to see what it’s doing.
> Unfortunately all of the existing handlers are installed on the root handler, which means that every message goes to them. The WarnHandler sees only logging.WARN and higher, but the RotateHandler sees everything DEBUG and above; the only way to separate the get_foo_from_bar() messages is to set their level < 10 so that the rest of the logging system ignores them.
>
> I’ve attached a patch that makes a start at that. It compiles, but I haven’t yet figured out how to test it to make sure that it works.

Found a reliable way to crash with a None type error thanks to https://gramps-project.org/bugs/view.php?id=7720, so the attached patch is tested. Unless someone objects I’ll push it in a couple of days.

Crash Procedure:
Create a new Family Tree and open it.
Import either data.gramps or example.gramps
Switch to the Places tab, select the References tab in the bottom-bar, and select a place with references.
Switch to the Families tab
Open the Family Tree Manager and delete the open database, then close the manager window.
Switch back to the Places tab. The references will still be there, though the ListView is now empty.
Double-click on one of the references. The Unhandled Exception Window comes up.

Regards,
John Ralls

[Nick Hall]

On 06/09/14 00:35, John Ralls wrote:
> Found a reliable way to crash with a None type error thanks tohttps://gramps-project.org/bugs/view.php?id=7720, so the attached patch is tested. Unless someone objects I’ll push it in a couple of days.
>
> Crash Procedure:
> Create a new Family Tree and open it.
> Import either data.gramps or example.gramps
> Switch to the Places tab, select the References tab in the bottom-bar, and select a place with references.
> Switch to the Families tab
> Open the Family Tree Manager and delete the open database, then close the manager window.
> Switch back to the Places tab. The references will still be there, though the ListView is now empty.
> Double-click on one of the references. The Unhandled Exception Window comes up.

Yes. This is a well known bug (#2092). We just haven't had the time to
fix it yet.

There are a few things I don't like about the patch:

1. Alerts are logged where None returns are expected. It is valid for
the get_*_from_gramps_id and proxy methods to return None.

2. A single alert in get_from_handle is all that is necessary. When the
handle is None or empty no data will be returned.

3. It is neater to use "is None" rather than "== None" in comparisons.

Is a new logging level really necessary? Perhaps this shouldn't go into
the maintenance branches.

Regards,


Nick.

[John Ralls]

On Sep 7, 2014, at 3:26 AM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 06/09/14 00:35, John Ralls wrote:
>> Found a reliable way to crash with a None type error thanks tohttps://gramps-project.org/bugs/view.php?id=7720, so the attached patch is tested. Unless someone objects I’ll push it in a couple of days.
>>
>> Crash Procedure:
>> Create a new Family Tree and open it.
>> Import either data.gramps or example.gramps
>> Switch to the Places tab, select the References tab in the bottom-bar, and select a place with references.
>> Switch to the Families tab
>> Open the Family Tree Manager and delete the open database, then close the manager window.
>> Switch back to the Places tab. The references will still be there, though the ListView is now empty.
>> Double-click on one of the references. The Unhandled Exception Window comes up.
>
> Yes. This is a well known bug (#2092). We just haven't had the time to
> fix it yet.

More than one bug, I guess, and since 2092 is pretty ancient, obviously not a high priority. Anyway, I repeated the procedure to save others from studying the bug to learn how to crash Gramps easily and repeatably. Crashing is the easiest way to test the patch.

>
> There are a few things I don't like about the patch:
>
> 1. Alerts are logged where None returns are expected. It is valid for
> the get_*_from_gramps_id and proxy methods to return None.
>
> 2. A single alert in get_from_handle is all that is necessary. When the
> handle is None or empty no data will be returned.
>
> 3. It is neater to use "is None" rather than "== None" in comparisons.
>
> Is a new logging level really necessary? Perhaps this shouldn't go into
> the maintenance branches.
>

Nick,

How do you propose for get_from_handle() to know that None returns are expected?

The problem is that there are lots of places where None returns are expected and properly handled, but also lots of places where they aren't. The places where it's not expected require some illumination about why it happened to help decide how None returns should be handled.

Tim demanded that the log messages should not go to the existing rotate handler. You can look back in this thread to see the arguments. The new level is necessary only to accommodate that demand. I don't see that adding a logging level and a handler for it as a reason that it shouldn't be backported.

Regards,
John Ralls

[Nick Hall]

On 07/09/14 15:49, John Ralls wrote:
> How do you propose for get_from_handle() to know that None returns are expected?

I don't think that we need to know.

> The problem is that there are lots of places where None returns are expected and properly handled, but also lots of places where they aren't. The places where it's not expected require some illumination about why it happened to help decide how None returns should be handled.

The get_from_handle method will return None when:

1. It is passed None or an empty string.
2. It is passed a handle as a type test.
3. It is passed a handle from a broken reference.

Unfortunately (1) happens quite frequently. It doesn't actually cause a
problem, but the code could be improved. Most of these problems seem to
be in the gramplets (probably my code).

Case (2) isn't used much. I don't expect to see errors here. Again, we
could improve the code to use the "has" database methods.

Case (3) is a big problem. The broken link could have occurred in a
previous session. Extra logging isn't going to be much help.

The get_*_from_handle methods are designed to return None. However,
they are a common cause of errors when the code is not written to
correctly handle proxies. Fortunately, these bugs are very easy to
track down and fix.

The get_*_from_gramps_id methods probably expect an id given by the
user. I would expect them to check for a None return. I haven't seen
any related bugs.

I suggest a single debug message just before None is returned in the
get_from_handle method.

Nick.

>
> Tim demanded that the log messages should not go to the existing rotate handler. You can look back in this thread to see the arguments. The new level is necessary only to accommodate that demand. I don't see that adding a logging level and a handler for it as a reason that it shouldn't be backported.

[Tim Lyons]

Nick Hall wrote
> The get_from_handle method will return None when:
>
> 1. It is passed None or an empty string.
> 2. It is passed a handle as a type test.
> 3. It is passed a handle from a broken reference.

The trouble is, we don't know which has happened.

It simply isn't good enough to tell the user that he must recreate the
problem, and then re-run with some mysterious additional parameter (debug)
which he probably will have real difficulties in using with the Mac or
Windows AIO. (That is the reason that a debug message is not good enough).

The idea is that when he gets the Nonetype error, there should be enough
diagnostics produced so that we can either fix the problem, or tell the user
that is database is broken, and provide advice as to how to fix it (e.g. run
the check and repair).

Regards,
Tim.



--
View this message in context: http://gramps.1791082.n4.nabble.com/cause-for-NoneType-object-has-no-attribute-tp4666879p4667142.html
Sent from the GRAMPS - Dev mailing list archive at Nabble.com.

[John Ralls]

On Sep 7, 2014, at 11:10 AM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 07/09/14 15:49, John Ralls wrote:
>> How do you propose for get_from_handle() to know that None returns are expected?
>
> I don't think that we need to know.
>
>> The problem is that there are lots of places where None returns are expected and properly handled, but also lots of places where they aren't. The places where it's not expected require some illumination about why it happened to help decide how None returns should be handled.
>
> The get_from_handle method will return None when:
>
> 1. It is passed None or an empty string.
> 2. It is passed a handle as a type test.
> 3. It is passed a handle from a broken reference.
>
> Unfortunately (1) happens quite frequently. It doesn't actually cause a problem, but the code could be improved. Most of these problems seem to be in the gramplets (probably my code).

How do you know it doesn't cause a problem? If the calling function doesn't check the handle and doesn't check the return, it will crash when it tries to dereference the return.

>
> Case (2) isn't used much. I don't expect to see errors here. Again, we could improve the code to use the "has" database methods.

Case 2 is mistated: It should be "It is passed a handle from a table other than the one it is told to look in". Yes, that's sometimes done deliberately as a test, but far more often it's done as an assumption. At least one recent bug was the function got a list of handles without specifying the types, tested if each was a person and assumed it was a family otherwise. That assumption wasn't valid, so it crashed. find_backlink_handles() is called 154 times in gramps, and only 15 of them use a qualifier list to limit the types of handles retrieved.

>
> Case (3) is a big problem. The broken link could have occurred in a previous session. Extra logging isn't going to be much help.

Case 2 and 3 are indistinguishable at present. We know only that the handle isn't present in the table we looked in, not that the handle doesn't exist in any table.

> The get_*_from_handle methods are designed to return None. However, they are a common cause of errors when the code is not written to correctly handle proxies. Fortunately, these bugs are very easy to track down and fix.
>
> The get_*_from_gramps_id methods probably expect an id given by the user. I would expect them to check for a None return. I haven't seen any related bugs.
>
> I suggest a single debug message just before None is returned in the get_from_handle method.

Without specifying why it did so? That *would* be useless.

Regards,
John Ralls

[Nick Hall]

On 08/09/14 00:55, John Ralls wrote:
>> Unfortunately (1) happens quite frequently. It doesn't actually cause a problem, but the code could be improved. Most of these problems seem to be in the gramplets (probably my code).
> How do you know it doesn't cause a problem? If the calling function doesn't check the handle and doesn't check the return, it will crash when it tries to dereference the return.
>

They do check the return. If they didn't the existing error message
would be sufficient, and the bug would be very quick to fix.

Nick.

[Nick Hall]

On 08/09/14 00:55, John Ralls wrote:
>> Case (2) isn't used much. I don't expect to see errors here. Again, we could improve the code to use the "has" database methods.
> Case 2 is mistated: It should be "It is passed a handle from a table other than the one it is told to look in". Yes, that's sometimes done deliberately as a test, but far more often it's done as an assumption. At least one recent bug was the function got a list of handles without specifying the types, tested if each was a person and assumed it was a family otherwise. That assumption wasn't valid, so it crashed. find_backlink_handles() is called 154 times in gramps, and only 15 of them use a qualifier list to limit the types of handles retrieved.
>

These backlinks bugs are also very easy to spot and fix. The latest bug
in the place report was a good example. I just looked at the code
immediately before the error message.


Nick.

[Nick Hall]

On 08/09/14 00:55, John Ralls wrote:
>> Case (3) is a big problem. The broken link could have occurred in a previous session. Extra logging isn't going to be much help.
> Case 2 and 3 are indistinguishable at present. We know only that the handle isn't present in the table we looked in, not that the handle doesn't exist in any table.
>
These are easily distinguishable by looking at the code.

Nick.

[Tim Lyons]

Nick Hall wrote
> On 08/09/14 00:55, John Ralls wrote:
>>> Case (3) is a big problem. The broken link could have occurred in a
>>> previous session. Extra logging isn't going to be much help.
>> Case 2 and 3 are indistinguishable at present. We know only that the
>> handle isn't present in the table we looked in, not that the handle
>> doesn't exist in any table.
>>
> These are easily distinguishable by looking at the code.

Surely not. There might be both a bug in the code and a handle from a broken
reference. You wouldn't know which particularly caused the exception in this
instance.



--
View this message in context: http://gramps.1791082.n4.nabble.com/cause-for-NoneType-object-has-no-attribute-tp4666879p4667155.html
Sent from the GRAMPS - Dev mailing list archive at Nabble.com.

[Nick Hall]

On 08/09/14 14:43, Tim Lyons wrote:
>> These are easily distinguishable by looking at the code.
> Surely not. There might be both a bug in the code and a handle from a broken
> reference. You wouldn't know which particularly caused the exception in this
> instance.

These bugs are easy to fix. Please assign them to me. Most of them
seem to be duplicates of a bug I have already fixed.

I am also aware of the cause of the broken references. I'll put this on
the roadmap for 4.1.1 and fix it.

Nick.

[John Ralls]

On Sep 8, 2014, at 7:29 AM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 08/09/14 14:43, Tim Lyons wrote:
>>> These are easily distinguishable by looking at the code.
>> Surely not. There might be both a bug in the code and a handle from a broken
>> reference. You wouldn't know which particularly caused the exception in this
>> instance.
>
> These bugs are easy to fix. Please assign them to me. Most of them
> seem to be duplicates of a bug I have already fixed.

I started to do that, but there are a lot of them and I don't want to be accused of vandalizing the bug database.

You mean that you consider all NoneType bugs easy to fix and you want all of them assigned to you, right?

Regards,
John Ralls

[Nick Hall]

On 08/09/14 16:09, John Ralls wrote:
>> These bugs are easy to fix. Please assign them to me. Most of them
>> >seem to be duplicates of a bug I have already fixed.
> I started to do that, but there are a lot of them and I don't want to be accused of vandalizing the bug database.
>
> You mean that you consider all NoneType bugs easy to fix and you want all of them assigned to you, right?

Yes. The NoneType bugs where a primary object is expected. There may be
others where the object expected is something else - I haven't looked at
those yet.

Thanks for doing that. I'll then close any duplicates and put the
remaining ones on the roadmap where appropriate.

It would be nice to get all these fixed for the 4.1.1 release.

Nick.

[John Ralls]

On Sep 8, 2014, at 9:20 AM, Nick Hall <nick-***@gramps-project.org> wrote:

> On 08/09/14 16:09, John Ralls wrote:
>>> These bugs are easy to fix. Please assign them to me. Most of them
>>> >seem to be duplicates of a bug I have already fixed.
>> I started to do that, but there are a lot of them and I don't want to be accused of vandalizing the bug database.
>>
>> You mean that you consider all NoneType bugs easy to fix and you want all of them assigned to you, right?
>
> Yes. The NoneType bugs where a primary object is expected. There may be others where the object expected is something else - I haven't looked at those yet.
>
> Thanks for doing that. I'll then close any duplicates and put the remaining ones on the roadmap where appropriate.
>
> It would be nice to get all these fixed for the 4.1.1 release.

Done, I think. 23 in all. Looks like some of them are duplicates of each other and some may be duplicates of others that you've already fixed.

FWIW that accounts for about 1/3 of the NoneType bugs.

Regards,
John Ralls

[Nick Hall]

On 08/09/14 00:55, John Ralls wrote:
>> >I suggest a single debug message just before None is returned in the get_from_handle method.
> Without specifying why it did so? That*would* be useless.
The debug could be useful because it would immediately eliminate proxy
code as the cause.

Nick.

[John Ralls]

On Aug 25, 2014, at 2:25 PM, Tim Lyons <***@gmail.com> wrote:

> Is changing all the code to raise exceptions instead of returning None really
> a good idea?
>
> It seems to me that there are a huge number of places to change - all the
> get functions should be changed, and all the places where they are called.
> This includes plug-ins etc, not just the main-line code.
>
> Just wondering.

Tim,

A valid question.

The get functions are mostly convenience functions, so that’s not so much of a problem. Only a few needed changes. There were more changes to docstrings than to code (including a bunch of get_foo_from_handle where the doctoring said the arg was a GrampsId).

You’re right that those get functions are widely used, but most of them don’t check the return value. If I’m right about the subject errors, then failure to check those return types is the problem, but we don’t know where to look because it isn’t in the stack trace. Yes, we can go through and audit the codebase to find all of the places where the return value isn’t checked, but even if the audit is perfect it will be a temporary fix because someone will come along and write new code that doesn’t check the return value. An unhandled exception about a bad handle removes the mystery, tells us why the exception was raised (handle was None, wrong type, wasn’t found or was private/filtered), and tells us what the handle’s repr was. That’s a lot more clues for debugging the problem and may point straight at the fix.

Can you think of a less invasive way to get there?

Regards,
John Ralls

[Enno Borgsteede]

Tim,
> Is changing all the code to raise exceptions instead of returning None really
> a good idea?
I think it is for all cases where None indicates an error reading data,
or a cached list of handles being corrupted. When illegal (null) types
are found IN the database, if that is possible, an exception might not
be a good idea, because it might get in the way with repair.

We have a large amount of NoneType errors in the bug tracker, which seem
to indicate problems reading data, some of which are very difficult to
reproduce. I want these errors to be caught at the lowest level
possible, because they may indicate corruption of MY data.
> It seems to me that there are a huge number of places to change - all the
> get functions should be changed, and all the places where they are called.
> This includes plug-ins etc, not just the main-line code.
Where these checks indicate corruption of data, exceptions are rare, and
to me it makes no sense to change the calling code for that. That is,
because if corrupted data is found, I prefer that it is not read at all,
so in most cases I want the calling code to be killed.

Of course this does not apply where None is a programmed outcome for an
empty result, where empty results can exist in normal program operation.
That's a total different thing. I want an exception to be used for
errors, and I won't accept those being ignored by sloppy programmers. No
way.

regards,

Enno

[Nick Hall]

On 25/08/14 19:47, John Ralls wrote:
>>> I'm happy with that. The gen/errors.py file already defines a DbError and DatabaseError class.
>> >
>> >I’m not sure either of them are the right one to use, though. DbError says it’s for Berkeley DB errors. Unrecognized handles aren’t likely to be BDB’s fault, so that’s out IMO. DatabaseError says “database errors”. I’m not sure that it qualifies either; there isn’t necessarily anything wrong with the database. I like HandleError, "Error used to report wrong database handle errors”. That seems perfect. Proposed patch for master attached.
>> >
>> >How far back should I do this? All the way to 3.4?
> Looking through the uses of get.*from_handle to add try/except for cases where a None value is actually handled (there aren’t many so far…) I find that there’s another group of functions that can return None, get.*from_gramps_id(). I’m inclined to define a new Exception GrampsIdError and raise that instead of return None from the root of them, __get_obj_from_gramps_id in read.py for all of the same reasons.
>
> Any reason not to?

I missed the HandleError. It is definitely the one to use. A new
GrampsIdError also sounds good to me.

I don't see a problem going back to 3.4.

Nick.

[Jerome]

Enno,

I am not certain that the gramplet still works with gramps 4.1, even
with the right version number set!

I found the old reference[1] which makes me thinking that there were
some changes since 3.2 branch, at least around yield on gramplet
environment..

At a glance, it was an experimental way for updating displayed records,
something pseudo-dynamic!

Today, maybe we should comment the lines around yield() for displaying
records?

[1] https://gramps-project.org/bugs/view.php?id=4295


regards,

JérÎme

Le dim. 24 août 2014 at 18:22, Enno Borgsteede <***@gmail.com> a
écrit :
> I also tried the gramplet mentioned by Jerome in another thread,
> changing its version check to 4.1, and saw no private objects in his
> database.
>
>
> regards,
>
> Enno
>
>
> ------------------------------------------------------------------------------
> Slashdot TV.
> Video for Nerds. Stuff that matters.
> http://tv.slashdot.org/
> _______________________________________________
> Gramps-devel mailing list
> Gramps-***@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/gramps-devel
>

[Jerome]

My bad, the code still works!
But it ignores some primary records, like citations.

The problem is rather that a failure by loading the textual report,
will also skip a proper load for the gramplet too, both on the same
file...

File ".gramps/gramps34/plugins/private/Private.py", line 32, in
<module>
from ReportBase import Report, ReportUtils, MenuReportOptions
ImportError: No module named ReportBase
None
Error loading gramplet 'Private Gramplet': skipping content

A quick fix is maybe to remove the report section (after line 154).



Le dim. 24 août 2014 at 19:22, Jerome <***@yahoo.fr> a écrit :
> Enno,
>
> I am not certain that the gramplet still works with gramps 4.1, even
> with the right version number set!
>
>>
>> ------------------------------------------------------------------------------
>> Slashdot TV.
>> Video for Nerds. Stuff that matters.
>> http://tv.slashdot.org/
>> _______________________________________________
>> Gramps-devel mailing list
>> Gramps-***@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/gramps-devel
>>

[Enno Borgsteede]

Jerome,
> My bad, the code still works!
> But it ignores some primary records, like citations.
>
> The problem is rather that a failure by loading the textual report,
> will also skip a proper load for the gramplet too, both on the same
> file...
>
> File ".gramps/gramps34/plugins/private/Private.py", line 32, in <module>
> from ReportBase import Report, ReportUtils, MenuReportOptions
> ImportError: No module named ReportBase
> None
> Error loading gramplet 'Private Gramplet': skipping content
>
> A quick fix is maybe to remove the report section (after line 154).
Thank you, but there's more than that, I found. It's only visible in
console, but right now I'm too tired to go on with that, because there
is no private object involved. If there was, it would have triggered an
exception like Nick mentioned, but in this database, that was certainly
not the case.

thanks again,

Enno

[Nick Hall]

On 24/08/14 17:22, Enno Borgsteede wrote:
>> Yes. The code is trying to find witnesses for a family event. The
>> >error will occur if a witness is returned as a backlink, but is excluded
>> >by the proxy. Although the backlink handle will be valid, the proxy
>> >will return None. The private proxy has code to avoid this, but the
>> >filter proxy has a FIXME comment in the find_backlink_handles method.
> OK, thanks. I checked reporter's database again, and found no error when
> I export to GEDCOM with the privacy filter checked. I also tried the
> gramplet mentioned by Jerome in another thread, changing its version
> check to 4.1, and saw no private objects in his database

I don't think the problem is with the private proxy, but it may be with
one of the other proxies. Before returning a handle, the
find_backlink_handles method of the private proxy checks to see if the
resultant object exists and is public.

The problem may not be with the proxies at all. We need some more
information from the user.

Nick.

[Enno Borgsteede]

Nick,
> I don't think the problem is with the private proxy, but it may be with
> one of the other proxies. Before returning a handle, the
> find_backlink_handles method of the private proxy checks to see if the
> resultant object exists and is public.
Newbie question: Can you tell what other proxies exist? I have no idea?
Are they listed on the dev wiki somewhere?
> The problem may not be with the proxies at all. We need some more
> information from the user.
I'm waiting for that. I sent the privatized backup to Michel, and he
replied that like me he got no errors, just freezes. This might indicate
that we're tracing a ghost, but given that there are other reports with
NoneType errors where they should normally not appear, I prefer to be sure.

regards,

Enno

[Nick Hall]

On 25/08/14 23:40, Enno Borgsteede wrote:
>> I don't think the problem is with the private proxy, but it may be with
>> >one of the other proxies. Before returning a handle, the
>> >find_backlink_handles method of the private proxy checks to see if the
>> >resultant object exists and is public.
> Newbie question: Can you tell what other proxies exist? I have no idea?
> Are they listed on the dev wiki somewhere?

There are only four of them. Have a look in the gramps/gen/proxy directory.


>> >The problem may not be with the proxies at all. We need some more
>> >information from the user.
> I'm waiting for that. I sent the privatized backup to Michel, and he
> replied that like me he got no errors, just freezes. This might indicate
> that we're tracing a ghost, but given that there are other reports with
> NoneType errors where they should normally not appear, I prefer to be sure.

The other NoneType errors that we often see are when the database closes
unexpectedly. Gramps doesn't work well without an open database.

See:

2092: Problems when no database is open

https://gramps-project.org/bugs/view.php?id=2092


Nick.

[Nick Hall]

On 24/08/14 17:22, Enno Borgsteede wrote:
>> There is also a possible privacy issue with this code. Suppose that
>> >both the person and event are public, but the event reference is
>> >private. The person will be returned as a backlink, but would not be
>> >reachable in the forward direction.
> Hehe, nice. Can I interpret that a design error, or could there be a
> genuine use case behind this? Is it something like not wanting to
> display that a person attended an event?

Exactly. The person and event could be public, but the fact that the
person attended the event is private.

The proxy checks that the person is public when returning backlinks, but
not that event reference is public. The report code doesn't check for
this either.

Nick.