return 404 for "not a directory" errors #1409

Merged
merged 1 commit into from Feb 16, 2017

Projects

None yet

2 participants

@mastercactapus
Collaborator

Fixes #1408

@mholt
Owner
mholt commented Feb 8, 2017

Thanks! I'm kind of watching golang/go#18974 and now golang/go#18984 to see what happens and when/if it gets fixed in the standard lib. If it does get fixed there, we could remove this hack later, which means we'd want to accompany it with a TODO of some sort.

@mholt

Based on how those issues upstream went, it looks like we're stuck with doing this. Thanks for taking care of it!

caddyhttp/staticfiles/fileserver.go
+
+ return perr.Err.Error() == "not a directory"
+}
+
@mholt
mholt Feb 8, 2017 Owner

Can we move this lower in the file to keep the serveFile and ServeHTTP methods close together?

@mholt
mholt Feb 8, 2017 Owner

Also a godoc on this would be useful.

@mastercactapus
mastercactapus Feb 8, 2017 Collaborator

Yeah, I'm actually going to revisit this and update the PR with the fix I'm using/submitting to golang. I'll add the TODO and comments as well -- hopefully sometime today.

@mastercactapus mastercactapus add fix from golang/go
18edf58
@mastercactapus
Collaborator

@mholt I think this is ready to be looked at again when you get a chance. The fix has now been merged into golang/go. I've used that implementation here, and added TODO comments to remove it when the fix is released.

Let me know if you want me to revise anything!

@mholt
Owner
mholt commented Feb 14, 2017

Thanks @mastercactapus - I see they merged a fix into Go. Am I correct in understanding that this patch is temporary, then, until they release a Go version that has your patch?

@mastercactapus
Collaborator

@mholt That is correct. I also tested without this patch against Go master to make sure it was resolved (and how I found an additional case). This is the same code that got merged into Go, adapted of course.

@mholt
Owner
mholt commented Feb 16, 2017

@mastercactapus Cool. When that patch is released, will you update Caddy so this patch won't be redundant? If so, I'll merge this.

@mastercactapus
Collaborator

@mholt definitely, I can own it.

@mholt
mholt approved these changes Feb 16, 2017 View changes

Thank you for working on this!

@mholt mholt merged commit bdb61f4 into mholt:master Feb 16, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@mholt
Owner
mholt commented Feb 16, 2017

@mastercactapus Mind if I invite you to be a collaborator? I can't remember if I've done this for you yet, but it doesn't look like I have. This way you could contribute in a branch instead of a fork, and help with reviewing pull requests and issues when you are able.

@mastercactapus
Collaborator

@mholt I don't mind, I'd be happy to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment