[FIXED JENKINS-33201 Java JSON exception with an empty parametrized build.] #2444
Conversation
| .getJSONObject("hudson-model-ParametersDefinitionProperty"); | ||
| if ((parameterDefinitionProperty.getBoolean("specified") == true) | ||
| && !parameterDefinitionProperty.has("parameterDefinitions")) { | ||
| throw new Failure(Messages.Hudson_NoParamsSpecified()); |
There was a problem hiding this comment.
Should throw FormException at least
oleg-nenashev
added
the
needs-more-reviews
|
I think it would be better to just ignore empty property instead of throwing exxceptions |
|
I have changed to FormException. Please check now. |
| */ | ||
| public static void checkForEmptyParameters(JSONObject jsonProperties) throws FormException{ | ||
| JSONObject parameterDefinitionProperty = jsonProperties | ||
| .getJSONObject("hudson-model-ParametersDefinitionProperty"); |
There was a problem hiding this comment.
This file uses tabs, not spaces
So it cannot have been reverted.
…sure that every Queue.Item.task corresponds to a live Job, lest nextBuildNumber be bogus.
… are not available .
|
Seems you've rebased some master branch changes instead of merging them. Not a problem if we squash the PR during merging. |
| * @param jsonProperties | ||
| * @throws FormException | ||
| */ | ||
| public static void checkForEmptyParameters(JSONObject jsonProperties) throws FormException{ |
There was a problem hiding this comment.
I do not see this method as a part of public API (especially in the Jenkins class). There are hardcoded values for jobs only. Would be better to just inline this method to the code or to move it to a private method in the Job class
|
And regarding the comment "
Please squash these commits, so that my commits wont mess up the git log in master 👍 |
| * This handles the situation when Parameterized build checkbox is checked | ||
| * but no parameters are selected. User will be redirected to an error page | ||
| * with proper error message. | ||
| * @param jsonProperties |
There was a problem hiding this comment.
Just for the future. Please avoid using empty Javadoc definitions. Default Javadoc linting behavior in JDK8 considers it as an error
|
weak 👍 for merging with squash |
I will take care of it in future. |
|
@rupinr this is instruction for the merger (GitHub provides directly the ability to squash before merge) |
|
@Vlatombe So, I guess nothing is pending from my side. Right? |
|
@rupinr Merged the PR. Thanks for your contribution! |
|
Thanks rupinr. |
| private static void checkForEmptyParameters(JSONObject jsonProperties) throws FormException{ | ||
| JSONObject parameterDefinitionProperty = jsonProperties.getJSONObject("hudson-model-ParametersDefinitionProperty"); | ||
| if ((parameterDefinitionProperty.getBoolean("specified") == true)&& !parameterDefinitionProperty.has("parameterDefinitions")) { | ||
| throw new FormException(Messages.Hudson_NoParamsSpecified(),"parameterDefinitions"); |
There was a problem hiding this comment.
As noted in #2589, I think this is overkill. Better to just ignore the checkbox.
At least part of the impetus for this issue was dealt with a while back by a revert of PR jenkinsci#2444, but this gives us a better solution anyway by guaranteeing we have a null property rather than one without parameters.
I have handled this issue gracefully. Now user will be redirected to an error page giving information about the error (Similar to how it is handled for empty job/view name)