Opened 12 years ago

Closed 12 years ago

#17497 closed Cleanup/optimization (fixed)

Confusing exception message when using values_list with relations

Reported by: Jonas Obrist Owned by: antoviaque
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: ojiidotch@…, antoviaque 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

Assuming following models:

from django.db import models


class Reporter(models.Model):
    first_name = models.CharField(max_length=30)
    last_name = models.CharField(max_length=30)
    email = models.EmailField()

    def __unicode__(self):
        return u"%s %s" % (self.first_name, self.last_name)

class Article(models.Model):
    headline = models.CharField(max_length=100)
    pub_date = models.DateField()
    reporter = models.ForeignKey(Reporter)

    def __unicode__(self):
        return self.headline

    class Meta:
        ordering = ('headline',)

Running this query:

Article.objects.values_list('reporter__notafield')

Will report this error message:

"Cannot resolve keyword 'reporter__notafield' into field. Choices are: headline, id, pub_date, reporter"

This might look as if values_list only allows fields on a model and not relations. I propose the error message to change to something like "Cannot resolve keyword 'notafield' into field. Choices are 'article', 'email', 'first_name', 'id', 'last_name'".

I've attached a simple patch with a test case.

Attachments (2)

170988ee0f93c74d7c21661663940c5fc7e6928b.patch (1.3 KB ) - added by Jonas Obrist 12 years ago.
test case
valueslisterrormessage.diff (2.6 KB ) - added by antoviaque 12 years ago.

Download all attachments as: .zip

Change History (9)

by Jonas Obrist, 12 years ago

test case

comment:1 by Jonas Obrist, 12 years ago

Cc: ojiidotch@… added

comment:2 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:3 by antoviaque, 12 years ago

Cc: antoviaque added
Owner: changed from nobody to antoviaque

comment:4 by antoviaque, 12 years ago

Has patch: set

Cf attached file for a first attempt at fixing this. Since I'm new to django development, please be gentle in the review : )

One thing that is unsatisfactory about this patch is that it doesn't properly handle the case where the QuerySet has an extra(select=...) and a lookup error on the first name of a field_names containing a LOOKUP_SEP. In such a case, it should correctly show the list of available field names on the right model, but won't show the available extra() parameters, which could in turn be confusing.

However, the only way I could see to solve this was to mingle with the FieldError() exception, to include the model & field name on which the exception occurs. But I wasn't too sure about this, and it seems a little overkill.

Anyway - if you see a better way to solve this, let me know, I'll be happy to implement it differently.

by antoviaque, 12 years ago

Attachment: valueslisterrormessage.diff added

comment:5 by Jonas Obrist, 12 years ago

Triage Stage: AcceptedReady for checkin

I would say better error messages for extra could be a separate ticket. The supplied patch applied cleanly to trunk and tests passed. Also manually checked that the output is what I would expect. LGTM.

comment:6 by Anssi Kääriäinen, 12 years ago

I am looking into committing this one. Let me see if I understand what is happening currently:

  1. The part 2 of the lookup raises a FieldError
  2. That FieldError is caught at level 1
  3. A level 1 error message is generated, so you see the reporter__nonexisting part for the original model, not the level 2 part for the reporter model.

The patch changes the logic in a way that the level 1 message is let through. Is this correct?

The only question I have is if there is some easy way to show the lookup path, that is how we ended up to the nonexisting lookup. Something like 'the whole lookup was reporter__nonexisting', or "the lookup prefix was "reporter__". If there is no easy way I can live with the above patch, if there is an easy way it would be a nice addition.

comment:7 by Anssi Kääriäinen <akaariai@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [fcad6c48f07bdc6346c065849a87f0f02afb6f94]:

Fixed #17497 -- Corrected FieldError message in add_fields()

The erroneous message was user visible in values_list() calls.

Thanks to ojii for report and review, and to antoviaque for the patch.

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