Comment (by claudep):
#18973 was a duplicate with another use case.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:13>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* cc: mmitar@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:14>
Comment (by tim.dawborn):
#20275 is another duplicate. Any chance of bumping this?
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:15>
* status: new => assigned
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* owner: nobody => unaizalakain
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:16>
Comment (by unaizalakain):
PR sent: https://github.com/django/django/pull/1824
Requests made with `django.test.Client.login` and
`django.test.Client.logout` respect defaults defined in
`django.test.Client`
instantiation and are processed through middleware now.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:17>
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:18>
Comment (by loic84):
I'm not too sure about this ticket.
`login` and `logout` would be the only requests that go through
middleware. (refs. #9159)
Also going through the middleware is not necessarily sufficient to be
useful, what if it the middleware tests for the HTTP verb (hardcoded to
`GET` currently)? or for a specific IP address (hardcoded to '127.0.0.1')
or even for a specific endpoint (hardcoded to '/'). For this to work,
`login` and `logout` should accept `**extra` argument like the other
methods.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:19>
Comment (by unaizalakain):
It's kind of messy but all the `get`, `post`, etc. go through middleware.
This methods all call the `self.generic` method which in turn calls the
`self.request` method. Now, this method doesn't do a big thing in
`RequestFactory` but it's subclassed in `Client` and makes use of
`self.handler` which builds the request through middleware.
Now, the `self.request` returns directly a response and in `login` and
`logout` we need the source request also. I thought about returning
`(request, response)` tuples in `self.request` but that wouldn't be
backwards compatible so I directly call `self.handler` witch returns me
the source request attached.
Now that I think it, if `self.handler` attaches the source request, I
could also get it (I think) through `self.request`. Gonna look into it.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:20>
Comment (by unaizalakain):
Yep, tests run smoothly, so `self.request()._request` instead of
`self.handler(self._base_environ)._request` ;-)
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:21>
Comment (by loic84):
@unaizalakain, true for `self.request` ultimately being called.
The issue with the request environ remains though, what do you think?
It's worth noting that `login` already uses the `**kwargs` wildcard for
`**credentials`, so it can't be used for `**extra`, IMO not a big deal if
`login` and `logout` are not exactly consistent with the other methods as
they operate at a higher level.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:22>
Comment (by unaizalakain):
It's a pity that `username` and `password` kwargs cannot be assumed
because of the auth backend plugability. Though it would be better not to,
it seems to me that the only possible solution is to add a `extra` kwarg
that accepts a dict although I don't know if it's a really needed feature
(though it doesn't really hurt to have it) because of the possibility of
setting custom env vars directly at the `Client` at instantiation or
modifying `Client.defaults`.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:23>
Comment (by loic84):
As mentioned on the PR, I would rename the newly added `response._request`
to `response.request_instance` so it's not confusing that there is a
`response._request` and a `response._request` which are two different
things.
FTR: Ideally the old `response.request` would become a getter with a
deprecation warning that proxies `request_instance.environ`, or be
renamed to `request_data`; however making such property would require a
custom `Response` class which is probably overkill.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:24>
Comment (by unaizalakain):
Done, thanks for the review!!
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:25>
Comment (by unaizalakain):
Once again, thanks for your review, PR updated ;-)
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:26>
* stage: Accepted => Ready for checkin
Comment:
LGTM.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:27>
* owner: unaizalakain => akaariai
Comment:
I am going to do some minor final polish & commit this one.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:28>
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4fdd51b73240bf9c8d9472fcc45df699f0714755"]:
{{{
#!CommitTicketReference repository=""
revision="4fdd51b73240bf9c8d9472fcc45df699f0714755"
Fixed #15179 -- middlewares not applied for test client login()
Requests made with django.test.Client.login() and logout() respect
defaults defined in django.test.Client instantiation and are processed
through middleware.
Thanks to Loic for the reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:29>
* status: closed => new
* needs_better_patch: 0 => 1
* resolution: fixed =>
Comment:
I think there's a serious problem with this approach.
The "fake" request is used to generate a real response, which means that
any test that uses `Client.login()` will be coupled to the behavior of an
arbitrary piece of view code (by default whatever handles a GET of '/').
So if that view happens to raise an exception, any test that uses
`Client.login()` will fail. This kind of coupling does not seem like a
good idea in the test suite (especially considering that such an exception
needn't even be the result of a bug; it could just be that the test
database wasn't put in an appropriate state, since that view isn't what's
being tested).
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:30>
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* severity: Normal => Release blocker
* stage: Ready for checkin => Accepted
Comment:
Seems like it could be a legitimate concern, although I'm not sure how
often it will be a problem in practice.
It wasn't initially clear to me where the "real response" was generated; I
believe that refers to the logic in the `self.request()` calls that were
added.
Marking as a release blocker so we make a decision before the 1.7 release.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:31>
* version: master => 1.7-beta-2
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:32>
Comment (by marfire):
FWIW this isn't purely theoretical - I came across this after upgrading to
1.7b1 and getting a huge number of test failures on working code. (As I
hinted at above, the failures were because I hadn't set up the test
database to handle a `GET` on '/'.)
The relevant change is
[https://github.com/django/django/commit/4fdd51b73240bf9c8d9472fcc45df699f0714755
#diff-97160f50594424a40f2621d5a3c581ccL549 here]. The call to `request()`
does a full request / response cycle. Unfortunately I don't know this area
of the code well enough to suggest a solution. Is there a way to point the
code at a dummy view that just returns an `HttpResponse`?
Even with such a change there will still be some coupling with the other
(non-authentication) middleware components. That doesn't seem like a
problem, though, since it's hard to imagine any request-based test working
if the middleware doesn't work. But if we care we could make this feature
opt-in with a `use_middleware` keyword argument to `login()`. That would
minimize coupling and ensure full backward-compatibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:33>
* cc: k@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:34>
Comment (by loic84):
How about taking advantage of https://docs.djangoproject.com/en/dev/ref
/request-response/#django.http.HttpRequest.urlconf.
We could set a custom urlconf that contains a login/logout view from
`contrib.auth` and use these.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:35>
Comment (by aaugustin):
Given the concerns expressed before committing this patch and the results
seen during the beta, I would suggest to revert the change. The approach
seems too fragile. It touches too many layers.
I would also have closed the ticket as wontfix.
`Client.login()` is a simple convenience feature. If it doesn't work, it
isn't hard to write a `LoginMixin` providing a suitable `login` method for
your website.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:36>
Comment (by loic84):
I have a proof of concept, that makes `Client.login()` and `logout()` go
through a normal request cycle:
https://github.com/loic/django/compare/ticket15179
This also unveiled a cache issue with `@override_settings(ROOT_URLCONF)`.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:37>
* has_patch: 0 => 1
Comment:
I like Loic's POC as it obsoletes a lot of the custom code in the
`Client.login()` and `logout()` methods. My only concern is that there may
be some subtle differences in behavior that are not tested. Worst case, I
think we could revert this change in 1.7 and punt it to 1.8.
What do you think, Kevin?
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:38>
Comment (by marfire):
Agreed, I like the approach in Loic's patch. There's still an inherent
tension here - anything that uses `Client.login()` is going to end up
inadvertently testing the user's middleware. That seems OK to me, since I
imagine that any test that uses `Client.login()` is going to end up making
another request as well, so the middleware will be exercised in either
case.
My imagination is limited, however. :-) Given where we are in the cycle
(and Aymeric's objections), I would lean towards reverting this in 1.7 and
taking Loic's approach going forward.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:39>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"aabceadd7d7a61468b0dc7dc9d560a770abae0cf"]:
{{{
#!CommitTicketReference repository=""
revision="aabceadd7d7a61468b0dc7dc9d560a770abae0cf"
Revert "Fixed #15179 -- middlewares not applied for test client login()"
This reverts commit 4fdd51b73240bf9c8d9472fcc45df699f0714755.
See the ticket for concerns with this implementation; it will be
revisited.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:40>
Comment (by Tim Graham <timograham@…>):
In [changeset:"1d20693fa67f314de08e51af57af6fb5460b7b0c"]:
{{{
#!CommitTicketReference repository=""
revision="1d20693fa67f314de08e51af57af6fb5460b7b0c"
[1.7.x] Revert "Fixed #15179 -- middlewares not applied for test client
login()"
This reverts commit 4fdd51b73240bf9c8d9472fcc45df699f0714755.
See the ticket for concerns with this implementation; it will be
revisited.
Backport of aabceadd7d from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:41>
* status: closed => new
* has_patch: 1 => 0
* resolution: fixed =>
* severity: Release blocker => Normal
Comment:
Reverted from master/1.7 for now and removed release blocker flag.
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:42>
* version: 1.7-beta-2 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/15179#comment:43>
Comment (by Simon Charette):
With the introduction of `force_login` in 1.9 I think that the latest
concerns raised about Loic's patch can be addressed by pointing users are
using `force_login` if they really want to avoid going through middleware.