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.

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

Create StaticHttpContentFactory and other cleanups #9043

Merged
merged 15 commits into from
Dec 23, 2022

Conversation

lachlan-roberts
Copy link
Contributor

  • Create StaticHttpContentFactory for stylesheet.
  • Move all HttpContent related classes into separate package.
  • Fix potential bug with FileMappingHttpContentFactory.
  • Other minor cleanups.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@joakime joakime added this to the 12.0.x milestone Dec 14, 2022
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

Not sure whats going on here, all these failing tests pass for me locally and I can step though and everything looks normal.

@joakime
Copy link
Contributor

joakime commented Dec 21, 2022

@lachlan-roberts something funny is going on with the Resource.getFileName() on CI.

On a local run, I see ...

2022-12-21 14:19:20.428:DEBUG:oejes.ServletHolder:qtp829149076-36: Servlet.init null for org.eclipse.jetty.ee10.servlet.DefaultServlet-20b2475a
2022-12-21 14:19:20.437:DEBUG:oejes.DefaultServlet:qtp829149076-36:   .baseResource = file:///home/joakim/code/jetty/jetty.project-12.0.x/jetty-ee10/jetty-ee10-servlet/target/tests/oejes.DefaultServletTest/testListingProperUrlEncoding/docroot/
2022-12-21 14:19:20.437:DEBUG:oejes.DefaultServlet:qtp829149076-36:   .resourceFactory = null
2022-12-21 14:19:20.437:DEBUG:oejes.DefaultServlet:qtp829149076-36:   .resourceService = org.eclipse.jetty.ee10.servlet.DefaultServlet$ServletResourceService@13154c37(contentFactory=PreCompressedHttpContentFactory@362df692[org.eclipse.jetty.http.content.VirtualHttpContentFactory@4a879256(factory=ResourceContentFactory[org.eclipse.jetty.util.resource.ResourceFactory$1@3721d992]@241977500, resource=file:/home/joakim/code/jetty/jetty.project-12.0.x/jetty-core/jetty-server/target/classes/org/eclipse/jetty/server/jetty-dir.css, matchSuffix=/jetty-dir.css, contentType=text/css),[]], dirAllowed=true, redirectWelcome=false)
2022-12-21 14:19:20.438:DEBUG:oejes.DefaultServlet:qtp829149076-36:   .isPathInfoOnly = false
2022-12-21 14:19:20.438:DEBUG:oejes.DefaultServlet:qtp829149076-36:   .welcomeExactServlets = false
2022-12-21 14:19:20.438:DEBUG:oejes.DefaultServlet:qtp829149076-36:   .welcomeServlets = false
2022-12-21 14:19:22.477:DEBUG:oejes.DefaultServlet:qtp829149076-36: doGet(req=org.eclipse.jetty.ee10.servlet.ServletContextRequest$ServletApiRequest@4d12235d, resp=org.eclipse.jetty.ee10.servlet.ServletContextResponse$ServletApiResponse@2a907f5d) pathInContext=/dir/, included=false
2022-12-21 14:20:35.070:DEBUG:oejes.DefaultServlet:qtp829149076-36: content = null

But on CI (with DEBUG enabled) I see ...

