Fix Overlay of Combined Resources #10760

janbartel · 2023-10-20T01:47:31Z

Changes to the Resource class in jetty-12 lead to the breakage of war overlays for the jetty maven plugin, and also the scanning of directories when the base resource of a webapp is a CombinedResource.

gregw

I know this was my suggested fix, but it is not correct. We need to stay within the Resource abstraction.

...etty-ee10-annotations/src/main/java/org/eclipse/jetty/ee10/annotations/AnnotationParser.java

...etty-ee10-maven-plugin/src/main/java/org/eclipse/jetty/ee10/maven/plugin/OverlayManager.java

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources

joakime

Out of 6 suspect APIs.
I've found 2 broken APIs so far.
Still working on writing tests for the other 4.

CombinedResource.copyTo(Resource)

test if destination is a different file system type (BROKEN, have fixed code)

CombinedResource.contains(Resource other)

test if other is a different file system type

CombinedResource.getPathTo(Resource other)

test if other is a different file system type

Resource.contains(Resource other)

test if other is a different file system type
test if self is 1 JAR, and other is a different JAR

Resource.getPathTo(Resource other)

test if other is a different file system type (BROKEN, not possible to fix, revert?)

Resource.getAllResources()

test what happens if you have a symlink loop

joakime · 2023-10-24T14:41:18Z

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/FileID.java

-        Path relative = base.relativize(path);
-
-        int count = relative.getNameCount();
+        int count = path.getNameCount();


This version of FileID.isHidden() isn't useful for anyone.

Only the isHidden(Path base, Path path) version is meaningful.

If we have a baseResource like /path/.hidden/jetty/base and a test path within it of web/index.html, then this should return false on isHidden(base, path).

All the two arg version was/is doing is getting the relative path and checking the segments. This is just the version for code that already has the relative path.

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/FileID.java

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/CombinedResource.java

+ handles cross file system copy as well.

+ handles cross resource better.

…ize path.

joakime · 2023-10-24T21:05:56Z

I created a PR #10776 with extra testing (and fixes) for this PR.
@gregw need you to review that one too.

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/CombinedResourceTest.java

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources Reduced fallback to URI string manipulation.

gregw · 2023-10-25T01:21:28Z

@joakime I merged your PR, but have subsequently modified it to reduce the usage of URI string manipulation, as that is very error prone (for example a startWith is not sufficient as you also need to check the next character is a '/').

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources Reduced fallback to URI string manipulation.

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/FileID.java

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/CombinedResource.java

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java

...etty-ee10-maven-plugin/src/main/java/org/eclipse/jetty/ee10/maven/plugin/OverlayManager.java

.../jetty-ee9-annotations/src/main/java/org/eclipse/jetty/ee9/annotations/AnnotationParser.java

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources Reduced fallback to URI string manipulation.

joakime · 2023-10-25T17:56:01Z

This branch is failing the following tests ...

[ERROR] Failures: 
[ERROR]   ResourceListingTest.testBasicResourceXHtmlListingDeep:101 expected: <true> but was: <false>
[ERROR]   ResourceListingTest.testBasicResourceXHtmlListingRoot:72 expected: <true> but was: <false>
[ERROR]   ResourceListingTest.testListingEncoding:293 expected: <true> but was: <false>
[ERROR]   ResourceListingTest.testListingFilenamesOnly:199 expected: <true> but was: <false>\
[ERROR]   ResourceListingTest.testListingProperUrlEncoding:238 expected: <true> but was: <false>
[ERROR]   ResourceListingTest.testListingWithQuestionMarks:269 expected: <true> but was: <false>
[ERROR]   ResourceListingTest.testResourceCollectionXHtmlListingContext:143 expected: <true> but was: <false>

Oddly, these are failing on assertTrue(isValidXHtml(content)); tests.

joakime · 2023-10-25T17:59:36Z

Ah, those are failing due to ...

java.io.IOException: Server returned HTTP response code: 429 for URL: http://www.w3.org/MarkUp/DTD/xhtml-object-1.mod
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:2000)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1589)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLEntityManager.setupCurrentEntity(XMLEntityManager.java:677)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLEntityManager.startEntity(XMLEntityManager.java:1397)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLEntityManager.startEntity(XMLEntityManager.java:1333)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDTDScannerImpl.startPE(XMLDTDScannerImpl.java:732)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDTDScannerImpl.skipSeparator(XMLDTDScannerImpl.java:2101)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDTDScannerImpl.scanDecls(XMLDTDScannerImpl.java:2064)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDTDScannerImpl.scanDTDExternalSubset(XMLDTDScannerImpl.java:299)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl$DTDDriver.dispatch(XMLDocumentScannerImpl.java:1165)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl$DTDDriver.next(XMLDocumentScannerImpl.java:1040)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl$PrologDriver.next(XMLDocumentScannerImpl.java:943)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl.next(XMLDocumentScannerImpl.java:605)
	at java.xml/com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl.scanDocument(XMLDocumentFragmentScannerImpl.java:542)
	at java.xml/com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:889)
	at java.xml/com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:825)
	at java.xml/com.sun.org.apache.xerces.internal.parsers.XMLParser.parse(XMLParser.java:141)
	at java.xml/com.sun.org.apache.xerces.internal.parsers.DOMParser.parse(DOMParser.java:247)
	at java.xml/com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderImpl.parse(DocumentBuilderImpl.java:342)
	at java.xml/javax.xml.parsers.DocumentBuilder.parse(DocumentBuilder.java:122)
	at org.eclipse.jetty.server@12.0.3-SNAPSHOT/org.eclipse.jetty.server.ResourceListingTest.isValidXHtml(ResourceListingTest.java:335)
	at org.eclipse.jetty.server@12.0.3-SNAPSHOT/org.eclipse.jetty.server.ResourceListingTest.testBasicResourceXHtmlListingDeep(ResourceListingTest.java:101)

