[JENKINS-48625] Restore binding of doCheckUrl methods and add some initial checks #841
Conversation
|
Thanks for the proposal. I started the code review today by walking through the new test in the debugger and watching that it reached all the branches of doCheckUrl(). Good work! I pushed a commit in f99b832 . You are welcome to discard those changes if you're uncomfortable with them. The key things that I would suggest for your consideration in the test are:
|
|
Thank you for reviewing the PR @MarkEWaite . I've seen many of the suggestions you have mentioned implemented in GitClientTest (Git-Client-Plugin), I'll follow these suggestions and take it as a reference point. Thanks for the commit f99b832, those changes (good-practices) will give me a head start on making a better test class. |
src/main/java/hudson/plugins/git/browser/GitBlitRepositoryBrowser.java
Outdated
Show resolved
Hide resolved
src/main/java/hudson/plugins/git/browser/GitBlitRepositoryBrowser.java
Outdated
Show resolved
Hide resolved
|
LGTM, thanks for the fixing effort! |
I had to read very carefully to detect the syntactic errors in the URL. The comment helps me see more clearly.
Avoid undue load on any one server for this test.
MarkEWaite
changed the title
|
@Saucistophe thanks for the review! @MarkEWaite I have taken the liberty of tweaking the code a little seeing this test case |
|
That's a great choice. I assume you'll delete that test or rework it so that it is useful based on your change? |
I have re-worked the test, now it asserts that the FormValidation message is "Invalid URL" |
| @@ -105,7 +103,7 @@ public FormValidation doCheckRepoUrl(@AncestorInPath Item project, @QueryParamet | |||
| return FormValidation.ok(); | |||
| } | |||
| FormValidation response; | |||
| if (checkURIFormat(cleanUrl)) { | |||
| if (checkURIFormat(cleanUrl, "gitblit")) { | |||
There was a problem hiding this comment.
I don't think you can reasonably assume that a gitblit server must include gitblit in the hostname. Assembla only provides a hosted solution, so it is safe to check their specific hostname. Gitblit provides a solution that is either hosted or on premise.
There was a problem hiding this comment.
Okay, I never anticipated this obvious case. My implementation was based on this assumption that the hostname of the URL will contain the browser name (for ex: http://gitblit.example.com/).
As far as my understanding goes, either I can:
- remove this assumption, but then would have to provide a custom implementation for Assembla, i.e making checkURIFormat an abstract method.
- branch the logic into two cases, hosted check and on-premise check.
What do you think?
There was a problem hiding this comment.
I think that Assembla is the exception in this case. Most git hosting providers allow a hosted implementation and an implementation that is installed and maintained inside the user network.
I think it would be better to place checkURIFormat (or some more specific implementation of it) inside the Assembla specific class and then place something more general in the GitRepositoryBrowser class. The part of checkURIFormat that confirms the URL is an http or https URL is useful in all cases. The part that checks for a specific substring in the hostname is likely limited to Assembla and possibly a few others.
There was a problem hiding this comment.
Most git hosting providers allow a hosted implementation and an implementation that is installed and maintained inside the user network.
This means that creating a simple host name check is not feasible, hence I have created a separate checkURIFormatAndHostName for AssemblaWeb since Assembla provides a hosted solution only. For others, it's just a plain UrlValidatior check. For reference, the commit is 4a0915a .
src/main/java/hudson/plugins/git/browser/GitRepositoryBrowser.java
Outdated
Show resolved
Hide resolved
src/main/java/hudson/plugins/git/browser/GitRepositoryBrowser.java
Outdated
Show resolved
Hide resolved
| return FormValidation.ok(); | ||
| } | ||
| FormValidation response; | ||
| if (checkURIFormat(cleanUrl)) { | ||
| if (checkURIFormat(cleanUrl, "gerrit")) { |
There was a problem hiding this comment.
Can't assume that the Gerrit server includes "gerrit" in its hostname.
There was a problem hiding this comment.
Agreed, implementation has been changed.
| return FormValidation.ok(); | ||
| } | ||
| FormValidation response; | ||
| if (checkURIFormat(cleanUrl)) { | ||
| if (checkURIFormat(cleanUrl, "viewgit")) { |
There was a problem hiding this comment.
Can't assume viewgit server contains viewgit in its hostname, I think?
compile time errors corrected. This reverts commit 454397e.
| }.check(); | ||
| }.check(); | ||
| } else { | ||
| response = FormValidation.error(Messages.invalidUrl()); |
There was a problem hiding this comment.
I think this can be replaced with a return instead of storing the value the returning one statement later.
| response = FormValidation.error(Messages.invalidUrl()); | |
| return FormValidation.error(Messages.invalidUrl()); |
There was a problem hiding this comment.
Agreed, changed it.
| } else { | ||
| response = FormValidation.error(Messages.invalidUrl()); | ||
| } | ||
| return response; |
There was a problem hiding this comment.
I think this can be removed as part of removing the temporary variable response.
| return response; |
| } else { | ||
| return FormValidation.error("This is a valid URL but it doesn't look like ViewGit"); | ||
| } | ||
| FormValidation response; |
There was a problem hiding this comment.
I don't think this temporary is required, since the call to checkURIFormat can return from both the true and the false conditions.
| FormValidation response; |
There was a problem hiding this comment.
Agreed, changed it.
| throws UnsupportedEncodingException { | ||
| return param(url).add("p=" + projectName).add("a=commitdiff").add("h=" + path.getChangeSet().getId()) + "#" + URLEncoder.encode(path.getPath(),"UTF-8").toString(); | ||
| } | ||
| private String buildCommitDiffSpec(URL url, Path path) |
There was a problem hiding this comment.
Don't "fix" whitespace because it causes unnecessary conflicts in other pull requests. Refer to the style guide in the contributing guideline for this repo. In many other places it would be a good thing to correct the horribly inconsistent code formatting style. In this particular plugin, it is not a good thing to correct the code formatting style.
| private String buildCommitDiffSpec(URL url, Path path) | |
| private String buildCommitDiffSpec(URL url, Path path) |
There was a problem hiding this comment.
I apologize I've constantly ignored whitespace conflicts, I ran "Reformat Code" option available in IntelliJ which fixed all these whitespaces.
I have tried to fix all of them in the latest commit, I hope I have reverted all these fixes.
|
@MarkEWaite after the latest commit 5f48308, are there any more changes you would recommend? If not so, then I can create similar UTs for the other three browsers as well and move forward with the status of this PR. |
|
I need to review the current code again. It will likely be tomorrow before I can review it. |
MarkEWaite
added
bugfix
|
Since you have just mentioned that you are going to be busy next week, are you planning to review this PR and PR-830 within the time frame of the new release? I just need your comments on the current commits and I will try to push the changes after your review as fast as possible. |
|
Thanks for asking! Based on travel schedule, I'll likely release 4.2 tomorrow as it currently exists on the master branch then reassign the remaining pull requests to a later release (4.2.1 or 4.3). |
Refspec expansion is handled in another pull request and should be evaluated separately.
|
Thanks for the review Mark! I don't suspect any further changes right now, you may merge it. Apart from this PR, if there is any other documentation related task or fix pending for the new release, would be glad to help! |
JENKINS-48625
The earlier doCheckUrl methods used to open a connection to validate a particular browser URL but that can be avoided (mostly) by adding some initial checks based on URL syntax validity and domain name match of the particular browser.
This change has been implemented for AssemblaWeb, GitBlitRepositoryBrowser, Gitiles and ViewGitWeb.
Note: A test file has been written for AssemblaWeb class, if it is considered okay then will do the same thing for the other 3 browser classes as well.
Submitter Checklist
Types of changes
Reviewer Checklist