Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #140 - Convert OrderWrt to IntegerField #142

Merged
merged 1 commit into from
Nov 19, 2014

Conversation

jwhitlock
Copy link
Contributor

An OrderWrt field named _order is added when the Meta.order_with_respect_to option is used on the model. This code:

  • Switches it to an IntegerField in the historical model
  • Ignores changes in ordering (done via update, so save() signals are not generated)
  • Restores the old value when loading from a history_object, which may
    result in non-deterministic ordering until set_RELATED_order() is
    called again.

The ordering could be 'frozen' by calling set_RELATED_order(get_RELATED_order()), but it's not clear that would be better, especially considering multiple restores from history.

See #140 for the full discussion, and PR #141 for an aborted attempt to drop the ordering field in the history model.

An OrderWrt field named _order is added when the
Meta.order_with_respect_to option is used on the model.  This code:
- Switches it to an IntegerField in the historical model
- Restores the old value when loading from a history_object, which may
  result in non-deterministic ordering until set_RELATED_order() is
  called again.
@macro1
Copy link
Collaborator

macro1 commented Nov 18, 2014

_order is a virtual field representing database ordering. It is not a value actually stored in the database. These changes will make _order a real column on the historical model... Which seems wrong.

@jwhitlock I was trying to reproduce the issue in a test project, but I wasn't having any luck. Did you see it in both Django 1.6 and 1.7?

@jwhitlock
Copy link
Contributor Author

No, _order is a real database field, with the 0-based order of the item with respect to the related model.

Up until recently, I've been using Django 1.6 without migrations and SQLite. I switched to PostgreSQL about a month ago, which is strict about nulls and string lengths, while SQLite is pretty loose. In the last few days I switched to Django 1.7 and made my first migrations, which is why I started noticing this. It is possible that South does not exhibit this bug.

I've set up a project to demonstrate the bug:

https://github.com/jwhitlock/dsh-orderwrt-bug

To see the bug:

$ git clone https://github.com/jwhitlock/dsh-orderwrt-bug.git
$ cd dsh-orderwrt-bug
$ pip install -r requirements.txt
$ mkvirtualenv dsh-orderwrt-bug
$ git checkout before_ordered
$ ./manage.py test  # Tests pass
$ ./manage.py migrate
$ sqlite3 db.sqlite3 ".schema sample_serieswork"  # See below
$ git checkout after_ordered
$ ./manage.py test  # Tests fail
...
  File "dsh-orderwrt-bug/lib/python2.7/site-packages/django/db/backends/sqlite3/base.py", line 485, in execute
    return Database.Cursor.execute(self, query, params)
django.db.utils.OperationalError: no such column: sample_historicalserieswork._order
$ ./manage.py migrate # Succeeds
$ sqlite3 db.sqlite3 ".schema sample_serieswork"  # See below

The SQLite schema before ordered_with_respect_to is:

CREATE TABLE "sample_serieswork" (
    "id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, 
    "title" varchar(100) NOT NULL,
    "series_id" integer NOT NULL REFERENCES "sample_series" ("id"));
CREATE INDEX sample_serieswork_a08cee2d ON "sample_serieswork" ("series_id");

After it is added, the schema is:

CREATE TABLE "sample_serieswork" (
    "id" integer NOT NULL PRIMARY KEY AUTOINCREMENT,
     "title" varchar(100) NOT NULL,
     "series_id" integer NOT NULL REFERENCES "sample_series" ("id"),
     "_order" integer NOT NULL);
CREATE INDEX sample_serieswork_a08cee2d ON "sample_serieswork" ("series_id");

@macro1
Copy link
Collaborator

macro1 commented Nov 19, 2014

Interesting... I didn't see it in the migration definitions, and I couldn't think of any other reason to make a special field type for it. I guess this makes sense in that case, though I still think it's odd.

Looking at Model._save_table() it looks like the only time _order gets updated is on insert, and it requires an extra query, so this is the only option we have.

I think I'm OK with what you have here if it solves the issues you've been having. Maybe we should recommend not using order_with_respect_to if there are plans to revert instances often in the documentation.

Thank you for setting up that example project by the way, it helped a lot.

macro1 added a commit that referenced this pull request Nov 19, 2014
Fix #140 - Convert OrderWrt to IntegerField
@macro1 macro1 merged commit 3bf5a96 into jazzband:master Nov 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants