This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
- # beginners (106)
- # boot (124)
- # cider (11)
- # clojure (105)
- # clojure-poland (2)
- # clojure-russia (28)
- # clojurescript (89)
- # core-async (14)
- # cursive (10)
- # datomic (7)
- # emacs (12)
- # garden (5)
- # hoplon (345)
- # immutant (127)
- # mount (2)
- # off-topic (24)
- # om (24)
- # onyx (8)
- # parinfer (51)
- # proton (2)
- # slack-help (4)
- # spacemacs (1)
@weavejester: in the servlet spec, path-info is always returned decoded from HttpServletRequest.getPathInfo()
@tcrawley: Right, but one of the nice things about Ring is that we don’t need to make the same mistakes as servlets. Because URL encode/decode is lossy, particularly since Java changes “+” into “ “, it makes more sense to keep path-info raw.
ah, so you're saying immutant should instead calculate the raw path-info when constructing the request map instead of using
getPathInfo? That might work
Yep. I actually had assumed that the servlet spec didn’t decode PathInfo, which is why there isn’t a huge amount of documentation for it.
So you can just use
(ring.util.request/set-context request (.getContextPath servlet-request))
@weavejester: i take your point, but since context and path-info are strictly servlet things (not ring), why not follow their spec? someone familiar with getPathInfo isn't going to expect to see that "+" in :path-info
jcrossley3: They’re not strictly servlet things. Other libraries, like Compojure, make use of them.
I also don’t see the point of adopting mistakes made in earlier libraries just for compatibility.
They’re not in the Ring spec, but they are in the Ring library, like :params or :session.
https://groups.google.com/forum/#!topic/rack-devel/3fRiLmm-pT8 some background, i guess
:path-info might have been a poorly chosen name, considering that it’s slightly different to the PATH_INFO in CGI, and getPathInfo in servlets.
I think that's where my confusion came from - since it's not in the ring spec, and named the same as the servlet value, I assumed it was the servlet value
I had assumed that :path-info in Ring was the same as the servlet version, but it turns out that’s not the case.
I might use
:path in future and deprecate
:path-info… but that likely won’t happen for a while.
Fortunately Ring doesn’t need to, so we can break with tradition a little. Ideally it would be named something different, but for now it can just be documented better.
I know you've stated that it shouldn't be part of the ring spec, but if it was part of the spec, it would be a stronger stance than documentation of the implementation's behavior would be
Well, the Ring spec just covers HTTP. However, it might be worth putting up something else for the keys introduced by the library itself, like :context, :path-info, :params, :session and so forth.
@weavejester: not to dissuade you from fixing the web, but do you see no benefit in keeping ring at least a little in-line with wsgi, rack, servlets, etc?
e.g. after introducing :path, you might then decode :path-info rather than deprecate it
jcrossley3: I do see a benefit in being consistent. However (a) it’s too late now, and (b) the problems a decoded :path-info provides outweigh the advantages of compatibility, IMO.
i can respect that, but like you say, it'd be good to be clear about where you diverge with CGI, et. al.
jcrossley3: I don't see users getting confused about :path-info being raw in ring, as long as it is clearly documented
The only reason there isn’t documentation is because I didn’t realise that it was different
and clearly no one has complained about it until now, and the complaint was "this shouldn't be decoded"
It sounds like nothing is really going to change, and this “bug” such as it is will remain.
It’s certainly not a huge deal to pass in a builder to Undertow to make sure that the url and path info is not decoded, so as to let Ring and Compojure do their thing. But seems like we may need better documentation for both Immutant (in terms of what Undertow is doing) and Ring/Compojure (in terms of where stuff is actually decoded) to help users not flail about when they run into what I did.
Apologies if I’m misunderstanding what the above discussion concluded; I just don’t see where it addresses my particular issue.
@ddellacosta: I’ll add some documentation to the
set-context functions, and probably add a page to the wiki as well documenting the keys that the Ring library adds above and beyond the Ring specification. It’s also a possibility that I can add this into the servlet library as well.
I see—so one will still have to toggle decoding off in Undertow. (I realize this is not your particular concern @weavejester, just clarifying)
Please note also that where I first encountered this was in Compojure’s handling of params extracted from
@ddellacosta: I think we will fix this in Immutant by populating the request map with a :path-info that isn't decoded
and in the meantime I will continue to pass in the builder w/DECODE_URL toggled off to handle this, as a workaround
cool, I'm glad you have a workaround for now. I'd love to get an Immutant release out in the next couple of weeks
sure thing—and if I can contribute anything else (happy to write the patch if you’d like) just let me know.
if you want to write a patch, that would be swell - you would have to fix it in two places
the former is used when inside WildFly or EAP, the latter when using undertow directly
(I assume I’ll have to possibly extract that info from somewhere else if .getPathInfo/.getRelativePath returns the decoded value by default)
you won't be able to prevent the decoding of the values returned by
getRelativePath - you'll have to ignore those methods and extract the path-info from the uri by stripping off the context, yeah
will let you know if I’m going to be slower than by Monday @tcrawley, if that works for you
@ddellacosta: well, this is embarrassing - I did know about path-info being escaped, and how that was the wrong behavior, because I fixed it in Immutant 1: https://github.com/immutant/immutant/tree/1.x/modules/web/src/main/clojure/immutant/web/servlet.clj#L31
In fact, I remember it being that way in Immutant 2, but without the comment to explain why
that looks like the right fix too; was trying to find some utils already available to pull that info out of
I did plumb through the commits for Immutant 2, but can't find anywhere where we had it setting a raw path-info, so I feel less dumb
okay, good to know. And you should definitely not feel dumb about this, easy thing to understand