r8-forks / webapp-improved

Automatically exported from code.google.com/p/webapp-improved
Other
0 stars 0 forks source link

WSGIApplication.url_for fails #1

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Call url_for on an app

What is the expected output? What do you see instead?
To return the url for the named route. Instead it fails with attribute error. 
Apps do not have a response: 
http://code.google.com/p/webapp-improved/source/browse/webapp2/__init__.py#1100

Also it doesn't make sense that Router.build takes a response as an argument. 
No router.build implementation uses it.

Original issue reported on code.google.com by evlogimenos on 8 Aug 2010 at 11:42

GoogleCodeExporter commented 9 years ago
webapp2.WSGIApplication sets a request attribute that is used to build URLs. 
There are tests with WSGIApplication.url_for() and RequestHandler.url_for() 
working, so I'm confused with your report. Are you using 
webapp2.WSGIApplication or the one from webapp? Please provide some more info.

-------

About Router.build arguments:

Router.build takes a request as argument (not response). The purpose is that 
extended routes can use the request info to build URLs (say, a route that 
builds an absolute URL or a URL for a subdomain). 

Other routing systems don't take request as argument because they infer environ 
variables in some other way to build absolute URLs (e.g., Routes reads 
os.environ or uses information from middleware; werkzeug.routing is bound to a 
Request or environ). 
We are just being very explicit here because environ would need to read i some 
way. 

Alternatively it could take environ as argument or simply use os.environ; using 
request is not bad in the context of webapp, imo.

Router.build is intended to be a "low level" API. Users should use 
app.url_for() or RequestHandler.url_for() instead (both call builder passing 
the current request object).

Original comment by rodrigo.moraes on 10 Aug 2010 at 8:52

GoogleCodeExporter commented 9 years ago
Sorry, replace response with request in the bug report. Unfortunately I cannot 
edit it.

The way WSGIApplication.url_for is written works only in the context of 
processing a request. If I create my application like so:

app = webapp2.WSGIApplication(...)
app.url_for(...)

This won't work. This is actually used when you are doing tests with WebTest. 
You want to use url_for to generate the url to your app and then all them to 
test their output.

Original comment by evlogimenos on 11 Aug 2010 at 11:05

GoogleCodeExporter commented 9 years ago
Hi. 
Oh, it works with webtest, as demonstrated in our tests (we do have quite a few 
url_for tests). Please see:

    http://code.google.com/p/webapp-improved/source/browse/tests/test_routing.py
    http://code.google.com/p/webapp-improved/source/browse/tests/test_handler.py

There are several strategies you can use. The simplest one is: just set a blank 
Request in the app. webob.Request has an utility exactly for this. See:

    app = webapp2.WSGIApplication(...)
    app.request = Request.blank('/')
    app.url_for(...)

Notice that Request is only needed when generating absolute URLs.

Original comment by rodrigo.moraes on 12 Aug 2010 at 9:05

GoogleCodeExporter commented 9 years ago
It works if you do app.request = None as well. The point is that this is 
non-obvious. Either the documentation of url_for() should be changed to 
specifically say that this function should not be called outside handling a 
request or without explicitly setting application.request. Alternatively it 
should be changed to not need the request.

Original comment by evlogimenos on 12 Aug 2010 at 10:25

GoogleCodeExporter commented 9 years ago
Sorry for the confusion. The purpose of having request is to be able to build 
absolute URLs, and this is not something that impedes testing (it is actually 
there to help testing), but a warning can be added.

I can also change it to not need a request object, but then os.environ will be 
used to build absolute URLs. See that then in a testing environment you'd need 
to set the environ variables that are expected in a HTTP request, or absolute 
URLs could not be built. I think this makes testing more difficult (and is the 
reason why request is passed explicitly).

Given these options, what you think is best?
A) Leave request argument, document its requirement in url_for().
B) Remove request argument, use os.environ to build absolute URLs.
C) make **environ** an optional argument (from request.environ), then use it 
when available and fallback to os.environ when not (helps testing, makes it 
possible to use build() out of a request).

Thanks for your feedback.

Original comment by rodrigo.moraes on 13 Aug 2010 at 2:11

GoogleCodeExporter commented 9 years ago
I would go for B and document it in url_for. You need to tamper with the 
environ for all sorts of things for appengine testing to work anyway. You also 
need to modify it to emulate logged in users/admins and whatnot during testing. 
With that in mind modifying the environ is something you *have* to do to test 
so its ok to need it for absolute urls as well.

Original comment by evlogimenos on 13 Aug 2010 at 11:20

GoogleCodeExporter commented 9 years ago
After asking for some advice about this issue, I decided to let request be 
passed explicitly and added the following note to url_for() docs:

.. note::
   This method, like :meth:`WSGIApplication.url_for`, needs the request
   attribute to be set to build absolute URLs. This is because some
   routes may need to retrieve information from the request to set the
   URL host. We pass the request object explicitly instead of relying 
   on ``os.environ`` mainly for better testability, but it also helps 
   middleware.

Thanks for your feedback.

Original comment by rodrigo.moraes on 17 Aug 2010 at 1:01