konlue commented on issue #7607: URL: https://github.com/apache/shenyu/issues/7607#issuecomment-6159931513
Hi <mark>@Aias00</mark>, I'd like to take this one. While verifying on master I found the issue's evidence doesn't match the current code **(though I may be missing something on my end)**, so I'd like to confirm a revised scope before sending a PR. All commands below are copy-paste reproducible on master. ### 1. `commons-io` looks used in shenyu-admin ``` $ git grep -n "org.apache.commons.io" -- shenyu-admin shenyu-admin/src/main/java/org/apache/shenyu/admin/mode/cluster/filter/ClusterForwardFilter.java:25:import org.apache.commons.io.IOUtils; shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/SandboxServiceImpl.java:23:import org.apache.commons.io.IOUtils; ``` Call site 1 — [ClusterForwardFilter.java#L143-L151](https://github.com/apache/shenyu/blob/master/shenyu-admin/src/main/java/org/apache/shenyu/admin/mode/cluster/filter/ClusterForwardFilter.java#L143-L151), `IOUtils.copy` at [L148](https://github.com/apache/shenyu/blob/master/shenyu-admin/src/main/java/org/apache/shenyu/admin/mode/cluster/filter/ClusterForwardFilter.java#L148): ```java copyHeaders(responseEntity.getHeaders(), response); // fix cors error response.addHeader("Access-Control-Allow-Origin", "*"); response.addHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE"); // write response back IOUtils.copy(new ByteArrayInputStream(Objects.requireNonNull(responseEntity.getBody())), response.getOutputStream()); response.getOutputStream().flush(); ``` Call site 2 — [SandboxServiceImpl.java#L118-L126](https://github.com/apache/shenyu/blob/master/shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/SandboxServiceImpl.java#L118-L126), `IOUtils.copy` at [L123](https://github.com/apache/shenyu/blob/master/shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/SandboxServiceImpl.java#L123): ```java response.addHeader("sandbox-beforesign", UriUtils.encode(signContentWithoutSecret, StandardCharsets.UTF_8)); response.addHeader("sandbox-sign", UriUtils.encode(sign, StandardCharsets.UTF_8)); } IOUtils.copy(body.byteStream(), response.getOutputStream()); response.flushBuffer(); ``` If these two usages are real, removing the `commons-io` dependency as-is would break compilation. Would it be acceptable to first replace both `IOUtils.copy` calls with `org.springframework.util.StreamUtils.copy` (already on the classpath via spring-core, and closer to the coding guideline of preferring project/framework utils), and then remove the dependency block from `shenyu-admin/pom.xml` ([L295-L299](https://github.com/apache/shenyu/blob/master/shenyu-admin/pom.xml#L295-L299))? ### 2. `commons-collections4` is used by 65 files, not 2 ``` $ git grep -l "org.apache.commons.collections4" -- shenyu-admin | wc -l 65 ``` The two files named in the issue are indeed among them, but so are core listeners/aspect/controller code, e.g. [AbstractDataChangedListener.java#L30](https://github.com/apache/shenyu/blob/master/shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/AbstractDataChangedListener.java#L30), [HttpLongPollingDataChangedListener.java#L21](https://github.com/apache/shenyu/blob/master/shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/http/HttpLongPollingDataChangedListener.java#L21), [WebsocketCollector.java#L22](https://github.com/apache/shenyu/blob/master/shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/websocket/WebsocketCollector.java#L22): <details> <summary>first 15 of the 65 importing files</summary> ``` shenyu-admin/src/main/java/org/apache/shenyu/admin/aspect/DataPermissionAspect.java shenyu-admin/src/main/java/org/apache/shenyu/admin/config/properties/DashboardProperties.java shenyu-admin/src/main/java/org/apache/shenyu/admin/controller/AiProxyApiKeyController.java shenyu-admin/src/main/java/org/apache/shenyu/admin/controller/ApiDocController.java <- named in the issue shenyu-admin/src/main/java/org/apache/shenyu/admin/controller/DashboardUserController.java shenyu-admin/src/main/java/org/apache/shenyu/admin/controller/ResourceController.java shenyu-admin/src/main/java/org/apache/shenyu/admin/discovery/DiscoveryDataChangedEventSyncListener.java shenyu-admin/src/main/java/org/apache/shenyu/admin/disruptor/executor/RegisterServerConsumerExecutor.java shenyu-admin/src/main/java/org/apache/shenyu/admin/disruptor/subscriber/URIRegisterExecutorSubscriber.java shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/AbstractDataChangedListener.java shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/AiProxySelectorResolverInvalidator.java shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/http/HttpLongPollingDataChangedListener.java shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/websocket/WebsocketCollector.java shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/websocket/WebsocketDataChangedListener.java shenyu-admin/src/main/java/org/apache/shenyu/admin/model/vo/SelectorConditionVO.java ``` </details> So removing `commons-collections4` is not feasible now. And even in the future it wouldn't unlock via the apidoc cleanup: as of writing, #7592 and #7593 are still **open** — the removal hasn't started (per #7592 it also needs a coordinated removal of the dashboard-side UI, shenyu-dashboard#719, in the same release train) — and even once that lands, 60+ of the 65 usages remain. I'd suggest keeping `commons-collections4` and narrowing this issue's scope to `commons-io` only. ### 3. `<commons-io.version>` is not an orphaned property It's referenced by two other modules, so the property must stay; only shenyu-admin's dependency block goes away: ``` $ grep -rn "commons-io.version" --include="pom.xml" . pom.xml:146: <commons-io.version>2.11.0</commons-io.version> shenyu-admin/pom.xml:298: <version>${commons-io.version}</version> shenyu-plugin/shenyu-plugin-security/shenyu-plugin-cryptor/pom.xml:32: <version>${commons-io.version}</version> shenyu-protocol/shenyu-protocol-tcp/pom.xml:53: <version>${commons-io.version}</version> ``` e.g. [shenyu-plugin-cryptor/pom.xml#L29-L33](https://github.com/apache/shenyu/blob/master/shenyu-plugin/shenyu-plugin-security/shenyu-plugin-cryptor/pom.xml#L29-L33) and [shenyu-protocol-tcp/pom.xml#L50-L54](https://github.com/apache/shenyu/blob/master/shenyu-protocol/shenyu-protocol-tcp/pom.xml#L50-L54). ### Revised plan If this scope sounds right, I'll send a PR that: (1) replaces the two `IOUtils.copy` call sites with `StreamUtils.copy` (identical semantics: buffered copy, stream left open for the caller), (2) removes the `commons-io` dependency from `shenyu-admin/pom.xml` only, (3) keeps `${commons-io.version}` for cryptor/protocol-tcp. Verified with `mvn -pl shenyu-admin dependency:analyze` and a green build. Happy to adjust if you'd rather keep `commons-io` and leave the call sites untouched. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
