1996fanrui commented on code in PR #1831: URL: https://github.com/apache/incubator-streampark/pull/1831#discussion_r996308692
########## streampark-console/streampark-console-webapp/src/api/variable.js: ########## @@ -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. + */ + +import api from './index' +import http from '@/utils/request' + +export function list (queryParam) { + return http.post(api.Variable.LIST, queryParam) +} + +export function update (queryParam) { + return http.put(api.Variable.UPDATE, queryParam) +} + +export function get (queryParam) { + return http.get(api.Variable.GET, queryParam) Review Comment: This method can be deleted as well. ########## streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/system/service/impl/VariableServiceImpl.java: ########## @@ -0,0 +1,120 @@ +/* + * 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.streampark.console.system.service.impl; + +import org.apache.streampark.console.base.domain.RestRequest; +import org.apache.streampark.console.base.exception.ApiAlertException; +import org.apache.streampark.console.core.entity.Application; +import org.apache.streampark.console.core.service.ApplicationService; +import org.apache.streampark.console.core.service.CommonService; +import org.apache.streampark.console.system.entity.Variable; +import org.apache.streampark.console.system.mapper.VariableMapper; +import org.apache.streampark.console.system.service.VariableService; + +import com.baomidou.mybatisplus.core.conditions.query.LambdaQueryWrapper; +import com.baomidou.mybatisplus.core.metadata.IPage; +import com.baomidou.mybatisplus.extension.plugins.pagination.Page; +import com.baomidou.mybatisplus.extension.service.impl.ServiceImpl; +import lombok.SneakyThrows; +import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +import java.util.List; + +@Slf4j +@Service +@Transactional(propagation = Propagation.SUPPORTS, readOnly = true, rollbackFor = Exception.class) +public class VariableServiceImpl extends ServiceImpl<VariableMapper, Variable> implements VariableService { + + @Autowired + private ApplicationService applicationService; + + @Autowired + private CommonService commonService; + + @Override + @Transactional(rollbackFor = Exception.class) + public void createVariable(Variable variable) throws Exception { + if (isExists(variable)) { Review Comment: And `isExists` is just called inside `VariableServiceImpl`, why do you define it in the VariableService? It is strongly recommended to delete this method. because: - Ambiguous method name - It is just called once ########## streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/system/service/impl/VariableServiceImpl.java: ########## @@ -0,0 +1,120 @@ +/* + * 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.streampark.console.system.service.impl; + +import org.apache.streampark.console.base.domain.RestRequest; +import org.apache.streampark.console.base.exception.ApiAlertException; +import org.apache.streampark.console.core.entity.Application; +import org.apache.streampark.console.core.service.ApplicationService; +import org.apache.streampark.console.core.service.CommonService; +import org.apache.streampark.console.system.entity.Variable; +import org.apache.streampark.console.system.mapper.VariableMapper; +import org.apache.streampark.console.system.service.VariableService; + +import com.baomidou.mybatisplus.core.conditions.query.LambdaQueryWrapper; +import com.baomidou.mybatisplus.core.metadata.IPage; +import com.baomidou.mybatisplus.extension.plugins.pagination.Page; +import com.baomidou.mybatisplus.extension.service.impl.ServiceImpl; +import lombok.SneakyThrows; +import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +import java.util.List; + +@Slf4j +@Service +@Transactional(propagation = Propagation.SUPPORTS, readOnly = true, rollbackFor = Exception.class) +public class VariableServiceImpl extends ServiceImpl<VariableMapper, Variable> implements VariableService { + + @Autowired + private ApplicationService applicationService; + + @Autowired + private CommonService commonService; + + @Override + @Transactional(rollbackFor = Exception.class) + public void createVariable(Variable variable) throws Exception { + if (isExists(variable)) { + throw new ApiAlertException("Sorry, the variable code already exists."); + } + variable.setCreator(commonService.getCurrentUser().getUserId()); + this.save(variable); + } + + @Override + public IPage<Variable> findVariables(Variable variable, RestRequest request) { + Page<Variable> page = new Page<>(); + page.setCurrent(request.getPageNum()); + page.setSize(request.getPageSize()); + return this.baseMapper.findVariable(page, variable); + } + + @Override + public void updateVariable(Variable variable) throws Exception { + Variable findVariable = this.findByVariableCode(variable.getTeamId(), variable.getVariableCode()); + if (findVariable == null) { + throw new ApiAlertException("Sorry, the variable code already exists."); + } + if (findVariable.getId().longValue() != variable.getId().longValue()) { + throw new ApiAlertException("Sorry, the variable id is inconsistent."); + } + updateById(variable); + } + + @Override + public void deleteVariable(Variable variable) throws Exception { + Variable findVariable = this.findByVariableCode(variable.getTeamId(), variable.getVariableCode()); + if (findVariable == null) { + throw new ApiAlertException("Sorry, the variable code already exists."); + } + if (findVariable.getId().longValue() != variable.getId().longValue()) { + throw new ApiAlertException("Sorry, the variable id is inconsistent."); + } + List<Application> dependApplications = this.findDependByCode(variable); Review Comment: If the `findDependByCode` just be called inside VariableServiceImpl, why do you define this method in the VariableService? As I understand, `findDependByCode` should be private. ########## streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/system/service/VariableService.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.streampark.console.system.service; + +import org.apache.streampark.console.base.domain.RestRequest; +import org.apache.streampark.console.core.entity.Application; +import org.apache.streampark.console.system.entity.Variable; + +import com.baomidou.mybatisplus.core.metadata.IPage; +import com.baomidou.mybatisplus.extension.service.IService; + +import java.util.List; + +public interface VariableService extends IService<Variable> { + + /** + * find variable + * + * @param variable variable + * @param restRequest queryRequest + * @return IPage + */ + IPage<Variable> findVariables(Variable variable, RestRequest restRequest); + + /** + * get variables through team + * @param teamId + * @return + */ + List<Variable> findByTeamId(Long teamId); + + /** + * create variable + * + * @param variable variable + */ + void createVariable(Variable variable) throws Exception; + + /** + * delete variable + * + * @param variable variable + */ + void deleteVariable(Variable variable) throws Exception; + + /** + * update variable + * + * @param variable variable + */ + void updateVariable(Variable variable) throws Exception; + + Variable findByVariableCode(Long teamId, String variableCode); + + boolean isExists(Variable variable); + + List<Application> findDependByCode(Variable variable); Review Comment: method is ByCode, but the parameter isn't code, it's Variable. How about rename to f`indDependApplications(Variable variable)`? ########## streampark-console/streampark-console-webapp/src/api/index.js: ########## @@ -209,6 +209,15 @@ export default { UPDATE: '/menu/update', ROUTER: '/menu/router' }, + Variable: { + LIST: '/variable/list', + UPDATE: '/variable/update', + GET: '/variable/get', Review Comment: I checked, `VariableController` doesn't have the get method. ########## streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/system/service/impl/VariableServiceImpl.java: ########## @@ -0,0 +1,120 @@ +/* + * 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.streampark.console.system.service.impl; + +import org.apache.streampark.console.base.domain.RestRequest; +import org.apache.streampark.console.base.exception.ApiAlertException; +import org.apache.streampark.console.core.entity.Application; +import org.apache.streampark.console.core.service.ApplicationService; +import org.apache.streampark.console.core.service.CommonService; +import org.apache.streampark.console.system.entity.Variable; +import org.apache.streampark.console.system.mapper.VariableMapper; +import org.apache.streampark.console.system.service.VariableService; + +import com.baomidou.mybatisplus.core.conditions.query.LambdaQueryWrapper; +import com.baomidou.mybatisplus.core.metadata.IPage; +import com.baomidou.mybatisplus.extension.plugins.pagination.Page; +import com.baomidou.mybatisplus.extension.service.impl.ServiceImpl; +import lombok.SneakyThrows; +import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +import java.util.List; + +@Slf4j +@Service +@Transactional(propagation = Propagation.SUPPORTS, readOnly = true, rollbackFor = Exception.class) +public class VariableServiceImpl extends ServiceImpl<VariableMapper, Variable> implements VariableService { + + @Autowired + private ApplicationService applicationService; + + @Autowired + private CommonService commonService; + + @Override + @Transactional(rollbackFor = Exception.class) + public void createVariable(Variable variable) throws Exception { + if (isExists(variable)) { + throw new ApiAlertException("Sorry, the variable code already exists."); + } + variable.setCreator(commonService.getCurrentUser().getUserId()); + this.save(variable); + } + + @Override + public IPage<Variable> findVariables(Variable variable, RestRequest request) { + Page<Variable> page = new Page<>(); + page.setCurrent(request.getPageNum()); + page.setSize(request.getPageSize()); + return this.baseMapper.findVariable(page, variable); + } + + @Override + public void updateVariable(Variable variable) throws Exception { + Variable findVariable = this.findByVariableCode(variable.getTeamId(), variable.getVariableCode()); + if (findVariable == null) { + throw new ApiAlertException("Sorry, the variable code already exists."); + } + if (findVariable.getId().longValue() != variable.getId().longValue()) { + throw new ApiAlertException("Sorry, the variable id is inconsistent."); + } + updateById(variable); + } + + @Override + public void deleteVariable(Variable variable) throws Exception { + Variable findVariable = this.findByVariableCode(variable.getTeamId(), variable.getVariableCode()); + if (findVariable == null) { + throw new ApiAlertException("Sorry, the variable code already exists."); + } + if (findVariable.getId().longValue() != variable.getId().longValue()) { + throw new ApiAlertException("Sorry, the variable id is inconsistent."); + } + List<Application> dependApplications = this.findDependByCode(variable); + if (!(dependApplications == null || dependApplications.isEmpty())) { Review Comment: How about `dependApplications != null && !dependApplications.isEmpty()`? Or could we ensure the return value of `findDependByCode(variable)` is always not null? We can return an empty list~. If we can, the condition will be more simple, it can be `!dependApplications.isEmpty()` ########## streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/system/service/impl/VariableServiceImpl.java: ########## @@ -0,0 +1,120 @@ +/* + * 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.streampark.console.system.service.impl; + +import org.apache.streampark.console.base.domain.RestRequest; +import org.apache.streampark.console.base.exception.ApiAlertException; +import org.apache.streampark.console.core.entity.Application; +import org.apache.streampark.console.core.service.ApplicationService; +import org.apache.streampark.console.core.service.CommonService; +import org.apache.streampark.console.system.entity.Variable; +import org.apache.streampark.console.system.mapper.VariableMapper; +import org.apache.streampark.console.system.service.VariableService; + +import com.baomidou.mybatisplus.core.conditions.query.LambdaQueryWrapper; +import com.baomidou.mybatisplus.core.metadata.IPage; +import com.baomidou.mybatisplus.extension.plugins.pagination.Page; +import com.baomidou.mybatisplus.extension.service.impl.ServiceImpl; +import lombok.SneakyThrows; +import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +import java.util.List; + +@Slf4j +@Service +@Transactional(propagation = Propagation.SUPPORTS, readOnly = true, rollbackFor = Exception.class) +public class VariableServiceImpl extends ServiceImpl<VariableMapper, Variable> implements VariableService { + + @Autowired + private ApplicationService applicationService; + + @Autowired + private CommonService commonService; + + @Override + @Transactional(rollbackFor = Exception.class) + public void createVariable(Variable variable) throws Exception { + if (isExists(variable)) { + throw new ApiAlertException("Sorry, the variable code already exists."); + } + variable.setCreator(commonService.getCurrentUser().getUserId()); + this.save(variable); + } + + @Override + public IPage<Variable> findVariables(Variable variable, RestRequest request) { + Page<Variable> page = new Page<>(); + page.setCurrent(request.getPageNum()); + page.setSize(request.getPageSize()); + return this.baseMapper.findVariable(page, variable); + } + + @Override + public void updateVariable(Variable variable) throws Exception { + Variable findVariable = this.findByVariableCode(variable.getTeamId(), variable.getVariableCode()); + if (findVariable == null) { + throw new ApiAlertException("Sorry, the variable code already exists."); + } + if (findVariable.getId().longValue() != variable.getId().longValue()) { + throw new ApiAlertException("Sorry, the variable id is inconsistent."); + } + updateById(variable); + } + + @Override + public void deleteVariable(Variable variable) throws Exception { + Variable findVariable = this.findByVariableCode(variable.getTeamId(), variable.getVariableCode()); + if (findVariable == null) { + throw new ApiAlertException("Sorry, the variable code already exists."); + } + if (findVariable.getId().longValue() != variable.getId().longValue()) { + throw new ApiAlertException("Sorry, the variable id is inconsistent."); + } + List<Application> dependApplications = this.findDependByCode(variable); + if (!(dependApplications == null || dependApplications.isEmpty())) { + throw new ApiAlertException(String.format("Sorry, this variable is being used by [%s] applications.", dependApplications.size())); + } + this.removeById(findVariable.getId()); + } + + @Override + public Variable findByVariableCode(Long teamId, String variableCode) { + return baseMapper.selectOne(new LambdaQueryWrapper<Variable>() + .eq(Variable::getVariableCode, variableCode) + .eq(Variable::getTeamId, teamId)); + } + + @Override + public List<Variable> findByTeamId(Long teamId) { + return baseMapper.selectByTeamId(teamId); + } + + @Override + public boolean isExists(Variable variable) { + return this.findByVariableCode(variable.getTeamId(), variable.getVariableCode()) != null; + } Review Comment: Could you delete this method? The method name isn't clear. If you don't see the detailed code and just see the method name, do you know how this method checks if the variable exists? I see the method name, I thought it check by `variable.getId()` Ambiguous method names introduce potential risks and bugs. It can easily lead to bugs if called by other developers. ########## streampark-console/streampark-console-service/src/main/java/org/apache/streampark/console/system/service/VariableService.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.streampark.console.system.service; + +import org.apache.streampark.console.base.domain.RestRequest; +import org.apache.streampark.console.core.entity.Application; +import org.apache.streampark.console.system.entity.Variable; + +import com.baomidou.mybatisplus.core.metadata.IPage; +import com.baomidou.mybatisplus.extension.service.IService; + +import java.util.List; + +public interface VariableService extends IService<Variable> { + + /** + * find variable + * + * @param variable variable + * @param restRequest queryRequest + * @return IPage + */ + IPage<Variable> findVariables(Variable variable, RestRequest restRequest); + + /** + * get variables through team + * @param teamId + * @return + */ + List<Variable> findByTeamId(Long teamId); + + /** + * create variable + * + * @param variable variable + */ + void createVariable(Variable variable) throws Exception; + + /** + * delete variable + * + * @param variable variable + */ + void deleteVariable(Variable variable) throws Exception; + + /** + * update variable + * + * @param variable variable + */ + void updateVariable(Variable variable) throws Exception; + + Variable findByVariableCode(Long teamId, String variableCode); Review Comment: findByTeamIdAndVariableCode? -- 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]
