search

ckan / ckanext-spatial

Geospatial extension for CKAN
http://docs.ckan.org/projects/ckanext-spatial
125 stars 192 forks source link

Fix shapely deprecation warning #272

Closed mutantsan closed 2 years ago

mutantsan commented 2 years ago

There are some shapely deprecations that related to ckanext-spatial:

  1. asShape is deprecated in favor of shape
  2. geom objects are immutable now
frafra commented 2 years ago

Fix #271. Any idea why the tests fail?

mutantsan commented 2 years ago

I had a problem with testing ckanext-spatial, it doesn't work for me. I'm going to try again, but I see there are a lot of recent commits with dumb comments, and it's hard to understand why do we need all of them.

amercader commented 2 years ago

@mutantsan what do you mean with "recent commits with dumb comments"? is this on this repo, if so in what branch? There's only one failure here and it looks like it could be related to the changes (probably the test needs to be adapted)

frafra commented 2 years ago

@amercader I think it refers to all the "WIP: debug print" and similar commits that are on master.

frafra commented 2 years ago

Ok, I think I found it.

tk.ValidationError does not return an HTTP error code. This is a CKAN bug in my opinion. There is a difference on the error triggered: asShape fails using ValueError providing a meaningful message, while shape fails using AttributeError, which is caught by a generic Exception handler.

ckanext-spatial does not look at the HTTP code, as CKAN does not seem to handle that correctly, so it relies on the output, which is different if a generic Exception is triggered. The test is not generic enough.

This is the output: Spatial: Error creating geometry: Context does not provide geo interface This is what CKAN tests look for: https://github.com/ckan/ckanext-spatial/blob/992b2753fc24d0abb12ced5cf5aaa3a853ca9ea4/ckanext/spatial/tests/functional/test_package.py#L186

There is a test already for checking if Error is included in the reply. Dropping the last assertion seems reasonable to me.

Error handling:

https://github.com/ckan/ckanext-spatial/blob/b54b641c12c6a3a02f2fe1b3f13d5c09b43ca1e6/ckanext/spatial/plugin/__init__.py#L135-L145

Example:

>>> asShape({"Type":"Bad_GeoJSON","a":2})
Traceback (most recent call last):
  File "/usr/lib/ckan/venv/lib/python3.8/site-packages/shapely/geometry/geo.py", line 175, in asShape
    geom_type = ob.get("type").lower()
AttributeError: 'NoneType' object has no attribute 'lower'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/ckan/venv/lib/python3.8/site-packages/shapely/geometry/geo.py", line 177, in asShape
    raise ValueError("Context does not provide geo interface")
ValueError: Context does not provide geo interface
>>> shape({"Type":"Bad_GeoJSON","a":2})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/ckan/venv/lib/python3.8/site-packages/shapely/geometry/geo.py", line 102, in shape
    geom_type = ob.get("type").lower()
AttributeError: 'NoneType' object has no attribute 'lower'
frafra commented 2 years ago

@amercader This PR can be closed, as PR #273 has been merged.