Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15304 closed Bug (wontfix)

`Model.objects.create()` returns `long` instead of `int`.

Reported by: mrmachine Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: int long primary key create get
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Some database backends (I've only tested postgresql, sqlite seems OK) seem to return a long PK instead of int when creating new model objects. When getting an existing model object, int is correctly returned. I did find a comment about this in the tutorial, but I don't think this is by design and I think this difference in behaviour between new and existing objects and database backends is buggy and violates the principle of least surprise.

It might be a bit of an edge case, but we were stumped for a while trying to figure out why created objects had a PK of a different type to existing objects. We initially noticed the problem because we use a pickle field to store various types of data -- PKs, dates and times, booleans, dicts, etc. -- and when passing in the value of a newly created PK to the pickle field we were then unable to filter on it unless we explicitly searched for long values.

So to work-around this issue we need to a) know the type of the PK that should be returned for existing objects and coerce it before passing to the pickle field if it does not match.

Attachments (1)

15304-pk-type-r15539.diff (1.9 KB) - added by mrmachine 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by mrmachine

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set

Added a test.

comment:2 Changed 3 years ago by Alex

This test is wrong. There's no guarntee that the objects will have the same identity, in fact, unless the primary key is less than 255 they won't.

comment:3 Changed 3 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

The trick here is that the pk is returned by the INSERT command, but isn't passed through the normal post-processing channels that a SELECT statement is subjected to.

Also -- Alex is correct-- the test isn't accurate. "is" checks identity, not equality. You need to check equality in the type and value of pk, not the identity of pk.

comment:4 Changed 3 years ago by mrmachine

Thanks for the feedback. Updated the test the check the equality of the type of each primary key value.

comment:5 Changed 3 years ago by mrmachine

  • Patch needs improvement unset

Updated the patch to include a fix. I don't think this needs documentation, so I'm removing the "patch needs improvement" flag.

Changed 3 years ago by mrmachine

comment:6 Changed 3 years ago by mrmachine

Actually, there was that reference to some databases returning long integers in an example comment. I've removed that in the latest patch.

comment:7 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 3 years ago by dmclain

  • Easy pickings unset
  • Triage Stage changed from Accepted to Ready for checkin

It looks good to me on sqlite and postgres, I don't have a test setup for other DBs, but given that it's just running the PK through the same output function that is being called on all other fields, it seems unlikely to cause issues.

comment:9 Changed 3 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from new to closed
  • Triage Stage changed from Ready for checkin to Design decision needed
  • UI/UX unset

I don't think this is worth it. Without a compelling case for why code should particularly care about the int/long difference, the extra complexity isn't needed. Numbers are numbers for almost all intents and purposes and who is writing code working with models that cares about the int versus long differentiation.

Should only be reopened with an example of non-trivial code that fails without this change. "I prefer it to be an integer" is not enough.

comment:10 Changed 3 years ago by mrmachine

  • Easy pickings set

This ticket was accepted by a core dev. Work was done on a patch based on feedback received. Why is it now marked wontfix? The ticket description might be lacking a code example, and this might be an edge case, but this is a real world problem in a real project that had us stumped for a few hours.

"we use a pickle field to store various types of data -- PKs, dates and times, booleans, dicts, etc. -- and when passing in the value of a newly created PK to the pickle field we were then unable to filter on it unless we explicitly searched for long values."

Our PickleField.get_prep_value() method converts a python value passed to QuerySet.filter() (or .get(), etc.) into a pickle string so that we can filter on pickle fields. The pickle string for 1 is not the same as 1L, so our filter doesn't return the data we expect. To work-around this problem, we need to determine the type for primary key fields and coerce the value on newly created model objects explicitly in our code before pickling.

The primary key value returned by Model.objects.create() and Model.objects.get() should be the same. It seems to be a clear bug that the primary key value could be different types (and not the type specified in your model class), and I don't see any downside in making it so. How is it "not worth it" to fix? Do I need to include my implementation of PickleField in the tests?

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.