Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: plugin resource loading to only load from plugin itself instead of delegating to core #4663

Merged
merged 2 commits into from
Sep 27, 2023

Conversation

guqing
Copy link
Member

@guqing guqing commented Sep 26, 2023

What type of PR is this?

/kind improvement
/area core
/area plugin
/milestone 2.10.x

What this PR does / why we need it:

重构插件类加载器以优化当插件的 resources 目录资源与 Halo 中同名时加载不到的问题

how to test it?

  1. 在创建的 resources/extensions 目录创建一个与 halo 的 resources/extensions 目录中已存在的同名 yaml
  2. 使用插件观察插件的同名文件 yaml 是否被 apply 到 halo 中
  3. 测试插件的其他功能是否正常比如静态资源加载如 logo 等

Which issue(s) this PR fixes:

Fixes #4610

Does this PR introduce a user-facing change?

重构插件类加载器以优化当插件的 resources 目录资源与 Halo 中同名时加载不到的问题

@f2c-ci-robot f2c-ci-robot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/improvement Categorizes issue or PR as related to a improvement. labels Sep 26, 2023
@f2c-ci-robot f2c-ci-robot bot added this to the 2.10.0 milestone Sep 26, 2023
@f2c-ci-robot f2c-ci-robot bot requested a review from lan-yonghui September 26, 2023 07:23
@f2c-ci-robot f2c-ci-robot bot added the area/core Issues or PRs related to the Halo Core label Sep 26, 2023
@f2c-ci-robot f2c-ci-robot bot requested a review from minliacom September 26, 2023 07:23
@f2c-ci-robot f2c-ci-robot bot added the area/plugin Issues or PRs related to the Plugin Provider label Sep 26, 2023
@guqing guqing modified the milestones: 2.10.0, 2.10.x Sep 26, 2023
@guqing
Copy link
Member Author

guqing commented Sep 26, 2023

/hold wait for #4589 is merged, perhaps there is a conflict that needs to be resolved

@f2c-ci-robot f2c-ci-robot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2023
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #4663 (a815db0) into main (9b310ca) will increase coverage by 0.09%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4663      +/-   ##
============================================
+ Coverage     60.95%   61.05%   +0.09%     
  Complexity     2610     2610              
============================================
  Files           379      379              
  Lines         13551    13606      +55     
  Branches        961      960       -1     
============================================
+ Hits           8260     8307      +47     
- Misses         4827     4832       +5     
- Partials        464      467       +3     
Files Coverage Δ
...a/run/halo/app/plugin/PluginAutoConfiguration.java 48.75% <100.00%> (+3.40%) ⬆️

... and 2 files with indirect coverage changes

@JohnNiang JohnNiang modified the milestones: 2.10.x, 2.10.0 Sep 27, 2023
@f2c-ci-robot f2c-ci-robot bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 27, 2023
@guqing
Copy link
Member Author

guqing commented Sep 27, 2023

/unhold

@f2c-ci-robot f2c-ci-robot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2023
Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

/lgtm

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2023
@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Sep 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JohnNiang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2023
@f2c-ci-robot f2c-ci-robot bot merged commit 86db26a into halo-dev:main Sep 27, 2023
2 checks passed
f2c-ci-robot bot pushed a commit to halo-dev/plugin-s3 that referenced this pull request Oct 8, 2023
Halo modified the [class loading order](halo-dev/halo#4663) of plugins in 2.10.x to P (Plugin) D (Dependencies) A (Application), resulting in a LinkageError when the current plugin is actually run.

```java
java.lang.LinkageError: loader constraint violation: when resolving method 'reactor.core.publisher.Flux reactor.core.publisher.Mono.thenMany(org.reactivestreams.Publisher)' the class loader org.pf4j.PluginClassLoader @5da2e534 of the current class, run/halo/s3os/S3OsAttachmentHandler, and the class loader org.springframework.boot.loader.LaunchedURLClassLoader @87aac27 for the method's defining class, reactor/core/publisher/Mono, have different Class objects for the type org/reactivestreams/Publisher used in the signature (run.halo.s3os.S3OsAttachmentHandler is in unnamed module of loader org.pf4j.PluginClassLoader @5da2e534, parent loader org.springframework.boot.loader.LaunchedURLClassLoader @87aac27; reactor.core.publisher.Mono is in unnamed module of loader org.springframework.boot.loader.LaunchedURLClassLoader @87aac27, parent loader 'app')
        at run.halo.s3os.S3OsAttachmentHandler.lambda$upload$28(S3OsAttachmentHandler.java:298) ~[na:na]
        at reactor.core.publisher.MonoUsing.subscribe(MonoUsing.java:85) ~[reactor-core-3.5.10.jar:3.5.10]
        at reactor.core.publisher.Mono.subscribe(Mono.java:4495) ~[reactor-core-3.5.10.jar:3.5.10]
        at reactor.core.publisher.MonoSubscribeOn$SubscribeOnSubscriber.run(MonoSubscribeOn.java:126) ~[reactor-core-3.5.10.jar:3.5.10]
        at reactor.core.scheduler.WorkerTask.call(WorkerTask.java:84) ~[reactor-core-3.5.10.jar:3.5.10]
        at reactor.core.scheduler.WorkerTask.call(WorkerTask.java:37) ~[reactor-core-3.5.10.jar:3.5.10]
        at java.base/java.util.concurrent.FutureTask.run(Unknown Source) ~[na:na]
        at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source) ~[na:na]
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) ~[na:na]
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) ~[na:na]
        at java.base/java.lang.Thread.run(Unknown Source) ~[na:na]
```

We just remove the reactive-streams dependency that is already in Halo to resolve the problem.

Fixes halo-dev/halo#4676

/kind bug

```release-note
修复在 Halo 2.10 中无法正常上传的问题
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/core Issues or PRs related to the Halo Core area/plugin Issues or PRs related to the Plugin Provider kind/improvement Categorizes issue or PR as related to a improvement. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

插件启动时 extensions 下的文件名如果和 Halo 的有重名会无法加载
2 participants