Opened 8 months ago

Closed 8 months ago

Last modified 6 months ago

#36129 closed Bug (invalid)

Geodjango migrations ignore change of dimension in MultiPointField

Reported by: Paweł Kotiuk Owned by:
Component: GIS Version: dev
Severity: Normal Keywords: migrations
Cc: Claude Paroz Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I think I found a bug in GeoDjango migration mechanism.

Reproduction

  1. Create a simple model 
class MultiPointTester(models.Model):
  multipoint = models.MultiPointField(
      dim=2,
      blank=True,
      null=True,
      default="SRID=4326;MULTIPOINT EMPTY",
  )

and test for it

class MultipointTest(TransactionTestCase):
    def test_multipoint_creation(self):
        instance = models.MultiPointTester()
        instance.save()
        self.assertEqual(instance.multipoint.ewkt, "SRID=4326;MULTIPOINT EMPTY")
  1. Generate migration for it ./manage.py makemigrations
  2. Run test - it succeeds ✅
  3. Change dim from 2 to 3 and create migration It contains migration like this (it does not mention dim change): 
      operations = [
      migrations.AlterField(
          model_name='multipointtester',
          name='multipoint',
          field=django.contrib.gis.db.models.fields.MultiPointField(
                blank=True, 
                default='SRID=4326;MULTIPOINT EMPTY', 
                null=True, srid=4326),
      ),
  ]
  1. Run test again - it fails ❌ with error: E ValueError: Cannot alter field test.MultiPointTester.multipoint into test.MultiPointTester.multipoint - they do not properly define db_type (are you using a badly-written custom field?)
  2. Remove all existing migration files and again generate migration ./manage.py makemigrations
  3. Run test again - - it succeeds ✅

Is is a bug, or do I make some kind of mistake in the code?

Attachments (1)

repro_project.zip​ (14.0 KB ) - added by Paweł Kotiuk 6 months ago.

Download all attachments as: .zip

Change History (8)

comment:1 by Paweł Kotiuk, 8 months ago

This bug may linked with

#28809 and #29058

comment:2 by Natalia Bidart, 8 months ago

Cc: Claude Paroz added
Keywords: migrations added
Resolution: → invalid
Status: new → closed
Type: Uncategorized → Bug
Version: 5.1 → dev

Hello Pawel, thank you for this report. I agree this is similar to the tickets you linked and also to #23902. Can you specify what backend are you using?

Based on the previous tickets, and following the comments from Claude, it seems quite challenging to generate a migration that makes sense for the backend, in that how the added dimension would work with respect to the existing values in the DB? For your reproducer specifically, my understanding is that your last step works because the test DB is created with a single and "clean" migration where the dim=3 since the start.

Having said that, I have tried this in a test project and using the admin to create a point when dim=2. Then I changed to dim=3 and the point was migrated in the DB as follows (this is PostGIS): SRID=4326;MULTIPOINT Z ((-0.0051498413085937 0.0168228146996821 0)). But I can't add new points because of the chosen default, which does not have the third dimension:

Exception Type: DataError at /admin/geodjangoapp/multipointtester/add/
Exception Value: Column has Z dimension but geometry does not

So, I think that this specific report is about the default being incorrect for a 3-dimension multipoint (which would consitute an user error), and not so much about migration generation failure, but I'm not a GIS expert. I was going to propose for you to reach our in the forum, but I see you have already ​done so. Because of my local testing, I'll be closing this ticket as invalid following the ​ticket triaging process, but feel free to reopen if you find more specific information detailing that Django is at fault in this specific case, specifically once you remove the potentially incorrect default in place.

Thanks again!

comment:3 by Paweł Kotiuk, 8 months ago

Hello Natalia, thank you for quick response.
Originally I discovered this bug, when I wanted to decrease number of dimensions, and I was surprised that every single unit test in my test suite failed. And generated migration (even for database generated from scratch) always fails with: ValueError: Cannot alter field...

Can you specify what backend are you using?

My database backend is django.contrib.gis.db.backends.spatialite

in that how the added dimension would work with respect to the existing values in the DB?

For now migration breaks entire database, even when it is recreated from scratch only for tests.
I think it should work in the same way as migration for providing new required value for model. So we should at least get a prompt asking about default value we want to use.

it seems quite challenging to generate a migration that makes sense for the backend

In case of migration from 2 to 3 dimensions yes, but in case of migrating from 3 dimensions to 2 it should be rather simple. Just delete Z axis.
I think , that at least this single case should be somehow tackled (and in other cases some warning should be shown).

I think this issue should be reopened, because it causes unexpected problems with django project by generating broken migrations and it should be somehow tackled. Either by providing reliable migration mechanism or at least by showing warning during migration or generating migration files.

comment:4 by Paweł Kotiuk, 8 months ago

find more specific information detailing that Django is at fault in this specific case, specifically once you remove the potentially incorrect default in place.

This problem also exists when there is no default.

You can try to reproduce with simplified model without default (and simpler tests)

model 

class MultiPointTester(models.Model):
    multipoint = models.MultiPointField(
        dim=3,
        blank=True,
        null=True,
    )

test 

class MultipointTest(TransactionTestCase):
    def test_multipoint_creation(self):
        instance = models.MultiPointTester()
        instance.save()

    def test_always_working(self):
        self.assertEqual(1, 1)

Migration from 3 to 2 still breaks every test (even test_always_working)

in reply to:  4 ; comment:5 by Natalia Bidart, 8 months ago

Replying to Paweł Kotiuk:

Migration from 3 to 2 still breaks every test (even test_always_working)

Thank you Paweł for the extra information though sadly I'm not being able to reproduce (though I'm using PostGIS). Can you please try these two things:

  1. Can you use TestCase instead of TransactionTestCase.
  2. Can you try to reproduce using PostGIS?

I tried all sort of migrations from dim 3 to 2, then from 2 to 3, and adding points in the admin always work. Also the test as shown (but using TestCase) always passes for me as well.

Thank you!

by Paweł Kotiuk, 6 months ago

Attachment: repro_project.zip​ added

in reply to:  5 comment:6 by Paweł Kotiuk, 6 months ago

Replying to Natalia Bidart:

  1. Can you use TestCase instead of TransactionTestCase.

Yes, I can. Why do you think it makes a difference?

  1. Can you try to reproduce using PostGIS?

EDIT

No

For PostGIS migration looks fine

model 

class Migration(migrations.Migration):
    dependencies = [
        ("repro", "0001_initial"),
    ]

    operations = [
        migrations.AlterField(
            model_name="multipointtester",
            name="multipoint",
            field=django.contrib.gis.db.models.fields.MultiPointField(
                blank=True,
                default="SRID=4326;MULTIPOINT EMPTY",
                dim=3,
                null=True,
                srid=4326,
            ),
        ),
    ]

So, I think that this specific report is about the default being incorrect for a 3-dimension multipoint (which would consitute an user error), and not so much about migration generation failure

I think you are right about that.

Last edited 6 months ago by Paweł Kotiuk (previous) (diff)

comment:7 by Paweł Kotiuk, 6 months ago

I have simpler reproduction using postGIS this time:

  1. Download repro_project.zip I uploaded and unpack it.
  2. Launch postgis using docker docker run --rm -it -e POSTGRES_PASSWORD=password -p 5432:5432 postgis/postgis
  3. Open terminal and launch commands ./manage.py makemigrations and ./manage.py test -> everything is fine
  4. Change dim from 2 to 3 in the models.py
  5. launch commands ./manage.py makemigrations and ./manage.py test

Anyway, I will check the forum and return with findings if I would have any.

Last edited 6 months ago by Paweł Kotiuk (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top