[JENKINS-49331] Handle exceptions when rebuilding with invalid choice parameter

[Fredrik93]

See JENKINS-49331.

Testing done

Testing done by compiling the project and running the light-test profile provided

Proposed changelog entries

• JENKINS-49331, human-readable text

[welcome]

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

[MarkEWaite]

Thanks for the pull request!

The proposed change doesn't seem to change the behavior much from the existing behavior. An "Oops" page is shown to the user when they enter an invalid value for a choice parameter with this change, just as an "Oops" page was shown to the user before this change.

The "Oops" page that I see looks like this:

Thanks for completing the "Testing done" entry.

Please provide a proposed changelog entry that more completely describes the change in terms that a user will understand.

Please also reinsert the rest of the pull request template and complete the checkboxes. It helps you as a submitter and it helps maintainers as reviewers.

[MarkEWaite]

I tested this interactively by performing the following steps:

1. Compile with mvn -am -pl war,bom -Pquick-build clean install
2. Run with mvn -pl war jetty:run
3. Install the rebuilder plugin (required to duplicate the issue)
4. Restart Jenkins
5. Create a freestyle project that takes a choice parameter "CHOSEN_LETTER" with several single letter choices ("A", "B", ...)
6. Run the freestyle project, choosing various parameters and confirm they succeed
7. Click the "Rebuild last" link on the job page, and enter an invalid value in the CHOSEN_LETTER field
8. Click the button to run the job

That results in the "Oops" page that is mentioned in #9027 (review)

Without this change, the "Oops" page looks like this:

[MarkEWaite]

I'm not sure what the user expects when they rebuild with an invalid value for a choice parameter. I assume they expect the job to not be executed because an invalid value was provided.

The checkValue method can either throw an unchecked exception (like IllegalArgumentException in the current code and in this pull request) or its callers can be modified to accept a StringParameterValue return value and then they return that result. If the returned StringParameterValue is returned as null for invalid parameter values, then the "Oops" page is not shown to the user, but the rebuild then proceeds and uses the default value of the parameter, instead of using the invalid value that was entered by the user. That is a change from the current behavior and will certainly surprise users.

I don't think that changing the signature of checkValue to throw a checked exception (like IOException) will help because the callers inside the ChoiceParameterDefinition class will need to catch that exception and do something with it. As far as I can tell, the only alternatives are to return the StringParameterValue or to return null and neither of those alternatives will stop the job from being executed.

[Fredrik93]

Hi Mark, i really appreciate your comments, theyre really helpful to me. im trying to reproduce the issue the way that you described, i also agree with your last comment and ill try to make the correct adjustments.

[MarkEWaite]

Hi Mark, i really appreciate your comments, theyre really helpful to me. im trying to reproduce the issue the way that you described, i also agree with your last comment and ill try to make the correct adjustments.

Thanks again for the pull request. Unfortunately, my comments only highlight the problem and don't really offer a solution. I don't understand how to solve the problem without wider changes than I think would be appropriate for this case.

[Fredrik93]

oh, i see. I might have underestimated the issue. I picked an issue from the newbie-friendly issues. What are your thoughts on this, do you reckon that i should abandon this task and try my hand at something else, more simple? I understand thats it of course based on my skills and its hard for you to know, but just changing method took me a while, so fiddling with anything more complex would probably be too much for me

[MarkEWaite]

oh, i see. I might have underestimated the issue. I picked an issue from the newbie-friendly issues. What are your thoughts on this, do you reckon that i should abandon this task and try my hand at something else, more simple? I understand thats it of course based on my skills and its hard for you to know, but just changing method took me a while, so fiddling with anything more complex would probably be too much for me

I think that this issue is incorrectly labeled as "newbie-friendly". I think you are wise to choose another issue. If you choose to work on another issue, you can close this pull request.