TRUNK-5134 - Replace path constructions using separator char #2135

MalshaL · 2017-04-19T02:17:57Z

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-5134

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).

No? -> read here on how to squash multiple commits into one
My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

mention-bot · 2017-04-19T02:17:58Z

@MalshaL, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bmamlin, @teleivo and @lluismf to be potential reviewers.

coveralls · 2017-04-19T02:25:45Z

Coverage increased (+0.006%) to 56.133% when pulling cebc97e on MalshaL:TRUNK-5134 into 2aed056 on openmrs:master.

teleivo

thank @MalshaL !

one main note: the goal of this ticket is to get rid of all the manual path constructions in the code using File.separator or System.getProperty("file.separator") and File.separatorChar

you havent replaced all of them.

check out the different constructors available https://docs.oracle.com/javase/7/docs/api/java/io/File.html
you can use a File as parent to construct a new File from it appending a child string.

teleivo · 2017-04-19T06:16:55Z

web/src/main/java/org/openmrs/module/web/WebModuleUtil.java

@@ -126,7 +126,7 @@ public static boolean startModule(Module mod, ServletContext servletContext, boo
 realPath = System.getProperty("user.dir");
 }

- File webInf = new File(realPath + "/WEB-INF".replace("/", File.separator));
+ File webInf = new File(realPath, "WEB-INF".replace("/", File.separator));


@MalshaL you need to remove the string replace call, see that before the "/" was replaced with the File.separator to make it cross-platform. thats not needed when letting File construct the path.

teleivo · 2017-04-19T06:17:01Z

web/src/main/java/org/openmrs/module/web/WebModuleUtil.java

@@ -240,7 +240,7 @@ public static boolean startModule(Module mod, ServletContext servletContext, boo
 if (root.getElementsByTagName("dwr").getLength() > 0) {

 // get the dwr-module.xml file that we're appending our code to
- File f = new File(realPath + "/WEB-INF/dwr-modules.xml".replace("/", File.separator));
+ File f = new File(realPath, "WEB-INF/dwr-modules.xml".replace("/", File.separator));


I think this can be replaced with File(webInf, "dwr-modules.xml")

check out the different constructors available https://docs.oracle.com/javase/7/docs/api/java/io/File.html
you can use a File as parent to construct a new File from it appending a child string.

teleivo · 2017-04-19T06:21:18Z

web/src/main/java/org/openmrs/module/web/WebModuleUtil.java

@@ -775,7 +775,7 @@ public static void stopModule(Module mod, ServletContext servletContext, boolean
 if (root.getElementsByTagName("dwr").getLength() > 0) {

 // get the dwr-module.xml file that we're appending our code to
- File f = new File(realPath + "/WEB-INF/dwr-modules.xml".replace("/", File.separator));
+ File f = new File(realPath, "WEB-INF/dwr-modules.xml".replace("/", File.separator));


same as above, the goal of this ticket is to get rid of all the manual path constructions in the code using File.separator or System.getProperty("file.separator") and File.separatorChar

teleivo · 2017-04-19T06:21:27Z

web/src/main/java/org/openmrs/module/web/WebModuleUtil.java

- StreamResult result = new StreamResult(new File(realPath
-  + "/WEB-INF/dwr-modules.xml".replace("/", File.separator)));
+ StreamResult result = new StreamResult(new File(realPath,
+  "WEB-INF/dwr-modules.xml".replace("/", File.separator)));


teleivo · 2017-04-19T06:24:55Z

api/src/main/java/org/openmrs/util/UpgradeUtil.java

@@ -24,11 +25,11 @@
 import org.openmrs.api.APIException;

 public class UpgradeUtil {
-


@MalshaL can you please double check your formatter settings am wondering why there are so many whitespace formatting changes in this file, I dont get them with my eclipse config (https://wiki.openmrs.org/display/docs/How-To+Setup+And+Use+Your+IDE).

teleivo · 2017-04-24T06:29:13Z

@MalshaL are you still on this? would like to merge and not keep the PR open for so long

teleivo · 2017-04-29T05:21:23Z

@MalshaL hope you are well! can you please tell us if you are still going to work on this, we need a few simple changes from you to get this merged. having to recheck with contributors on their PRs over and over again takes time of the few reviewers. please respond. thanks a lot!

MalshaL · 2017-04-30T04:15:59Z

@teleivo my sincere apologies for not being able to work on this due to an unavoidable circumstance. :( I will make the necessary changes and commit as soon as possible.

dkayiwa · 2017-05-02T08:28:07Z

@MalshaL did you see the merge conflicts on this? 😊

coveralls · 2017-05-03T09:42:37Z

Coverage decreased (-0.007%) to 57.163% when pulling f9724af on MalshaL:TRUNK-5134 into 7606d3e on openmrs:master.

wluyima · 2017-05-03T15:36:51Z

Ignore my previous comment about using SystemUtils

teleivo

please address comments.

generally make sure to use try-with-resource as per comment and

File outFile = new File(webInf, "view", ...);

teleivo · 2017-05-03T18:44:32Z

api/src/main/java/org/openmrs/util/UpgradeUtil.java

- try (FileInputStream fis = new FileInputStream(filePath)) {
-
+ try {
+ FileInputStream fis = new FileInputStream(


@MalshaL please also use the try-with-resource statement
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
instead of calling the close() yourself.

this has just been fixed in https://issues.openmrs.org/browse/TRUNK-5031

teleivo · 2017-05-03T18:45:32Z

api/src/main/java/org/openmrs/util/UpgradeUtil.java

- try (PreparedStatement select = connection.prepareStatement("select uuid from concept where concept_id = ?")) {
+ public static String getConceptUuid(Connection connection, int conceptId) throws SQLException {
+ PreparedStatement select = connection.prepareStatement("select uuid from concept where concept_id = ?");
+ try {


do not change this here, this has been fixed in
https://issues.openmrs.org/browse/TRUNK-5031

you are currently overriding his fix.

teleivo · 2017-05-03T18:46:37Z

api/src/main/java/org/openmrs/util/UpgradeUtil.java

 ResultSet resultSet = select.executeQuery();
 if (resultSet.next()) {
 return resultSet.getString(1);
 } else {
 throw new IllegalArgumentException("Concept not found " + conceptId);
 }
 }
+ finally {


you need to discard your changes.

you can see the changes that introduced the try-with-resource at
b99cdc8

from https://issues.openmrs.org/browse/TRUNK-5031

this relates to the entire UpgradeUtil.java

teleivo · 2017-05-03T18:49:30Z

web/src/main/java/org/openmrs/module/web/WebModuleUtil.java

- String folderPath = realPath + "/WEB-INF/view/module/" + mod.getModuleIdAsPath();
- File outFile = new File(folderPath.replace("/", File.separator));
+ //String folderPath = realPath + "/WEB-INF/view/module/" + mod.getModuleIdAsPath();
+ File outFile = Paths.get(realPath, "WEB-INF", "view", "module", mod.getModuleIdAsPath()).toFile();


@MalshaL cant you just use

File outFile = new File(webInf, "view", ...);

instead of using the Paths.get() ?

Also please do not comment out code! Always remove it. We use git, if we need it back we can easily get it.

teleivo · 2017-05-03T18:50:16Z

web/src/main/java/org/openmrs/module/web/WebModuleUtil.java

- String absPath = realPath + "/WEB-INF/view/module/" + moduleId;
- File moduleWebFolder = new File(absPath.replace("/", File.separator));
+ //String absPath = realPath + "/WEB-INF/view/module/" + moduleId;
+ File moduleWebFolder = Paths.get(realPath, "WEB-INF", "view", "module", moduleId).toFile();


same as my last comment above this one

teleivo · 2017-05-03T18:50:26Z

web/src/main/java/org/openmrs/module/web/WebModuleUtil.java

@@ -775,7 +775,7 @@ public static void stopModule(Module mod, ServletContext servletContext, boolean
 if (root.getElementsByTagName("dwr").getLength() > 0) {

 // get the dwr-module.xml file that we're appending our code to
- File f = new File(realPath + "/WEB-INF/dwr-modules.xml".replace("/", File.separator));
+ File f = Paths.get(realPath, "WEB-INF", "dwr-modules.xml").toFile();


same, use new File()

teleivo · 2017-05-03T18:50:43Z

web/src/main/java/org/openmrs/module/web/WebModuleUtil.java

@@ -961,8 +961,7 @@ public static void createDwrModulesXml(String realPath) {
 TransformerFactory transformerFactory = TransformerFactory.newInstance();
 Transformer transformer = transformerFactory.newTransformer();
 DOMSource source = new DOMSource(doc);
- StreamResult result = new StreamResult(new File(realPath
- + "/WEB-INF/dwr-modules.xml".replace("/", File.separator)));
+ StreamResult result = new StreamResult(Paths.get(realPath, "WEB-INF", "dwr-modules.xml").toFile());


teleivo · 2017-05-03T18:50:49Z

web/src/main/java/org/openmrs/web/filter/initialization/TestInstallUtil.java

@@ -67,8 +68,7 @@ protected static boolean addTestData(String host, int port, String databaseName,

 //For stand-alone, use explicit path to the mysql executable.
 String runDirectory = System.getProperties().getProperty("user.dir");
- File file = new File(runDirectory + File.separatorChar + "database" + File.separatorChar + "bin"
- + File.separatorChar + "mysql");
+ File file = Paths.get(runDirectory, "database", "bin", "mysql").toFile();


teleivo · 2017-05-03T18:50:55Z

web/src/test/java/org/openmrs/web/test/WebModuleActivatorTest.java

@@ -41,8 +42,7 @@ public void createWebInfFolderIfNotExist() {
 //when run from the IDE and this folder does not exist, some tests fail with
 //org.openmrs.module.ModuleException: Unable to load module messages from file: 
 // /Projects/openmrs/core/web/target/test-classes/WEB-INF/module_messages_fr.properties
-
- File folder = new File("target" + File.separatorChar + "test-classes" + File.separatorChar + "WEB-INF");
+ File folder = Paths.get("target", "test-classes", "WEB-INF").toFile();


dkayiwa · 2017-05-08T10:00:51Z

@MalshaL am closing this for now. Will reopen when you are ready to reclaim the ticket. 😊

teleivo requested changes Apr 19, 2017

View reviewed changes

TRUNK-5134 - Replace path constructions using separator char

f9724af

MalshaL force-pushed the TRUNK-5134 branch from cebc97e to f9724af Compare May 3, 2017 09:32

teleivo requested changes May 3, 2017

View reviewed changes

dkayiwa closed this May 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TRUNK-5134 - Replace path constructions using separator char #2135

TRUNK-5134 - Replace path constructions using separator char #2135

MalshaL commented Apr 19, 2017 •

edited

Loading

mention-bot commented Apr 19, 2017

coveralls commented Apr 19, 2017

teleivo left a comment

teleivo Apr 19, 2017

teleivo Apr 19, 2017

teleivo Apr 19, 2017

teleivo Apr 19, 2017

teleivo Apr 19, 2017

teleivo commented Apr 24, 2017

teleivo commented Apr 29, 2017

MalshaL commented Apr 30, 2017

dkayiwa commented May 2, 2017

coveralls commented May 3, 2017

wluyima commented May 3, 2017

teleivo left a comment

teleivo May 3, 2017

teleivo May 3, 2017

teleivo May 3, 2017

teleivo May 3, 2017

teleivo May 3, 2017

teleivo May 3, 2017

teleivo May 3, 2017

teleivo May 3, 2017

teleivo May 3, 2017

teleivo May 3, 2017

dkayiwa commented May 8, 2017

		@@ -24,11 +25,11 @@
		import org.openmrs.api.APIException;

		public class UpgradeUtil {

TRUNK-5134 - Replace path constructions using separator char #2135

TRUNK-5134 - Replace path constructions using separator char #2135

Conversation

MalshaL commented Apr 19, 2017 • edited Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

mention-bot commented Apr 19, 2017

coveralls commented Apr 19, 2017

teleivo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teleivo commented Apr 24, 2017

teleivo commented Apr 29, 2017

MalshaL commented Apr 30, 2017

dkayiwa commented May 2, 2017

coveralls commented May 3, 2017

wluyima commented May 3, 2017

teleivo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented May 8, 2017

MalshaL commented Apr 19, 2017 •

edited

Loading