Something @olamy is taking care of.

I'll merge this branch up from jetty-12.0.x HEAD now, it should correct these test failures.

…overlay-combined-resources

gregw · 2023-10-25T20:46:23Z

Merging as CI failures are unrelated.

janbartel added 2 commits October 20, 2023 12:26

Fix overlays with CombinedResource

8c79083

Apply fix to ee9; remove comments in pom.xml

c50c78e

janbartel requested review from gregw, joakime and sbordet October 20, 2023 01:47

janbartel self-assigned this Oct 20, 2023

Add WEB-INF/classes to overlay

3714d2d

gregw requested changes Oct 22, 2023

View reviewed changes

...etty-ee10-annotations/src/main/java/org/eclipse/jetty/ee10/annotations/AnnotationParser.java Outdated Show resolved Hide resolved

gregw reviewed Oct 22, 2023

View reviewed changes

...etty-ee10-maven-plugin/src/main/java/org/eclipse/jetty/ee10/maven/plugin/OverlayManager.java Show resolved Hide resolved

gregw added 8 commits October 23, 2023 13:08

Improvements to CombinedResource for AnnotationParser

afafa69

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser

Improvements to CombinedResource for AnnotationParser

3570f46

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser

Improvements to CombinedResource for AnnotationParser

da3b0ce

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser

Improvements to CombinedResource for AnnotationParser

7c78683

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources

Improvements to CombinedResource for AnnotationParser

762e401

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources

Improvements to CombinedResource for AnnotationParser

9c5a4ac

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources

Improvements to CombinedResource for AnnotationParser

3d39e47

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources

Improvements to CombinedResource for AnnotationParser

68c5316

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources

gregw previously approved these changes Oct 24, 2023

View reviewed changes

gregw requested review from lorban and removed request for sbordet October 24, 2023 00:02

joakime requested changes Oct 24, 2023

View reviewed changes

joakime added 5 commits October 24, 2023 14:36

Fixes #10760 - Improved CombinedResource.copyTo and PathResource.copyTo

7418d55

+ handles cross file system copy as well.

Fixes #10760 - Improved Resource.contains and Resource.getPathTo

3bfec41

+ handles cross resource better.

Fixes #10760 - Improved CombinedResource.contains

7824a00

+ handles cross resource better.

Fixes #10760 - Skip AnnotationParser candidate if not able to relativ…

8cec6ad

…ize path.

Fixes #10760 - Add Resource.getAllResources testcase for symlink loops

7eff032

joakime mentioned this pull request Oct 24, 2023

More testing on branch jetty-12.0.x-overlay-combined-resources with eye on mixed resources #10776

Merged

Fixes #10760 - Remove PathResource.TreeCopyFileVisitor (no longer used)

91b94c7

Fixes #10760 - Checkstyle fix

156292b

joakime added 2 commits October 24, 2023 17:09

Fixes #10760 - Copy resources in reverse for proper Overlay behavior

82aea42

Fixes #10760 - Remove StringUtil.equals

16dea21

janbartel commented Oct 24, 2023

View reviewed changes

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/CombinedResourceTest.java Show resolved Hide resolved

Improvements to CombinedResource for AnnotationParser

283e2af

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources Reduced fallback to URI string manipulation.

gregw dismissed their stale review via 283e2af October 25, 2023 01:17

gregw requested a review from joakime October 25, 2023 01:21

gregw added 2 commits October 25, 2023 12:30

Improvements to CombinedResource for AnnotationParser

4cb7287

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources Reduced fallback to URI string manipulation.

Improvements to CombinedResource for AnnotationParser

c3f55dd

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources Reduced fallback to URI string manipulation.

lorban reviewed Oct 25, 2023

View reviewed changes

Improvements to CombinedResource for AnnotationParser

475902a

Added `contains` and `getPathTo` methods to the Resource API, so they can be used by the AnnotationParser Fixed numerous bugs in CombinedResource list and getAllResources Reduced fallback to URI string manipulation.

gregw requested review from lorban and gregw October 25, 2023 11:38

lorban approved these changes Oct 25, 2023

View reviewed changes

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…

ef491a0

…overlay-combined-resources

joakime approved these changes Oct 25, 2023

View reviewed changes

gregw merged commit 3ae4396 into jetty-12.0.x Oct 25, 2023
2 checks passed

joakime deleted the jetty-12.0.x-overlay-combined-resources branch October 26, 2023 15:44

joakime changed the title ~~Jetty 12.0.x overlay combined resources~~ Fix Overlay of Combined Resources Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Overlay of Combined Resources #10760

Fix Overlay of Combined Resources #10760

janbartel commented Oct 20, 2023

gregw left a comment

joakime left a comment

joakime Oct 24, 2023

gregw Oct 24, 2023

joakime commented Oct 24, 2023

gregw commented Oct 25, 2023

joakime commented Oct 25, 2023

joakime commented Oct 25, 2023

gregw commented Oct 25, 2023

Fix Overlay of Combined Resources #10760

Fix Overlay of Combined Resources #10760

Conversation

janbartel commented Oct 20, 2023

gregw left a comment

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

joakime Oct 24, 2023

Choose a reason for hiding this comment

gregw Oct 24, 2023

Choose a reason for hiding this comment

joakime commented Oct 24, 2023

gregw commented Oct 25, 2023

joakime commented Oct 25, 2023

joakime commented Oct 25, 2023

gregw commented Oct 25, 2023