Copilot commented on code in PR #6315: URL: https://github.com/apache/shenyu/pull/6315#discussion_r3021712923
########## shenyu-common/src/main/java/org/apache/shenyu/common/enums/UpstreamManualStatusEnum.java: ########## @@ -0,0 +1,60 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.shenyu.common.enums; + +import java.util.Arrays; + +/** + * Manual upstream status. + */ +public enum UpstreamManualStatusEnum { + + /** + * No manual override. + */ + NONE, + + /** + * Force upstream offline manually. + */ + FORCE_OFFLINE; + + /** + * Normalize status value. + * + * @param manualStatus status value + * @return normalized enum name + */ + public static String normalize(final String manualStatus) { + return Arrays.stream(values()) + .filter(value -> value.name().equalsIgnoreCase(manualStatus)) + .findFirst() + .orElse(NONE) + .name(); + } Review Comment: `normalize` will throw a NullPointerException when `manualStatus` is null (because `equalsIgnoreCase` is called on a null argument). This is reachable via multiple call sites in this PR (e.g., `new Upstream.Builder()` where `builder.manualStatus` is unset). Make `normalize` null/blank-safe (e.g., treat null/blank as `NONE`) to prevent construction-time NPEs and make `isForceOffline` safe. ########## shenyu-admin/src/main/resources/mappers/discovery-upstream-sqlmap.xml: ########## @@ -176,17 +185,19 @@ du.date_created, du.date_updated, du.discovery_handler_id, + du.namespace_id, du.protocol, du.upstream_url, du.upstream_status, + du.manual_status, du.weight, du.props FROM discovery_upstream du Review Comment: `Base_Column_List` appears to include a full `FROM discovery_upstream du` clause, but `selectByDiscoveryHandlerIdAndUrl` appends `from discovery_upstream` again. This will generate invalid SQL (duplicate FROM / unexpected alias usage). Fix by making `Base_Column_List` columns-only (no FROM) and alias consistently, or remove the extra `from discovery_upstream` and reuse the same alias (`du`) in the WHERE clause. ########## shenyu-admin/src/main/resources/mappers/discovery-upstream-sqlmap.xml: ########## @@ -239,4 +252,10 @@ WHERE discovery_handler_id = #{discoveryHandlerId} and upstream_url = #{upstreamUrl} </update> + <update id="updateManualStatusByUrl"> + UPDATE discovery_upstream + SET manual_status = #{manualStatus} Review Comment: Manual status updates currently don’t touch `date_updated`. If `date_updated` is used for auditing, sync decisions, or operational visibility elsewhere in the system, this will make manual changes harder to trace. Consider updating `date_updated` in the same statement (mandatory if other update paths always maintain `date_updated`). ```suggestion SET manual_status = #{manualStatus}, date_updated = CURRENT_TIMESTAMP ``` ########## shenyu-admin/src/main/resources/mappers/discovery-upstream-sqlmap.xml: ########## @@ -176,17 +185,19 @@ du.date_created, du.date_updated, du.discovery_handler_id, + du.namespace_id, du.protocol, du.upstream_url, du.upstream_status, + du.manual_status, du.weight, du.props FROM discovery_upstream du INNER JOIN discovery_rel dr ON du.discovery_handler_id = dr.discovery_handler_id </sql> <select id="selectByDiscoveryHandlerIdAndUrl" - resultType="org.apache.shenyu.admin.model.entity.DiscoveryUpstreamDO"> + resultMap="BaseResultMap"> SELECT <include refid="Base_Column_List"/> from discovery_upstream where discovery_handler_id = #{discoveryHandlerId} and upstream_url = #{upstreamUrl} for update </select> Review Comment: `Base_Column_List` appears to include a full `FROM discovery_upstream du` clause, but `selectByDiscoveryHandlerIdAndUrl` appends `from discovery_upstream` again. This will generate invalid SQL (duplicate FROM / unexpected alias usage). Fix by making `Base_Column_List` columns-only (no FROM) and alias consistently, or remove the extra `from discovery_upstream` and reuse the same alias (`du`) in the WHERE clause. ########## shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/factory/LoadBalancerFactory.java: ########## @@ -41,7 +42,17 @@ private LoadBalancerFactory() { * @return the upstream */ public static Upstream selector(final List<Upstream> upstreamList, final String algorithm, final LoadBalanceData data) { + if (Objects.isNull(upstreamList)) { + return null; + } + List<Upstream> availableUpstreamList = upstreamList.stream() + .filter(Objects::nonNull) + .filter(upstream -> !upstream.isManualOffline()) + .toList(); + if (availableUpstreamList.isEmpty()) { + return null; + } LoadBalancer loadBalance = ExtensionLoader.getExtensionLoader(LoadBalancer.class).getJoin(algorithm); - return loadBalance.select(upstreamList, data); + return loadBalance.select(availableUpstreamList, data); } Review Comment: Using `Stream#toList()` requires newer JDKs (Java 16+). If this project/module targets Java 8/11 (common for libraries), this will not compile. Prefer `collect(Collectors.toList())` (or a project-standard collector) for broader compatibility. ########## shenyu-admin/src/main/java/org/apache/shenyu/admin/model/dto/DiscoveryUpstreamDTO.java: ########## @@ -284,4 +291,22 @@ public String getNamespaceId() { public void setNamespaceId(final String namespaceId) { this.namespaceId = namespaceId; } + + /** + * get manualStatus. + * + * @return manualStatus + */ + public String getManualStatus() { + return manualStatus; + } + + /** + * set manualStatus. + * + * @param manualStatus manualStatus + */ + public void setManualStatus(final String manualStatus) { + this.manualStatus = Objects.isNull(manualStatus) ? null : UpstreamManualStatusEnum.normalize(manualStatus); + } Review Comment: This setter treats an empty string as a provided value (it will be normalized, likely to `NONE`), while other parts of the service use `StringUtils.hasLength(...)` to detect 'not provided' and preserve existing DB state. To keep request semantics consistent, consider treating blank/empty as `null` here as well (so callers can omit the field without accidentally overwriting to `NONE`). ########## shenyu-admin/src/main/java/org/apache/shenyu/admin/model/dto/UpstreamManualStatusDTO.java: ########## @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.shenyu.admin.model.dto; + +import jakarta.validation.constraints.NotBlank; + +/** + * Manual upstream status request. + */ +public class UpstreamManualStatusDTO { + + /** + * selector id. + */ + @NotBlank(message = "selectorId can't be null") + private String selectorId; + + /** + * upstream url. + */ + @NotBlank(message = "url can't be null") Review Comment: `@NotBlank` validates both null and blank/whitespace. The message currently says \"can't be null\", which is misleading for empty or whitespace values. Consider changing the messages to \"can't be blank\" (or \"can't be null or blank\") to match the actual constraint behavior. ```suggestion @NotBlank(message = "selectorId can't be blank") private String selectorId; /** * upstream url. */ @NotBlank(message = "url can't be blank") ``` ########## shenyu-admin/src/main/java/org/apache/shenyu/admin/model/dto/UpstreamManualStatusDTO.java: ########## @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.shenyu.admin.model.dto; + +import jakarta.validation.constraints.NotBlank; + +/** + * Manual upstream status request. + */ +public class UpstreamManualStatusDTO { + + /** + * selector id. + */ + @NotBlank(message = "selectorId can't be null") + private String selectorId; + + /** + * upstream url. + */ + @NotBlank(message = "url can't be null") Review Comment: `@NotBlank` validates both null and blank/whitespace. The message currently says \"can't be null\", which is misleading for empty or whitespace values. Consider changing the messages to \"can't be blank\" (or \"can't be null or blank\") to match the actual constraint behavior. ```suggestion @NotBlank(message = "selectorId can't be null or blank") private String selectorId; /** * upstream url. */ @NotBlank(message = "url can't be null or blank") ``` ########## shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/DiscoveryUpstreamServiceImpl.java: ########## @@ -123,6 +132,13 @@ public int updateBatch(final String discoveryHandlerId, final List<DiscoveryUpst public void nativeCreateOrUpdate(final DiscoveryUpstreamDTO discoveryUpstreamDTO) { DiscoveryUpstreamDO discoveryUpstreamDO = DiscoveryUpstreamDO.buildDiscoveryUpstreamDO(discoveryUpstreamDTO); if (StringUtils.hasLength(discoveryUpstreamDTO.getId())) { + if (!StringUtils.hasLength(discoveryUpstreamDTO.getManualStatus())) { + DiscoveryUpstreamDO existingRecord = discoveryUpstreamMapper.selectByDiscoveryHandlerIdAndUrl( + discoveryUpstreamDO.getDiscoveryHandlerId(), discoveryUpstreamDO.getUpstreamUrl()); + if (Objects.nonNull(existingRecord)) { + discoveryUpstreamDO.setManualStatus(existingRecord.getManualStatus()); + } + } discoveryUpstreamMapper.updateSelective(discoveryUpstreamDO); Review Comment: In the update-by-id path (`hasLength(id)`), the fallback lookup for preserving `manualStatus` uses `(discoveryHandlerId, upstreamUrl)` instead of the primary key `id`. If the update changes `upstreamUrl` (or if there are duplicates/edge cases), this can fail to find the existing record and inadvertently reset/alter `manualStatus`. Prefer selecting the existing row by `id` in this branch when preserving fields. -- 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]
