chore: Enable sharing spring context in pf4j plugins#37002
chore: Enable sharing spring context in pf4j plugins#37002nidhi-nair wants to merge 2 commits intoreleasefrom
Conversation
WalkthroughThe changes introduced in this pull request involve the addition of a new dependency to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/server/appsmith-interfaces/pom.xml (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BasePlugin.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/PluginConfiguration.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BasePlugin.java (2)
6-6: LGTM: Import statement added for SpringPlugin.The import statement for
SpringPluginis correctly added, aligning with the change in class inheritance.
8-8: Inheritance changed to SpringPlugin. Verify plugin behavior.The change from
PlugintoSpringPluginaligns with the PR objectives. This modification enables sharing of the Spring context in pf4j plugins.Ensure that all plugins extending
BasePluginstill function correctly with this change. Run the following script to identify all classes that extendBasePlugin:✅ Verification successful
Inheritance changed to SpringPlugin successfully. All extending classes are accounted for.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all classes that extend BasePlugin # Test: Search for classes extending BasePlugin rg -t java "class\s+\w+\s+extends\s+BasePlugin"Length of output: 6564
app/server/appsmith-interfaces/pom.xml (1)
64-69: Consider updating pf4j-spring to a newer version.The addition of pf4j-spring aligns with the PR objectives. However, version 0.8.0 is from 2020. A newer version might offer improved features or bug fixes.
To check for newer versions, run:
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/PluginConfiguration.java (4)
21-22: Method signature update aligns with context sharingThe
pluginManagernow acceptsApplicationContext, which is necessary for enabling plugins to share the Spring context.
52-55: VerifyApplicationContextconsistency insetApplicationContextEnsure that
setApplicationContextis called appropriately so that both theCustomPluginManagerand itsCustomSpringExtensionFactoryhave the correctApplicationContext.
64-75: Plugin initialization sequence is correctly implementedThe
initmethod properly loads and starts plugins and injects extensions into the Spring context.
91-99: Extension creation handles context inheritance properlyThe
createmethod inCustomSpringExtensionFactorycorrectly sets the parentApplicationContextfor plugin extensions.
| public CustomPluginManager(ApplicationContext applicationContext) { | ||
| super(); | ||
| this.applicationContext = applicationContext; | ||
| this.ef = new CustomSpringExtensionFactory(this); |
There was a problem hiding this comment.
Eliminate redundant initialization of ef
The CustomSpringExtensionFactory instance ef is initialized both in the constructor and in createExtensionFactory(). This duplication is unnecessary.
Apply this diff to remove the redundant initialization:
- this.ef = new CustomSpringExtensionFactory(this);Also applies to: 41-44
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BasePlugin.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BasePlugin.java (2)
6-7: LGTM: Import statements are correct.The new imports for
SpringPluginandApplicationContextare necessary for the changes made to the class.
18-21: Clarify the purpose of returning null in createApplicationContext().The
createApplicationContext()method currently returns null. This might not be the intended behavior for creating an application context. Consider implementing this method to return a validApplicationContextor provide a comment explaining why it returns null.Could you clarify the reasoning behind this implementation?
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BasePlugin.java
Show resolved
Hide resolved
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Closing as not important right now without native git |
1 participant
Description
As of today, we do not use any DI concepts in plugins because our extensions are unable to use the same application context as server. This PR allows us to extend from the same application context. Implications of clashes across plugins will need to be checked, and conventions and compile time checks for it need to be established.
Automation
/ok-to-test tags="@tag.Git,@tag.Sanity"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11452764576
Commit: 7547480
Cypress dashboard.
Tags: @tag.Git,@tag.Sanity
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ServerSide/OnLoadTests/OnLoadActions_Spec.ts
- cypress/e2e/Regression/ServerSide/QueryPane/DSDocs_Spec.ts
List of identified flaky tests.Tue, 22 Oct 2024 04:34:00 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
pf4j-springto facilitate this integration.Improvements
BasePluginclass to extendSpringPlugin, allowing better compatibility with Spring's plugin architecture.PluginConfigurationto utilize Spring'sApplicationContext, improving plugin lifecycle management and extension creation.