2022-12-21 18:47:17.767:DEBUG:oejes.ServletHolder:qtp1871048194-1492: Servlet.init null for org.eclipse.jetty.ee10.servlet.DefaultServlet-269222ae
2022-12-21 18:47:17.768:DEBUG:oejes.DefaultServlet:qtp1871048194-1492:   .baseResource = file:///home/jenkins/agent/workspace/jetty.project_PR-9043/jetty-ee10/jetty-ee10-servlet/target/tests/oejes.DefaultServletTest/testListingProperUrlEncoding/docroot/
2022-12-21 18:47:17.768:DEBUG:oejes.DefaultServlet:qtp1871048194-1492:   .resourceFactory = null
2022-12-21 18:47:17.768:DEBUG:oejes.DefaultServlet:qtp1871048194-1492:   .resourceService = org.eclipse.jetty.ee10.servlet.DefaultServlet$ServletResourceService@12f62694(contentFactory=PreCompressedHttpContentFactory@2a2b201a[org.eclipse.jetty.http.content.VirtualHttpContentFactory@6d5678dc(factory=ResourceContentFactory[org.eclipse.jetty.util.resource.ResourceFactory$1@3a456b4a]@421805180, resource=jar:file:///home/jenkins/agent/workspace/jetty.project_PR-9043/jetty-core/jetty-server/target/jetty-server-12.0.0-SNAPSHOT.jar!/org/eclipse/jetty/server/jetty-dir.css, matchSuffix=/, contentType=text/css),[]], dirAllowed=true, redirectWelcome=false)
2022-12-21 18:47:17.768:DEBUG:oejes.DefaultServlet:qtp1871048194-1492:   .isPathInfoOnly = false
2022-12-21 18:47:17.768:DEBUG:oejes.DefaultServlet:qtp1871048194-1492:   .welcomeExactServlets = false
2022-12-21 18:47:17.768:DEBUG:oejes.DefaultServlet:qtp1871048194-1492:   .welcomeServlets = false
2022-12-21 18:47:17.768:DEBUG:oejes.DefaultServlet:qtp1871048194-1492: doGet(req=org.eclipse.jetty.ee10.servlet.ServletContextRequest$ServletApiRequest@499db7e8, resp=org.eclipse.jetty.ee10.servlet.ServletContextResponse$ServletApiResponse@17d3a69f) pathInContext=/dir/, included=false
2022-12-21 18:47:17.768:DEBUG:oejes.DefaultServlet:qtp1871048194-1492: content = CompressedFormatsHttpContent@2facbfc5[ResourceHttpContent@3ea28e41{r=jar:file:///home/jenkins/agent/workspace/jetty.project_PR-9043/jetty-core/jetty-server/target/jetty-server-12.0.0-SNAPSHOT.jar!/org/eclipse/jetty/server/jetty-dir.css,ct=text/css}]

The CI run for pathInContext = "/dir/" results in an HttpContent of CompressedFormatsHttpContent@2facbfc5[ResourceHttpContent@3ea28e41{r=jar:file:///home/jenkins/agent/workspace/jetty.project_PR-9043/jetty-core/jetty-server/target/jetty-server-12.0.0-SNAPSHOT.jar!/org/eclipse/jetty/server/jetty-dir.css,ct=text/css}]

But on the local run, the same pathInContext = "/dir/" results in a null HttpContent.

If we look at the state of the VirtualHttpContentFactory we can see the following

Env VirtualHttpContentFactory Value
CI resource=jar:file:///home/jenkins/agent/workspace/jetty.project_PR-9043/jetty-core/jetty-server/target/jetty-server-12.0.0-SNAPSHOT.jar!/org/eclipse/jetty/server/jetty-dir.css
Local resource=file:/home/joakim/code/jetty/jetty.project-12.0.x/jetty-core/jetty-server/target/classes/org/eclipse/jetty/server/jetty-dir.css
CI matchSuffix=/
Local matchSuffix=/jetty-dir.css
CI contentType=text/css
Local contentType=text/css

The type of resource is different, file:// vs jar:file:// and the "matchSuffix" is different.

@joakime
Copy link
Contributor

joakime commented Dec 21, 2022

@lachlan-roberts I think I got a solution.

The MemoryResource doesn't have adequate testing, nor did the FileID.getFileName(URI) that it relies on.
The fix in commit 0485fdf should take care of this issue.

@@ -254,7 +258,7 @@ private String getInitParameter(String name, String... deprecated)
for (String d : deprecated)
{
value = super.getInitParameter(d);
if (value != name)
if (value != null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a legit fix to the Deprecation warning.

@joakime
Copy link
Contributor

joakime commented Dec 22, 2022

@lachlan-roberts this PR is now green

@lachlan-roberts
Copy link
Contributor Author

thanks @joakime, I will merge once I get an approved review

@lachlan-roberts lachlan-roberts merged commit 0e95953 into jetty-12.0.x Dec 23, 2022
@lachlan-roberts lachlan-roberts deleted the jetty-12.0.x-httpcontentFactoryCleanup branch December 23, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants