funkybob / django-sniplates

Template snippet libraries for Django
MIT License
57 stars 18 forks source link

Possible to get rid of `display`? #33

Closed sergei-maertens closed 9 years ago

sergei-maertens commented 9 years ago

I have this use case: an inline_formset with a ModelForm, these are the models involved:

class Build(models.Model):
    title = models.CharField(max_length=50)

class BuildPhoto(models.Model):
    build = models.ForeignKey(Build)
    photo = models.ForeignKey('albums.Photo')
    url = models.URLField(blank=True)

    def __unicode__(self):
        return self.build.title  # simplified, but traverses the FK

class BuildPhotoForm(forms.ModelForm):
    class Meta:
        model = BuildPhoto
        widgets = {'photo': forms.HiddenInput}

Now, when I make an inlineformset for BuildPhotoForm with BuildPhoto, Django adds the 'id' field in BuildPhotoForm (django.forms.models.BaseModelFormSet.add_fields). This field is a ModelChoiceField and the queryset is not configurable, except if you subclass the BaseFormSet and override add_fields to modify this.

This is a HiddenInput widget by default, just like the 'photo' field. However, for these widgets in sniplates, the 'choices' is evaluated, while for ModelChoiceFields field.choices is a ModelChoiceIterator and thus lazy. This is very costly for the id field in my case, as it traverses the FK, leading to a query bomb in my template. I have no interest in the choices. It seems to me that choices is only evaluated to get the display of the field, so I vote for getting rid of display. It doesn't make a lot of sense in multiple-value fields anyway. Or, if it's needed, make it lazy.

Concrete example: I was getting +3000 queries before subclassing, after 'fixing' it in my subclassed BaseFormSet, I'm only getting 27 queries.

Thoughts?

funkybob commented 9 years ago

There's already a request to make choices and display lazy... I'm working on it for one of the branches...

sergei-maertens commented 9 years ago

Allright, sounds great, I couldn't find a related issue here so figured I'd open it up

funkybob commented 9 years ago

The current code only finds display for non-(list, tuple) values... but I guess that's tripping on querysets and the like...

sergei-maertens commented 9 years ago

I don't have an issue with display per se, I just noticed that the queryset gets evaluated here: https://github.com/funkybob/django-sniplates/blob/master/sniplates/templatetags/sniplates.py#L325

display is not throwing an error or anything, it's just also evaluating the queryset I think. The main performance issue is in the evaluation of the choices to force_text them. I do remember that begin necessary for values like None I think... I'm not sure anymore, shouldn't the DTL call force_text on any text nodes in the template?

funkybob commented 9 years ago

Yeah, I wanted to make display and choices lazy ...

sergei-maertens commented 9 years ago

Yes, I understood, I probably didn't fully comprehend what you meant with the previous comment, I thought there was some unclarity regarding display in my case. I'll patiently wait for your improvements . :)

funkybob commented 9 years ago

OK... I've just committed what may fix all this... as well as add a new attribute: raw_value

Please test the feature/pluggable-explosives branch and let me know how it fucks up :)

sergei-maertens commented 9 years ago

This is great, the lazy value/choices reduced my total queries to 2 in best case, 4 in worst case. A success after previously 3000+ queries for worst case.

I have a few remarks, but I'll open a new issue to keep things clear and trackable. I'll close this one.