box.Open: Do not call file.NewFileR when IsDir true, fix #198

[nlepage]

Hi 👋

This is an attempt to fix issue #198

The redirect loop seems to happen because isDir=true is lost before returning to http.serveFile...

Not sure this is the right fix, but it worked for me.

Thx in advance for your review.

[markbates]

Would you mind adding a test to make sure this won’t regress? Thanks.

[nlepage]

Would you mind adding a test to make sure this won’t regress? Thanks.

Yep no problem.

[davidovich]

LGTM

[nlepage]

I reused Test_Box_Open and added assertions on f.Stat().IsDir().

The test is failing on the path "/baz" which has isDir=false, although "baz" has isDir=true...

It appears that the test doesn't cover the new condition I added in box.Open() !

Both paths "baz" and "/baz" are not found by box.Resolve() and are resolved by box.openWoExt() !

I'm going to dig a little further on this...

BTW the CI reports a success although the test is failing !

[nlepage]

I've been looking into box.openWoExt().

First thing I noticed is that unlike box.Resolve() it doesn't trim the leading / of the path, which explains the difference of results between "baz" and "/baz".

And secondly, I'm surprised box.openWoExt() is even there, my guess would have been that it's the job of the http package to look for an index file.

So I looked into http.FileServer() and it has the mechanic to serve the index file in place of the dir: https://golang.org/src/net/http/fs.go?s=16499:16554#L592

I also tested the behavior of http.Dir, and it never looks for an index file but rather returns isDir=true when appropriate.

So I think the behavior of Box should be the same, and we should remove box.openWoExt(), @markbates what do you think ?

[nlepage]

Sorry for the spam 🙂

I've been looking at the different resolvers in v2/file/resolver, and their interactions with box.Box, and I noticed some differences in their behavior:

• In packed mode, HexGzip is not responsible for telling if a path is a directory, but the Box is able to because Box.SetResolver is called and fills the dirMap
• In disk mode, resolver.Disk is responsible for telling if a path is a directory, so the Box doesn't have to use the dirMap
• In tests, resolver.InMemory is not responsible for telling if a path is a directory, but openWoExt() calls box.HasDir() which will fill the dirMap

In 3 cases the behavior is rather different, and this makes the tests written for box.Open() a little irrelevant...

@markbates don't you think a more consistent behavior would be better ?

[markbates]

Absolutely!

[nlepage]

OK, I'm not sure where to go from here...

@markbates could you give me some pointers ?

In your opinion is it the responsibility of the Box or of the resolvers to tell if a path is a directory ?

[markbates]

put it all in Box would be central place to do it, however, without digging deeper, I can't tell for sure whether or not the implementation would have to be dependent on the resolver.

This is a notoriously tricky part of Packr, even going back to v1. It's a bit of a whack-a-mole issue, as you're finding out. :)

I appreciate you trying to fix this.

[nlepage]

No problem, I'll try something.

[comp500]

If anyone wants to depend on this PR before it is merged (using go modules), add the following directive to the bottom of your go.mod file (keeping the normal packr dependency in your require directive):

replace github.com/gobuffalo/packr/v2 => github.com/nlepage/packr/v2 v2.2.1-0.20190515165437-bf153a1f0078