Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#23940 closed Bug (fixed)

ORM should allow model fields named "exact"

Reported by: zhiyajun11 Owned by: Simon Charette <charette.s@…>
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: zhiyajun11, Yuki Izumi, info+coding@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi guys, when i try to access the related model of a model who has a field named exact by related-manager, a 'exact=true' limit will be add to the sql automatically. As show below:

My models is:

import datetime
from django.db import models
from django.utils import timezone
# Create your models here.

class Question(models.Model):
    question_text = models.CharField(max_length=200)
    #pub_date = models.DateTimeField('date published')
    pub_date = models.IntegerField()
    exact = models.NullBooleanField() 
    #exact = models.IntegerField(default=0)
    def was_published_recently(self):
        return self.pub_date >= timezone.now() - datetime.timedelta(days=1)


class Choice(models.Model):
    question = models.ForeignKey(Question, related_name = 'choices')
    choice_text = models.CharField(max_length=200)
    votes = models.IntegerField(default=0)

The shell command is:

In [1]: from polls.models import Question
 
In [2]: Question.objects.all()
Out[2]: [<Question: Question object>, <Question: Question object>]
 
In [3]: q = Question.objects.get(pk=1)
 
In [4]: q
Out[4]: <Question: Question object>
 
In [5]: q.choices.all()
Out[5]: []
 
In [6]: from django.db import connection
 
In [7]: connection.queries[-1]
Out[7]:
{u'sql': u'QUERY = u'SELECT "polls_choice"."id", "polls_choice"."question_id", "polls_choice"."choice_text", "polls_choice"."votes" FROM "polls_choice" INNER JOIN "polls_question" ON ( "polls_choice"."question_id" = "polls_question"."id" ) WHERE "polls_question"."exact" = %s LIMIT 21' - PARAMS = (True,)',
u'time': u'0.000'}

Ps: this problem also show up on django1.6.

Attachments (1)

tests_ticket23940.patch (942 bytes) - added by Baptiste Mispelon 5 years ago.
Tests for reproducing the issue

Download all attachments as: .zip

Change History (27)

comment:1 Changed 5 years ago by zhiyajun11

Cc: zhiyajun11 added

comment:2 Changed 5 years ago by zhiyajun11

Type: UncategorizedBug

comment:3 Changed 5 years ago by Thomas Chaumeny

Triage Stage: UnreviewedAccepted

comment:4 Changed 5 years ago by Thomas Chaumeny

