Return useful data from /pluginManager/api/json by default#26351
Return useful data from /pluginManager/api/json by default#26351himanshu748 wants to merge 4 commits intojenkinsci:masterfrom
Conversation
|
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
comment-ops-bot
bot
added
rfe
There was a problem hiding this comment.
Pull request overview
This PR fixes JENKINS-27993 by making /pluginManager/api/json return meaningful PluginWrapper details at the default API depth, instead of {} entries, by increasing export visibility for key properties.
Changes:
- Increase
@Exportedvisibility to2for keyPluginWrapperproperties so they appear at default depth underPluginManager.plugins. - Add
@ExportedtoPluginWrapper#getDisplayName()as the non-deprecated alternative togetLongName(). - Add an integration test validating that
/pluginManager/api/jsonreturns non-empty plugin objects and expected fields.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/src/main/java/hudson/PluginWrapper.java | Raises export visibility for commonly needed plugin metadata (and exports displayName) so nested plugin entries are populated at default depth. |
| test/src/test/java/hudson/PluginManagerTest.java | Adds a regression test for /pluginManager/api/json ensuring plugin details are present without specifying depth. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Verify key properties are present at default depth (visibility = 2) | ||
| assertNotNull(htmlPublisher.optString("shortName", null), "shortName should be exported at default depth"); | ||
| assertNotNull(htmlPublisher.optString("version", null), "version should be exported at default depth"); | ||
| assertNotNull(htmlPublisher.optString("longName", null), "longName should be exported at default depth"); | ||
| assertNotNull(htmlPublisher.optString("displayName", null), "displayName should be exported at default depth"); | ||
| assertTrue(htmlPublisher.has("active"), "active should be exported at default depth"); | ||
| assertTrue(htmlPublisher.has("enabled"), "enabled should be exported at default depth"); | ||
| assertTrue(htmlPublisher.has("hasUpdate"), "hasUpdate should be exported at default depth"); |
There was a problem hiding this comment.
The PR description says the new test verifies that the response includes url, but this test currently never asserts that url is present. Either add an assertion for url (and, if needed for stability, set up update site data like prevalidateConfig() does with /plugins/htmlpublisher-update-center.json) or update the PR description to match what’s actually being tested.
MarkEWaite
changed the title
MarkEWaite
left a comment
MarkEWaite
left a comment
There was a problem hiding this comment.
Thanks for the pull request and thanks for including a test of the changes.
There are two changes that I believe are needed.
daniel-beck
left a comment
daniel-beck
left a comment
There was a problem hiding this comment.
Do not change lines unrelated to your pull request.
himanshu748
force-pushed
the
fix/JENKINS-27993-pluginmanager-api
branch
2 times, most recently
from
c25d2e1 to
4db6bed
Compare
…ties The /pluginManager/api/json endpoint returned empty objects for each plugin because all @exported properties had default visibility (1), which is not rendered when nested inside PluginManager at the default API depth. Increase visibility to 2 on key properties (shortName, version, longName, displayName, url, active, enabled, hasUpdate) so they appear at the default depth. Also add @exported to getDisplayName() as the non-deprecated replacement for getLongName().
himanshu748
force-pushed
the
fix/JENKINS-27993-pluginmanager-api
branch
from
4db6bed to
9193501
Compare
MarkEWaite
left a comment
MarkEWaite
left a comment
There was a problem hiding this comment.
Please don't force push changes. It breaks the link between pull request comments and the commits. We will squash merge at the end, so flawed steps along the way are not an issue.
Please remove the @Exported(visibility = 2) change from getLongName or explain why you must retain it.
- Remove @exported from deprecated properties lengthName & backupVersion - Fix Issue link in PluginManagerTest from JENKINS-27993 to issues/21047
himanshu748
left a comment
himanshu748
left a comment
There was a problem hiding this comment.
Thanks for the review. I have removed @Exported(visibility = 2) from the deprecated getLongName() method, fixed the issue link in the test to point to GitHub, and updated the PR description to accurately reflect that url is not being asserted in this test since it requires update site metadata. Pushed without a force push.
|
|
||
| /** | ||
| * Makes sure that thread context classloader isn't used by {@link UberClassLoader}, or else | ||
| * Makes sure that thread context classloader isn't used by |
There was a problem hiding this comment.
No reformatting please. This applies to most changed lines in this file.
There was a problem hiding this comment.
Apologies for the noise. All unrelated formatting changes have been reverted in ca4cdf2. The test file diff is now purely additive (+30/-0) — only the new test method, zero reformatting.
Reverts all auto-formatter whitespace/reformatting changes that were unrelated to this PR. Only retains the new test method for /pluginManager/api/json.
MarkEWaite
left a comment
MarkEWaite
left a comment
There was a problem hiding this comment.
Thanks. Change looks good to me
MarkEWaite
added
the
squash-merge-me
@Exportedvisibility to2for keyPluginWrapperproperties so they appear at default depth underPluginManager.plugins.@ExportedtoPluginWrapper#getDisplayName()as the non-deprecated alternative togetLongName()./pluginManager/api/jsonreturns non-empty plugin objects and expected fields.