Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#22001 closed Bug (fixed)

GeoDjango (contrib.gis) does not create geometry columns in 1.7a2

Reported by: Stefano Costa Owned by: Eric Palakovich Carr
Component: GIS Version: dev
Severity: Release blocker Keywords:
Cc: Eric Palakovich Carr, django@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I am testing Django 1.7a2 on a very small project+app.

So far, with both PostGIS and SpatiaLite I have found the following problem. Basically the geometry column is included in the migration.py file, but not in the SQL migration.

models.py:

from django.contrib.gis.db import models

# Create your models here.

class Location(models.Model):
    name = models.CharField(max_length=100)
    geom = models.PointField(srid=2100)
    objects = models.GeoManager()

    def __unicode__(self):
        return self.name

0001 myapp/0001_initial.py:

# encoding: utf8
from django.db import models, migrations
import django.contrib.gis.db.models.fields


class Migration(migrations.Migration):

    dependencies = [
    ]

    operations = [
        migrations.CreateModel(
            name='Location',
            fields=[
                (u'id', models.AutoField(verbose_name=u'ID', serialize=False, auto_created=True, primary_key=True)),
                ('name', models.CharField(max_length=100)),
                ('geom', django.contrib.gis.db.models.fields.PointField(srid=2100)),
            ],
            options={
            },
            bases=(models.Model,),
        ),
    ]

Output from sqlmigrate:

(venv)[steko@ogma myproject]$ python manage.py sqlmigrate myapp 0001
CREATE TABLE "myapp_location" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "name" varchar(100) NOT NULL);

This happens both with PostGIS and SpatiaLite. Perhaps obvious: it works well with Django 1.6. My current system is Fedora 20.

Change History (36)

comment:1 by Stefano Costa, 11 years ago

Something went wrong with my submission: apologies.

sqlmigrate output with PostGIS (index but no column):

CREATE TABLE "myapp_location" ("id" serial NOT NULL PRIMARY KEY, "name" varchar(100) NOT NULL);
CREATE INDEX "myapp_location_geom_id" ON "myapp_location" USING GIST ("geom" );

Forgot to add that I am using Python 2.7.5 and Django is installed in a virtualenv.

Last edited 11 years ago by Stefano Costa (previous) (diff)

comment:2 by Marc Tamlyn, 11 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

I haven't verified this, but assuming it is the case then it's definitely a release blocker.

comment:3 by anonymous, 11 years ago

I also ran into this problem with a project. The root of the problem comes from the data_types dict inside postgres and sqlite's DatabaseCreation classes. This dict appears to be responsible for matching a field defined in the model to it's proper SQL statement:

# django/db/backends/postgresql_psycopg2/creation.py
data_types = {
    'AutoField': 'serial',
    'BinaryField': 'bytea',
    'BooleanField': 'boolean',
    'CharField': 'varchar(%(max_length)s)',
    # ... snip ...
    'TimeField': 'time',
}

The DatabaseCreation class at django/db/backends/sqlite3/creation.py is suffers from the same problem, which is none of the GeometryFields are defined in this dict. It could look something like this if it was fixed:

# django/db/backends/postgresql_psycopg2/creation.py
data_types = {
    'AutoField': 'serial',
    'BinaryField': 'bytea',
    'BooleanField': 'boolean',
    'CharField': 'varchar(%(max_length)s)',
    # ... snip ...
    'TimeField': 'time',
    # Add an entry for each Geometry field in Geodjango
    'PointField': 'geometry(POINT,%(srid))'
    'LineStringField': 'geometry(LINESTRING,%(srid))'
    # ... etc ...
}

That appears to be the fix from my brief look over the code. But I worry that I'm missing something.

comment:4 by Eric Palakovich Carr, 11 years ago

Oh, and I made the above comment. I forgot to login.

comment:5 by Akis Kesoglou, 11 years ago

FWIW, #21909 is related to this ticket, which I opened and then closed as invalid as I couldn't reproduce it anymore. The attached test project might save someone a couple of minutes.

in reply to:  3 comment:6 by Stefano Costa, 11 years ago

Replying to anonymous:

I also ran into this problem with a project. The root of the problem comes from the data_types dict inside postgres and sqlite's DatabaseCreation classes.

So given this is specific to contrib.gis probably the patch should be in django/contrib/gis/db/backends/spatialite/creation.py and django/contrib/gis/db/backends/postgis/creation.py respectively, I think?

comment:7 by Eric Palakovich Carr, 11 years ago

Cc: Eric Palakovich Carr added
Owner: changed from nobody to Eric Palakovich Carr
Status: newassigned

comment:8 by Eric Palakovich Carr, 11 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

Ok, I've submitted a pull request for this ticket:

https://github.com/django/django/pull/2366

I have a few things I'm worried about:

  • I duplicated some supporting test code from django/tests/migrations to django/django/contrib/gis/tests/migrations.
  • I don't have any tests to make sure changing special geo-specific attributes for fields like sridand geography create a useful migration for the changed field.
  • I only ran the test I added against postgis.

Hopefully those wiser then me can chime in and set me straight. This is my first pull request for Django, so I'm certainly open to improvements.

Also, I set the triage stage as Ready for checkin and Has patch to True. I hope that's correct.

comment:9 by Baptiste Mispelon, 11 years ago

Triage Stage: Ready for checkinAccepted

Hi,

The ready for checkin status is for when someone has reviewed your patch and deemed it ready for a core dev to check it in.
Using the has flag checkbox is enough to indicate you've provided a patch (or in this case, a pull request).

THere's more information on our documentation if you're interested: https://docs.djangoproject.com/en/1.6/internals/contributing/triaging-tickets/

Thanks.

comment:10 by Stefano Costa, 11 years ago

I am not 100% sure this is the right place to raise the issue but it seems definitely related to the one that I originally reported. The sqlclear output for the same app is (perhaps obviously) missing the necessary DELETE FROM geometry_columns WHERE f_table_name = mytable; so even if the geometry column was created correctly (as happens in 1.6) clearing the database would not work as expected. I don't know if your proposed patch fixes this other issue as well.

in reply to:  10 comment:11 by Eric Palakovich Carr, 11 years ago

Replying to steko:

I am not 100% sure this is the right place to raise the issue but it seems definitely related to the one that I originally reported. The sqlclear output for the same app is (perhaps obviously) missing the necessary DELETE FROM geometry_columns WHERE f_table_name = mytable; so even if the geometry column was created correctly (as happens in 1.6) clearing the database would not work as expected. I don't know if your proposed patch fixes this other issue as well.

It "should" fix that problem too. But I'll add a test or two for altering migrations (e.g. deleting a geometry field in a model).

Version 0, edited 11 years ago by Eric Palakovich Carr (next)

comment:12 by Andrew Godwin, 11 years ago

Owner: changed from Eric Palakovich Carr to Andrew Godwin

comment:13 by Shai Berger, 11 years ago

I Left some comments on the PR; at this time, it is not yet RFC.

comment:14 by Eric Palakovich Carr, 11 years ago

Thanks, shai. I'll try and address your comments tonight, assuming I haven't been booted off the ticket.

Given Andrew has taken ownership of the ticket, does that mean I should stop? I'm eager to contribute something to Django (particularly if it's related to Geodjango), but I understand if Andrew wants to take care of a blocker himself.

comment:15 by Shai Berger, 11 years ago

I believe Andrew intended to imply that he's taking responsibility for the ticket, being a blocker in the migrations. I'd expect that, if he thought your whole approach is wrong, he'd say so. I'm sure he'll appreciate all the help he can get.

comment:16 by Andrew Godwin, 11 years ago

Shai is right, I just claimed it due to lack of activity and wanting to put it on my to do list. Please reclaim it if you want to keep going!

comment:17 by Eric Palakovich Carr, 11 years ago

Owner: changed from Andrew Godwin to Eric Palakovich Carr

Well alright then. Yoink!

comment:18 by Shai Berger, 11 years ago

Needs tests: set
Patch needs improvement: set

#22260 (just opened) appears to be related.

comment:19 by Daniel Hahler, 11 years ago

@andrewgodwin: with #22260 the issue is that db_type is not called, but only db_parameters, and its get_internal_type return value is not present in the lookup table (with django-durationfield).

Here is a proposed pull request: https://github.com/django/django/pull/2425

comment:20 by Daniel Hahler, 11 years ago

It seems like @andrewgodwin changed the behavior of db_type in https://github.com/django/django/commit/ca9c3cd.

It is unclear if the CHECK (%s) suffix added there is used and was made intentionally, since calls to db_type have been replaced with calls to db_parameters.

comment:21 by Eric Palakovich Carr, 11 years ago

Ok, I added in @blueyed's code into my pull request. I haven't added any more tests yet though. Currently there's one test added to the gis contrib app that checks the Creation operation forwards and backwards with GeometryField fields.

I'm happy to add more, but the test I've added seems to cover the error in this ticket (i.e. fails without the fix). And the changes from @blueyed causes the test to pass, so yay?

Let me know what else needs to be done.

Last edited 11 years ago by Eric Palakovich Carr (previous) (diff)

comment:22 by Daniel Hahler, 11 years ago

Cc: django@… added

The documentation should probably get verified/clarified:

The :meth:.db_type method is called by Django when the framework
constructs the CREATE TABLE statements for your application -- that is,
when you first create your tables. It is also called when constructing a
WHERE clause that includes the model field -- that is, when you retrieve data
using QuerySet methods like get(), filter(), and exclude() and have
the model field as an argument. It's not called at any other time, so it can afford to
execute slightly complex code, such as the connection.settings_dict check in
the above example.

comment:23 by Eric Palakovich Carr, 11 years ago

Needs tests: unset

comment:24 by Eric Palakovich Carr, 11 years ago

Needs documentation: set

comment:25 by Marc Tamlyn <marc.tamlyn@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In d22b291890c1736a40c0ad97448c7318df2eebb2:

Fixed #22001 -- Ensure db_type is respected.

db_parameters should respect an already existing db_type method and
return that as its type string. In particular, this was causing some
fields from gis to not be generated.

Thanks to @bigsassy and @blueyed for their work on the patch.

Also fixed #22260

comment:26 by Tim Graham, 11 years ago

Has patch: unset
Needs documentation: unset
Patch needs improvement: unset
Resolution: fixed
Status: closednew

This broke the build and the follow up commit 834d78ffc3a7eb62d55ac269f83ac0e7473662ea did not resolve all the issues as there are still many errors when running the GIS tests.

comment:27 by Eric Palakovich Carr, 11 years ago

Ugh, I'm sorry for breaking the build. I'm trying to resolve the errors reported in Jenkins.

I'm running into one problem, though. My test I added is breaking when ran against MySQL. I think I've now resolved the problem (pull request coming), and I'm now trying to run the test using MySQL for the database and I'm getting an error:

Failed to install index for distapp.SouthTexasZipcode model: (1252, 'All parts of a SPATIAL index must be NOT NULL')

That's coming from this model:

class SouthTexasZipcode(NamedModel):
    "Model for a few South Texas ZIP codes."
    poly = models.PolygonField(srid=32140, null=True)

I'm new to using mysql as a GIS (I've always used PostGIS), but from what I'm reading MySQL spatial fields must ALWAYS be declared with NOT NULL. This would mean the null=True is the cause of the problem here.

Which means there's always been a problem. Which is impossible. So what am I doing wrong? Here's how I have the settings file setup for running the tests:

DATABASES = {
    'default': {
        'ENGINE': 'django.contrib.gis.db.backends.mysql',
        'NAME': 'django_default',
        'USER': '<insert user>',
        'PASSWORD': '<insert password>',
        'OPTIONS': {
            'init_command': 'SET storage_engine=MyISAM',
        }
    },
    'other': {
        'ENGINE': 'django.contrib.gis.db.backends.mysql',
        'NAME': 'django_other',
        'USER': '<insert user>',
        'PASSWORD': '<insert password>',
        'OPTIONS': {
            'init_command': 'SET storage_engine=MyISAM',
        }
    }
}

SECRET_KEY = "django_tests_secret_key"

# Use a fast hasher to speed up tests.
PASSWORD_HASHERS = (
    'django.contrib.auth.hashers.MD5PasswordHasher',
)

Last edited 11 years ago by Eric Palakovich Carr (previous) (diff)

comment:28 by Tim Graham, 11 years ago

The warning on Jenkins (from a build that passed) is: Failed to install index for distapp.SouthTexasZipcode model: (1464, "The used table type doesn't support SPATIAL indexes").

I guess Jenkins uses InnoDB rather than MyISAM. Does the warning you encountered cause any errors/failures or prevent the tests from running?

comment:29 by Eric Palakovich Carr, 11 years ago

Yes, the error I receive prevents the tests from running. I should note I had received the error you mentioned at first, due to using InnoDB. Upon switching the storage engine to MyISAM (see settings file above), those errors were replaced with the Failed to install index for distapp.SouthTexasZipcode model: (1252, 'All parts of a SPATIAL index must be NOT NULL') error.

I'll try and resolve these issues tonight, particularly looking at how Jenkins is configured when running these tests. It still seems weird that these tests weren't failing earlier, and what would have changed to cause this to start happening now.

comment:30 by Tim Graham, 11 years ago

I guess you should use InnoDB like Jenkins does for now. Florian pushed some fixes earlier and now there's only an error about BLOB/TEXT column 'name' used in key specification without a key length for MySQL GIS.

comment:31 by Eric Palakovich Carr, 11 years ago

Ah, good. THAT I know exactly what the problem is, and can fix right away. I'll have something ready in a little bit here.

comment:32 by Eric Palakovich Carr, 11 years ago

Ok, I created a pull request to fix that bug.

https://github.com/django/django/pull/2444

I'm still having issues getting these tests to run ing mysql, but it turns out the problem I posted above isn't the issue. Those were actually warnings, and appear in the Jenkins console log for the build you just posted. My error is actually this:

django.db.utils.ProgrammingError: (1064, "You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '%s ORDER BY `django_content_type`.`name` ASC' at line 1")

Does that look familiar? I'm still looking into what I may be doing wrong.

comment:33 by Eric Palakovich Carr, 11 years ago

Ok, I got it working on python 2.7 (and the regression is fixed with the PR above).

I had been running the tests earlier in python 3.2 on my ubuntu box here. I'm thinking it may be a problem with how I built the MySQL-python library for it. Anyway, the pull request above should resolve the failing test on MySQL.

comment:34 by Tim Graham <timograham@…>, 11 years ago

In c11b9346d4ebc10d8d98945a974016a5e85f25e6:

Fixed migration so MySQL GIS test doesn't fail; refs #22001.

The test previously failed with the error:
(1170, "BLOB/TEXT column 'name' used in key specification without a key length")

comment:35 by Tim Graham, 11 years ago

Resolution: fixed
Status: newclosed

Jenkins is back to green!

comment:36 by Eric Palakovich Carr, 11 years ago

Great! Thanks for your patience, everyone :)

Note: See TracTickets for help on using tickets.
Back to Top