Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for URLs in route decorator #154

Open
wsanchez opened this issue Dec 16, 2016 · 12 comments
Open

Support for URLs in route decorator #154

wsanchez opened this issue Dec 16, 2016 · 12 comments
Assignees
Labels

Comments

@wsanchez
Copy link
Member

The route decorator should accept URL objects as well as strings.

@glyph
Copy link
Member

glyph commented Jul 13, 2017

I feel super bad for @mksh now, having put all this work into #163, but… looking at this PR, I was wondering… why? First and foremost, what's the benefit of allowing this?

On the "why" not side, it seems like using URL here just adds some type confusion here, because how do you specify that you want a placeholder and a parameter here? Adding <param> syntax to a .child call? That seems weird because .asURI() is going to escape those characters, since they're not really supposed to be there, at which point you get %3C %3E elements in the URL that werkzeug can't make any sense of. Do we want to put in some other kind of placeholder? How should the documentation be modified to address this? How do we enforce the invariant on the URL object that it be absolute but not supply a hostname?

@wsanchez
Copy link
Member Author

How do we enforce the invariant on the URL object that it be absolute but not supply a hostname?

That cab easily be checked by route()

@wsanchez
Copy link
Member Author

why? First and foremost, what's the benefit of allowing this?

Well, here's an example.

I have a bunch of URLs for my application somewhere; I need to use them in places other than the route decorators because elsewhere I want to have links to them, and I always use these constants.

That means that in the route decorates, I'm always calling .asText(), which seems janky. Here's an example.

Adding <param> syntax to a .child call? That seems weird because .asURI() is going to escape those characters, since they're not really supposed to be there, at which point you get %3C %3E elements in the URL that werkzeug can't make any sense of.

I'm not sure why you'd be calling asURI() on these before passing them on to Werkzeug. Why do you think one needs to?

@glyph
Copy link
Member

glyph commented Jul 15, 2017

Thanks for the more detailed explanation!

@glyph
Copy link
Member

glyph commented Jul 15, 2017

I'm not sure why you'd be calling asURI() on these before passing them on to Werkzeug. Why do you think one needs to?

I'm not suggesting that one does, but rather, that including <> in URLs is not entirely valid, and so normally if you were going to use URLs like that you'd have to normalize them somehow first; and that since such (supposedly idempotent and semantically neutral) normalization would break the interface with Werkzeug, it seems like a somewhat fragile interface.

@glyph
Copy link
Member

glyph commented Jul 15, 2017

Moreover it feels to me (in a somewhat vague, abstract way; I'm open to being convinced that this is silly) that some normalization ought to be standard-ly applied when we accept URLs here, so as to avoid weirdness. Is that just asIRI()?

@glyph
Copy link
Member

glyph commented Jul 15, 2017

Verifying that : in the path portion of the URL doesn't cause anything to break is somewhat reassuring though, I just did some experiments with hyperlink now.

@glyph
Copy link
Member

glyph commented Jul 15, 2017

I have a bunch of URLs for my application somewhere; I need to use them in places other than the route decorators because elsewhere I want to have links to them, and I always use these constants.

One potential issue / antipattern with this idiom is that the parametric portions of the URL which cause Klein to pass you extra arguments are now hidden behind an extra layer of indirection. Maybe this is just fine, but it feels like a slightly odd departure from the way klein apps usually co-locate their parameter descriptions and their function declarations.

On the other hand this is consistent with the ongoing work on #126 , which does move the form parameters to be somewhat more distant from the method definition. So I'm kinda just thinking out loud here.

@wsanchez
Copy link
Member Author

wsanchez commented Aug 2, 2017

It's possible that something like #173 will change my mind here, which is to say that instead of constants somewhere, I can add a rout for it's URL, and that would be how I get that URL elsewhere, and asking could require that I provide the parametrized components, solving that weirdness also.

More thinking out loud:

A problem I'm running into is that in cases where you have a composed application (App A has routes into App B), you need to somehow be able to compose the URLs, though that's a problem with the "URL constants list somewhere" approach.

That, perhaps, is orthogonal and should be thought about primarily in another ticket (I think composition is harder than is desirable), but I want to make sure that we think through it a little bit here, as it seems related.

Here's my example… I have this web app. Note the *Application attributes, which are the child applications, and the route methods ("*Endpoint") for them at the bottom of the file. Then not that in this child application, I have this janky _unprefix thing to remove the part of the URL that's "consumed" by the parent app.

This is a lame coupling of the child applications to the parent, so poor composition (these used to be mix-in classes, so it's better than what I had before), and I haven't thought of a natural way to get rid of it.

@wsanchez
Copy link
Member Author

wsanchez commented Aug 2, 2017

I guess what this means is that #173's url_for has to be understood as a producing a URL relative to that router's "root", but that may not be where you get exposed on the web server. But you probably want to know how it' exposed… that necessarily isn't a thing the router should know (if can be parked anywhere), but I think there's needs to be something global (at least, global to the web site that owns the total URL space) that can put this information together.

@IlyaSkriblovsky
Copy link
Contributor

@wsanchez, I came to Klein from the evil Django side where hardcoding and manually composing URLs is a big no-no. So #173 is actually inspired by Django's reverse.

But url_for() won't help much if you are composing per-subapp Klein routers like in your example. I came up with a different approach with the single Klein instance but nevertheless clean separation of concerns: https://gist.github.com/IlyaSkriblovsky/37f10b975e13176aabaf9767ad8ea648. This way there is only one router and we can build full URLs.

(It is possible though that klein app isn't running on the root of url space, for example by reverse-proxying. Possible way to deal with it is to pass some url_prefix to Klein.__init__().

@wsanchez
Copy link
Member Author

wsanchez commented Aug 9, 2017

@IlyaSkriblovsky thanks, this is useful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants