Ticket 19445

[Luke Plant]

Hi all,

Can I get some feedback on what to do about ticket 19445?

https://code.djangoproject.com/ticket/19445

I have re-opened this ticket because I don't think it was addressed
satisfactorily. I apologise in advance that this is a bit involved!

The fundamental problem is that the validation routines attempt to do
static validation of a ModelAdmin class, but ModelAdmin has gained
methods that mean that many of the things that we want to validate are
only generated at run time (by which I mean the point when a request is
received).

So ModelAdmin.get_form() is the method used to create a form class, but
the validation routines want to validate ModelAdmin.form . Even without
subclassing ModelAdmin.get_form(), it's possible to create a form class
that will not pass validation, but will work fine in practice (see
ticket for more details).

The main reason I'm bothered about this is ticket 19733, which is about
requiring the ModelForm.Meta.fields attribute to be present. Please see
below for why 19733 is involved.

Even without #19733, it seems that validation of ModelAdmin.form is
broken in the general case. The attempts to fix it so far were quite hacky:

https://github.com/django/django/commit/0694d2196f0fadde37ff2d002a9a4a8edb3ca504

And I'm fairly sure there are other bugs with validation related to the
dynamic methods such as get_fieldsets() and get_readonly_fields(), which
both take a request object.

Since we can't do static validation properly, I'm tempted to remove it
altogether, but I imagine the static validation will catch lots of
problems, which is pretty helpful for newbies. A large part of the
ModelAdmin validation would need to be removed - everything that depends
on knowing what fields are going to be present.

One possibility is to remove the static validation, but where specific
errors are common (e.g. misnamed fields), we try to make the error
handling further down the line much more friendly.

If you read the current validation routines, there are quite a few
places where potential errors have to be ignored (e.g. a field that
doesn't exist on the model is referred to), because there are other
things which could mean that it is actually correct. To me this implies
that validation is happening at the wrong point.

~~~~~~~~~~~~

Why it matters for #19733:

https://code.djangoproject.com/ticket/19733

#19733 will (eventually) require ModelForms to have Meta.fields
explicitly set. (Or, alternatively, Meta.exclude, but this is discouraged).

To cover the case where we really do want all fields, you can do 'fields
= "__all__"', as discussed previously on django-devs.

However, if you are creating a ModelForm to be used solely in the admin,
this is not good. We've agreed that that admin should use all fields by
default, and it has its own ways of specifying which fields to use.
Having to specify anything about fields on the custom ModelForm is
confusing.

There is one good solution I've found: specify a ModelForm without a
Meta inner class, or at least without a Meta.model attribute.

This already works fine with the admin. The ModelAdmin already knows the
model to use, and happily constructs the Meta inner class for your form
if it is missing.

This is a really good solution, because:

1) It's DRY - you specify everything once:

###########
class MyModel(models.Model):
field1 = models.BooleanField()

class MyForm(forms.ModelForm):
def clean(self, *args):
# ...

class MyAdmin(admin.ModelAdmin):
form = MyForm
fieldsets = (
('Group', {
'fields': ['field1']
}),
)

admin.site.register(MyAdmin, MyModel)
###########


2) It is secure.

If we do 'MyForm.Meta.fields = "__all__"' just to satisfy ModelForm,
then not only is it confusing, we've also created a functioning
ModelForm that could end up being used outside the admin, which has all
fields included and is therefore potentially insecure. We want to avoid
that.

3) It provides a simple upgrade path.for people who hit the new
requirement on ModelForm, where they were only using them for ModelAdmin.

Instead of asking people to add a fields attribute, we tell them to
remove the whole Meta inner class, or at least the model attribute. This
can be in the upgrade notes.

4) It works well in terms of implementation.

The admin uses all the ModelForm machinery in terms of location Model
fields. However, we need ModelForms to have the opposite behaviour from
the admin in this respect - ModelForms should not use all fields unless
explicitly told to.

This means that the obvious place to patch up the mismatch is in the
admin implementation - it should create an appropriate ModelForm.Meta
class for you, as it does currently.


