Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-47324

FilePath API in Jenkins should propagate errors

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      Almost all methods in https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/FilePath.java use obsolete pre-Java7 API, which does not propagate errors.

      • The code should be updated to java.nio.Files: https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html
      • Methods should propagate errors via IOExceptions where possible (and log errors to java.util.logging.Logger otherwise)
      • Runtime exceptions from the new API should be caught if the methods throw them (e.g. InvalidPathException)

        Attachments

          Issue Links

            Activity

            oleg_nenashev Oleg Nenashev created issue -
            oleg_nenashev Oleg Nenashev made changes -
            Field Original Value New Value
            Project Security Issues [ 10180 ] Jenkins [ 10172 ]
            Key SECURITY-625 JENKINS-47324
            Workflow Security v1.2 [ 223061 ] JNJira + In-Review [ 223062 ]
            Status Untriaged [ 10001 ] Open [ 1 ]
            Component/s core [ 15593 ]
            Component/s core [ 15738 ]
            oleg_nenashev Oleg Nenashev made changes -
            Labels diagnostics diagnostics newbie-friendly
            oleg_nenashev Oleg Nenashev made changes -
            Issue Type Bug [ 1 ] Improvement [ 4 ]
            teilo James Nord made changes -
            Link This issue relates to JENKINS-34855 [ JENKINS-34855 ]
            teilo James Nord made changes -
            Link This issue relates to JENKINS-36088 [ JENKINS-36088 ]
            marykomar Mariia Komar made changes -
            Assignee Mariia Komar [ marykomar ]
            Hide
            krishbhasin Krishan Bhasin added a comment -

            Is this issue taken? I'd like to have a crack at it if not

            Show
            krishbhasin Krishan Bhasin added a comment - Is this issue taken? I'd like to have a crack at it if not
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            Krishan Bhasin yes. Mariia Komar has taken the issue, but so far there is no progress on it AFAICT. There may be other issues around of such kind. Actually you can look for any obsolete Java 6 File API usage (File#mkdirs() & Co) in the core and create patches for that

            Show
            oleg_nenashev Oleg Nenashev added a comment - Krishan Bhasin yes. Mariia Komar has taken the issue, but so far there is no progress on it AFAICT. There may be other issues around of such kind. Actually you can look for any obsolete Java 6 File API usage (File#mkdirs() & Co) in the core and create patches for that
            Hide
            marykomar Mariia Komar added a comment -

            Yes, I'm working on it.

            Show
            marykomar Mariia Komar added a comment - Yes, I'm working on it.
            marykomar Mariia Komar made changes -
            Assignee Mariia Komar [ marykomar ]
            marykomar Mariia Komar made changes -
            Assignee Mariia Komar [ marykomar ]
            Hide
            marykomar Mariia Komar added a comment -

            Krishan Bhasin my plans have changed, and I, unfortunately, can't work on this issue, so if you want you can take it.

            Show
            marykomar Mariia Komar added a comment - Krishan Bhasin my plans have changed, and I, unfortunately, can't work on this issue, so if you want you can take it.
            marykomar Mariia Komar made changes -
            Assignee Mariia Komar [ marykomar ]
            batmat Baptiste Mathus made changes -
            Assignee Krishan Bhasin [ krishbhasin ]
            Hide
            batmat Baptiste Mathus added a comment -

            I've assigned Krishan Bhasin to clarify. Thanks a lot to you both to try and help! If you need any help, please do not hesitate to come ask questions and discuss on the jenkins-developers mailing list!

            Show
            batmat Baptiste Mathus added a comment - I've assigned Krishan Bhasin to clarify. Thanks a lot to you both to try and help! If you need any help, please do not hesitate to come ask questions and discuss on the jenkins-developers mailing list!
            Hide
            krishbhasin Krishan Bhasin added a comment -

            I've had some fun getting the Messages wired into my IDE, it still complains that it doesn't know what to do with the fields...

             

            With the actual code changes, I'm not sure how far 'up the chain' it needs to go. There seem to be utility methods (like filterNonNull) that are used a bit, that return a 'SoloFilePathFilter', defined further up in the project. Should I also start modifying that class to make this work?

            Show
            krishbhasin Krishan Bhasin added a comment - I've had some fun getting the Messages wired into my IDE, it still complains that it doesn't know what to do with the fields...   With the actual code changes, I'm not sure how far 'up the chain' it needs to go. There seem to be utility methods (like filterNonNull) that are used a bit, that return a 'SoloFilePathFilter', defined further up in the project. Should I also start modifying that class to make this work?
            Hide
            krishbhasin Krishan Bhasin added a comment -

            I should say, I'm using this to help with the code changes.

            Show
            krishbhasin Krishan Bhasin added a comment - I should say, I'm using this  to help with the code changes.
            Hide
            dnusbaum Devin Nusbaum added a comment -

            Krishan Bhasin IMHO I probably wouldn't go that far just yet. I'd focus on replacing the calls to methods like File#renameTo and File#mkdirs with calls to Files.move and Files.createDirectories. I believe many of the methods that take File as argument can't be converted to take Path because they are being sent over remoting channels and Path isn't serializable. File#toPath is your friend!

            I was working on some things related to this before I realized there was an existing ticket being worked on by someone else, so feel free to look at the changes in my branch in case they are useful to you. I'll stop working on that branch and leave the ticket in your hands for now

            Show
            dnusbaum Devin Nusbaum added a comment - Krishan Bhasin  IMHO I probably wouldn't go that far just yet. I'd focus on replacing the calls to methods like File#renameTo and File#mkdirs with calls to Files.move and Files.createDirectories . I believe many of the methods that take File as argument can't be converted to take Path because they are being sent over remoting channels and Path isn't serializable. File#toPath is your friend! I was working on some things related to this before I realized there was an existing ticket being worked on by someone else, so feel free to look at the changes in my branch in case they are useful to you. I'll stop working on that branch and leave the ticket in your hands for now
            Hide
            krishbhasin Krishan Bhasin added a comment -

            I've had a look at your changes, and they make sense, but I feel a bit out of my depth trying to migrate any further methods over to the Java 7 API... I'll give it a go and put what I have in a PR, and maybe someone can help give me pointers from there?

            Show
            krishbhasin Krishan Bhasin added a comment - I've had a look at your changes, and they make sense, but I feel a bit out of my depth trying to migrate any further methods over to the Java 7 API... I'll give it a go and put what I have in a PR, and maybe someone can help give me pointers from there?
            Hide
            dnusbaum Devin Nusbaum added a comment - - edited

            Krishan Bhasin That sounds like a good plan to me, any improvement will be welcome, and there's no need to do everything at once. Feel free to mention @dwnusbaum on the PR and I'll take a look at it.

            Show
            dnusbaum Devin Nusbaum added a comment - - edited Krishan Bhasin  That sounds like a good plan to me, any improvement will be welcome, and there's no need to do everything at once. Feel free to mention @dwnusbaum on the PR and I'll take a look at it.
            oleg_nenashev Oleg Nenashev made changes -
            Link This issue relates to JENKINS-48227 [ JENKINS-48227 ]
            Hide
            dnusbaum Devin Nusbaum added a comment -

            Krishan Bhasin Do you still plan on filing a PR for this?

            Show
            dnusbaum Devin Nusbaum added a comment - Krishan Bhasin Do you still plan on filing a PR for this?
            Hide
            krishbhasin Krishan Bhasin added a comment -

            Devin Nusbaum Yeah sorry it's been a busy few weeks (holiday then knocked out with flu). I will get to this before the end of the year, I'm still recovering from the illness.

            Show
            krishbhasin Krishan Bhasin added a comment - Devin Nusbaum Yeah sorry it's been a busy few weeks (holiday then knocked out with flu). I will get to this before the end of the year, I'm still recovering from the illness.
            Hide
            dnusbaum Devin Nusbaum added a comment -

            Krishan Bhasin Ok that sounds good. I hope you're feeling better soon.

            Show
            dnusbaum Devin Nusbaum added a comment - Krishan Bhasin Ok that sounds good. I hope you're feeling better soon.
            dnusbaum Devin Nusbaum made changes -
            Link This issue relates to JENKINS-48405 [ JENKINS-48405 ]
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Krishan Bhasin
            Path:
            core/src/main/java/hudson/FilePath.java
            core/src/main/java/hudson/util/IOUtils.java
            http://jenkins-ci.org/commit/jenkins/09bcc5d6538b3cfffbf71228ebd1679e3e20d8b2
            Log:
            JENKINS-47324 - Reduce usage of File.mkdirs() in FilePath and IOUtils (#3173)

            • Move an instance of renameTo() to Files.move()
            • Replace an instance of File.toURI() with an instance of Path.toUri()
            • Replace mkdirs() with Files.createDirectories()
              Replace mkdir() with Files.createTempDirectory()
            • Undo addition of createTempDirectory() as per review comments
            • Return to use of FilePath#mkdirs(File) and instead modify it to use the new API.
            • Undo addition of toPath() in a URI conversion as it brings no benefits.
            • Replace new uses of toPath() with Util.fileToPath() to pre-handle runtime exceptions
            • Remove * import.
              move mkdirs() to using FilePath method instead of File method.
            • Make IOUtils.mkdirs(File) use Java7 API calls
            • Add back accidentally removed imports.
            • Fixed use of wildcard import
            • Use utility method fileToPath() to handle potential exception thrown
            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Krishan Bhasin Path: core/src/main/java/hudson/FilePath.java core/src/main/java/hudson/util/IOUtils.java http://jenkins-ci.org/commit/jenkins/09bcc5d6538b3cfffbf71228ebd1679e3e20d8b2 Log: JENKINS-47324 - Reduce usage of File.mkdirs() in FilePath and IOUtils (#3173) Move an instance of renameTo() to Files.move() Replace an instance of File.toURI() with an instance of Path.toUri() Replace mkdirs() with Files.createDirectories() Replace mkdir() with Files.createTempDirectory() Undo addition of createTempDirectory() as per review comments Return to use of FilePath#mkdirs(File) and instead modify it to use the new API. Undo addition of toPath() in a URI conversion as it brings no benefits. Replace new uses of toPath() with Util.fileToPath() to pre-handle runtime exceptions Remove * import. move mkdirs() to using FilePath method instead of File method. Make IOUtils.mkdirs(File) use Java7 API calls Add back accidentally removed imports. Fixed use of wildcard import Use utility method fileToPath() to handle potential exception thrown
            Hide
            danielbeck Daniel Beck added a comment -

            Oleg Nenashev Is this issue fixed towards 2.96 or still WIP?

            Show
            danielbeck Daniel Beck added a comment - Oleg Nenashev Is this issue fixed towards 2.96 or still WIP?
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            Daniel Beck IIUC there are still some changes to be done in this ticket

            Show
            oleg_nenashev Oleg Nenashev added a comment - Daniel Beck IIUC there are still some changes to be done in this ticket
            batmat Baptiste Mathus made changes -
            Remote Link This issue links to "PR-3173 from KrishanBhasin (Web Link)" [ 22177 ]
            batmat Baptiste Mathus made changes -
            Labels diagnostics newbie-friendly diagnostics missing-acceptance-criteria needs-details newbie-friendly
            Hide
            batmat Baptiste Mathus added a comment -

            I feel like the acceptance criteria are slightly unclear, then it makes it hard to be sure this is done or not. I see at least the PR from Krishan Bhasin and Devin Nusbaum in https://github.com/jenkinsci/jenkins/pull/3135.

             Oleg Nenashev given you filed this, *when you have time* could you possibly add acceptance criteria so it's easily assessable by anyone if/when this would be done?

            Spassiba!

            Show
            batmat Baptiste Mathus added a comment - I feel like the acceptance criteria are slightly unclear, then it makes it hard to be sure this is done or not. I see at least the PR from Krishan Bhasin and Devin Nusbaum  in https://github.com/jenkinsci/jenkins/pull/3135.   Oleg Nenashev given you filed this, * when you have time * could you possibly add acceptance criteria so it's easily assessable by anyone if/when this would be done? Spassiba!
            Hide
            batmat Baptiste Mathus added a comment - - edited

            Krishan Bhasin would you be BTW be available/willing to handle the possible remainder of this task, if any? We're happy to provide any guidance for this. Thanks!

            Show
            batmat Baptiste Mathus added a comment - - edited Krishan Bhasin would you be BTW be available/willing to handle the possible remainder of this task, if any? We're happy to provide any guidance for this. Thanks!
            Hide
            dnusbaum Devin Nusbaum added a comment -

            Krishan submitted https://github.com/jenkinsci/jenkins/pull/3173, https://github.com/jenkinsci/jenkins/pull/3135 from me was more related to JENKINS-36088. There are likely other things that could be addressed, but I think we got most of the lowest-hanging fruit looking at the related issues and glancing quickly through FilePath.java.

            Show
            dnusbaum Devin Nusbaum added a comment - Krishan submitted https://github.com/jenkinsci/jenkins/pull/3173 , https://github.com/jenkinsci/jenkins/pull/3135 from me was more related to JENKINS-36088 . There are likely other things that could be addressed, but I think we got most of the lowest-hanging fruit looking at the related issues and glancing quickly through FilePath.java .
            Hide
            batmat Baptiste Mathus added a comment -

            OK, then let's consider this fixed until proven otherwise. Like I wrote above anyway, it can be reopened anytime once we know about left things to be still fixed. Thanks for the feedback Devin.

            Show
            batmat Baptiste Mathus added a comment - OK, then let's consider this fixed until proven otherwise. Like I wrote above anyway, it can be reopened anytime once we know about left things to be still fixed. Thanks for the feedback Devin.
            batmat Baptiste Mathus made changes -
            Status Open [ 1 ] Closed [ 6 ]
            Resolution Done [ 10000 ]
            dnusbaum Devin Nusbaum made changes -
            Remote Link This issue links to "jenkinsci/jenkins#3864 (Web Link)" [ 22307 ]

              People

              • Assignee:
                krishbhasin Krishan Bhasin
                Reporter:
                oleg_nenashev Oleg Nenashev
              • Votes:
                1 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: