Simple optional path to jobs api #22

Dhivyaa21 · 2018-05-19T23:06:44Z

@cdancy @martinda - This pull-request is for issue#21

I tried to get the value of `@PathParam(OPTIONAL_FOLDER_PATH_PARAM) String optionalFolderPath' variable inside the filter method of ScrubNullFolderParam.java, I was not successful. I think the values of pathparam variables were resolved already in the filter method.

But I found an annotation in jclouds called @ParamParser here which can intercept the path parameter variable values of the request. I have an initial implementation of this feature. I do have live tests for this change and it works locally for me. Let me know what you guys think.

cdancy · 2018-05-20T01:02:46Z

Good start. Yeah I totally forgot about ParamParser and in my opinion that is where ALL the logic surrounding this and scrubbing should go. If you want to combine the scrub filter functionality into your param parser that would be great. After that we can see how things look.

Also ... Make sure to upper case class names.

cdancy · 2018-05-20T01:07:21Z

And If you combine things you should be able to remove the optional-folder-path constant we use everywhere. Let me know if you need any help with this.

martinda

Thanks for this Dhivyaa21. My review is complete.

Using ParamParser is really cool. I've been trying to find the documentation but other than the javadoc I didn't find much. Is there book or some other that I have missed?

martinda · 2018-05-20T23:18:46Z

src/main/java/com/cdancy/jenkins/rest/JenkinsUtils.java

+     *
+     * @param folderPath simple folder path expression
+     *
+     * @return amendedFodlerPath which optional folder


which -> with

Why say "optional" ?

martinda · 2018-05-20T23:21:38Z

src/main/java/com/cdancy/jenkins/rest/JenkinsUtils.java

+     *         path reconstructed to match the jenkins URL style.
+     */
+    public static String amendFolderPath(String folderPath) {
+        Pattern pattern = Pattern.compile("(job/[^!@#$%^&*<>]+/*)+");


"job" is both a valid folder name and a valid job name. Meaning something like http://localhost:8080/job/job/job/job/job/job definitely works in Jenkins.

I do not see a way to tell whether "job/job" means nested folders both called "job", or if the user is passing the "job/" prefix to a folder called "job".

One more thing is that this regex tries to say what makes a valid Jenkins name. I would propose that the jenkins-rest library leaves it up to Jenkins to make that determination (it is done in checkGoodName).

I think this check cannot be made. The input has to be processed regardless of its content.

Agreed!

Given that it is hard to discriminate jobname and the prefix, I think we should support either the jenkins URL style(with 'job/') or the simple folder path expression and not both.

martinda · 2018-05-20T23:28:03Z

src/main/java/com/cdancy/jenkins/rest/JenkinsUtils.java

+            if(i < folderNames.length-1) {
+                amendedPath += "/";
+            }
+        }


I would like to propose a solution which deals more gracefully with invalid forward slashes in the input:

import org.apache.commons.lang3.StringUtils; ... String[] pathElements = StringUtils.split(folderPath, "/"); return "job/" + StringUtils.join(pathElements, "/job/");

This creates a dependency on compile('org.apache.commons:commons-lang3:3.7') if this is an acceptable solution.

The StringUtils.split method will remove leading, trailing and duplicate occurrences of "/", leading to a clean list of pathElements. Then it becomes a simple matter of calling StringUtils.join.

I agree with cdancy, the null scrubber belongs here somewhere.

Also account for pathElements when it is an empty array.

I don't want to add another library dependency for just one method if it can be helped. Instead we can recreate the method in our utilities file if need be.

martinda · 2018-05-21T11:01:02Z

src/test/java/com/cdancy/jenkins/rest/features/JobsApiLiveTest.java

@@ -27,6 +27,8 @@

 import com.cdancy.jenkins.rest.domain.plugins.Plugin;
 import com.cdancy.jenkins.rest.domain.plugins.Plugins;
+import com.cdancy.jenkins.rest.filters.ScrubNullFolderParam;
+import org.jclouds.http.HttpRequest;


Are these two imports needed?

martinda · 2018-05-21T12:33:59Z

src/main/java/com/cdancy/jenkins/rest/parsers/optionalFolderPathParser.java

+ * Turn the optionalFolderPath param to jenkins URL style if needed
+ */
+@Singleton
+public class optionalFolderPathParser implements Function<Object,String> {


Class names should start with an uppercase letter, so is the filename containing the class.

Typo! My bad. :(

martinda · 2018-05-21T16:06:31Z

src/main/java/com/cdancy/jenkins/rest/JenkinsUtils.java

+     * @return amendedFodlerPath which optional folder
+     *         path reconstructed to match the jenkins URL style.
+     */
+    public static String amendFolderPath(String folderPath) {


The folderPath input should be declared final to go along with the conventions of this project.

martinda · 2018-05-21T16:20:51Z

src/main/java/com/cdancy/jenkins/rest/JenkinsUtils.java

+    /**
+     * amend the path to job by appending "/job/" before every folder name
+     *
+     * @param folderPath simple folder path expression


simple folder path expression without the leading "job/" paths in front of folder names.

Dhivyaa21 · 2018-05-22T19:03:52Z

@cdancy - I removed the OPTIONAL_FOLDER_PATH_PARAM from the jobsApi class, replaced the null path with an empty string but I could not replace the request's base path "/" in the paramparser. May be we still need the ScrubNullFolderParam filter. Let me know what you think.

cdancy · 2018-05-22T19:06:07Z

src/main/java/com/cdancy/jenkins/rest/parsers/OptionalFolderPathParser.java

+    public static final String EMPTY_STRING = "";
+    @Override
+    public String apply(Object optionalFolderPath) {
+        return (optionalFolderPath == null) ? EMPTY_STRING : JenkinsUtils.amendFolderPath(String.class.cast(optionalFolderPath));


Lets bring that logic into this parser until we have a use-case not to do so.

cdancy · 2018-05-22T19:06:34Z

@Dhivyaa21 one minor comment to address but all-in-all it LGTM.

cdancy · 2018-05-22T19:08:19Z

but I could not replace the request's base path "/" in the paramparser

@Dhivyaa21 can you provide an example of what you mean?

Dhivyaa21 · 2018-05-22T19:27:48Z

@cdancy The request path to creating a job is /{optionalFolderPath}/createItem. The paramparser replaces {optionalFolderPath} with an empty string if it is null which leaves the path as //createItem. This is why the tests are failing. I am not getting how to get rid of that extra "/" in the optionalFolderPathParser class.

cdancy · 2018-05-22T19:39:25Z

@Dhivyaa21 just checked out your branch and looking into things now

cdancy · 2018-05-22T20:31:13Z

@Dhivyaa21 so I think what we can do is replace the @Path annotations with something like:

@Path("{optionalFolderPath}job/{name}/api/json") // note the missing forward-slash behind _job_

We can then change the param-parser to ensure there is a leading forward-slash appended to the end if we have content otherwise return an empty-string. This way jclouds will be happy. We'll have to relax a bit on the rules we set which I think is fine. If devs want to append a forward slash to the end of their optional-path-param I think allowing it is fine for the sake of getting this moving forward.

We should also capitalize that class and merge the contents from the jenkins-utils into the parser class.

Dhivyaa21 · 2018-05-22T21:47:33Z

@cdancy - Removing the trailing forward slash is a good idea. I have modified the code, Please review.

cdancy · 2018-05-22T22:37:17Z

src/main/java/com/cdancy/jenkins/rest/parsers/OptionalFolderPathParser.java

+        String folderPath = String.class.cast(optionalFolderPath);
+        String amendedPath = "";
+        if(folderPath.startsWith("/") || folderPath.endsWith("/")) {
+            throw new RuntimeException("Incorrect folder Path format - " + folderPath);


I'm actually going to push back on this, and go against what I originally said, and say we allow both. If either are present just remove before processing. I can see someone doing something like: /hello/world and someone else looking at the api, not seeing a forward slash, and then do something like: hello/world/.

cdancy · 2018-05-22T22:39:01Z

src/main/java/com/cdancy/jenkins/rest/parsers/OptionalFolderPathParser.java

+        }
+        String[] folderNames = folderPath.split("/");
+        for (String folder:folderNames) {
+            amendedPath += "job/" + folder + "/";


I wonder about this: what happens if the user puts in a non-simple path? Meaning the fully qualified job/hello/job/world/job/build.

Like @martinda pointed, It is hard to say if "job/job" is a fully qualified path to a job or simple folder path(Apparently, folders can also be named as "job"). I am not sure if we can check for a fully qualified path before we amend it.

When users say job/hello/job/world/job/build this logic will go ahead add job/ before every word and probably would return a 404 not found status.

In the fully qualified job name job/hello/job/world/job/build, there are two things to consider:

The path (hello/world) - forward slashes used for nesting if necessary.

The job name (build) - forward slashes not allowed.

This pattern reminds me of the Java File(String parent, String child) constructor, for which there are effectively multiple constructors. It also reminds me of the createDirectories in nio Files.

Let's imagine for a moment that there is an API to create a folder path, and that it returns a JenkinsFolder instance:

JenkinsFolder createFolders(String folderName)

That api would create folders on Jenkins, thereby validating the user input (simple path expression) and returning some read-only instance representing that path.

Then there would be two APIs to create a job:

create(JenkinsFolder folder, String jobName)

create(String jobName)

Regarding the path, the regex to detect a fully qualified path is: Pattern pattern = Pattern.compile('(job/[^/]+?)(/job/[^/])*');. However it fails if the user is "crazy" enough to call a folder "job" and a nested folder "job", because in this case it is ambiguous.

Ultimately, only Jenkins can completely validate the folderPath and the jobName. IMO it is fine to let Jenkins return 404 if the user incorrectly specifies them. I would prefer that jenkins-rest does not interfere with what Jenkins can accept.

I also prefer a solution that is predictable from the user point of view. If the internals contain different behaviors that depend on the input value, those need to be explained in the wiki. Potentially, the more internal manipulations the jenkins-rest does, the more difficult it is for the user to debug.

I think a lot... perhaps too much... what do you think?

I have not considered all the cases. Specifically, the case where a JenkinsFolder already exists. In this case, createFolder(String folderName) is not the right call to create an instance of JenkinsFolder.

@martinda so is the thought here that users can ONLY pass in the "simple names"? Would it make sense to check each iteration and if it's not preceded with "job" then insert it otherwise assume OK? I'd like to support both the simple case and the normal case if possible.

Let's assume:

simple names, simple case: a/b/c

normal names, normal case: job/a/job/b/job/c (aka fully qualified folder)

so is the thought here that users can ONLY pass in the "simple names"?

Yes.

Would it make sense to check each iteration and if it's not preceded with "job" then insert it otherwise assume OK?

Yes but there is an ambiguity: a user can create a folder called "job". Unfortunately, this is possible (I tried it). Because of the ambiguity, the code will not work if a user creates nested folders called "job" (this would be strange, but Jenkins allows it).

I'd like to support both the simple case and the normal case if possible.

Me too. But I do not know what to do about the ambiguity. So in my mind the code should only support one of the two cases. I am good with either one.

@martinda while I though there must be something we can do, after playing with things for a while, I see that its impossible given that a job name can actually be job. If it weren't for this we'd be able to check but it's simply impossible to code for things if job is a valid name.

cdancy · 2018-05-23T15:34:52Z

src/main/java/com/cdancy/jenkins/rest/parsers/OptionalFolderPathParser.java

}
+

@Dhivyaa21 looks good. With all the string manipulation would it be better to use a StringBuilder here? It would remove the need to use regex's via the replaceAll method and instead we could just remove the first and last character if present. Would also speed up the concatenation of strings below.

cdancy · 2018-05-23T17:34:27Z

src/main/java/com/cdancy/jenkins/rest/parsers/OptionalFolderPathParser.java

+            path.deleteCharAt(path.length()-1);
+        }
+
+        String[] folders = path.toString().split("/");


Think we can get away with 1 StringBuilder here. Once we do this split we can zero out the path builder and just append to that.

cdancy · 2018-05-23T17:34:40Z

src/main/java/com/cdancy/jenkins/rest/parsers/OptionalFolderPathParser.java

+
+        String[] folders = path.toString().split("/");
+        for(String folder:folders) {
+            amendedPath.append("job/").append(folder).append("/");


Lets make these constants within this class as well.

@cdancy did you mean the "job/" should be static final String?

Exactly. That and references to "/". Basically anything we're creating more than once and will potentially be used multiple times through the lifecycle of this library being used. Java will most likely cache this correctly behind the scenes but still: I like to aggressively do so within my code ;)

cdancy · 2018-05-24T13:51:36Z

Pulling this in now. Thanks for the help everyone!

cdancy · 2018-05-24T17:46:12Z

Version 0.0.11 has been released.

Dhivyaa21 added 4 commits May 19, 2018 17:45

add a paramparser to intercept the value of optionalFolderPath param

1b253e9

add @ParamParser to jobsApi methods

9c93a88

add live tests

ec0ab2b

add mock test

90e4090

cdancy self-assigned this May 20, 2018

cdancy added the enhancement label May 20, 2018

cdancy added this to the v0.0.11 milestone May 20, 2018

martinda requested changes May 21, 2018

View reviewed changes

removed the OPTIONAL_FOLDER_PATH_PARAM static constant from jobsApi

ec97b45

cdancy reviewed May 22, 2018

View reviewed changes

Dhivyaa21 added 4 commits May 22, 2018 17:38

Moved the amend folder path logic to OptionalFolderPathParser class

88e289b

removed amendpath method from jenkinsUtils

a3c9956

removed forward slash after folder path

ec3cd0f

added more mock tests

a5b3728

cdancy reviewed May 22, 2018

View reviewed changes

Dhivyaa21 added 3 commits May 23, 2018 11:11

removed leading and trailing forward slash from the folder path

52ccaee

modify live tests

f0da5b5

modify mock tests

24deeba

cdancy reviewed May 23, 2018

View reviewed changes

Used StringBuilder for string manipulation

70170d8

cdancy reviewed May 23, 2018

View reviewed changes

Dhivyaa21 and others added 2 commits May 23, 2018 14:10

make the foldername prefix and separator constants

19ed833

Update OptionalFolderPathParser.java

89a0245

cdancy merged commit 4308a17 into cdancy:master May 24, 2018

cdancy mentioned this pull request May 24, 2018

optionalFolderPath to take simple path expression also. #21

Closed

Simple optional path to jobs api #22

Simple optional path to jobs api #22

Conversation

Dhivyaa21 commented May 19, 2018

cdancy commented May 20, 2018

cdancy commented May 20, 2018

martinda 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

Dhivyaa21 May 22, 2018 • edited

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

martinda May 21, 2018 • edited

Choose a reason for hiding this comment

Dhivyaa21 commented May 22, 2018 • edited

Choose a reason for hiding this comment

cdancy commented May 22, 2018

cdancy commented May 22, 2018

Dhivyaa21 commented May 22, 2018

cdancy commented May 22, 2018

cdancy commented May 22, 2018

Dhivyaa21 commented May 22, 2018

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

martinda May 22, 2018 • edited

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

Dhivyaa21 May 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdancy commented May 24, 2018

cdancy commented May 24, 2018

Dhivyaa21 May 22, 2018 •

edited

martinda May 21, 2018 •

edited

Dhivyaa21 commented May 22, 2018 •

edited

martinda May 22, 2018 •

edited

Dhivyaa21 May 23, 2018 •

edited