Issue #10084 - Directory results from getResourcePaths(String) should include trailing slash

[joakime]

• Adding support for trailing slashes on getResourcePaths(String) results.
• Backporting tests from ee10 to ee9
• Fixing bug in getRealPath(String) as well

[joakime]

This can be fixed with @lorban backporting his work in PR #9974 back to ee9 tree.

[lorban]

That would also require backporting the fix to 10/11.

[gregw]

Is it possible to resolve this within the resource classes? Previously the resource classes would have the trailing '/' on directories so it would be everywhere. Doing it just here as a post process risks not covering all usages.

[joakime]

The Resource object returns a List<Resource> so I think perhaps the place would be Resource.getFileName() ?

[gregw]

Yes getFileName, toString and anything else that returns the path as a string. Ideally even the URI should have the trailing slash. But
I don't think Path retains it.

[joakime]

Turns out fixing getFileName() breaks a lot of test cases.
See latest Jenkins build on this PR.
I'll noodle through them on monday.

[gregw]

Would be good to check if any of these texts existed in jetty 11 and see if they were changed to not expect the trailing slash

[joakime]

The Jetty 11 Resource.getName() does not return the trailing slash on directories.

However, the Jetty 11 Resource.list() returns a String array, and that array has the trailing slash.
The Resource.list() method in Jetty 11 is used ...

• ContextHandler.getResourcePaths(String) which returns a Set<String> on the Servlet API
• Resource.getListHTML - used to iterate a directory, and build a new set of Resources, that eventually are presented (in HTML)
• MetaInfConfiguration - iterates over WEB-INF/lib/*.jar files, and builds a new set of Resources that eventually wind up in the WebAppClassLoader
• WebAppClassLoader - used in addJars(Resource) to iterate a directory and build a new set of Resources that are added to the WebAppClassLoader
• Runner.Classpath.addJars(Resource) - same a WebAppClassLoader.addJars(Resource)

Turns out that in Jetty 12 we skip the String array step (eg: the Resource -> String array -> List of Resource -> iterate/process flow).
In Jetty 11, we don't rely on the results of Resource.getName() for anything critical, it seems to be used only for things like debugging, exception messages, keys in the annotation parser, etc.

[lorban]

That would also require backporting the fix to 10/11.

Stale review

[gregw]

@joakime whoa! you're dismissing my reviews now and merging anyway?!?!?!
I still believe this is the wrong approach of just fixing listing. It looks like the dropping of the trailing "/" for directories was made when you switched from FileResource to PathResource.
Fixing it like this just feels like playing what-a-mole with resource bugs.

[gregw]

@joakime I have pushed an extra test in c435bfd that checks that for a getResource(String) the result includes a trailing slash for a directory. This test passes, because it turns out that we already have the code in PathResource to ensure there is a trailing slash on the URI.

So I'm less concerned about this PR now... other than dismissing negative review and that it feels wrong that we already have a Resource that internally has the trailing '/' and that returns that in the URI, but that here we are using getFilename() that doesn't return the trailing slash, so we have to re-add it. Looks like make work to me and a chance to get something wrong. Surely we can ask a resource for it's segment relative to another and not have to do the work again externally?

[joakime]

See #10085 (comment)

The behavior in Jetty 9/10/11 for this ...

• Trailing slash was present only in ServletContext.getResourcePaths(String) usages
• Resource.list() was the place a trailing slash was done.
• Resource.list() returned a String[]
• Resource.list() was only used as-is for ServletContext.getResourcePaths(String) (all other uses of Resource.list() took the values and created another collection of Resource objects, which meant the trailing slash wasn't meaningful)
• The Resource.getName() method did not return a trailing slash for JarFile, File, and Path implementations.
• URLResource.getName() returned the trailing slash, but only if it was present on the input (it didn't add it)
• URLResource behavior with getName() was not applied to JarFile implementation.

[joakime]

@joakime I have pushed an extra test in c435bfd that checks that for a getResource(String) the result includes a trailing slash for a directory. This test passes, because it turns out that we already have the code in PathResource to ensure there is a trailing slash on the URI.

The getResource(String) returns a value in URL path terms, right? (not filesystem path terms).

The getRealPath(String) likely has the same requirements for trailing slash, but the returned value is in native file system path terms.

Examples:
We have a webapp (exploded directory) on windows, in path C:\jetty\webapps\context\.

• getRealPath("/WEB-INF/web.xml") should return C:\jetty\webapps\context\WEB-INF\web.xml
• getRealPath("/css/") should return C:\jetty\webapps\context\css\

Right?

[gregw]

Correct, getResource(String) returns a URL. This is correctly including a trailing slash for directories because our resource implementations correctly contain an internal URI that includes the trailing slash for directories.

It seams wrong/fragile that in order to implement getResourcePaths(String) that there is not some API in resource that can give reasonable access to this trailing slash. Instead this PR used a filename method, which probably rightly doesn't include the trailing slash and then has to recreate the trailing slash repeating existing logic.

It is not explicit what space that the return set of getResourcePaths(String) is in, but given that it doesn't have "real" in it's method name and the examples all include '/', then I think it is safe to assume it is a set of partial URI path segments, decoded (like most other servlet APIs). It should be possible to extract that from the URI in the resource.

But, this got merged without approval, so I guess it is done. But let's remember this when doing any future resource work and look to see if we can avoid duplicating this logic.