Add assertions and exceptions to upload handling

[uniomni]

GeoNode application at demo.riskinabox.org frequently crashes with stacktrace below.

First step to resolving this would be to introduce assertions to raise Exceptions with good error messages up the food chain.

Traceback:

File "/home/software/riab_env/lib/python2.6/site-packages/django/core/handlers/base.py" in get_response

1. response = callback(request, *callback_args, **callback_kwargs)

File "/home/software/riab/impact/views.py" in calculate

1. user=request.user,

File "/home/software/riab/risiko/utilities.py" in save_to_geonode

6. layer = file_upload(filename, user=user, title=title, overwrite=False)

File "/home/software/geonode/src/GeoNodePy/geonode/maps/utils.py" in file_upload

479. new_layer = save(layer, filename, theuser, overwrite)

File "/home/software/geonode/src/GeoNodePy/geonode/maps/utils.py" in save

353. owner=user,

File "/home/software/riab_env/lib/python2.6/site-packages/django/db/models/manager.py" in get_or_create

1. return self.get_query_set().get_or_create(**kwargs)

File "/home/software/riab_env/lib/python2.6/site-packages/django/db/models/query.py" in get_or_create

373. obj.save(force_insert=True, using=self.db)

File "/home/software/riab_env/lib/python2.6/site-packages/django/db/models/base.py" in save

435. self.save_base(using=using, force_insert=force_insert, force_update=force_update)

File "/home/software/riab_env/lib/python2.6/site-packages/django/db/models/base.py" in save_base

543. created=(not record_exists), raw=raw)

File "/home/software/riab_env/lib/python2.6/site-packages/django/dispatch/dispatcher.py" in send

1. response = receiver(signal=self, sender=sender, **named)

File "/home/software/geonode/src/GeoNodePy/geonode/maps/models.py" in post_save_layer

1. instance.save_to_geonetwork()

File "/home/software/geonode/src/GeoNodePy/geonode/maps/models.py" in save_to_geonetwork

868. md_link = gn.create_from_layer(self)

File "/home/software/geonode/src/GeoNodePy/geonode/geonetwork.py" in create_from_layer

91. self.set_metadata_privs(layer.uuid, {"all": {"view": True}})

File "/home/software/geonode/src/GeoNodePy/geonode/geonetwork.py" in set_metadata_privs

1. data_dbid = doc.find('metadata/{ http://www.fao.org/geonetwork}info/id').text

Exception Type: AttributeError at /api/v1/calculate/

Exception Value: 'NoneType' object has no attribute 'text'

[uniomni]

This is also reported in  https://github.com/AIFDR/riab/issues/47

[sbenthall]

Thanks for the ticket, Ole.

Changing the title of this ticket to clarify its scope. What's actionable at the moment is adding the assertions and exceptions to the upload handling.

Ariel, assigning to you because this rings a bell with that upload handling patch that you are currently working on (does that have a ticket yet?). Would it make sense for you to add these assertions and exceptions in the same patch? If I'm not mistaken, some better exception handling is already intended for that.

[sbenthall]

bumping up to 1.0.x trac as this could go in a point release, but setting to minor priority because it is not release blocking.

[uniomni]

I would have thought potential bugs or instabilities in the fundamental task of uploading a layer would be release blocking - hence the high priority rating. If this is not critical to GeoNode proper then 'minor' is fine although it is 'major' to the Risiko project.

Thanks
Ole

[sbenthall]

Ole,

Forgive me for being direct here, but I need to inform you about some aspects of our open source development process that can be difficult to explain.

There are two reasons why this ticket, for an unreliably reproduced Risiko issue, is an invalid ticket.

First, it does not have steps for reliable reproduction. Generally speaking, tickets for potential bugs are invalid. Tickets for bugs need to include steps for reliably reproducing them. Otherwise, they are not actionable by developers on the project.

(As an aside, a potential bug in GeoNode cannot be a release blocking issue, because otherwise, GeoNode would never have software releases. All software has potential bugs.)

Second, this appears to be a ticket for a Risiko issue, not a GeoNode one. You have only found this bug using Risiko, and have not found the bug in GeoNode. Given that evidence, there is a high probability that the bug has something to do with the Risiko code, and not the GeoNode code. This is not the Risiko issue tracker, so a bug report on Risiko should not go here.

There is a valid request in this ticket though, which is why I have not closed it as 'invalid'. What is valid about this ticket is the request for assertions and exception handling on the upload process. Hence the clarification in scope.

In my own opinion, these assertions and exception handling are welcome but should not be release blocking.

To be clear, labeling a ticket as "minor" in this issue tracker is not an indication of anybody's practical or emotional stake in the ticket. Rather, it is a convention we use to show that a ticket is not release blocking.

At some point, when we are ready to do another point release (I believe the 1.0.1 release is already in the release candidate stage and so will not take further contributions unless a bug is reliably reproduced in it), I hope the PSC or greater community of committers will get together and do a triage of the 1.0.x tickets and figure out which should be considered blocking. But, again by the norms of our development process, by default all new issues coming from the greater community are not release blocking and hence 'minor' priority.

Does this make sense? Since open source software development is a complex, distributed process with by its nature many stakeholders, we need to be methodical and scientific in the way we process reported issues.

[sbenthall]

Given discussion about exception handling as part of Rock Solid, I think we should definitely address this in the next release.

[dwinslow]

I think some further specification is needed here. What assertions and exceptions should be handled?

Steps to re-trigger the unhandled exceptions you are seeing would be useful too.

[ingenieroariel]

Ole's original intentions for this ticket were to suggest stronger checks that would avoid exceptions like: NoneType or XML Syntax errors from making it's way all the way up to the user interface without more information about the component that is failing and why.

In this respect, both GeoServer and GeoNetwork HTML based exceptions (and/or HTTP status codes) should be parsed correctly in the underlying libraries (gsconfig.py, owslib).

David's commit to gsconfig.py [1] is a great step on this direction, how to trigger these kind of errors in GeoNetwork or GeoServer is not really know (and is the reason there are not individual tickets for the problems that have happened).

This ticket is also related to #993 , could be seen as 'giving tools for the end user' to give more meaningful reports for #993.

Ticket #966 for example gives: '-1 aborted' whenever GeoServer times out (or is not accessible). The spirit of that ticket and this one seems to be: 'More explicit messages when things fail so we can one day fix #993'

Ariel.

[1]  https://github.com/dwins/gsconfig.py/commit/1e15c5bd4fbf8f3df057bb782d344ce017e6c74a

[dwinslow]

I think we should close this ticket - a fair amount of work has gone into addressing the issues and "add assertions" is not a task that can really ever be "finished". Better to encourage developers to keep in mind that not everything always succeeds and to file tickets about specific reproducible errors.