Code

Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#2108 closed defect (fixed)

Empty models result in invalid SQL for Sqlite

Reported by: andy@… Owned by: mtredinnick
Component: Database layer (models, ORM) Version:
Severity: normal Keywords: sqlite
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

A model with no fields, e.g.

class EmptyModel(models.Model):

pass

will cause an OperationalError to be raised when an instance is saved:

m = EmptyModel()
m.save()

if the database is sqlite3.
In this case the SQL executed is:

INSERT INTO "testapp_emptymodel" () VALUES ()

It should instead be generating the following SQL:

INSERT INTO "testapp_emptymodel" (id) VALUES (NULL)

Attachments (0)

Change History (7)

comment:1 Changed 8 years ago by mtredinnick

This is indeed a bug, but the proposed solution is not correct. A primary key in a database is unique and not NULL, so trying to set id to be NULL is not a good idea, even if it might happen to work in SQLite. We should be not saving anything in this case.

comment:2 Changed 8 years ago by mtredinnick

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

(In [3104]) Fixed #2108 -- do not try to save an empty model.

comment:3 Changed 8 years ago by andy@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

Inserting NULL into an automatically-incremented field is a common idiom that works in sqlite, mysql, and sql server at least.

Regardless, your solution -- to not save anything -- is wrong. There are cases where a model might have no fields but an id. The one I've got at the moment is a shopping basket, which at the moment does not refer to any other models -- but other models refer to it. If the basket silently fails to be saved, then the other model objects will also fail, all without a clear reason.

Either Django should support tables with just an id field, or it should throw a ValidationError or some such. Silently failing is not good.

comment:4 Changed 8 years ago by mtredinnick

  • Owner changed from adrian to mtredinnick
  • Status changed from reopened to new

I understand your use case now. Sounds reasonable, although a bit of an unusual case.

Inserting NULL in that field is, as a rule, not correct -- it is invalid SQL and it's sort of a shame that MySQL and SQL server permit it. PostgreSQL rejects it. The main problem is SQLite. We should be executing insert into mytable (id) values (DEFAULT) but SQLite does not understand the standard DEFAULT value, so you must use NULL in that case for integer primary keys.

I have made a patch that now inserts the new elements each time and has a backend-specific hook to get the right value to insert (either DEFAULT or NULL, as appropriate). Will commit it in a moment and get somebody on the mailing list to test the SQL Server case.

comment:5 Changed 8 years ago by mtredinnick

  • Status changed from new to assigned

comment:6 Changed 8 years ago by mtredinnick

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

(In [3115]) Fixed #2108 -- allow saving of empty models, rather than just dropping them.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.