Allow removing plugin that's optionally extended

[cwperks]

Description

This is a small PR to fix an issue calling ./bin/opensearch-plugin remove <plugin-name> on a plugin that is optionally extended from another installed plugin. Given that the plugin is optionally extensible, it means the extending plugin does not require for it to be installed.

Tested by installing workload-management, then security and then removing workload-management.

Before the change it fails with the message from the linked issue. After the change workload-management can be removed as expected.

Related Issues

Resolves #20387

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[coderabbitai]

📝 Walkthrough

Walkthrough

findPluginsByDependency now returns two lists (required dependents and optional extenders). The plugin removal flow uses both lists: required dependents still block removal, while optional extenders are collected and emitted as a warning. CHANGELOG updated with a Fixed entry.

Changes

Cohort / File(s)	Summary
Changelog CHANGELOG.md	Added a Fixed entry: "Allow removing plugin that's optionally extended" with PR reference.
Plugins lookup server/src/main/java/org/opensearch/plugins/PluginsService.java	findPluginsByDependency(Path,String) signature changed to return Tuple<List<String>, List<String>> where v1 = required dependents and v2 = optionally extended dependents; logic splits dependents into required vs optional and Javadoc updated.
CLI removal command distribution/tools/plugin-cli/src/main/java/org/opensearch/tools/cli/plugin/RemovePluginCommand.java	Adjusted to unpack the Tuple from findPluginsByDependency, block removal if required dependents exist, and emit a warning when optional extenders are present (no change to external CLI signature/flow).
Tests server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java	Updated tests to expect and assert on the Tuple return (using v1() and v2()) instead of a single list.

Sequence Diagram(s)

sequenceDiagram
  actor User
  participant CLI as RemovePluginCommand
  participant Service as PluginsService
  participant FS as PluginsDir/FileSystem

  User->>CLI: remove <pluginName>
  CLI->>Service: findPluginsByDependency(pluginsDir, pluginName)
  Service-->>CLI: Tuple(requiredBy, optionallyExtendedBy)
  alt requiredBy non-empty
    CLI->>User: print error blocking removal (list requiredBy)
  else requiredBy empty
    alt optionallyExtendedBy non-empty
      CLI->>User: print warning (list optionallyExtendedBy)
    end
    CLI->>FS: delete plugin directory
    FS-->>CLI: deletion result
    CLI->>User: print success/failure
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• kmk142789

Poem

🐇
I nibbled bytes beneath the moonlight,
optional ties loosened in the night.
Hop, patch, remove — the path is clear,
upgrades dance, the burrow cheers. 🎵

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Allow removing plugin that's optionally extended' clearly and concisely summarizes the main change: enabling plugin removal when optionally extended rather than blocking it.
Description check	✅ Passed	The PR description covers the issue (plugin removal blocked when optionally extended), the solution approach, testing performed, and the linked issue reference, but does not explicitly state whether the functionality test checkbox is truly completed.
Linked Issues check	✅ Passed	The PR fully addresses issue #20387 by modifying the plugin removal logic to distinguish between required and optional dependencies, allowing removal when only optional extensions exist.
Out of Scope Changes check	✅ Passed	All changes are directly in scope: the Tuple return type change, removal logic update, test adjustments, and changelog entry all serve the goal of allowing removal of optionally-extended plugins.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java (1)

1187-1198: Consider adding test coverage for optional extensions.

The tests correctly validate the Tuple unpacking for required dependencies (v1), but all test cases expect v2 (optional extensions) to be empty. Consider adding a test case with a plugin that has an optional extended plugin (e.g., extended.plugins = "base-plugin;optional=true") to verify that such plugins appear in v2.

Example test case for optional extensions

// Add another plugin with optional extension
Path plugin4Dir = pluginsDir.resolve("plugin4");
PluginTestUtil.writePluginProperties(
    plugin4Dir,
    "description",
    "Plugin 4",
    "name",
    "plugin4",
    "version",
    "1.0",
    "opensearch.version",
    Version.CURRENT.toString(),
    "java.version",
    "1.8",
    "classname",
    "Plugin4",
    "extended.plugins",
    "base-plugin;optional=true"
);

