Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#15125 closed (wontfix)

UpdateView should introspect form_class instead of requiring you to pass the model

Reported by: sontek Owned by: nobody
Component: Generic views Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


If you use UpdateView and pass form_class, it should be able to get the model class from Meta instead of requiring you to pass the information to as_view().

Attachments (2)

introspec_meta.2.diff (2.8 KB) - added by sontek 6 years ago.
introspec_meta.diff (5.2 KB) - added by sontek 6 years ago.
patch with russelm's suggestions and some docs about the new method

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by sontek

comment:1 Changed 6 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Accepted on the basis that yes, model information will be available under some circumstances, and shouldn't require double-entry. However, I have two problems with the provided patch.

Firstly, this presumes that form_class has a _meta.model, which it wont unless you have a ModelForm. Forms don't have to be model forms in order to save a model.

Secondly, I'd rather see this handled by introducing a get_model() entry point, rather than using the constructor. The default implementation would just return self.model, but this would provide the flexibility for arbitrary control of the source model, including introspection of form_class when possible.

Changed 6 years ago by sontek

patch with russelm's suggestions and some docs about the new method

comment:2 Changed 6 years ago by jezdez

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:3 Changed 6 years ago by russellm

  • Triage Stage changed from Ready for checkin to Accepted

This isn't quite right -- again, form_class doesn't *necessarily* have a _meta attribute, because it might not be a ModelForm.

I think there may be a better way to handle this, which has the added benefit of fixing #15274. We may not be able to reliably get the model class from form_class, but we *can* get it from self.object, which always exists on an UpdateView.

comment:4 Changed 6 years ago by russellm

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

Looking at this more closely, I'm not sure this can be achieved:

  • UpdateView calls get_object() to find the object to edit
  • get_object() calls get_queryset()
  • If a queryset hasn't been specified, get_queryset() then calls get_model()
  • get_model() calls get_form_class()
  • If a form_class hasn't been specified, get_form_class() calls get_queryset().

This means that get_model depends on get_queryset, and get_queryset depends on get_model. This dependency chain can be resolved in certain circumstances -- the provided test case is one such case. However, in the general case, it's a minefield. Any of these functions can be overridden -- get_object, get_model, or get_queryset -- which means that there needs to be a clear dependency of information, or else you could easily get stuck in a recursive loop of calls. The simplest way to break it is to not specify a form_class OR a queryset, but the complex dependency chain combined with extensibility strikes me as an accident waiting to happen.

Closing wontfix, but only because I can't see a way to build this (and then explain it clearly) in the general case. Feel free to reopen if you can find a cleaner way to put the pieces together.

comment:5 Changed 5 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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