Copilot commented on code in PR #7851: URL: https://github.com/apache/incubator-seata/pull/7851#discussion_r2612820410
########## console/src/main/resources/static/console-fe/src/utils/requestV2.ts: ########## @@ -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. + */ +import axios, { AxiosInstance, AxiosResponse } from 'axios'; +import { Message } from '@alifd/next'; +import { get } from 'lodash'; +import { AUTHORIZATION_HEADER } from '@/contants'; +import { getCurrentLocaleObj } from '@/reducers/locale'; + +const API_GENERAL_ERROR_MESSAGE: string = 'Request error, please try again later!'; + +const requestV2 = () => { + const instance: AxiosInstance = axios.create({ + baseURL: 'api/v2', + method: 'get', + }); + + instance.interceptors.request.use((config: any) => { + let authHeader: string | null = localStorage.getItem(AUTHORIZATION_HEADER); + // add jwt header + if (config.headers) { + config.headers[AUTHORIZATION_HEADER] = authHeader; + } + return config; + }) + + instance.interceptors.response.use( + (response: AxiosResponse): Promise<any> => { + const code = get(response, 'data.code'); + if (response.status === 200 && code === '200') { + return Promise.resolve(get(response, 'data')); + } else { + const currentLocale = getCurrentLocaleObj(); + const errorText = + (currentLocale.codeMessage as any)[code] || + get(response, 'data.message') || + get(response, 'data.errorMsg') || + response.statusText; + Message.error(errorText || `Request error ${code}: ${get(response, 'config.url', '')}`); + return Promise.reject(response); + } + }, + error => { + if (error.response) { + const { status } = error.response; + if (status === 403 || status === 401) { + (window as any).globalHistory.replace('/login'); + return; Review Comment: Missing error handler return statement. When status is 403 or 401, the function redirects to login but doesn't return a rejected promise, which could lead to unhandled promise continuation. Add `return Promise.reject(error);` after the return statement on line 61 for consistency. ```suggestion return Promise.reject(error); ``` ########## console/src/main/resources/static/console-fe/src/locales/zh-cn.ts: ########## @@ -88,6 +95,24 @@ const zhCn: ILocale = { operateTitle: '操作', deleteGlobalLockTitle: '删除全局锁', }, + codeMessage: { + 200: '服务器成功返回请求的数据。', + 201: '新建或修改数据成功。', + 202: '一个请求已经进入后台排队（异步任务）。', + 204: '删除数据成功。', + 400: '发出的请求有错误，服务器没有进行新建或修改数据的操作。', + 401: '用户没有权限（令牌、用户名、密码错误）。', + 403: '用户得到授权，但是访问是被禁止的。', + 404: '发出的请求针对的是不存在的记录，服务器没有进行操作。', + 406: '请求的格式不可得。', + 410: '请求的资源被永久删除，且不会再得到的。', + 422: '当创建一个对象时，发生一个验证错误。', + 500: '服务器发生错误，请检查服务器。', + 502: '网关错误。', + 503: '服务不可用，服务器暂时过载或维护。', + 504: '网关超时。', + '-1000': '项目名称已存在，请使用其他名称', Review Comment: Inconsistent punctuation in Chinese text. Line 114 uses a Chinese comma (，) while line 82 uses a period. For consistency in the locale file, either use Chinese punctuation throughout (full-width comma "，" and period "。") or use English punctuation consistently. ```suggestion '-1000': '项目名称已存在，请使用其他名称。', ``` ########## namingserver/src/main/java/org/apache/seata/namingserver/manager/NamingManager.java: ########## @@ -477,24 +493,89 @@ public Result<String> changeGroup(String namespace, String vGroup, String cluste } public SingleResult<Map<String, NamespaceVO>> namespace() { - // namespace->cluster->vgroups + // Collect data using helper method + NamespaceData data = collectNamespaceData(); + Map<String, NamespaceVO> namespaceVOs = new HashMap<>(); - Map<String /* VGroup */, ConcurrentMap<String /* namespace */, NamespaceBO>> currentVGourpMap = - new HashMap<>(vGroupMap.asMap()); - if (currentVGourpMap.isEmpty()) { - namespaceClusterDataMap.forEach((namespace, clusterDataMap) -> { + if (data.vgroupsMap.isEmpty()) { + data.clustersMap.forEach((namespace, clusters) -> { NamespaceVO namespaceVO = new NamespaceVO(); - namespaceVO.setClusters(new ArrayList<>(clusterDataMap.keySet())); + namespaceVO.setClusters(new ArrayList<>(clusters)); namespaceVOs.put(namespace, namespaceVO); }); - return SingleResult.success(namespaceVOs); + } else { + data.vgroupsMap.forEach((namespace, vgroups) -> { + NamespaceVO namespaceVO = namespaceVOs.computeIfAbsent(namespace, k -> new NamespaceVO()); + namespaceVO.setClusters(new ArrayList<>(data.clustersMap.get(namespace))); + namespaceVO.setVgroups(new ArrayList<>(vgroups)); + }); } - currentVGourpMap.forEach((vGroup, namespaceMap) -> namespaceMap.forEach( - (namespace, namespaceBO) -> namespaceBO.getClusterMap().forEach((clusterName, clusterBO) -> { - NamespaceVO namespaceVO = namespaceVOs.computeIfAbsent(namespace, value -> new NamespaceVO()); - namespaceVO.getClusters().add(clusterName); - namespaceVO.getVgroups().add(vGroup); - }))); + + return SingleResult.success(namespaceVOs); + } + + public SingleResult<Map<String, org.apache.seata.namingserver.entity.vo.v2.NamespaceVO>> namespaceV2() { + // Collect data using helper method + NamespaceData data = collectNamespaceData(); + + // Build NamespaceVOv2 + Map<String, org.apache.seata.namingserver.entity.vo.v2.NamespaceVO> namespaceVOs = new HashMap<>(); + data.clustersMap.forEach((namespace, clusters) -> { + org.apache.seata.namingserver.entity.vo.v2.NamespaceVO namespaceVO = + new org.apache.seata.namingserver.entity.vo.v2.NamespaceVO(); + Map<String, List<String>> clusterVgList = new HashMap<>(); + Map<String, Set<String>> clusterVgSet = data.clusterVgroupsMap.get(namespace); + clusters.forEach(cluster -> { + Set<String> vgSet = clusterVgSet.get(cluster); + clusterVgList.put(cluster, vgSet != null ? new ArrayList<>(vgSet) : new ArrayList<>()); + }); Review Comment: Potential NullPointerException: The code retrieves `clusterVgSet` from the map but doesn't check if it's null before using it in the forEach loop. If `data.clusterVgroupsMap.get(namespace)` returns null, calling `clusterVgSet.get(cluster)` on line 529 will throw a NullPointerException. Add a null check for `clusterVgSet` before the forEach loop. ########## console/src/main/resources/static/console-fe/src/utils/requestV2.ts: ########## @@ -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. + */ +import axios, { AxiosInstance, AxiosResponse } from 'axios'; +import { Message } from '@alifd/next'; +import { get } from 'lodash'; +import { AUTHORIZATION_HEADER } from '@/contants'; +import { getCurrentLocaleObj } from '@/reducers/locale'; + +const API_GENERAL_ERROR_MESSAGE: string = 'Request error, please try again later!'; Review Comment: The API_GENERAL_ERROR_MESSAGE constant is declared but never used in this file. The same hardcoded string 'Request error, please try again later!' is used directly on line 63. Either use the constant consistently or remove it to avoid dead code. ########## namingserver/src/main/java/org/apache/seata/namingserver/controller/NamingControllerV2.java: ########## @@ -0,0 +1,52 @@ +/* + * 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.seata.namingserver.controller; + +import org.apache.seata.common.result.SingleResult; +import org.apache.seata.namingserver.entity.vo.v2.NamespaceVO; +import org.apache.seata.namingserver.manager.NamingManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RestController; + +import javax.annotation.Resource; +import java.util.Map; + +@RestController +@RequestMapping(value = {"/naming/v2", "/api/v2/naming"}) +public class NamingControllerV2 { + + private static final Logger LOGGER = LoggerFactory.getLogger(NamingControllerV2.class); Review Comment: The LOGGER field is declared but never used in this controller. Either use it for logging important operations (like successful namespace retrievals or errors), or remove the unused declaration to avoid unnecessary code clutter. ########## console/src/main/resources/static/console-fe/src/pages/TransactionInfo/TransactionInfo.tsx: ########## @@ -839,6 +865,41 @@ class TransactionInfo extends React.Component<GlobalProps, TransactionInfoState> }); } + showCreateVGroupDialog = () => { + this.setState({ + createVGroupDialogVisible: true, + vGroupName: '', + }); + } + + closeCreateVGroupDialog = () => { + this.setState({ + createVGroupDialogVisible: false, + vGroupName: '', + }); + } + + handleCreateVGroup = () => { + const { locale = {} } = this.props; + const { createVGroupErrorMessage, createVGroupSuccessMessage, createVGroupFailMessage } = locale; + const { namespace, cluster } = this.state.globalSessionParam; + const { vGroupName } = this.state; + if (!namespace || !cluster || !vGroupName.trim()) { + Message.error(createVGroupErrorMessage); + return; + } + addGroup(namespace, cluster, vGroupName.trim()).then(() => { + Message.success(createVGroupSuccessMessage); + this.closeCreateVGroupDialog(); + // Delay 5 seconds before reloading namespaces to get the latest vgroup list + setTimeout(() => { + this.loadNamespaces(); + }, 5000); + }).catch((error) => { + Message.error(lodashGet(error, 'data.message') || createVGroupFailMessage); Review Comment: Error message is not internationalized. The error message `createVGroupFailMessage` from locale is used, but if the backend returns a specific error (via `lodashGet(error, 'data.message')`), that message is not translated and will be in the backend's default language. Consider adding proper error message handling or ensure backend error messages are also internationalized. ```suggestion // Optionally log the backend error for debugging if (process.env.NODE_ENV !== 'production') { // eslint-disable-next-line no-console console.error('Create VGroup failed:', lodashGet(error, 'data.message') || error); } Message.error(createVGroupFailMessage); ``` ########## namingserver/src/main/java/org/apache/seata/namingserver/entity/vo/v2/NamespaceVO.java: ########## @@ -0,0 +1,51 @@ +/* + * 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.seata.namingserver.entity.vo.v2; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * Value Object representing namespace information for the v2 API. + * <p> + * This class provides a mapping between cluster names and their associated vgroup lists. + * It is used in the v2 version of the API and may differ from the original {@code NamespaceVO} + * (in the v1 or root package) in its structure or the semantics of its fields. + * <p> + * <b>Differences from the original NamespaceVO:</b> + * <ul> + * <li>Located in the {@code org.apache.seata.namingserver.entity.vo.v2} package, indicating it is for the v2 API.</li> + * <li>Contains a {@code clusterVgroups} map, which maps cluster names to lists of vgroup names.</li> + * <li>May have a different structure or additional fields compared to the original version.</li> + * </ul> + * <p> + * API consumers should refer to this class when interacting with the v2 endpoints to understand + * the data structure being returned. + */ +public class NamespaceVO { + + Map<String, List<String>> clusterVgroups = new HashMap<>(); Review Comment: The field lacks proper access modifier and should be private for encapsulation. Java best practices recommend explicitly declaring field visibility. Change the field declaration to `private Map<String, List<String>> clusterVgroups = new HashMap<>();` ```suggestion private Map<String, List<String>> clusterVgroups = new HashMap<>(); ``` ########## console/src/main/resources/static/console-fe/src/pages/TransactionInfo/TransactionInfo.tsx: ########## @@ -839,6 +865,41 @@ class TransactionInfo extends React.Component<GlobalProps, TransactionInfoState> }); } + showCreateVGroupDialog = () => { + this.setState({ + createVGroupDialogVisible: true, + vGroupName: '', + }); + } + + closeCreateVGroupDialog = () => { + this.setState({ + createVGroupDialogVisible: false, + vGroupName: '', + }); + } + + handleCreateVGroup = () => { + const { locale = {} } = this.props; + const { createVGroupErrorMessage, createVGroupSuccessMessage, createVGroupFailMessage } = locale; + const { namespace, cluster } = this.state.globalSessionParam; + const { vGroupName } = this.state; + if (!namespace || !cluster || !vGroupName.trim()) { + Message.error(createVGroupErrorMessage); + return; + } + addGroup(namespace, cluster, vGroupName.trim()).then(() => { + Message.success(createVGroupSuccessMessage); + this.closeCreateVGroupDialog(); + // Delay 5 seconds before reloading namespaces to get the latest vgroup list + setTimeout(() => { + this.loadNamespaces(); + }, 5000); Review Comment: The setTimeout delay of 5 seconds is hardcoded as a magic number without explanation. Consider extracting this to a named constant like `VGROUP_REFRESH_DELAY_MS = 5000` to improve maintainability and make the purpose clearer. Additionally, the 5-second delay seems arbitrary - consider if there's a better approach such as polling the API until the vgroup appears, or reducing the delay if the backend processing is typically faster. ########## console/src/main/resources/static/console-fe/src/service/transactionInfo.ts: ########## @@ -239,3 +247,16 @@ export async function startBranchData(params: BranchSessionParam): Promise<any> }); return result; } + +export async function addGroup(namespace: string, clusterName: string, vGroup: string, unitName?: string): Promise<any> { + let result = await request('/naming/addGroup', { + method: 'POST', + params: { + namespace, + clusterName, + vGroup, + unitName, + }, + }); + return result; +} Review Comment: The API design is inconsistent. The existing `/naming/addGroup` endpoint uses POST method (as shown in NamingController.java), but this v2 API calls it via the same endpoint path without creating a corresponding v2 endpoint. Consider either creating a `/naming/v2/group` or `/api/v2/naming/group` POST endpoint in NamingControllerV2 for consistency with the v2 API design pattern, or document why the v1 endpoint is being reused. ########## console/src/main/resources/static/console-fe/src/service/transactionInfo.ts: ########## @@ -49,6 +50,13 @@ export async function fetchNamespace():Promise<any> { return result.data; } +export async function fetchNamespaceV2():Promise<any> { + const result = await requestV2.get('/naming/namespace', { + method: 'get', + }); Review Comment: The 'method' parameter is redundant. The axios instance's .get() method already specifies the HTTP method as GET, so passing `method: 'get'` in the config object is unnecessary and can be removed for consistency. ########## console/src/main/resources/static/console-fe/src/utils/requestV2.ts: ########## @@ -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. + */ +import axios, { AxiosInstance, AxiosResponse } from 'axios'; +import { Message } from '@alifd/next'; +import { get } from 'lodash'; +import { AUTHORIZATION_HEADER } from '@/contants'; +import { getCurrentLocaleObj } from '@/reducers/locale'; + +const API_GENERAL_ERROR_MESSAGE: string = 'Request error, please try again later!'; + +const requestV2 = () => { + const instance: AxiosInstance = axios.create({ + baseURL: 'api/v2', + method: 'get', + }); + + instance.interceptors.request.use((config: any) => { + let authHeader: string | null = localStorage.getItem(AUTHORIZATION_HEADER); + // add jwt header + if (config.headers) { + config.headers[AUTHORIZATION_HEADER] = authHeader; + } + return config; + }) + + instance.interceptors.response.use( + (response: AxiosResponse): Promise<any> => { + const code = get(response, 'data.code'); + if (response.status === 200 && code === '200') { + return Promise.resolve(get(response, 'data')); + } else { + const currentLocale = getCurrentLocaleObj(); + const errorText = + (currentLocale.codeMessage as any)[code] || + get(response, 'data.message') || + get(response, 'data.errorMsg') || + response.statusText; + Message.error(errorText || `Request error ${code}: ${get(response, 'config.url', '')}`); + return Promise.reject(response); + } + }, + error => { + if (error.response) { + const { status } = error.response; + if (status === 403 || status === 401) { + (window as any).globalHistory.replace('/login'); + return; + } + Message.error(`HTTP ERROR: ${status}`); + } else { + Message.error(API_GENERAL_ERROR_MESSAGE); + } + return Promise.reject(error); + } + ); + + return instance; +}; + +export default requestV2(); Review Comment: Code duplication: The requestV2.ts file is an almost exact duplicate of request.ts with only the baseURL changed from 'api/v1' to 'api/v2'. This violates the DRY principle. Consider refactoring to create a shared request factory function that accepts a baseURL parameter, or use a single instance with dynamic path prefixing. ```suggestion import createRequestInstance from './request'; export default createRequestInstance('api/v2'); ``` ########## console/src/main/resources/static/console-fe/src/service/globalLockInfo.ts: ########## @@ -33,7 +34,7 @@ export type GlobalLockParam = { }; export async function fetchNamespace():Promise<any> { - const result = await request.get('/naming/namespace', { + const result = await requestV2.get('/namespace', { method: 'get', }); Review Comment: The 'method' parameter is redundant. The axios instance's .get() method already specifies the HTTP method as GET, so passing `method: 'get'` in the config object is unnecessary and can be removed. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
