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.

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

Refactor, Updates to pom.xml, Handled potential NullPointerException issue #218

Closed
wants to merge 4 commits into from

Conversation

shivajee98
Copy link
Contributor

@shivajee98 shivajee98 commented Jan 19, 2024

pom.xml updated

Testing done

Submitter checklist

Preview Give feedback

@shivajee98 shivajee98 requested a review from a team as a code owner January 19, 2024 08:56
* @param upstreamBuildNumber Upstream build number.
* @return the form validation result.
*/
public FormValidation doCheckUpstreamBuildNumber(

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing POST/RequirePOST annotation Warning

Potential CSRF vulnerability: If DescriptorImpl#doCheckUpstreamBuildNumber connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
public static final class DescriptorImpl extends BuildStepDescriptor<Builder> {

public FormValidation doCheckProjectName(
@AncestorInPath Job<?,?> anc, @QueryParameter String value) {
public FormValidation doCheckProjectName(@AncestorInPath Job<?, ?> anc, @QueryParameter String value) {

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing POST/RequirePOST annotation Warning

Potential CSRF vulnerability: If DescriptorImpl#doCheckProjectName connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
* @param currentJob job the configuring job.
* @return The proposed project candidates.
*/
public AutoCompletionCandidates doAutoCompleteProjectNames(

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing POST/RequirePOST annotation Warning

Potential CSRF vulnerability: If DescriptorImpl#doAutoCompleteProjectNames connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
* @param job the configuring job.
* @return ok if all projects are found and a warning otherwise.
*/
public FormValidation doCheckProjectNames(

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing POST/RequirePOST annotation Warning

Potential CSRF vulnerability: If DescriptorImpl#doCheckProjectNames connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
* @param project Ancestor project.
* @return the autocompletion candidates.
*/
public AutoCompletionCandidates doAutoCompleteUpstreamProjectName(

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing POST/RequirePOST annotation Warning

Potential CSRF vulnerability: If DescriptorImpl#doAutoCompleteUpstreamProjectName connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
public static class DescriptorImpl extends Descriptor<BuildSelector> {
@Override
public String getDisplayName() {
return Messages.PermalinkBuildSelector_DisplayName();
}

public ComboBoxModel doFillIdItems(@AncestorInPath Job copyingJob, @RelativePath("..") @QueryParameter("projectName") String projectName) {
public ComboBoxModel doFillIdItems(

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing POST/RequirePOST annotation Warning

Potential CSRF vulnerability: If DescriptorImpl#doFillIdItems connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
@@ -441,8 +436,7 @@
* @throws IOException servlet communication errors.
*/
@Restricted(DoNotUse.class)
public void doHelpDetailedSteps(StaplerResponse rsp) throws IOException
{
public void doHelpDetailedSteps(StaplerResponse rsp) throws IOException {

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing POST/RequirePOST annotation Warning

Potential CSRF vulnerability: If LegacyJobConfigMigrationMonitor#doHelpDetailedSteps connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
* @param project Ancestor project.
* @return the autocompletion candidates.
*/
public AutoCompletionCandidates doAutoCompleteUpstreamProjectName(

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing permission check Warning

Potential missing permission check in DescriptorImpl#doAutoCompleteUpstreamProjectName
public static class DescriptorImpl extends Descriptor<BuildSelector> {
@Override
public String getDisplayName() {
return Messages.PermalinkBuildSelector_DisplayName();
}

public ComboBoxModel doFillIdItems(@AncestorInPath Job copyingJob, @RelativePath("..") @QueryParameter("projectName") String projectName) {
public ComboBoxModel doFillIdItems(

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing permission check Warning

Potential missing permission check in DescriptorImpl#doFillIdItems
@@ -441,8 +436,7 @@
* @throws IOException servlet communication errors.
*/
@Restricted(DoNotUse.class)
public void doHelpDetailedSteps(StaplerResponse rsp) throws IOException
{
public void doHelpDetailedSteps(StaplerResponse rsp) throws IOException {

Check warning

Code scanning / Jenkins Security Scan

Stapler: Missing permission check Warning

Potential missing permission check in LegacyJobConfigMigrationMonitor#doHelpDetailedSteps
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request.

Please split this pull request into separate pull requests for each task that you are performing. A single pull request that combines multiple types of changes is much more difficult to review than a pull request that performs a single type of change.

It is very difficult to find the change that attempts to handle a potential null pointer issue because it is hidden among the other changes.

Please retain the pull request template in all pull requests. The checklist in the pull request template reminds you (as a contributor) of the things to consider in the pull request. The checklist in the pull request template shows me (as a maintainer) of the things that have been done for the pull request.

Please include a description of the testing that you performed in each pull request to confirm that the changes have been executed by a computer. The "Testing done" section is required to have an answer. The comment in the pull request template says:

Provide a clear description of how this change was tested. At minimum this should include proof that a computer has executed the changed lines. Ideally this should include an automated test or an explanation as to why this change has no tests. Note that automated test coverage is less than complete, so a successful PR build does not necessarily imply that a computer has executed the changed lines. If automated test coverage does not exist for the lines you are changing, you must describe the scenario(s) in which you manually tested the change. For frontend changes, include screenshots of the relevant page(s) before and after the change. For refactoring and code cleanup changes, exercise the code before and after the change and verify the behavior remains the same.

The reformat of pom.xml and the Java source files should be done by enabling spotless rather than by performing a formatting operation from within your IDE. IDE based formatting is specific to the IDE and causes unnecessary changes between submitters. Many Jenkins plugins use spotless to automate the formatting of plugin source code. You can propose that change for this plugin as well. The necessary change to pom.xml is available in the following examples:

A discussion of spotless for Jenkins plugins is available in the Jenkins developer mailing list. That discussion includes links to other pull requests.

Please run mvn clean verify locally before opening the pull request so that you can see the spotbugs issues before ci.jenkins.io reports them. That also assures that you can run the automated tests of the plugin successfully. The newly introduced spotbugs warning needs to be resolved.

Thanks again for the pull request.

@shivajee98
Copy link
Contributor Author

Thanks for the pull request.

Please split this pull request into separate pull requests for each task that you are performing. A single pull request that combines multiple types of changes is much more difficult to review than a pull request that performs a single type of change.

It is very difficult to find the change that attempts to handle a potential null pointer issue because it is hidden among the other changes.

Please retain the pull request template in all pull requests. The checklist in the pull request template reminds you (as a contributor) of the things to consider in the pull request. The checklist in the pull request template shows me (as a maintainer) of the things that have been done for the pull request.

Please include a description of the testing that you performed in each pull request to confirm that the changes have been executed by a computer. The "Testing done" section is required to have an answer. The comment in the pull request template says:

Provide a clear description of how this change was tested. At minimum this should include proof that a computer has executed the changed lines. Ideally this should include an automated test or an explanation as to why this change has no tests. Note that automated test coverage is less than complete, so a successful PR build does not necessarily imply that a computer has executed the changed lines. If automated test coverage does not exist for the lines you are changing, you must describe the scenario(s) in which you manually tested the change. For frontend changes, include screenshots of the relevant page(s) before and after the change. For refactoring and code cleanup changes, exercise the code before and after the change and verify the behavior remains the same.

The reformat of pom.xml and the Java source files should be done by enabling spotless rather than by performing a formatting operation from within your IDE. IDE based formatting is specific to the IDE and causes unnecessary changes between submitters. Many Jenkins plugins use spotless to automate the formatting of plugin source code. You can propose that change for this plugin as well. The necessary change to pom.xml is available in the following examples:

A discussion of spotless for Jenkins plugins is available in the Jenkins developer mailing list. That discussion includes links to other pull requests.

Thanks again for the pull request.

As a newbie contributor I would like to know that why the checks are getting failed for this plugin. I only ran the command "mvn spotless:apply" in the terminal.

@MarkEWaite
Copy link
Contributor

As a newbie contributor I would like to know that why the checks are getting failed for this plugin

You can investigate that yourself in order to learn why the checks are failing. Read the source code at the location where the spotbugs warning is reported. Compare the source code before the change with the source code after the change. Review the spotbugs description of the warning.

I suspect it is because the thing that you thought was a refactoring (where refactoring is defined as a behavior preserving transformation of the source code) is probably not a refactoring. Something changed that you did not expect to change. I admit that I am guessing because I'm not planning to spend the effort to identify what went wrong in this pull request. The pull request includes too many changes. When you split it into multiple pull requests, you'll be more likely to detect the root of the problem because there are fewer changes to compare.

@shivajee98
Copy link
Contributor Author

Thanks for your time, I will try to fix that.

@shivajee98 shivajee98 closed this Jan 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants