Opened 8 years ago

Closed 4 years ago

#5535 closed New feature (fixed)

.get() should allow myforeignkey_id lookups

Reported by: dcramer Owned by: mtredinnick
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: rkm, simon29 Triage Stage: Ready for checkin
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description (last modified by gwilson)

To allow easy accessibility as you can with .create on foreignkey_id calls, .get should allow the same.

e.g.

MyModel.objects.create(author_id=1) # Works
MyModel.objects.create(author=Author(id=1)) # Works
MyModel.objects.get(author_id=1) # Doesn't Work
MyModel.objects.get(author=Author(id=1)) # Works
MyModel.objects.get_or_create(author_id=1) # Doesn't Work

Attachments (2)

5535.1.diff (3.5 KB) - added by koniiiik 4 years ago.
5535.2.diff (4.3 KB) - added by koniiiik 4 years ago.
Patch with documentation

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by ubernostrum

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

This feels like it's trading consistency of lookup syntax (always use double-underscore) for gains in edge cases involving major performance constraints. I'm not sure that's the wisest course for Django to take.

comment:2 follow-up: Changed 8 years ago by ubernostrum

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

Also, on further reflection: your point here seems to be entirely about get_or_create, which takes the keyword argument defaults and will accept the "_id" syntax there, so you can already do this.

comment:3 Changed 8 years ago by anonymous

  • Resolution worksforme deleted
  • Status changed from closed to reopened

This isn't just about get_or_create - you can't use fkeyname_id in .get() at all:

Model:

class A(models.Model):
    name = models.CharField(maxlength=30)
    fkey = models.ForeignKey('B', null=True)

Query:

>>> A.objects.get(fkey_id=1)
TypeError: Cannot resolve keyword 'fkey_id' into field. Choices are: id, name, fkey

comment:4 in reply to: ↑ 2 Changed 8 years ago by dcramer

Replying to ubernostrum:

Also, on further reflection: your point here seems to be entirely about get_or_create, which takes the keyword argument defaults and will accept the "_id" syntax there, so you can already do this.

The problem is you can't use it outside of defaults. And that's where .get() itself comes into play.

comment:5 Changed 8 years ago by anonymous

Shouldn't that be fkey__id=1 (with two underscores)?

comment:6 Changed 8 years ago by dcramer

You can use id on .get(), that's not the problem. The problem is you cannot use _id, as the actual field is myforeignkey_id by default, and works fine with .create()

comment:7 Changed 8 years ago by ubernostrum

I still don't see why the lookup syntax needs to be made inconsistent just for sake of an implementation detail of foreign keys.

comment:8 Changed 8 years ago by dcramer

It's exactly about consistency. You can use myfield_id on creation queries, why cant you use it on get queries?

comment:9 Changed 8 years ago by ubernostrum

First off, the syntax we have right now is consistent. Any time you're working with a relation, you use a double underscore. It is true that there's an undocumented attribute on models which stores the raw value of a related object, and that this attribute has a single underscore in its name, but the Django ORM has never exposed the ability to perform lookups on arbitrary model attributes, even when those attributes spring from things in the database -- lookups are enabled for the names of the model's fields, and nothing else -- so it's hard to argue for any inconsistency existing in our not exposing lookups on one such attribute.

Second, I can't honestly see what the gain is here or what problem it would solve, other than saving a single character's worth of typing. The get_or_create() use case has already been shown not to apply because you can already do what you want with get_or_create().

So since it'd make the ORM lookup syntax inconsistent, and since no real-world problem has been advanced which would be solved by doing so, I'd be a strong -1.

comment:10 Changed 8 years ago by ubernostrum

In the above, "stores the raw value of a related object" should be "stores the raw primary-key value of a related object". Silly typo.

comment:11 Changed 8 years ago by dcramer

The benefit is you don't need an instance of the object to call it.

If you use .create() you need an instance, unless you pass it as !_id, if you use .get you don't need an instance.

If you use .get_or_create, you are forced to have an instance as .create requires it, and .get doesn't handle !_id.

A dirty hack is saying MyModel.objects.get_or_create(foreignkey=SomeModel(id=5)) but it's quite unneeded and could cause problems w/ the shared memory patch.

comment:12 Changed 8 years ago by ubernostrum

If you're back to create and get_or_create, then again this is a "worksforme": you can use the _id syntax just fine there, and I don't know why you keep trying to bring that up. In create you can pass it directly, and in get_or_create you can pass it in defaults. And there's nowhere else you need an instance, because the __id syntax for lookups works just fine with the actual primary-key value.

So, again: where's the gain? What can't you do right now that you could do if we changed the lookup syntax?

comment:13 Changed 8 years ago by gwilson

  • Description modified (diff)

fixed description formatting.

comment:15 Changed 5 years ago by simon29

  • Cc rkm simon29 added

Old open ticket but still no decision here.

Convention is that myfk's raw value equivalent is myfk_id.

IMHO this is handy and should be implemented across the board, ie filter(), get(), and get_or_create(); not just for create().

In particular, for filter(), since the superfluous join is performance penalty enough to make it useless in many situations.

If we can reach a consensus I'll supply the patch with docs and tests.

comment:16 Changed 5 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick
  • Status changed from reopened to new
  • Triage Stage changed from Design decision needed to Accepted

Tentatively based on experience over the last period since 1.0. Dependent upon the internal implementation not reintroducing a measurable performance degradation (which is a reason _id version was removed in queryset-refactor.

comment:17 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to New feature

Changed 4 years ago by koniiiik

comment:18 Changed 4 years ago by koniiiik

  • Easy pickings unset
  • Has patch set
  • Needs documentation set

The attached patch should make this feature available. Basically, the required code has been in for quite a while but disabled.

comment:19 Changed 4 years ago by jacob

  • Triage Stage changed from Accepted to Ready for checkin

Probably needs a bit of documentation, most likely in the model reference. Probably somewhere under the docs for get()?

Changed 4 years ago by koniiiik

Patch with documentation

comment:20 Changed 4 years ago by jezdez

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

In [16473]:

Fixed #5535 -- Allow using an explicit foreign key in get() calls. Thanks, Michal Petrucha.

Note: See TracTickets for help on using tickets.
Back to Top