Copilot commented on code in PR #6096: URL: https://github.com/apache/shenyu/pull/6096#discussion_r2276312288
########## shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/RegistryServiceImpl.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.service.impl; + +import org.apache.commons.lang3.StringUtils; +import org.apache.shenyu.admin.exception.ShenyuAdminException; +import org.apache.shenyu.admin.mapper.RegistryMapper; +import org.apache.shenyu.admin.model.dto.RegistryDTO; +import org.apache.shenyu.admin.model.entity.RegistryDO; +import org.apache.shenyu.admin.model.page.CommonPager; +import org.apache.shenyu.admin.model.page.PageResultUtils; +import org.apache.shenyu.admin.model.query.RegistryQuery; +import org.apache.shenyu.admin.model.vo.RegistryVO; +import org.apache.shenyu.admin.service.RegistryService; +import org.apache.shenyu.admin.transfer.RegistryTransfer; +import org.apache.shenyu.admin.utils.ShenyuResultMessage; +import org.apache.shenyu.common.utils.UUIDUtils; +import org.springframework.stereotype.Service; + +import java.sql.Timestamp; +import java.util.List; +import java.util.Objects; +import java.util.stream.Collectors; + +@Service +public class RegistryServiceImpl implements RegistryService { + + private final RegistryMapper registryMapper; + + public RegistryServiceImpl(final RegistryMapper registryMapper) { + this.registryMapper = registryMapper; + } + + @Override + public RegistryVO createOrUpdate(final RegistryDTO registryDTO) { + return StringUtils.isBlank(registryDTO.getId()) + ? this.create(registryDTO) : this.update(registryDTO); + } + + @Override + public CommonPager<RegistryVO> listByPage(final RegistryQuery registryQuery) { + return PageResultUtils.result(registryQuery.getPageParameter(), () -> registryMapper.countByQuery(registryQuery), () -> registryMapper.selectByQuery(registryQuery) + .stream() + .map(RegistryTransfer.INSTANCE::mapToVo) + .collect(Collectors.toList())); + } + + @Override + public String delete(final List<String> ids) { + // todo check selector handle + registryMapper.deleteByIds(ids); + return ShenyuResultMessage.DELETE_SUCCESS; + } + + @Override + public RegistryVO findById(final String id) { + return RegistryTransfer.INSTANCE.mapToVo(registryMapper.selectById(id)); + } + + @Override + public RegistryVO findByRegistryId(final String registryId) { + return RegistryTransfer.INSTANCE.mapToVo(registryMapper.selectByRegistryId(registryId)); + } + + @Override + public List<RegistryVO> listAll() { + List<RegistryDO> registryDOS = registryMapper.selectAll(); + return registryDOS.stream().map(RegistryTransfer.INSTANCE::mapToVo).collect(Collectors.toList()); + } + + + private RegistryVO create(final RegistryDTO registryDTO) { + RegistryDO existRegistryDO = registryMapper.selectByRegistryId(registryDTO.getRegistryId()); + if (Objects.nonNull(existRegistryDO)) { + throw new ShenyuAdminException("registry_id is already exist"); + } + + Timestamp currentTime = new Timestamp(System.currentTimeMillis()); + String id = UUIDUtils.getInstance().generateShortUuid(); + RegistryDO registryDO = RegistryDO.builder() + .id(id) + .registryId(registryDTO.getRegistryId()) + .protocol(registryDTO.getProtocol()) + .address(registryDTO.getAddress()) + .namespace(registryDTO.getNamespace()) + .username(registryDTO.getUsername()) + .password(registryDTO.getPassword()) + .group(registryDTO.getGroup()) + .dateCreated(currentTime) + .dateUpdated(currentTime) + .build(); + registryMapper.insert(registryDO); + + return RegistryTransfer.INSTANCE.mapToVo(registryDO); + } + + private RegistryVO update(final RegistryDTO registryDTO) { Review Comment: The update method does not check if a registry with the new registryId already exists, which could cause duplicate registry_id violations if the registryId is being changed during update. ########## db/upgrade/2.7.0-upgrade-2.7.1-oracle.sql: ########## @@ -523,4 +523,47 @@ INSERT INTO permission (id, role_id, resource_id, date_created, date_updated) VA INSERT INTO permission (id, role_id, resource_id, date_created, date_updated) VALUES ('1697146861569542757', '1346358560427216896', '1844026199075534866', sysdate, sysdate); INSERT INTO permission (id, role_id, resource_id, date_created, date_updated) VALUES ('1697146861569542758', '1346358560427216896', '1844026199075534867', sysdate, sysdate); INSERT INTO permission (id, role_id, resource_id, date_created, date_updated) VALUES ('1697146861569542759', '1346358560427216896', '1844026199075534868', sysdate, sysdate); -INSERT INTO permission (id, role_id, resource_id, date_created, date_updated) VALUES ('1697146861569542760', '1346358560427216896', '1844026199075534869', sysdate, sysdate); \ No newline at end of file +INSERT INTO permission (id, role_id, resource_id, date_created, date_updated) VALUES ('1697146861569542760', '1346358560427216896', '1844026199075534869', sysdate, sysdate); + +CREATE TABLE "public"."registry_config" ( Review Comment: Oracle SQL uses "public" schema prefix which is not valid for Oracle. Oracle should use the actual schema name or omit the schema prefix. ```suggestion CREATE TABLE registry_config ( ``` ########## shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/RegistryServiceImpl.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.service.impl; + +import org.apache.commons.lang3.StringUtils; +import org.apache.shenyu.admin.exception.ShenyuAdminException; +import org.apache.shenyu.admin.mapper.RegistryMapper; +import org.apache.shenyu.admin.model.dto.RegistryDTO; +import org.apache.shenyu.admin.model.entity.RegistryDO; +import org.apache.shenyu.admin.model.page.CommonPager; +import org.apache.shenyu.admin.model.page.PageResultUtils; +import org.apache.shenyu.admin.model.query.RegistryQuery; +import org.apache.shenyu.admin.model.vo.RegistryVO; +import org.apache.shenyu.admin.service.RegistryService; +import org.apache.shenyu.admin.transfer.RegistryTransfer; +import org.apache.shenyu.admin.utils.ShenyuResultMessage; +import org.apache.shenyu.common.utils.UUIDUtils; +import org.springframework.stereotype.Service; + +import java.sql.Timestamp; +import java.util.List; +import java.util.Objects; +import java.util.stream.Collectors; + +@Service +public class RegistryServiceImpl implements RegistryService { + + private final RegistryMapper registryMapper; + + public RegistryServiceImpl(final RegistryMapper registryMapper) { + this.registryMapper = registryMapper; + } + + @Override + public RegistryVO createOrUpdate(final RegistryDTO registryDTO) { + return StringUtils.isBlank(registryDTO.getId()) + ? this.create(registryDTO) : this.update(registryDTO); + } + + @Override + public CommonPager<RegistryVO> listByPage(final RegistryQuery registryQuery) { + return PageResultUtils.result(registryQuery.getPageParameter(), () -> registryMapper.countByQuery(registryQuery), () -> registryMapper.selectByQuery(registryQuery) + .stream() + .map(RegistryTransfer.INSTANCE::mapToVo) + .collect(Collectors.toList())); + } + + @Override + public String delete(final List<String> ids) { + // todo check selector handle Review Comment: This TODO comment should be resolved or provide more specific details about what selector handle checking is needed before deletion. ```suggestion // Check if any selectors reference the registry IDs to be deleted int selectorCount = selectorMapper.countByRegistryIds(ids); if (selectorCount > 0) { throw new ShenyuAdminException("Cannot delete registry: one or more selectors reference the registry to be deleted."); } ``` ########## shenyu-admin/src/main/java/org/apache/shenyu/admin/model/entity/RegistryDO.java: ########## @@ -0,0 +1,361 @@ +/* + * 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.entity; + +import java.sql.Timestamp; + +/** + * Registry do. + */ +public class RegistryDO extends BaseDO { + + /** + * the model registryId. + */ + private String registryId; + + /** + * the model protocol. + */ + private String protocol; + + /** + * the model address. + */ + private String address; + + /** + * the model username. + */ + private String username; + + /** + * the model password. + */ + private String password; + + /** + * the model namespace. + */ + private String namespace; + + /** + * the model group. + */ + private String group; + + /** + * Gets the value of registryId. + * + * @return the value of registryId + */ + public String getRegistryId() { + return registryId; + } + + /** + * Sets the registryId. + * + * @param registryId registryId + */ + public void setRegistryId(final String registryId) { + this.registryId = registryId; + } + + /** + * Gets the value of protocol. + * + * @return the value of protocol + */ + public String getProtocol() { + return protocol; + } + + /** + * Sets the protocol. + * + * @param protocol protocol + */ + public void setProtocol(final String protocol) { + this.protocol = protocol; + } + + /** + * Gets the value of address. + * + * @return the value of address + */ + public String getAddress() { + return address; + } + + /** + * Sets the address. + * + * @param address address + */ + public void setAddress(final String address) { + this.address = address; + } + + /** + * Gets the value of username. + * + * @return the value of username + */ + public String getUsername() { + return username; + } + + /** + * Sets the username. + * + * @param username username + */ + public void setUsername(final String username) { + this.username = username; + } + + /** + * Gets the value of password. + * + * @return the value of password + */ + public String getPassword() { + return password; + } + + /** + * Sets the password. + * + * @param password password + */ + public void setPassword(final String password) { + this.password = password; + } + + /** + * Gets the value of namespace. + * + * @return the value of namespace + */ + public String getNamespace() { + return namespace; + } + + /** + * Sets the namespace. + * + * @param namespace namespace + */ + public void setNamespace(final String namespace) { + this.namespace = namespace; + } + + /** + * Gets the value of group. + * + * @return the value of group + */ + public String getGroup() { + return group; + } + + /** + * Sets the group. + * + * @param group group + */ + public void setGroup(final String group) { + this.group = group; + } + + /** + * builder. + * + * @return RegistryDOBuilder + */ + public static RegistryDOBuilder builder() { + return new RegistryDO.RegistryDOBuilder(); + } + + public static final class RegistryDOBuilder { + + private String id; + + private Timestamp dateCreated; + + private Timestamp dateUpdated; + + private String registryId; + + private String protocol; + + private String address; + + private String username; + + private String password; + + private String namespace; + + private String group; + + private RegistryDOBuilder() { + } + + /** + * builder. + * + * @return RegistryDO.RegistryDOBuilder + */ + public static RegistryDOBuilder builder() { + return new RegistryDOBuilder(); + } + Review Comment: The static builder() method in the inner builder class duplicates the builder() method in the parent class (line 193), creating redundant code that could confuse developers. ```suggestion ``` -- 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: notifications-unsubscr...@shenyu.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
