Code

Opened 2 years ago

Closed 21 months ago

#17497 closed Cleanup/optimization (fixed)

Confusing exception message when using values_list with relations

Reported by: ojii 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 ojii 2 years ago.
test case
valueslisterrormessage.diff (2.6 KB) - added by antoviaque 2 years ago.

Download all attachments as: .zip

Change History (9)

Changed 2 years ago by ojii

test case

comment:1 Changed 2 years ago by ojii

  • Cc ojiidotch@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

comment:3 Changed 2 years ago by antoviaque

  • Cc antoviaque added
  • Owner changed from nobody to antoviaque

comment:4 Changed 2 years ago by antoviaque

  • 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.

Changed 2 years ago by antoviaque

comment:5 Changed 2 years ago by ojii

  • Triage Stage changed from Accepted to Ready 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 Changed 2 years ago by akaariai

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 Changed 21 months ago by Anssi Kääriäinen <akaariai@…>

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

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.

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.