Open kolektiv opened 8 years ago
mexx
commented
8 years ago As I currently reread the section of the RFC about the path, I think the current implementation is encoding way to much characters. sub-delims as well as : and @ shouldn't be encoded as they are permitted as part of the path segment. Maybe it would already fix the issues.
kolektiv
commented
8 years ago I experimented with relaxing the encoding, but it still leaves the problem, just in slightly different cases. For example, consider the case of a template "/chars/{chars*}" where the intention is to read and write a comma separated list of chars. A client sending "/chars/%2F,%5E", intending to send the list of chars (as strings) [ "/", " " ] will cause a problem, as OWIN will decode that, and give "/chars//, " to the handler. There's no way of encoding that again, as there's no way to know which of the /s are intended! There are other cases, but that's an obvious one.
If there is a set of chars that will work predictably and correctly I'd love to take that route, but I'm not sure I hold out much hope of it. Happy to be told I'm wrong though :smile:
kolektiv
commented
8 years ago As a follow up question (when people get here!) which servers are people using? Katana/IIS? Suave? Something else? Thank you in advance!
kolektiv
commented
8 years ago For those interested, see this commit to my fork, https://github.com/kolektiv/freya/commit/eee2ef9a15a159c85036b4ab2a6097e19285d2f9 where I've introduced the concept of polyfills for OWIN, effectively. Freya.Polyfills and Freya.Polyfills.HttpListener are new, and may shortly be joined by a Suave equivalent.
Although an annoying step, it does get back to properly supporting URI Templates (and any use of paths and queries which has encoding with meaning, which anything using "just" OWIN can't do).
(An additional annoyance is that it breaks various tests currently, which I'm going to need to fix with some kind of test polyfill. However, that's not the end of the world, hopefully.)
statpro-awebb
commented
8 years ago I'm relatively new to OWIN (apart knowing what it is, and having an overview understanding), so I don't know the history here, but I see that there is a 1.0.1 draft, last updated in October 2014.
Is it not time to pick this up and aggressively push it forwards to address this issue (i.e. make the original path be a required key in the request data), as well as other issues that surely have arisen for people working with OWIN since 2014?
Of course I know that this course of action will take time and Freya will need a solution sooner, but if we at least agree on the name of the new key (or keys), add it and its meaning to the draft, then there is something written down that can be pointed to, for web server implementers.
Re. which web servers: I'm using Suave at the moment (@ademar has recently committed three fixes re. OWIN), but would love to know that I can run Freya happily on Kestrel and Nowin too, sometime fairly soon. To me Katana seems like a dead thing. For the future, and especially on Azure, IIS/Kestrel seems like the win.
But bottom-line: if Freya is going to continue to be OWIN-based, OWN must be evolved / fixed. A combination of OWIN and "OWIN bypass" sounds horrendous.
kolektiv
commented
8 years ago Ah, OWIN itself. Yes, I probably will try and pick up getting that in to a future spec, but the problem is that implementers (mainly Microsoft) aren't overly interested in sticking to the spec. The chances of getting evolutions pushed through are not all that high in terms of things like Kestrel.
I will certainly try and at least get the additional data added to the draft, and the choice of polyfill as a term was very much a conscious hope that it's more "pre-OWIN" than "OWIN-bypass". Only time will tell!
IIS/Kestrel and Suave are clearly important and will be tested with this approach. Kestrel is currently not an easy thing to work out, although I'll try! Nowin, I'm not sure - I'm not convinced many people use it in the wild, although I'm happy to hear otherwise.
Long term bottom-line, as you say: Decisions will have to be made around OWIN. The combination is not pleasant (although fairly unobtrusive in code). Given the attitude to standards, particularly from Microsoft, at some point it may be the right call to move Freya away from OWIN, but I'm hoping to avoid that. Whatever happens in that long term, the important thing will be to maintain the design and stability of the Freya principles.
statpro-awebb
commented
8 years ago Yes, I have little idea about Nowin uptake, but it is mentioned quite prominently on the new ASP.NET website's OWIN page. (Mike Hadlow also blogged about using it, sometime last year.)
Maybe an OWIN spec update to fix this issue (and others?) would be a good test case to see just how easy or difficult it's going to be to get Kestrel (or Kestrel's bridge to OWIN) to keep up with spec revisions... that would be useful information to know either way.
Re. Suave, I must say that @ademar has recently bent over backwards to address OWIN issues raised by both you and me:-
https://github.com/SuaveIO/suave/issues/398 https://github.com/SuaveIO/suave/issues/397 https://github.com/SuaveIO/suave/issues/387
kolektiv
commented
8 years ago Interesting on Nowin. Maybe I should revisit! True in terms of OWIN, going to take some politics...
On the Suave side, that's where I have least concerns. The Suave crew (@ademar, @haf, etc.) are easy to talk to and receptive to patches and help :smile:
haf
commented
8 years ago Thanks @kolektiv :heart_decoration:
A couple of issues have recently surfaced the flaw in OWIN that the original (percent-encoded) path and query is not available. This is problematic, in the case of Freya for routing - the URI template approach expects percent-encoded input.
A workaround was put in place (re-encoding the input before attempting a match) in the hope that this would fix the issue, but unfortunately it turns out that it doesn't - it has the side effect of encoding characters which were not previously encoded, breaking the matching algorithms for some URI Template expressions. (Many thanks to @mexx for attempting that fix, which was the right thing to try!)
In summary:
Looking at the issue, without changes to OWIN, there will never be a working solution to this without "bypassing" OWIN. The raw url is needed, and although it should be available, it isn't. Therefore the only solutions I can see are:
The second option I have working with HttpListener. I would expect that the Suave folks (@haf) might be kind enough to take pity on us and expose the raw path and query to the owin environment, even if non-standard (especially if I can provide a patch), so making that work would probably be fine too. Kestrel is a slightly unknown quantity, but I would expect possible.
I'm interested in opinions. It's not something that I want, it's not elegant (which I strongly dislike!) but it feels like the best of a bad situation. It'll have to be clearly documented, and I'm planning on having anything which depends on an adapter being present (such as the router) blow up with a custom exception giving a link to help docs and instructions when the adapter is not present. Not ideal, but it should only bite once, and in development not live!
On the plus side - it does bring back accurate and standards based routing within the scope of possibility.
Anyway, sorry for the small wall of text, i'm interested in opinions! Any thoughts from @mexx @ForNeVeR @haf @Vidarls @panesofglass - or anyone else I've not thought to ask, please mention them if you wish!