Fail fast when the frontend sends an invalid class name #322
Conversation
|
Does the core team have any thoughts on this? I think that failing fast is generally always preferable to make the error more clear to users. If there are concerns about the error being too specific to Jenkins or tables-to-divs, I can always remove the link from the error message and keep it more generic. |
| if (isArray) { | ||
| throw new IllegalArgumentException( | ||
| "The frontend sent an unexpected list of classes (" | ||
| + className |
There was a problem hiding this comment.
Please read JENKINS-67550 (which I linked to in the PR description). It contains an example value seen in the wild:
["org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition","org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition"]
There was a problem hiding this comment.
So we could also check, say,
className.contains(",")?
| "The frontend sent an unexpected list of classes (" | ||
| + className | ||
| + ") rather than an expected single class. See" | ||
| + " https://www.jenkins.io/doc/developer/views/table-to-div-migration/" |
There was a problem hiding this comment.
I am pretty sure tables-to-divs is not the only case where this problem arises, though it may be the most common one recently.
There was a problem hiding this comment.
Yeah, I have no strong preference about including the link in the error message. If we think that the error is better without the link I am happy to remove it.
There was a problem hiding this comment.
Oh definitely include the link. I just meant it might be appropriate to adjust the phrasing to explain that this is only one possible root cause.
There was a problem hiding this comment.
I will keep an eye out for other causes in the future and adjust the message then if needed.
Untested PR to return better error messages for users who are affected by tables-to-divs issues. In JENKINS-67218 and JENKINS-67550, a tables-to-divs issue caused the frontend to send a list of classes rather than a single class to the backend. This list eventually made its way to the class loader, resulting in confusing error messages. Users thought that the classloader couldn't load the classes because they weren't present, but the real reason class loading is failing is that we're passing a list rather than a single class to the class loader. This PR changes the code to fail fast in this situation and give the user an informative error message before the problem makes its way to lower layers.