Skip to content

Disable plugin loading on illegal/ not set plugin dir#86

Merged
infeo merged 2 commits intodevelopfrom
feature/no-plugin-loading-on-invalid-dir
Mar 12, 2026
Merged

Disable plugin loading on illegal/ not set plugin dir#86
infeo merged 2 commits intodevelopfrom
feature/no-plugin-loading-on-invalid-dir

Conversation

@infeo
Copy link
Member

@infeo infeo commented Mar 12, 2026

Only search directories, if pluginDir property is set and valid

@infeo infeo added this to the next milestone Mar 12, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 12, 2026

Walkthrough

The changes modify the ClassLoaderFactory to add explicit null and blank-value handling for the plugin directory system property. Previously, the code attempted home-directory expansion; this behavior is removed and replaced with direct path construction. The method now logs jar discovery only when jars are found and logs debug information on directory read failures. Test coverage is expanded with a new nested test class to verify behavior for undefined and blank/whitespace property values, replacing the previous test for home-directory expansion.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: adding validation checks to disable plugin loading when the plugin directory is not set or invalid.
Description check ✅ Passed The description directly relates to the changeset, explaining that the PR prevents plugin loading when the plugin directory is not configured or invalid.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/no-plugin-loading-on-invalid-dir

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java (1)

115-118: Consider cleaning up system property in afterEach.

The MockedStatic cleanup is correct. However, tests in this class (including existing ones at lines 72 and 95) modify the cryptomator.pluginDir system property without restoring its original state. This could cause test interference depending on execution order.

♻️ Suggested improvement
+		String originalPluginDir;
+
 		`@BeforeEach`
 		public void beforeEach() {
+			originalPluginDir = System.getProperty("cryptomator.pluginDir");
 			mockedClass = Mockito.mockStatic(ClassLoaderFactory.class);
 			mockedClass.when(() -> ClassLoaderFactory.forPluginDir()).thenCallRealMethod();
 			mockedClass.when(() -> ClassLoaderFactory.forPluginDirWithPath(any())).thenReturn(null);
 		}
 
 		`@AfterEach`
 		public void afterEach() {
 			mockedClass.close();
+			if (originalPluginDir == null) {
+				System.clearProperty("cryptomator.pluginDir");
+			} else {
+				System.setProperty("cryptomator.pluginDir", originalPluginDir);
+			}
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java`
around lines 115 - 118, The tests modify the system property
"cryptomator.pluginDir" but never restore it; update the test class to save the
original value (e.g., a private String originalPluginDir) before tests run and
restore it in the existing afterEach() method (after mockedClass.close()) so the
property is either reset to the saved value or removed if it was originally
null. Locate the setup where "cryptomator.pluginDir" is changed and add code to
capture System.getProperty("cryptomator.pluginDir") into originalPluginDir, and
in afterEach() perform conditional restore via System.setProperty or
System.clearProperty accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java`:
- Around line 115-118: The tests modify the system property
"cryptomator.pluginDir" but never restore it; update the test class to save the
original value (e.g., a private String originalPluginDir) before tests run and
restore it in the existing afterEach() method (after mockedClass.close()) so the
property is either reset to the saved value or removed if it was originally
null. Locate the setup where "cryptomator.pluginDir" is changed and add code to
capture System.getProperty("cryptomator.pluginDir") into originalPluginDir, and
in afterEach() perform conditional restore via System.setProperty or
System.clearProperty accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4532f0aa-2c4d-43d2-a895-2cc14dfc0248

📥 Commits

Reviewing files that changed from the base of the PR and between 59d3a25 and b38cfa6.

📒 Files selected for processing (2)
  • src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java
  • src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java

@infeo infeo merged commit 5d8010b into develop Mar 12, 2026
8 checks passed
@infeo infeo deleted the feature/no-plugin-loading-on-invalid-dir branch March 12, 2026 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant