Skip to content
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

ServletApiContext.getResourcePaths() doesn't respect the spec #10084

Closed
ppodgorsek opened this issue Jul 7, 2023 · 6 comments · Fixed by #10085
Closed

ServletApiContext.getResourcePaths() doesn't respect the spec #10084

ppodgorsek opened this issue Jul 7, 2023 · 6 comments · Fixed by #10085
Assignees
@joakime
Labels
Bug For general bugs on Jetty side
Milestone
12.0.x

Comments

@ppodgorsek
Copy link

Jetty version(s)

12.0.0.beta3

Java version/vendor (use: java -version)

openjdk version "17.0.7" 2023-04-18
OpenJDK Runtime Environment (Red_Hat-17.0.7.0.7-4.fc38) (build 17.0.7+7)
OpenJDK 64-Bit Server VM (Red_Hat-17.0.7.0.7-4.fc38) (build 17.0.7+7, mixed mode, sharing)

OS type/version

Fedora 38

Description

I am trying to list files and directories within a WEB-INF/ folder which has a structure similar to the one below:

/WEB-INF/
  lib/
  custom-taglibs/
    my-tablib.tld
  web.xml

According to the spec, a call to servletApiContext.getResourcePaths(/WEB-INF/) should return:

  • /WEB-INF/lib/
  • /WEB-INF/custom-taglibs/
  • /WEB-INF/web.xml

Instead, the method returns directory names without a trailing /:

  • /WEB-INF/lib
  • /WEB-INF/custom-taglibs
  • /WEB-INF/web.xml

This makes it much harder to determine which entries are subdirectories and which ones aren't.

How to reproduce?

Create subdirectories in an application's /WEB-INF/ folder and call ServletApiContext.getResourcePaths("/WEB-INF/").
@ppodgorsek ppodgorsek added the Bug For general bugs on Jetty side label Jul 7, 2023
@joakime
Copy link
Contributor

joakime commented Jul 7, 2023

Interesting...

The Servlet TCK doesn't actually test the Strings in the Set returned from getResourcePaths(String)

https://github.com/jakartaee/platform-tck/blob/10.0.0.Final/src/com/sun/ts/tests/servlet/api/jakarta_servlet/servletcontext/TestServlet.java#L195

Just that there are values. (empty set or null set are failures to the TCK)

@joakime
Copy link
Contributor

joakime commented Jul 7, 2023

And getResourcePaths(String) is not actually present in the SPEC document.

https://github.com/jakartaee/servlet/blob/6.0.0/spec/src/main/asciidoc/servlet-spec-body.adoc

@joakime
Copy link
Contributor

joakime commented Jul 7, 2023

The javadoc only shows an example with a trailing slash.

https://github.com/jakartaee/servlet/blob/6.0.0/api/src/main/java/jakarta/servlet/ServletContext.java#L172-L216

I think we should return the trailing slash, can't see why we would not.
Just amazed that this isn't in the TCK or SPEC. (hey, we pass the Servlet 6.0 TCK on Jetty 12.0.0.beta3, we should be good, right?)

@joakime joakime self-assigned this Jul 7, 2023
joakime added a commit that referenced this issue Jul 7, 2023
@joakime
Issue #10084 - Directory entries on return of getResourcePaths(String…
a4f7c02 
…) should include trailing slash
@joakime joakime moved this to 👀 In review in Jetty 12.0.0.beta4 Jul 7, 2023
@joakime joakime moved this from 👀 In review to 🏗 In progress in Jetty 12.0.0.beta4 Jul 7, 2023
@joakime joakime added this to the 12.0.x milestone Jul 7, 2023
@joakime joakime linked a pull request Jul 7, 2023 that will close this issue
Issue #10084 - Directory results from getResourcePaths(String) should include trailing slash #10085
Merged
joakime added a commit that referenced this issue Jul 7, 2023
@joakime
Issue #10084 - Fixing test case order of entries in collection expect…
8e4da80 
…ation
joakime added a commit that referenced this issue Jul 7, 2023
@joakime
Issue #10084 - Implementing fix for ee9
e9e0780
@joakime
Copy link
Contributor

joakime commented Jul 7, 2023

Opened PR #10085

joakime added a commit that referenced this issue Jul 7, 2023
@joakime
Issue #10084 - Fixing bug in ServletContext.getRealPath() impl
ea80c52
@ppodgorsek
Copy link
Author

Thank you for being so reactive, @joakime ! And thank you for the explanation, it's very good to know, I always thought the interface reflected the official spec. (that trailing / rule was already mentioned in the JavaDoc since Javax Servlet 2.3)

I'll definitely spend more time going through the TCK to learn more about it. :)

joakime added a commit that referenced this issue Jul 8, 2023
@joakime
Issue #10084 - Fixing tests in ee9 to make them compatible with ee8 c…
1d7a62e 
…onversion
joakime added a commit that referenced this issue Jul 8, 2023
@joakime
Issue #10084 - Correcting PathResource.getFileName
96a3208
joakime added a commit that referenced this issue Jul 8, 2023
@joakime
Issue #10084 - Fixing checkstyle
7875230
joakime added a commit that referenced this issue Jul 8, 2023
@joakime
Issue #10084 - Updating URLResource.getFileName() support
10fbabd
joakime added a commit that referenced this issue Jul 10, 2023
@joakime
Issue #10084 - Reverting change to Resource.getFileName to return tra…
f57cb04 
…iling slash on dirs

+ Bring Resource.getFileName in
  alignment with other JVM methods
  of the same name.
  eg: Path.getFileName
joakime added a commit that referenced this issue Jul 10, 2023
@joakime
Issue #10084 - Introduce trailing slash only on ServletContext.getRes…
4927b1d 
…ourcePaths(String) again
joakime added a commit that referenced this issue Jul 10, 2023
@joakime
Issue #10084 - Fixing comments
8172a46
joakime added a commit that referenced this issue Jul 11, 2023
@joakime
Issue #10084 - Addressing ResourceFactory usage review
9d6679b
joakime added a commit that referenced this issue Jul 11, 2023
@joakime
Issue #10084 - Directory results from getResourcePaths(String) shou…
c8fd1a1 
…ld include trailing slash (#10085)

* Issue #10084 - Directory entries on return of getResourcePaths(String) should include trailing slash
* Issue #10084 - Fixing test case order of entries in collection expectation
* Issue #10084 - Implementing fix for ee9
* Issue #10084 - Fixing bug in ServletContext.getRealPath() impl
* Issue #10084 - Fixing tests in ee9 to make them compatible with ee8 conversion
* Bring Resource.getFileName in alignment with other JVM methods of the same name. (eg: Path.getFileName)
@joakime
Copy link
Contributor

joakime commented Jul 11, 2023

PR #10085 is now merged into jetty-12.0.x HEAD

@joakime joakime closed this as completed Jul 11, 2023
@joakime joakime moved this from 🏗 In progress to ✅ Done in Jetty 12.0.0.beta4 Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@joakime joakime

Labels
Bug For general bugs on Jetty side
Projects
None yet
Milestone
12.0.x
2 participants
@joakime @ppodgorsek