Exception hides dependency isssue

[yoanisgil]

Hi,

On my first attempt to setup django-rest-framework-gis, I stumble upon an exception thrown here:

https://github.com/djangonauts/django-rest-framework-gis/blob/master/rest_framework_gis/fields.py#L44

So I put together a small example:

>>> import json
>>> t = json.dumps({ "type":  "Point", "coordinates":[45.563237,-73.582419]})
>>> t
'{"type": "Point", "coordinates": [45.563237, -73.582419]}'
>>> GEOSGeometry(t)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/srv/app/.virtualenvs/env/local/lib/python2.7/site-packages/django/contrib/gis/geos/geometry.py", line 64, in __init__
    raise ValueError('Initializing geometry from JSON input requires GDAL.')

which shows that the underlying issue was related to a missing library (python-gdal).

I think it's Ok to return a ValidationError but I think it might make sense to also add the message from the captured exception so users know what went wrong.

What do you think?

[nemesifier]

Hi @yoanisgil, good point. I hope that's the only case in which a ValueError is thrown though. Could you send a patch?

[yoanisgil]

Yes, of course I can send a patch. But I was thinking that it might be a better idea if we just resend the inner exception's message as a way to contextualize de error underneath. What do you think?

[nemesifier]

Not sure is a good idea. Apart from this case is there another case that would make it better for other exceptions?

[yoanisgil]

I cannot say and I think that's precisely the point. None could have possible seen the issue with hiding the real error by capturing ValueError and resending a ValidationError without further contextual information, which in my opinion is the real issue here.

We could break appart and provide a catch block specific to ValueError but are we 100% that this will fix the issue? Like what if the same arrives for say TypeError?

Are we sure that GEOSException, OGRException are symptoms of Invalid format: string or unicode input unrecognized as GeoJSON, WKT EWKT or HEXEWKB.? This is exactly what let me to open an issue here, because I was struggling and googling around for the right way to provide a point information, which was useless because I indeed had the right formatting but not all dependencies were installed.

In my opinion we could do something alone the lines:

        try:
            return GEOSGeometry(value)
        except (ValueError, GEOSException, OGRException, TypeError),e :
            raise ValidationError(_('Unable to convert to python object:  {}'.format(e.message))

In my particular use case that would have save me some valuable time searching around for the wrong answer ;).

[nemesifier]

On 12/10/2015 03:56 PM, Yoanis Gil wrote:

I cannot say and I think that's precisely the point. None could have possible seen the issue with hiding the real error by capturing |ValueError| and resending a |ValidationError| without further contextual information, which in my opinion is the real issue here.

We could break appart and provide a catch block specific to |ValueError| but are we 100% that this will fix the issue? Like what if the same arrives for say |TypeError|?

Are we sure that |GEOSException, OGRException| are symptoms of |Invalid format: string or unicode input unrecognized as GeoJSON, WKT EWKT or HEXEWKB.|?

I believe the answer was to this question was yes||. Although it's time to reconsider.

This is exactly what let me to open an issue here, because I was struggling and googling around for the right way to provide a point information, which was useless because I indeed had the right formatting but not all dependencies were installed.

I totally understand.

In my opinion we could do something alone the lines:

    try:
        return GEOSGeometry(value)
    except (ValueError, GEOSException, OGRException, TypeError),e :
        raise ValidationError(_('Unable to convert to python object: {}'.format(e.message))

In my particular use case that would have same me some valuable time searching around for the wrong answer ;).

I want to be careful with the exception message that is inserted here, because this exception message will be visible when interacting with the API.

I prefer treating "ValueError" and "OGRException" in the way you are proposing while keeping "GEOSException" how it is now.

[yoanisgil]

Ok. Will send you a PR then ;)

[nemesifier]

:+1:

[mafuz]

Please am new in django I spend days trying to get pass this error to no avail when trying to migrate after installing geodjango

ValueError: String input unrecognized as WKT EWKT, and HEXEWKB.