include ref in path as WW-5011 workaround by yasserzamani · Pull Request #353 · apache/struts

yasserzamani · 2019-04-23T09:28:47Z

include ref in path for StrutsApplicationResource as WW-5011 workaround (when real path or filename contains "#" character)

yasserzamani · 2019-04-23T09:49:22Z

FYI I checked all references (usages) of org.apache.tiles.request.ApplicationResource and org.apache.struts2.tiles.StrutsApplicationResource and couldn't find any scenario that proofs this PR breaks backward-compatibility. do you?

coveralls · 2019-04-23T10:02:34Z

Coverage decreased (-0.01%) to 47.014% when pulling fd91e75 on yasserzamani:WW-5011-2-5 into b23bfd4 on apache:struts-2-5-x.

lukaszlenart · 2019-04-23T10:21:06Z

This will break backward-compatibility if someone extends StrutsApplicationResource in his app.

yasserzamani · 2019-04-24T05:12:44Z

👍 I reset the PR to minimize backward-compatibility issues as much as possible :)

yasserzamani · 2019-04-24T05:39:57Z

Oh I realized .toURI().getPath() itself fixes issue. If wasn't possible, now it follows my previous solution.

ℹ️ Squash is recommended if accepted

JCgH4164838Gh792C124B5

Hello @yasserzamani .

Thanks for advising me of this PR. 😄

After reviewing the changes I've commented in locations where some risks could be addressed.

For 2.5.x I'm still more comfortable with an opt-in/opt-out mechanism, even with a fallback-oriented logic like you've created in this PR.

Maybe this PR could consider using the new logic by default but allow an opt-out flag "struts.tiles.originalFileResourceHandling" (similar but opposite to the opt-in flag in PR#348) ?

That strategy would essentially eliminate backwards-compatibility risk for 2.5.x but still allow for the "new and improved" logic to be transparent by default.

Either way good work on the PR so far. 👍

Let me know what you think and let me know if my comments make sense or not.

JCgH4164838Gh792C124B5 · 2019-04-25T01:06:16Z

plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsApplicationResource.java

+        String path;
+        try {
+            //workarounds WW-5011 because includes ref in path
+            path = url.toURI().getPath();


This might work, but I'm not sure whether a sub-context representation will always equate to a URL/URI anchor/fragment.

I'm not sure if I could understand well but URI doesn't have getRef method while URL has which means URI has included ref (i.e. what is after "#") in path. My added unit tests verify this.

JCgH4164838Gh792C124B5 · 2019-04-25T01:13:36Z

plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsApplicationResource.java

+            // fallback solution
+            path = url.getPath();
+
+            if (url.getRef() != null && !new File(path).exists()) {


Similar comment about sub-context representation and anchor/fragment "#".

A larger concern is there is now a File existence check happening here. That seems very risky as the URL can represent a non-file protocol resource.

But getLastModified method already considers it should be a file.

It seems getLastModified() considers the possibility that resource (path) is a file, but it doesn't assume it has to be a file.

If the path doesn't exist as a file (e.g. nonexistent file or non-file URL) then exists() will be false and getLastModified() just returns 0. (As a side note the base Tiles interface also allows for an IOException to be thrown if there's no modified date available ... but Struts has attempted to avoid that).

fixed :) - learnt from tiles URLApplicationResource.

JCgH4164838Gh792C124B5 · 2019-04-25T01:24:18Z

plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsApplicationResource.java

+    /**
+     * workarounds WW-5011
+     */
+    private static String getExistedPath(URL url) throws MalformedURLException {


Maybe getExistedPath() would be clearer with a name like getExtractedPath() ?

I named such because WW-5011 is because url.getPath doesn't exist always. What does Extracted refer?

I thought "Extracted" was more general - just indicates the method attempts to extract the path from the URL (whether it actually exists or not).
Since the URL resource may not be a file, a File.exists() test cannot always determine if the path actually exists or not...

fixed :) - learnt from tiles URLApplicationResource.

JCgH4164838Gh792C124B5 · 2019-04-25T01:32:14Z

plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsApplicationResource.java

    @Override
    public long getLastModified() throws IOException {
-        File file = new File(url.getPath());
+        File file = new File(super.getLocalePath());


I think this should call getExistedPath(url) (or its equivalent if it gets renamed).

The constructor used to call url.getPath() for its parameter, just like this method used to.
Since the constructor now calls getExistedPath(url), this method should probably call it too (for consistency).

We already have passed it to super and we don't need compute it once again. My added unit tests show it seems work.

The problem is super.getLocalePath() (from the Tiles code) doesn't guarantee return of the exact path string passed to super. Unfortunately the unit test can't easily check all possibilities, and changing the parameter passed risks breaking existing applications unexpectedly (in hard to predict ways).

If we want to avoid re-computing the path value, then maybe we should just add a new attribute to cache the URL path string in this PR and modify it like this:

final String urlPath; // Code up to the constructor public StrutsApplicationResource(URL url) throws MalformedURLException { urlPath = getExistedPath(url); super(urlPath); this.url = url; } // Intervening code @Override public long getLastModified() throws IOException { File file = new File(urlPath); // Rest of code

fixed :) - learnt from tiles URLApplicationResource.

JCgH4164838Gh792C124B5 · 2019-04-25T01:51:37Z

...ns/tiles/src/main/java/org/apache/struts2/tiles/StrutsWildcardServletApplicationContext.java

+                try {
+                    resources.add(new StrutsApplicationResource(entry.getValue()));
+                } catch (MalformedURLException e) {
+                    LOG.warn("Cannot access [{}]", entry.getValue(), e);


Not sure if this is better as a debug level instead of warn ?
It's clearer with a warn, but users might not expect warnings in the logs (since it wouldn't happen before), so a debug might be "safer".

We already have warn in current code a few lines above this newly added line :)

