8000 wai-app-static: links in listings are broken when used in conjunction with `UrlMap` · Issue #325 · yesodweb/wai · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

wai-app-static: links in listings are broken when used in conjunction with UrlMap #325

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.

Sign up for GitHub

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

Jump to bottom
Closed
soenkehahn opened this issue Jan 13, 2015 · 13 comments
Closed

wai-app-static: links in listings are broken when used in conjunction with UrlMap #325

soenkehahn opened this issue Jan 13, 2015 · 13 comments

Comments

@soenkehahn
Copy link
Contributor

When using UrlMap to mount a fileserver (created with wai-app-static) on a subpath, wai-app-static does not return a redirect to add a trailing /. This results in incorrect links in the file listing that is returned by default from wai-app-static.

I've tried to demonstrate the issue in this failing test case: soenkehahn@5e45529.

It could be that I'm misunderstanding this. Or am I supposed to add the redirection myself around the fileserver app?
@snoyberg
Copy link
Member

This bug report unfortunately doesn't surprise me. I'll look into this either today or tomorrow.

snoyberg added a commit that referenced this issue Jan 14, 2015
@snoyberg
Trailing slashes for non-root apps #325
b374057
@snoyberg
Copy link
Member

I've pushed some commits to the 325-wip branch. There are two bugs here:

  • UrlMap is mistakenly setting rawPathInfo, which should be left alone.
  • wai-app-static wasn't using rawPathInfo for detecting the proper paths.

I'm not sure if I can actually release this change to UrlMap, however, since it breaks semantics. I'll need to think about it more tomorrow (running out of the house for the rest of the day now), but input would be appreciated.

@gregwebs
Copy link
Member

I am not sure if the UrlMap behavior is wrong. I think it depends on the usage pattern.

@snoyberg
Copy link
Member

I believe it is incorrect... but the reason for that is my fault. I've mentioned in various places the specification for rawPathInfo, but I never actually wrote it in the docs, which was completely stupidity on my part. rawPathInfo is supposed to remain unchanged, for purposes of creating redirects. pathInfo is allowed to be modified by middlewares and the like. This came up originally when designing Yesod subsites.

I'm not sure if, at this point, it makes sense to codify this as required behavior. At the very least, it's worth pointing out as one original intention in the docs. Regarding UrlMap, I'm not sure if we should change it. We may need to introduce some parameter to control this behavior. But I still need to think about it a bit more.

@gregwebs
Copy link
Member

Would it be better named as redirectPath then? And also better to have special setter functions for it? Actually it would be best to have setters for both path fields.

@snoyberg
Copy link
Member

I think it's correctly named: it's the raw path submitted by the user. Setter functions would be great, but that ship has sailed (unless you're thinking of something different than abstract data types).

@gregwebs
Copy link
Member

well, we can create setter functions now and state that is the preferred way to operate on the fields, but you are right that we can't take away the existing ways to operate on the fields.

snoyberg added a commit that referenced this issue Jan 14, 2015
@snoyberg
Trailing slashes for non-root apps #325
000490a
@snoyberg
Copy link
Member

Doh, I'm an idiot, I merged this to master by mistake. I'll undo the UrlMap change for now, but everything else can stay as-is I think.

@soenkehahn
Copy link
Contributor Author

I have to confess that I cheated a bit on this issue, this actually came up in servant-server: https://github.com/haskell-servant/servant-server/issues/1. It's just that while trying to figure out what was going on I realized that I could reproduce the bug with UrlMap instead of servant-server. And that made for a much better test case.

servant-server however does not modify rawPathInfo, but only pathInfo. I just tested the newer version of wai-app-static with servant-server and everything works as expected there now. So from my side the bug is fixed. Many thanks!

(I'll leave this ticket open for the UrlMap thing, but please feel free to close it.)

@snoyberg
Copy link
Member

Cool thanks. I think in that case I'll update the wai docs to discuss this
issue without making it a requirement to change behavior and then release
both packages.

On Thu, Jan 15, 2015, 5:24 AM Sönke Hahn notifications@github.com wrote:

I have to confess that I cheated a bit on this issue, this actually came
up in servant-server: haskell-servant/servant-server#1
https://github.com/haskell-servant/servant-server/issues/1. It's just
that while trying to figure out what was going on I realized that I could
reproduce the bug with UrlMap instead of servant-server. And that made
for a much better test case.

servant-server however does not modify rawPathInfo, but only pathInfo.
I just tested the newer version of wai-app-static with servant-server and
everything works as expected there now. So from my side the bug is fixed.
Many thanks!

(I'll leave this ticket open for the UrlMap thing, but please feel free
to close it.)

—
Reply to this email directly or view it on GitHub
#325 (comment).

@soenkehahn
Copy link
Contributor Author

Great, much appreciated!

@snoyberg snoyberg mentioned this issue Jan 15, 2015
Closed
snoyberg added a commit that referenced this issue Jan 15, 2015
@snoyberg
Clarify documentation on rawPathInfo #325
c341349
@snoyberg
Copy link
Member

Looks like I did put in some documentation, but it wasn't clear enough. I've expanded the comments on rawPathInfo as well. I'm releasing both packages now, and I believe this issue can be closed.

snoyberg added a commit that referenced this issue Jan 15, 2015
@snoyberg
Revert UrlMap changes from #325
6506cf0
@soenkehahn
Copy link
Contributor Author
6939 soenkehahn commented Jan 16, 2015

Yes, you did. And IIRC I read it and that's why servant does the right thing. :) But the new comment is better.

Thanks for the releases!

@dbaynard dbaynard mentioned this issue Aug 5, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gregwebs @snoyberg @soenkehahn
0