Ticket #132 (closed defect: fixed)

Opened 6 years ago

Last modified 6 years ago

default=datetime.now incorrectly recognized a changed in r231

Reported by: Erik Allik <eallik@…> Owned by: andrew
Priority: critical Milestone: 0.6
Component: commands Version: 0.6-pre
Keywords: startmigration Cc:

Description

After upgrading to r231, startmigration --auto started "recognizing" DateTimeFields that have default=datetime.now as changed.

Right now, this is what the frozen model definition looks like in a migration:

            'created': ('models.DateTimeField', ["_('creation time')"], {'default': 'datetime.datetime(2009, 5, 5, 9, 40, 58, 998995)'}),

Revision 230 has it correct:

            'created': ('models.DateTimeField', ["_('creation time')"], {'default': 'datetime.datetime.now'}),

In one word, South thinks that datetime.now is the same as datetime.now() which it isn't. The problem persists in r234 which is currently the last one.

Change History

comment:1 Changed 6 years ago by Erik Allik <eallik@…>

P.S. Maybe unit testing would avoid bugs like this in the future?

comment:2 follow-up: ↓ 3 Changed 6 years ago by andrew

  • Status changed from new to closed
  • Resolution set to wontfix

This is a side-effect from fixing #128. Unfortunately, the best solution does result in this unfortunate side-effect; I recommend you simply ignore the change, either by removing the alter_column command from the autogenerated migration (this might leave you with an empty one, so wait till you have two changes), or edit the models definition in the previous migration to match.

comment:3 in reply to: ↑ 2 Changed 6 years ago by Erik Allik <eallik@…>

Replying to andrew:

This is a side-effect from fixing #128. Unfortunately, the best solution does result in this unfortunate side-effect; I recommend you simply ignore the change, either by removing the alter_column command from the autogenerated migration (this might leave you with an empty one, so wait till you have two changes), or edit the models definition in the previous migration to match.

To match what? The arbitrarilty chosen datetime value? Is it OK that a totally random datetime is stored in the frozen models? Looks wrong, but I might be wrong here...

comment:4 Changed 6 years ago by andrew

Well, the problem is that we can't really tell what kind of callable you're passing in for the default value. If it's datetime.now, sure, it will work, but if it's a function or method on the model, there's no hope in hell it'll still be around on a frozen model.

I could add a check to see if it's in the same models.py, but that eliminates functions from your project but not in the same file. I could whitelist django and core libs, but that's special casing and I regard it as generally a Bad Thing.

It's annoying, sure (you get bugs like #135), but it's the current approach I have. The whole approach of models dicts has some interesting problems; I'm gonna have to make some proper design decisions at some point (some people have expressed... distaste at the whole idea of storing models like this in the first place). For the moment, though, this is how it works; if you don't like it, stick to 0.5 for now...

comment:5 Changed 6 years ago by Erik Allik <eallik@…>

I still think there's a bug - how do you explain this? The startmigration --auto command is always "recognizing" all DateTimeFields with default=datetime.now as changed:

$ django-admin.py startmigration localsite blabla --auto
 ~ Changed field 'localsite.dish.created'.
 ~ Changed field 'localsite.searchstat.created'.
 ~ Changed field 'localsite.eatingplace.created'.
Created 0034_blabla.py.
$ django-admin.py startmigration localsite blabla --auto
 ~ Changed field 'localsite.dish.created'.
 ~ Changed field 'localsite.searchstat.created'.
 ~ Changed field 'localsite.eatingplace.created'.
Created 0035_blabla.py.$ django-admin.py startmigration localsite blabla --auto
 ~ Changed field 'localsite.dish.created'.
 ~ Changed field 'localsite.searchstat.created'.
 ~ Changed field 'localsite.eatingplace.created'.
Created 0036_blabla.py.

comment:6 Changed 6 years ago by andrew

Yeah, I know, the difficulty is that it's more of a backwards-incompatibility nuisance than a bug. To be honest, I might well stop the code evaluating functions; the alternative hackery seems far too nasty.

comment:7 Changed 6 years ago by andrew

OK, [237] rolls back the change which caused this. It was too much hassle.

comment:8 Changed 6 years ago by Erik Allik <eallik@…>

Not that it's important but you forgot to mark this as fixed.

comment:9 Changed 6 years ago by andrew

  • Status changed from closed to reopened
  • Resolution wontfix deleted

comment:10 Changed 6 years ago by andrew

  • Status changed from reopened to closed
  • Resolution set to fixed

True, it's nice to be consistent.

Note: See TracTickets for help on using tickets.