Refs #23804 -- RasterField initialization only if HAS_GDAL

[yellowcap]

The automatic instantiation of GDALRaster instances on field values fails if gdal is not installed.

[claudep]

I don't mind the definition split. But I'm not so enthusiast about conditionally importing some Field. Past personal experiences have shown that this can lead to surprising or hard-to-debug failures.
What about something like this:

diff --git a/django/contrib/gis/db/models/fields.py b/django/contrib/gis/db/models/fields.py
index 5cdb81e..2affd7f 100644
--- a/django/contrib/gis/db/models/fields.py
+++ b/django/contrib/gis/db/models/fields.py
@@ -1,7 +1,7 @@
 from django.contrib.gis import forms
 from django.contrib.gis.db.models.lookups import gis_lookups
 from django.contrib.gis.db.models.proxy import SpatialProxy
-from django.contrib.gis.gdal.raster.source import GDALRaster
+from django.contrib.gis.gdal import HAS_GDAL
 from django.contrib.gis.geometry.backend import Geometry, GeometryException
 from django.core.exceptions import ImproperlyConfigured
 from django.db.models.expressions import Expression
@@ -426,8 +426,12 @@ class RasterField(BaseSpatialField):

     def contribute_to_class(self, cls, name, **kwargs):
         super(RasterField, self).contribute_to_class(cls, name, **kwargs)
-        # Setup for lazy-instantiated Raster object. For large querysets, the
-        # instantiation of all GDALRasters can potentially be expensive. This
-        # delays the instantiation of the objects to the moment of evaluation
-        # of the raster attribute.
-        setattr(cls, self.attname, SpatialProxy(GDALRaster, self))
+        if HAS_GDAL:
+            from django.contrib.gis.gdal.raster.source import GDALRaster
+            # Setup for lazy-instantiated Raster object. For large querysets, the
+            # instantiation of all GDALRasters can potentially be expensive. This
+            # delays the instantiation of the objects to the moment of evaluation
+            # of the raster attribute.
+            setattr(cls, self.attname, SpatialProxy(GDALRaster, self))
+        else:
+            raise ImproperlyConfigured("RasterField requires GDAL.")

[yellowcap]

If conditional imports of fields are a bad idea, then splitting the fields module is not necessary.

So I just updated the pull request with a similar idea based on @claudep 's patch. For the field, GDAL is only required for instantiating the spatial proxy (GDALRaster object). So why not just allow returning the raw data and bypass the lazy instantiation?

[timgraham]

Maybe it would be worth adding a test for the error that runs on systems without GDAL? Also, let's reference the original ticket (#23804) rather than the new one. "Ref -> Refs" in message.

[yellowcap]

I simplified the patch a bit more and tried to add a test for this, but I did not find a way to make HAS_GDAL to be false. I tried overriding the GDAL_LIBRARY_PATH setting, which is used in django.contrib.gis.gdal.libgdal, but that module is loaded before the settings override has an effect. Any ideas on how to run a test where HAS_GDAL is set to be false?

[timgraham]

I was thinking that the test would only run on systems without GDAL installed so there wouldn't be a need to mock HAS_GDAL.

[yellowcap]

Ok, here is a more viable solution. The HAS_GDAL check is in the __init__ function of the RasterField. I added a test for systems without gdal and ran the test on a fresh ubuntu install without geo libraries, which worked fine.

[timgraham]

I think we need to accept *args here too (or at least at verbose_name=None. There's a convention to pass verbose_name as a positional arg: https://docs.djangoproject.com/en/1.8/topics/db/models/#verbose-field-names
Add a test for that too?

Claude, is this approach fine with you?

[claudep]

Yes, sounds good. And +1 for the positional argument. (_args, *_kwargs) doesn't cost much.

[yellowcap]

Added *args to RasterField __init__ and a test for the positional verbose name argument.

[timgraham]

merged in c0fff64, thanks!