There is, in fact, already a ModelAdmin in the test suite that fails
validation if it is updated according to the method above, which is what
brought me to ticket 19445.


~~~~~~~~~~~~~~


Thoughts?

Luke


--
CARLSON'S CONSOLATION
Nothing is ever a complete failure; it can always serve as a
bad example.

Luke Plant || http://lukeplant.me.uk/

[Carl Meyer]

Hi Luke,

On 05/01/2013 04:31 PM, Luke Plant wrote:
> Can I get some feedback on what to do about ticket 19445?
>
> https://code.djangoproject.com/ticket/19445
>
> I have re-opened this ticket because I don't think it was addressed
> satisfactorily. I apologise in advance that this is a bit involved!
>
> The fundamental problem is that the validation routines attempt to do
> static validation of a ModelAdmin class, but ModelAdmin has gained
> methods that mean that many of the things that we want to validate are
> only generated at run time (by which I mean the point when a request is
> received).
>
> So ModelAdmin.get_form() is the method used to create a form class, but
> the validation routines want to validate ModelAdmin.form . Even without
> subclassing ModelAdmin.get_form(), it's possible to create a form class
> that will not pass validation, but will work fine in practice (see
> ticket for more details).

I agree with you that we should simply remove unreliable static
validation for things that are not actually static, rather than
introducing ever-more-complex (and in this case, incorrect) heuristics
to guess whether static validation will work.

It would be great to enumerate some common mistakes that the static
validation we're removing was supposed to catch, and see in how many of
those cases we can provide a similarly-helpful error at runtime
(hopefully with no performance cost in the non-error case).

Carl

[Aymeric Augustin]

Hi Luke,

I'm also skeptical about validation when:
- it tries to analyze statically dynamic behaviors;
- it fails on code that actually works.

To me it looks like the admin outgrew its own validation, and
as a consequence the validation needs to be re-engineered.

Christopher Medrela applied for a GSoC on validation. While
his proposal doesn't address this specific problem, I think it
would make it easier to resolve.

Overall, when it comes to correctness vs. convenience, I sit
on the side of correctness :) so count me as +1 if you're
changing validation to stop assuming static behavior that
isn't guaranteed.

All this is fairly hand-vawy, because my only exposure to
the validation code until now is as a user, not as a developer.
However, I'm willing to dive in and provide more concrete
feedback on a patch.

--
Aymeric.

> --
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers?hl=en.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[Iván Raskovsky]

Hi Luke,

The static validation also breaks some third party apps like
django-hvad[0] and makes it very ugly to work around[1]. I agree with
removing it altogether, or at least make it simple to disable it when
using apps that change the form dynamically. Regards,
Iván

[0] https://github.com/KristianOellegaard/django-hvad
[1] https://github.com/KristianOellegaard/django-hvad/issues/10

[Russell Keith-Magee]

On Mon, May 6, 2013 at 12:46 AM, Aymeric Augustin <aymeric....@polytechnique.org> wrote:

Hi Luke,

I'm also skeptical about validation when:
- it tries to analyze statically dynamic behaviors;
- it fails on code that actually works.

To me it looks like the admin outgrew its own validation, and
as a consequence the validation needs to be re-engineered.

Christopher Medrela applied for a GSoC on validation. While
his proposal doesn't address this specific problem, I think it
would make it easier to resolve.

I don't think this is entirely true. Christophers' work will provide some nicer hooks for validation, which means we'll allow end users to write their own validation for their own field types etc, and won't need to have one great big "VALIDATE" method anymore. That might result in some refactoring of the Admin's validation code, but I'm not sure actually improving the Admin validation itself is on the project schedule.

Overall, when it comes to correctness vs. convenience, I sit
on the side of correctness :) so count me as +1 if you're
changing validation to stop assuming static behavior that
isn't guaranteed.

No disagreement here. Validation is supposed to be a helper to avoid common errors; if we can't do that reliably, we shouldn't be doing it at all. In this case, I suspect Luke is correct in saying that the better approach is better runtime error messages when a configuration won't work.

As for the "ModelForm without a Meta" approach -- I see what you're driving at, and I can see how it's an elegant solution to the problem. My concern is that we'd be encouraging people to pass around "half-baked" ModelForm definitions -- i.e., under any other circumstances, a ModelForm without a Meta wouldn't be valid. If someone puts that ModelForm into a forms.py, it's going to appear like it can be used as a form in general usage… except it can't. 

The only alternative that immediately comes to mind is allowing a flag to be passed into the constructor for a form that explicitly allows "insecure default all-fields" behaviour. The admin would always pass in that flag, users could if they wanted to (but at least they'd be opting in to a known potentially insecure behaviour). I'm not saying I particularly like this option either; I'm just floating it as an alternative.

Russ %-)

[Aymeric Augustin]

On 7 mai 2013, at 03:04, Russell Keith-Magee <rus...@keith-magee.com> wrote:

That might result in some refactoring of the Admin's validation code, but I'm not sure actually improving the Admin validation itself is on the project schedule.

I was mostly referring to the addition of validation warnings.

False positives are always annoying, but if they're non-fatal, at least they won't prevent Django from running.

-- 

Aymeric.

[Julien Phalip]

Hi Luke,

I remember having a discussion with other developers on IRC before committing this patch. While it did feel quite hacky, it seemed like a case of "practicality beats purity". Now, if this gets in the way of applying a proper fix for #19733 then indeed this should definitely be revisited.

As you've noted, the general issue is that many "static" ModelAdmin attributes already have their "dynamic" get_xxxx(request) companion method, and the trend has been to continue adding more of those methods. So static validation is going to get more and more difficult to handle properly.

For that reason, I'm also in agreement to eventually remove this type of validation altogether in favor or more explicit runtime error messages. While the current static validation can be useful, it's most likely that you would try testing the admin interface immediately after making a change anyway.

Perhaps a good way to approach this would be to start by cataloging everything that gets validated statically, then reproduce every corresponding error at runtime (with DEBUG=False) and pinpoint the errors that are confusing or not explicit enough.

Julien

[Luke Plant]

On 07/05/13 02:04, Russell Keith-Magee wrote:

> As for the "ModelForm without a Meta" approach -- I see what you're
> driving at, and I can see how it's an elegant solution to the problem.
> My concern is that we'd be encouraging people to pass around
> "half-baked" ModelForm definitions -- i.e., under any other
> circumstances, a ModelForm without a Meta wouldn't be valid. If someone
> puts that ModelForm into a forms.py, it's going to appear like it can be
> used as a form in general usage… except it can't.

Currently, a ModelForm without a Meta *is* valid, and will work in the
admin no problem. I don't know how long that has been true, but I've got
a Django 1.4 project where it works. ModelAdmin creates the Meta class
for you, or will fill in missing attributes (like 'model') if it is
incomplete.

The difference is that we would be encouraging that practice. But in the
docs, I would only be encouraging it for use in the admin, as the
solution to avoid having to specify 'fields' multiple times.

> The only alternative that immediately comes to mind is allowing a flag
> to be passed into the constructor for a form that explicitly allows
> "insecure default all-fields" behaviour. The admin would always pass in
> that flag, users could if they wanted to (but at least they'd be opting
> in to a known potentially insecure behaviour). I'm not saying I
> particularly like this option either; I'm just floating it as an
> alternative.

A flag to the constructor (by which I assume you mean __init__ not
__new__) is too late. If a model is inspected to create the form fields,
that happens when the ModelForm subclass is defined (within the
ModelFormMetaclass.__new__ method). And that isn't going to be something
we can change, because it would break all the code that depends on it,
like this:

class MyForm(forms.ModelForm):
class Meta:
model = MyModel

MyForm.base_fields['my_field'].widget = ...


...and lots of similar code

In many ways, setting 'Meta.fields = "__all__"' is exactly the flag you
are talking about, only it operates at the ModelForm metaclass level.

Regards,

Luke


--

Luke Plant || http://lukeplant.me.uk/