I'd like to argue in favor of exercising caution when using CBVs.
In my experience, CBVs shine when the requirements closely match the use
case for a certain CBV and all customizations are purely declarative, often
just setting the model. With the level of expressiveness that Python gives
us a class is a good way to put into code what the view should do. Although
one could argue that this kind of class does not align perfectly with how
OOP intends classes to be used. Central principles of OOP, or programming
with classes, are encapsulation and data hiding. When the requirements for
a CBV become more complex from a django framework point of view, i.e. when
it should do more than "save a model form with generic error handling" or
"pass a paginated queryset to the template", the programmer has to
overwrite methods in the class. In simple cases that's superficially
simple, but the program and data flow that the programmer needs to control
is hidden from his view in the class hierarchy. The more complex the class
hierarchy and the more methods are overridden, the harder it is to figure
out what the view does and whether it does it correctly.
Now an obvious strong point of CBVs is that it provides a lot of hooks (in
the form of method overrides) for customization and extension without code
duplication. OOP in Python also gives us mixins, which take this strong
point even further. However, in my experience, it is more important to make
the code easy to read and understand, than to make it simple to write. It's
also more important than to avoid duplication in the extent that CBVs and
OOP allow you to.
Finally, composition, deduplication and reuse are possible with functions
as well, albeit in different ways.
My view is that in the case of the auth views the readability and
understandability disadvantage of CBVs has led to this bug. In an FBVs such
an omission would have been more obvious.
In conclusion, while CBVs allow for greater code reuse, they are also much
harder to get right when the requirements are complex. What I'd do is:
- recommend CBVs for requirements that closely match the use cases of
DetailView, ListView, UpdateView and so on.
- advise against the use of CBVs in more complex cases; the number of lines
of imperative code in the CBV being a first indicator for complexity.
Markus
On Wednesday, November 23, 2016 at 9:00:32 AM UTC+1, Joachim Jablon wrote:
>
> I'm +1 with Baptiste, Ben, Josh and João.
>
> Also, Luke, you said :
>
>> 1. Recognise that CBVs are much harder to reason about, and therefore
>> require much more care.
>
> 2. Avoid using CBVs unless you really need them.
>>
>
> Just wanted to note that this means never. FBV vs CBV is a choice, there's
> really few cases, if any, where one or the other is *really needed*.
>
> From my (highly personal) point of view, some people will always be more
> at ease coding with CBV than with FBV and vice versa. FBVs are simpler at
> start, CBVs are more customizable, but in the end, you'll find a way to
> customize your FBV (a.k.a copy/pasting code) and the added complexity to
> your GCBV subclass probably won't be that high (chances are : you'll be
> adding some variable in the get_context_data(), while still calling super
> and that's it)
>
> Maybe what we should be providing is more a way to put the complicated
> security-important logic out of a view altogether. A PasswordChanger
> class/module/... that would do the necessary work but just this work,
> independantly from how a view works. and then CBVs/FBVs would just call
> functions/methods on that. But then if there's already too much
> abstractions in CBV for the taste of some...
>
> My 2 cents.
> Joachim
>
> Le mercredi 23 novembre 2016 06:16:24 UTC+1, Luke Plant a écrit :
>>
>> Hi all,
>>
>> First, I want to say that complex things fail in complex ways, so there
>> it's probably a fallacy to look for a single root cause. I agree with
>> various other points about mistakes that were made, but not others.
>> Particularly:
>>
>> On 22/11/16 12:41, Florian Apolloner wrote:
>>
>> Hi,
>>
>> On Monday, November 21, 2016 at 11:56:56 PM UTC+1, Tim Graham wrote:
>>>
>>> … that the existing tests would catch this type of obviously incorrect
>>> issue.
>>>
>>
>> I think that is the main issue here. I was also really surprised to
>> discover that the tests were missing cases like this -- then again, writing
>> good tests is hard and I know that I am personally one not to write good
>> tests usually. Either way, I think the way forward is to improve the
>> testsuite in this case.
>>
>>
>> I disagree with this, there was no issue with the existing test suite for
>> the FBV version. *Tests are never close to exhaustive and we should
>> never think they are or can be.* Tests are spot checks of correctness in
>> an infinite universe of possible input values. When trying to work out what
>> you should test in this vast space, there are a few sensible ways:
>>
>> 1. Look at the requirements, and pick spots that match each requirement.
>> In this case:
>>
>> - If a request has the required token, it should be allowed through
>> - If a request does not have the required token, it should not be
>> allowed through
>>
>> 2. Make some guesses about well known edge cases and test them (e.g the
>> empty string case etc.). However this could be a complete waste of time
>>
>> 3. Look at the implementation at work out where it might go wrong. This
>> one is really important in reality.
>>
>> And this is what the existing tests did. Out of the infinite number of
>> HTTP requests to test under point 1, they happened to pick a GET request,
>> which was perfectly sensible. In the case of FBVs, the control flow is so
>> clear that all of the following are equally silly tests to add:
>>
>> - Check that requests with query string parameter 'xyz' doesn't
>> introduce a back door
>> - Check that on leap years tokens are checked
>> - Check that the security is correct for requests that are
>> specifically GET, POST, HEAD, OPTIONS, DELETE....
>>
>>
>> The conclusion is this: in CBVs control flow is much, much less clear. In
>> the FBV the mistake would have been immediately obvious, the CBV base
>> classes obfuscated things to the point where it wasn't at all clear. To
>> write adequate tests for the CBV version, under point 3 above, you have
>> *much,
>> much more work* to do.
>>
>> I disagree that forcing people to write their own auth views is *
>> necessarily* worse than providing a CBV that people can customise. Given
>> this demonstration of how CBVs obfuscate control flow, it's quite possible
>> that writing an FBV from scratch will be less error prone than using a CBV
>> and subclassing, especially when we have encapsulated things like the token
>> checking. Personally I would be massively happier auditing a custom FBV
>> than a subclassed CBV, especially when you consider that the CBV subclass
>> is inheriting from a stack of classes that could change in later versions
>> of Django in some subtle way that breaks the code. Subclassing is not
>> necessarily safer, it just feels safer ("I'm only changing this one little
>> method"), and *that feeling of greater safety is itself a huge danger*.
>>
>> Going forward, I think we need to:
>>
>> 1. Recognise that CBVs are much harder to reason about, and therefore
>> require much more care.
>> 2. Avoid using CBVs unless you really need them.
>> 3. Recognise that tests for FBVs are inadequate against the CBV
>> translation.
>> 4. Not deprecate the FBVs. At the very least, they provide a sensible and
>> easy to understand starting point for people that want to copy the logic to
>> create their own, and do so in a way that they actually understand and can
>> audit.
>>
>> I'd be +0 on reverting to an FBV only, but it's part of a much bigger
>> discussion
>>
>> Regards,
>>
>> Luke
>>
>>
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/265a2cea-efcc-4b68-b857-018bc6cac392%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
