Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#8393 closed (worksforme)

Test client handler should call set_script_prefix

Reported by: jcassee Owned by: nobody
Component: Testing framework 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:

Description

Since [8015] every HTTP handler calls urlresolvers.set_script_prefix to set the prefix for reverse URL resolving. The only except in the handler used by the test client.

The problem I am solving may seem a bit esoteric. I'm trying to remove the necessity of monkey-patching the urlresolvers.reverse function in the (third-party) localeurl app. My solution is to append to the script prefix for the request, but because the test client handler does not reset it at every request my tests fail.

The attached patch makes the test client handler call set_script_prefix at every request.

Attachments (2)

8393-r8431.diff (990 bytes) - added by jcassee 6 years ago.
8393-r8442.diff (845 bytes) - added by jcassee 6 years ago.

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by jcassee

comment:1 Changed 6 years ago by jcassee

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

On second though, the test client should not depend on the environment. Just set the prefix to '/'. Updated patch.

Changed 6 years ago by jcassee

comment:2 Changed 6 years ago by mtredinnick

  • milestone set to 1.0 maybe

I'm a little worried that this is fixing some kind of problem at the wrong level. If you do absolutely nothing, it should be the same as if the script prefix is '/'. This is because lots of code that relies on the script prefix (anything calling reverse()) can be called outside of Django's request-response loop. For example, in a cronjob.

So I'm not sure why anything at all needs to be done here. If it does need to be done, why is that so? What fails if you don't make this change?

comment:3 Changed 6 years ago by jcassee

I'm not sure what I can tell you that is not already in the ticket summary. The patch is trying to fix a problem with this (pseudo-)code in middleware:

def process_request(self, request):
    prefix = urlresolvers.get_script_prefix()
    prefix += <something>
    urlresolvers.set_script_prefix(prefix)

Because the real handlers reset the prefix on every request this works. The test client, however, does not. This causes tests for this middleware to fail.

I notice now, however, that these handlers set the prefix after the middleware has loaded. I now wonder how come this code snippet (which I tested and use 'in real life') work.

comment:4 Changed 6 years ago by jcassee

Bah, loading != processing.

comment:5 Changed 6 years ago by jacob

  • milestone changed from 1.0 maybe to 1.0
  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 6 years ago by jacob

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

Looking at urlresolvers.get_script_prefix, I can't see how it could fail -- if the prefix isn't set, it'll just return / anyway. Without any more details -- and psudocode won't cut it -- I can't verify this is actually a bug. I suspect it's a problem in your code.

I'll take a shot in the dark, I suspect that it's a matter of you not cleaning up after yourself in your middleware: if you modify the script prefix in process_request, you need to clean it up at some point during the response. Otherwise the script prefix will keep growing, and that's broken.

comment:7 Changed 6 years ago by jacob

I should have also noted that if you can verify that it's a bug in Django please feel free to reopen the ticket.

comment:8 Changed 6 years ago by jcassee

Jacob, it's definitely a resource-cleanup problem. Thanks for pointing that out. The prefix is reset (set_script_prefix is called) at every request (at least when using FastCGI or the development server), so I got away with it. The test client does not reset it, so my code failed there.

comment:9 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.