Django

Code

Ticket #8669 (closed: fixed)

Opened 4 months ago

Last modified 4 months ago

All remaining create() methods also force an insert (continuation of r8670)

Reported by: Richard Davies <richard.davies@elastichosts.com> Assigned to: nobody
Milestone: 1.0 Component: Database layer (models, ORM)
Version: SVN Keywords:
Cc: richard.davies@elastichosts.com Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

#8419 / [8670] forced an SQL insert in the main create() and get_or_create() methods, preventing data loss from overwriting existing data - thanks Malcolm!

All other get_or_create() methods end up calling back to the main get_or_create().

However, there are a few extra create() methods which don't call back to the main create() - this patch ensures that those also force an insert, so cannot overwrite existing data.

Extra methods found via "find django -name '*.py' | xargs grep -F 'def create('" and "find django -name '*.py' | xargs grep -F 'def get_or_create('"

Attachments

create-always-inserts.diff (1.9 kB) - added by Richard Davies <richard.davies@elastichosts.com> on 08/29/08 03:16:21.
Fixes for 3 different create() methods
create-always-inserts.v2.diff (1.9 kB) - added by Richard Davies <richard.davies@elastichosts.com> on 08/29/08 12:55:07.
create-always-inserts.v3.diff (1.9 kB) - added by Richard Davies <richard.davies@elastichosts.com> on 08/31/08 03:45:25.
Fix test suite failures (was a silly one line error in last patch)
tests.diff (3.2 kB) - added by Richard Davies <richard.davies@elastichosts.com> on 08/31/08 04:48:24.
Extra tests (actually test changeset 8670, but that's the base of this ticket too)
authors.diff (397 bytes) - added by Richard Davies <richard.davies@elastichosts.com> on 08/31/08 04:53:02.
Have I done enough across various tickets to be in the authors list?
8669_tests.v2.diff (3.4 kB) - added by Richard Davies <richard.davies@elastichosts.com> on 08/31/08 19:32:21.
Have learnt that I got it wrong in #8739. Here's a revised set of tests incorporating that learning. There's also good extra tests in the v2.diffs inside tickets #8739 and #8740, which could also go in already

Change History

08/29/08 03:16:21 changed by Richard Davies <richard.davies@elastichosts.com>

  • attachment create-always-inserts.diff added.

Fixes for 3 different create() methods

08/29/08 11:55:26 changed by mtredinnick

  • needs_better_patch changed.
  • stage changed from Unreviewed to Design decision needed.
  • needs_tests changed.
  • needs_docs changed.

It's not clear that add() should do this. At some point, if you're playing with custom set primary keys, you have to act responsibly and by calling that method you are saying that you want to 'add' that object, which is what happens.

The generic relations one is probably in line with the other changes, but I'm going to think about the first two thirds of the patch a bit.

08/29/08 12:55:07 changed by Richard Davies <richard.davies@elastichosts.com>

  • attachment create-always-inserts.v2.diff added.

08/29/08 12:59:50 changed by Richard Davies <richard.davies@elastichosts.com>

Here's a different (better?) approach - which avoids changing add() and points everyone back to the main create() implementation, rather than maintaining multiple copies.

08/29/08 14:16:42 changed by mtredinnick

  • stage changed from Design decision needed to Ready for checkin.

The second patch is the way to go. It's a little annoying that overriding add() many-to-many now won't get called in the create case, but that's an acceptable trade-off.

I haven't checked that the full test suite passes with this patch yet, but, on the surface, it looks good to go.

08/29/08 19:09:33 changed by mtredinnick

  • needs_better_patch set to 1.
  • stage changed from Ready for checkin to Accepted.

This patch (the second one, which is the one we're considering) causes a lot of failures in the test suite, even with SQLite. They need to be fixed. I'd also like some confirmation that the revised version runs against the full test suite on a database that supports transacations (one of the PostgreSQL backends or Oracle).

Finally, given the scope of the change and the fact that it's already caused some problems, some tests to verify the new functionality would be a good idea.

08/31/08 03:45:25 changed by Richard Davies <richard.davies@elastichosts.com>

  • attachment create-always-inserts.v3.diff added.

Fix test suite failures (was a silly one line error in last patch)

08/31/08 04:48:24 changed by Richard Davies <richard.davies@elastichosts.com>

  • attachment tests.diff added.

Extra tests (actually test changeset 8670, but that's the base of this ticket too)

08/31/08 04:51:42 changed by Richard Davies <richard.davies@elastichosts.com>

Note: tests.diff includes creating an empty file "tests/modeltests/create/_init_.py", which doesn't show up in the web view of the diff

08/31/08 04:53:02 changed by Richard Davies <richard.davies@elastichosts.com>

  • attachment authors.diff added.

Have I done enough across various tickets to be in the authors list?

08/31/08 09:15:16 changed by Richard Davies <richard.davies@elastichosts.com>

The full test suite succeeds on SQLite with all of the above 3 patches (create-always-inserts.v3.diff, tests.diff, authors.diff).

In my (somewhat unusual) PostgreSQL set up, the test suite fails in several ways on unmodified trunk, even without these patches. I'll continue working on this tomorrow, both to post bugs to make it work initially, and also then to verify my actual patches.

08/31/08 18:29:53 changed by Richard Davies <richard.davies@elastichosts.com>

#8739 is a separate bug report against save(force_insert=True) failing on PostgreSQL. Since create() is built on that function, save(force_insert=True) will have to be fixed to get create() working on postgreSQL - all the work in this ticket makes that situation no better and no worse.

08/31/08 19:32:21 changed by Richard Davies <richard.davies@elastichosts.com>

  • attachment 8669_tests.v2.diff added.

Have learnt that I got it wrong in #8739. Here's a revised set of tests incorporating that learning. There's also good extra tests in the v2.diffs inside tickets #8739 and #8740, which could also go in already

09/01/08 11:06:05 changed by Richard Davies <richard.davies@elastichosts.com>

  • needs_better_patch deleted.

OK, I've validated this against the full test suite on both sqlite3 and postgresql_psycopg2. It is ready to check in.

The patches:

- create-always-inserts.v3.diff is the actual fix to the ticket

- authors.diff adds me to the authors list

- I have 3 sets of tests which could reasonably added to the test suite: 8669_tests.v2.diff (above), 8739_save_force_insert.v2.diff (on ticket #8739), 8740_save_force_update.v2.diff (on ticket #8740). In all cases, be careful to create _init_.py files, which the patches don't seem to do for me.

09/02/08 19:09:33 changed by mtredinnick

  • status changed from new to closed.
  • resolution set to fixed.

(In [8884]) Fixed #8669 -- Use a consistent version of create() across the board for model/field instance creation. Based on a patch from Richard Davies.


Add/Change #8669 (All remaining create() methods also force an insert (continuation of r8670))




Change Properties
Action