True ... but that warn it isn't in a loop like this one (which was what "caught my eye") 😉

👍 throw exception removed at all to honor lazy file existence check .

JCgH4164838Gh792C124B5 · 2019-04-25T02:26:33Z

plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsApplicationResource.java

+            }
+        }
+
+        if (!new File(path).exists()) {


(Oops ... comment accidentally dropped from my review, re-adding as a single comment)

The File existence checks are dangerous because the URL can be a non-file protocol resource.

For that reason it is recommended that the two File existence checks are either removed or the logic changed so that File existence checks are only performed after verifying the URL represents a file protocol resource. The check could be something like:

if ("file".equalsIgnoreCase(url.getProtocol())) { // Perform file existence checks } // Return extracted path String (for non-File URLs or File URLs that pass existence checks).

👍 good idea, thanks!

…ence

JCgH4164838Gh792C124B5 · 2019-04-27T23:27:08Z

plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsApplicationResource.java


    public StrutsApplicationResource(URL url) {
-        super(url.getPath());
+        super(getFilePath(url));


One last suggestion 😄 for the constructor (avoids calling getFilePath twice/regenerating the path string):

final String urlPath = getFilePath(url); super(urlPath); this.url = url; this.file = new File(urlPath);

JCgH4164838Gh792C124B5 · 2019-04-27T23:43:32Z

Hello @yasserzamani .

After your latest changes it LGTM 👍

Sorry to be a pain ... but I made one (last) minor suggestion above concerning the constructor, It's actually something you suggested earlier to avoid re-generating the path string twice, I'll go ahead and set feedback as approved whether you decide that additional change is worth it or not.

Thanks for all of your work on this PR. 😄

p.s. It looks like this PR had the same Java 8 Travis CI failure that shows up intermittently too.

yasserzamani · 2019-04-28T04:59:42Z

No, it's fine :) thanks for your support! - actually this PR is very better now than mine :)

I had tried to avoid re-computing the path but it says Call to super must be first statement in the constructor 😢

Travis build fixed by another commit 👍

JCgH4164838Gh792C124B5 · 2019-05-20T21:14:07Z

Hello @lukaszlenart and @aleksandr-m.

It looks like this PR is ready to go once one of you reviews the latest version and thinks it can be approved. Assuming it is approved/merged, then I can close my original PR for this issue. :)

lukaszlenart · 2019-05-21T06:21:43Z

Looks good to me, LGTM 👍

include ref in path for StrutsApplicationResource as WW-5011 workaround

803feba

yasserzamani force-pushed the WW-5011-2-5 branch from e1661cf to 803feba Compare April 24, 2019 05:06

yasserzamani changed the title ~~WW-5011 use Tiles already provided URLApplicationResource~~ include ref in path as WW-5011 workaround Apr 24, 2019

yasserzamani added 2 commits April 24, 2019 09:47

add license to newly added xml files in previous commit

0adc404

WW-5011 include ref in path via .toURI().getPath()

f0448a0

yasserzamani added the Ready For Merge label Apr 24, 2019

yasserzamani mentioned this pull request Apr 24, 2019

Proposed fix for WW-5011 for the 2.5.x branch #348

Closed

JCgH4164838Gh792C124B5 suggested changes Apr 25, 2019

View reviewed changes

JCgH4164838Gh792C124B5 reviewed Apr 25, 2019

View reviewed changes

WW-5011 check protocol and not throw exception due to lazy file exist…

9d32493

…ence

JCgH4164838Gh792C124B5 reviewed Apr 27, 2019

View reviewed changes

JCgH4164838Gh792C124B5 approved these changes Apr 27, 2019

View reviewed changes

decrease log file size to pass travis build

fd91e75

yasserzamani merged commit b0dd7c1 into apache:struts-2-5-x May 28, 2019

yasserzamani deleted the WW-5011-2-5 branch May 28, 2019 05:24

Conversation

yasserzamani commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yasserzamani commented Apr 23, 2019

Uh oh!

coveralls commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszlenart commented Apr 23, 2019

Uh oh!

yasserzamani commented Apr 24, 2019

Uh oh!

yasserzamani commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JCgH4164838Gh792C124B5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JCgH4164838Gh792C124B5 Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JCgH4164838Gh792C124B5 commented Apr 27, 2019

Uh oh!

yasserzamani commented Apr 28, 2019

Uh oh!

JCgH4164838Gh792C124B5 commented May 20, 2019

Uh oh!

lukaszlenart commented May 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yasserzamani commented Apr 23, 2019 •

edited

Loading

coveralls commented Apr 23, 2019 •

edited

Loading

yasserzamani commented Apr 24, 2019 •

edited

Loading

JCgH4164838Gh792C124B5 Apr 25, 2019 •

edited

Loading