Opened 7 years ago

Closed 8 months ago

#9104 closed Cleanup/optimization (fixed)

FieldDoesNotExist is defined in "confusing" place.

Reported by: telenieko Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Django has "django.core.exceptions" package/module where most exceptions are defined, including DB related ones (i.e: ObjectDoesNotExist). Lame users like me would expect FieldDoesNotExist to be defined on the same module but it's in django.db.fields

Maybe we could move the exception?

Change History (12)

comment:1 Changed 7 years ago by telenieko

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

comment:2 Changed 7 years ago by mtredinnick

We can't move it (backwards compatibility is important), but we could add an alias in the other place, maybe.

However, the reason for its current location is that this is not an exception that correct user code will ever see. You will see something like ObjectDoesExist because that can happen naturally. But your code should never be catching FieldDoesNotExist, since that would just mean you had made an error in your code 99% of the time (there are some introspection cases where it might not. I guess that is why the admin interface is using it). So declaring the exception where it is actually used and then only using it internally isn't a bad thing.

What's the use-case where you actually need to ever import this exception?

comment:3 Changed 7 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:4 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:5 Changed 4 years ago by aaugustin

  • Easy pickings unset
  • Resolution set to needsinfo
  • Status changed from new to closed
  • UI/UX unset

Malcolm explained why the code is written in this way, and his question stays unanswered after years.

comment:6 Changed 22 months ago by nickname123

Just came across this looking for the exception with Google search.

My use case is that I have a model form with extra fields on the form than what the model defines. I use these extra fields to auto populate some of the model fields.

I am iterating through the form fields to add HTML5 attrs during init. I need to catch FieldDoesNotExist when I try to check if the model field has blank=True to determine if the formfield is required or not.

from django.db.models.fields import FieldDoesNotExist

def __init__(self, *args, **kwargs):
        super(FooModel, self).__init__(*args, **kwargs)
        for key in self.fields:
            try:
                mfield = self.instance._meta.get_field_by_name(key)[0]
            except FieldDoesNotExist:
                pass
            else: 
                if not mfield.blank:
                    self.fields[key].widget.attrs["required"] = "required"

This isn't too important, so I am just leaving the comment in case Google brings me back here again before I grep.

Last edited 22 months ago by nickname123 (previous) (diff)

comment:7 Changed 14 months ago by wraus

  • Resolution needsinfo deleted
  • Status changed from closed to new

I'd like to reopen this just to hopefully have this oddity resolved.

One of the comments was talking about use cases, and I wanted to give the use case that I'm using for it to hopefully sway opinion a bit on making this simple change.

I'm working on a template tag that will allow me to iterate over a list of a model's fields / attributes and output it as a table. I want to grab the verbose_name of the field, given the field's name, and to do so, I get obj._meta.get_field(field_name). However, I also want the option to reference class attributes and functions, and obviously if I reference a function or attribute, there is no matching field. Thus, I need to call get_field, and catch FieldDoesNotExist to handle it as a function / attribute and get a separate label.

There doesn't seem to be a more "obvious" way to do this, and I ran into the issue of trying to find this oddly placed exception. I understand that moving the exception would be a serious change, but hopefully making it importable via django.core.exceptions will resolve this inconvenience.

comment:8 Changed 10 months ago by tomchristie

I believe that the correct resolution of this should be whatever decision is made in the official 'meta' API update.

Given that we don't yet document get_fields or FieldDoesNotExist anywhere, the new public API is the correct way to resolve this one way or the other.

https://github.com/django/django/pull/3114

comment:9 Changed 8 months ago by freakboy3742

This has been resolved as part of the Meta-refactor work, which is part of PR3114 - see commit f96990ff.

comment:10 Changed 8 months ago by claudep

Even if it is not documented, searching for FieldDoesNotExist on github for example reveals many usages of this import location in the wild, hence I think we should provide some deprecation path.

It should be possible for example to create a django/db/models/fields/FieldDoesNotExist.py module to trigger the deprecation warning on old import location (don't know if there is a cleaner way to deprecate import locations).

comment:11 Changed 8 months ago by claudep

Of course, my "trick" about a FieldDoesNotExist.py file only works if we want to loudly fail with an explicit message. Still looking for a way to raise a warning without ever calling the symbol (might be impossible...).

comment:12 Changed 8 months ago by Tim Graham <timograham@…>

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

In 8958170755b37ce346ae5257c1000bd936faa3b0:

Fixed #9104 -- Moved FieldDoesNotExist to core.exceptions

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