There are several parts of the ORM that append exact at the end of a filter expression if no other lookup is given (like there https://github.com/django/django/blob/master/django/db/models/sql/query.py#L975).

Should all lookup names be considered as reserved keywords ?

Last edited 5 years ago by Thomas Chaumeny (previous) (diff)

Changed 5 years ago by Baptiste Mispelon

Attachment: tests_ticket23940.patch added

Tests for reproducing the issue

comment:5 Changed 5 years ago by Baptiste Mispelon

Attached a testcase to reproduce the issue.

comment:6 Changed 5 years ago by Yuki Izumi

I'd be interested in trying to put together a patch that achieved this, if there was interest!

comment:7 Changed 5 years ago by Yuki Izumi

Cc: Yuki Izumi added

comment:8 in reply to:  6 Changed 5 years ago by zhiyajun11

Replying to kivikakk:

I'd be interested in trying to put together a patch that achieved this, if there was interest!

what you mean

comment:9 Changed 5 years ago by Tim Graham

Summary: problem when i access related objectDisallow/warn on fields named exact
Type: BugNew feature
Version: 1.7master

I think a system check that gives a warning or error on fields named exact would be appropriate.

comment:10 Changed 5 years ago by Nic West

Owner: changed from nobody to Nic West
Status: newassigned

comment:11 Changed 5 years ago by Nic West

I created a pull request here: https://github.com/django/django/pull/3831

comment:12 Changed 5 years ago by Berker Peksag

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:13 Changed 5 years ago by Markus Holtermann

I was thinking about a more general solution that takes all registered lookups and transforms into account.

       MarkusH | wouldn't it make more sense to raise errors if fields, transforms and/or lookups clash?
collinanderson | MarkusH: no idea. i haven't messed around with custom lookups
       MarkusH | jarshwah_: if you are around ---^
collinanderson | MarkusH: right, i was thinking that, but it's only an issue for primary_key fields, right?
collinanderson | MarkusH: or any field referenced by a foreignkey
       MarkusH | I think it's a general problem
       MarkusH | any field you can possibly reference via relations
       MarkusH | if you have a standalone model, no FK pointing to or from, you should be safe
       MarkusH | collinanderson: so, what do I do with the issue now? Do I set its triage state back to accepted after I dumped my thoughts there?
collinanderson | MarkusH: hah. i'm totally not a triager-expert.
collinanderson | MarkusH: but let's say you have book with fk->author
collinanderson | MarkusH: ohh, i get it. yeah
       MarkusH | ok
collinanderson | MarkusH: ohh, see, you could do either book_author_id__gte=2
collinanderson | MarkusH: or book__author__id__gte=2
collinanderson | MarkusH: both of those would work with a field on author named "gte"
collinanderson | right?
       MarkusH | yes
collinanderson | and django doesn't really do that internally at all, but id _does_ do __exact a bunch
collinanderson | right? (at least this ^^ is my assumption)
       MarkusH | but what about book__author__foo='bar' (where foo can be a field, transform or lookup) do?
collinanderson | right, and in that case it should be a field, not a transform or lookup
       MarkusH | afaik, if no lookup is given, exact is assumed
collinanderson | seems to me you if you wanted a transform/lookup in that case you could just do book__author__pk__foo='bar'
collinanderson | or book__author_id__foo='bar'
       MarkusH | ahh, right
       MarkusH | thanks
collinanderson | and maybe it's possible to change django so it allows for a field named "exact" in the same way
       MarkusH | hang on
collinanderson | (i'm saying this all in theory, i haven't actually tried any of this :)
       MarkusH | the test case attached to the PR has a model with a field "exact"
collinanderson | and i assume the bad news is it uses the lookup/transform instead of the field?
       MarkusH | ok. The PR is about preventing certain problems by not allowing them to happen
       MarkusH | it specializes the exact lookup
       MarkusH | it isn't about a way to find another way to access a field
       MarkusH | so, in practice, every lookup and transform should prevent a field of that name

comment:14 Changed 5 years ago by Collin Anderson

Marcus and I were talking on IRC about this.

  • Is this issue specific to exact? Are _all_ lookups a problem? Looking at the code it's probably an issue for iexact and isnull also.
  • Would it be possible to support having a field named "exact"?

comment:15 Changed 5 years ago by Josh Smeaton

I think we should try to fix this rather than add a warning. It should be possible to first check if the "part" is a field and use that if so.

.filter('field__exact__exact')

For each part ("field", "exact", "exact"):

try:
    get_field(part)
except:
    try:
        get_transform(part)
    except:
        get_lookup(part)

There's already similar code to this that first tries the transform then falls back to lookup. We should just need to introduce similar logic that first checks if a part is actually a field.

comment:16 Changed 5 years ago by Markus Holtermann

Cc: info+coding@… added
Triage Stage: Ready for checkinAccepted

Delaying the patch until further investigation

comment:17 Changed 5 years ago by Tim Graham

Easy pickings: unset
Has patch: unset
Summary: Disallow/warn on fields named exactORM should allow model fields named "exact"
Type: New featureBug

comment:18 Changed 5 years ago by Nic West

Owner: Nic West deleted
Status: assignednew

comment:19 Changed 5 years ago by Josh Smeaton

After doing some investigation with user phood on IRC, the problem seems local to fields called exact only. We traced the problem to:

https://github.com/django/django/blob/d450af8a2687ca2e90a8790eb567f9a25ebce85b/django/db/models/fields/related.py#L684

The related manager adds a filter called model__exact to fetch the correct related record. Unfortunately, other parts of the code interpret this as targeting the field exact rather than an exact match for the model (usually pk).

Changing:

self.core_filters = {'%s__exact' % rel_field.name: instance}  # old 
self.core_filters = {'%s__pk' % rel_field.name: instance.pk}  # new

Fixes the this problem, but breaks the test: foreign_object.tests.MultiColumnFKTests.test_reverse_query_returns_correct_result

So, still some work to do here. If there are any multi column fk experts around, it'd be good to hear from you.

comment:20 Changed 5 years ago by Simon Charette

Removing the explicit __exact lookup seems to work.

comment:21 Changed 5 years ago by Simon Charette

I suggest we open a new ticket to add field/lookup/transform collisions checks and add a test to my PR to make sure the reported issue doesn't regress.

Thoughts?

comment:22 Changed 5 years ago by Simon Charette

Has patch: set

comment:23 Changed 5 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:24 Changed 5 years ago by Simon Charette

The ticket for tracking mode field names, lookups and transforms collision is #24246.

comment:25 Changed 5 years ago by Simon Charette <charette.s@…>

Owner: set to Simon Charette <charette.s@…>
Resolution: fixed
Status: newclosed

In eb4cdfbdd64a95b303eaaa40a070521aa58362fd:

Fixed #23940 -- Allowed model fields to be named exact.

An explicit __exact lookup in the related managers filters
was interpreted as a reference to a foreign exact field.

Thanks to Trac alias zhiyajun11 for the report, Josh for the investigation,
Loïc for the test name and Tim for the review.

comment:26 Changed 5 years ago by Simon Charette <charette.s@…>

In a301061f88268255fc2660ca993a84b8bf12f9d2:

[1.8.x] Fixed #23940 -- Allowed model fields to be named exact.

An explicit __exact lookup in the related managers filters
was interpreted as a reference to a foreign exact field.

Thanks to Trac alias zhiyajun11 for the report, Josh for the investigation,
Loïc for the test name and Tim for the review.

Backport of eb4cdfbdd64a95b303eaaa40a070521aa58362fd from master

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