Tuple<List<String>, List<String>> basePluginDependentsWithOptional = PluginsService.findPluginsByDependency(pluginsDir, "base-plugin");
assertThat(basePluginDependentsWithOptional.v1(), containsInAnyOrder("plugin1", "plugin2"));
assertThat(basePluginDependentsWithOptional.v2(), containsInAnyOrder("plugin4"));

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 738237f and 3479cf5.

📒 Files selected for processing (3)

• distribution/tools/plugin-cli/src/main/java/org/opensearch/tools/cli/plugin/RemovePluginCommand.java
• server/src/main/java/org/opensearch/plugins/PluginsService.java
• server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2026-01-13T17:40:27.167Z

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20411
File: server/src/main/java/org/opensearch/index/codec/CodecService.java:112-133
Timestamp: 2026-01-13T17:40:27.167Z
Learning: Avoid capturing or evaluating a supplier (e.g., this::defaultCodec) upfront when passing it to a registry during object construction. If registries may replace defaults during iteration (as in EnginePlugin.getAdditionalCodecs), pass the supplier itself and only resolve it at use time. This ensures dynamic behavior is preserved during initialization and prevents premature binding of defaults in codecs/registry setup. This pattern should apply to similar initialization paths in Java server code where registries may mutate defaults during construction.

Applied to files:

• server/src/main/java/org/opensearch/plugins/PluginsService.java

🧬 Code graph analysis (2)

server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java (1)

server/src/main/java/org/opensearch/plugins/PluginsService.java (1)

• PluginsService (91-907)

distribution/tools/plugin-cli/src/main/java/org/opensearch/tools/cli/plugin/RemovePluginCommand.java (1)

server/src/main/java/org/opensearch/plugins/PluginsService.java (1)

• PluginsService (91-907)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)

• GitHub Check: gradle-check
• GitHub Check: detect-breaking-change
• GitHub Check: Mend Security Check
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: Analyze (java)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (25, ubuntu-24.04-arm)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: precommit (25, macos-15)

🔇 Additional comments (3)

distribution/tools/plugin-cli/src/main/java/org/opensearch/tools/cli/plugin/RemovePluginCommand.java (2)

41-41: LGTM!

Import correctly added for the Tuple class to support the new return type from findPluginsByDependency.

---

102-115: LGTM!

The implementation correctly:

1. Unpacks the Tuple into required (usedBy) and optional (optionallyExtendedBy) dependents
2. Blocks removal with an exception only when there are required dependents
3. Prints a user-friendly warning when optional extensions exist, allowing removal to proceed

This aligns with the PR objective to allow removing plugins that are only optionally extended.

server/src/main/java/org/opensearch/plugins/PluginsService.java (1)

408-432: LGTM!

The method implementation is clean and correct:

• Javadoc clearly documents the Tuple structure (v1 = required dependencies, v2 = optional dependencies)
• Logic correctly categorizes plugins using isExtendedPluginOptional()
• Returns immutable-safe new lists within the Tuple

This enables the RemovePluginCommand to differentiate between required and optional extensions.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

❌ Gradle check result for 6661e58: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 738237f: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.22%. Comparing base (f6c78d7) to head (3479cf5).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ensearch/tools/cli/plugin/RemovePluginCommand.java	60.00%	1 Missing and 1 partial ⚠️
...in/java/org/opensearch/plugins/PluginsService.java	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20417      +/-   ##
============================================
- Coverage     73.32%   73.22%   -0.11%     
+ Complexity    71862    71797      -65     
============================================
  Files          5793     5792       -1     
  Lines        328644   328629      -15     
  Branches      47313    47307       -6     
============================================
- Hits         240990   240629     -361     
- Misses        68324    68706     +382     
+ Partials      19330    19294      -36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

✅ Gradle check result for 3479cf5: SUCCESS