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

WW-4920 fix java.net.JarURLConnection#parseSpecs #209

Closed
wants to merge 3 commits into from
Closed

WW-4920 fix java.net.JarURLConnection#parseSpecs #209

wants to merge 3 commits into from

Conversation

yasserzamani
Copy link
Member

I inspired such fix from Spring Framework when I found it during my research at here.

Oracle WebLogic may report jar urls like "zip:C:/web-app-lib-path/some-jar.jar" but JDK's JarURLConnection#parseSpecs breaks on such urls.

While JDK's JarURLConnection#parseSpecs is private then I had to extend URLConnection instead.

@apache/struts-committers , I hope we can deliver this fix in Struts 2.5.15 instead of 2.6, please (?)

@coveralls
Copy link

coveralls commented Feb 16, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 46.655% when pulling f929673 on yasserzamani:struts_user_mbox_201802 into def4ada on apache:master.

aleksandr-m
aleksandr-m reviewed Feb 16, 2018
View reviewed changes
core/src/main/java/com/opensymphony/xwork2/util/fs/StrutsJarURLConnection.java Outdated
@@ -18,6 +18,8 @@
*/
package com.opensymphony.xwork2.util.fs;

import sun.net.www.ParseUtil;
Copy link
Contributor

Choose a reason for hiding this comment

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

sun.net.www.ParseUtil Sure?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was what my Oracle JDK 7 also imports in it's java.net.JarURLConnection class! I now researched and saw OpenJDK, Oracle JDK 8 and 9 also is same. I see sun.net.www.ParseUtil is inside rt.jar of these JDKs. It looks like opensymphony package name in Struts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't mean you should use it. ;) See Replace sun.net.www.ParseUtil#decode with public API and sun.net.www.ParseUtil.decode() vs java.net.URLDecoder.decode()

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch 👍 thanks! fixed.

@aleksandr-m
Copy link
Contributor

@yasserzamani 2.5.15 is already built. 2.5.15.1 can be released.

Is it still the same issue as in WW-4869? Maybe we just need to drop this fs stuff? We can refactor it later into its own plugin.

@yasserzamani
Copy link
Member Author

yasserzamani commented Feb 16, 2018 •
edited
Loading

@aleksandr-m , thank you for your comments and review. As Struts 2.5.15 is in test build status and release voting is not started yet, I thought maybe it's possible to rebuild a test build :)

I think WW-4869 isn't same. That was Struts itself issue but this one is either JDK issue or WebLogic issue. WebLogic reports jar as "zip:C:/..." but JDK's JarUrlConnection expects "zip:file:/C:/...".

During my research I saw frameworks (e.g. Spring) also have such issue with WebLogic<->JDK. They adapted themselves with both by their own implementation as JDK and WebLogic are bully 😉

@lukaszlenart
Copy link
Member

This is a bit over-engineered solution - basically the idea behind this solution (FileManager and so on) is to allow reload classes placed in a JAR (or ZIP in case of Weblogic) when working in devMode. If we cannot handle given type of a file we should ignore it, that's the proper solution.

Also, I was planning to drop this whole layer as it in mose case is useless.

@yasserzamani
Copy link
Member Author

👍 So I think I can close this, right? because it seems Struts already warns a message and ignores when cannot handle.

@yasserzamani
Copy link
Member Author

... and then closing WW-4920 as not a problem and continuing the release of 2.5.15.

@lukaszlenart
Copy link
Member

Nah... we can merge it as is, maybe someone is using it. I would double check if this happens only in devMode

@yasserzamani
Copy link
Member Author

👍 you're right. here is a good time to fix that also. In previous user reports, I remember this happens also in devMode=false but I'll check if I can fix it also here.

@yasserzamani
Copy link
Member Author

yasserzamani commented Feb 19, 2018 •
edited
Loading

4b89034 has deleted if (isReloadingConfigs()) { from all implemented FileManager#monitorFile methods. This causes Struts to monitor all loaded files always, regardless of if the value of STRUTS_CONFIGURATION_XML_RELOAD = "struts.configuration.xml.reload" is true or false. i.e. Struts may reload a file even when that constant is set to false because if (revision == null) { is false and Struts will use monitored revision in FileManager#fileNeedsReloading. As FileManager interface documents:

    /**
     * Adds file to list of monitored files if {@link #setReloadingConfigs(boolean)} is true
     *
     * @param fileUrl {@link URL} to file to be monitored
     */
    void monitorFile(URL fileUrl);

I think we must revert this change (other changes of this commit looks good). I'm just worry about:

        //watch class file
        if (isReloadEnabled()) {
            URL classFile = actionClass.getResource(actionClass.getSimpleName() + ".class");
            fileManager.monitorFile(classFile);
            loadedFileUrls.add(classFile.toString());
        }

in \convention\PackageBasedActionConfigBuilder.java. This piece of code seems expects monitorFile method always monitor but I think if it wants to monitor .class files, then it must use it's own internal mechanism because as documents say, the monitorFile method will monitor if STRUTS_CONFIGURATION_XML_RELOAD = "struts.configuration.xml.reload" is set to true by user.

Any way I feel OK to revert back that if statement and adding more heavy test covering all possible situations i.e. when that constant is set to true or false (two states) and when file is modified or not (two states) so total four states. @lukaszlenart , @apache/struts-committers ❓

@kwin
Copy link
Member

kwin commented Feb 19, 2018

Please do not mention apache/apache-committers as that groups contains roughly 2000 members. Not all of those are interested in getting mail about struts issues.

@janiversen
Copy link

janiversen commented Feb 19, 2018 via email

@iamaleksey
Copy link
Member

unsubscribe

@mcpierce
Copy link
Member

mcpierce commented Feb 19, 2018 via email

@yasserzamani
Copy link
Member Author

@apache/apache-committers , Ach! I'm really sorry 😭 github auto completed my at-sign 😢 please excuse me, sorry for inconvenience :(

@kwin
Copy link
Member

kwin commented Feb 19, 2018

@yasserzamani Why have you done it again? Even if it is for saying sorry this is not appropriate for a group containing 2000 people! Please stop this!

@mcpierce
Copy link
Member

mcpierce commented Feb 19, 2018 via email

@sctemme
Copy link

sctemme commented Feb 19, 2018 via email

@rgielen
Copy link
Member

rgielen commented Feb 19, 2018

@kwin Last time I checked apologizing for a fault and giving context to those wondering why they received unrelated mail could rightfully be considered polite and friendly. And yes, even for 2000 recipients.
@mcpierce your harsh order to stop being sloppy might feel suitable in some corporate environments. As an Apache community and foundation member I would ask you to reconsider this attitude. Raising awareness and being supportive to help avoiding future mistakes is great. Finger pointing, blaming and patronizing is as much an annoyance as mistakenly posting to 2000 users.

@Luke1410
Copy link

@rgielen, in defense for @kwin: he was pointing out that in @yasserzamani's apology he actually did re-mention the entire group again, causing the already unsubscribed people to be resubscribed yet again, if I get his point right. But anyway, these mistakes happen and if infra can do something about it from their side, it should be resolved for good in the future anyway. So no big deal IMO.

@mcpierce
Copy link
Member

mcpierce commented Feb 19, 2018 via email

@rgielen
Copy link
Member

rgielen commented Feb 19, 2018

@Luke1410 point taken. But we should rather blame the tool and maybe our setup here instead of a person acting in good faith.

@nabarunnag
Copy link

nabarunnag commented Feb 20, 2018 via email

@adufilie
Copy link
Member

Please merge and/or delete this PR so 2,000 people don't have to manually mute the thread.

@apache apache locked as spam and limited conversation to collaborators Feb 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aleksandr-m aleksandr-m aleksandr-m left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.