Copilot commented on code in PR #12482: URL: https://github.com/apache/cloudstack/pull/12482#discussion_r2711372878
########## engine/schema/src/main/resources/META-INF/db/procedures/cloud.insert_extension_custom_action_details_if_not_exists.sql: ########## @@ -0,0 +1,46 @@ +-- 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. + +DROP PROCEDURE IF EXISTS `cloud`.`INSERT_EXTENSION_CUSTOM_ACTION_DETAILS_IF_NOT_EXISTS`; +CREATE PROCEDURE `cloud`.`INSERT_EXTENSION_CUSTOM_ACTION_DETAILS_IF_NOT_EXISTS` ( + IN ext_name VARCHAR(255), + IN action_name VARCHAR(255), + IN param_json TEXT +) +BEGIN + DECLARE action_id BIGINT UNSIGNED +; SELECT `eca`.`id` INTO action_id FROM `cloud`.`extension_custom_action` `eca` + JOIN `cloud`.`extension` `e` ON `e`.`id` = `eca`.`extension_id` + WHERE `eca`.`name` = action_name AND `e`.`name` = ext_name LIMIT 1 Review Comment: The procedure does not validate whether action_id is NULL after the SELECT statement. If the extension or action with the given names do not exist, action_id will be NULL, and the subsequent operations will fail or produce incorrect results. Add a check to signal an error if the action is not found, similar to the pattern used in other procedures. ```suggestion WHERE `eca`.`name` = action_name AND `e`.`name` = ext_name LIMIT 1 ; IF action_id IS NULL THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Extension custom action not found'; END IF ``` ########## engine/schema/src/main/resources/META-INF/db/procedures/cloud.insert_category_if_not_exists.sql: ########## @@ -0,0 +1,27 @@ +-- 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. + +-- Add new OS categories if not present +DROP PROCEDURE IF EXISTS `cloud`.`INSERT_CATEGORY_IF_NOT_EXIST`; +CREATE PROCEDURE `cloud`.`INSERT_CATEGORY_IF_NOT_EXIST`(IN os_name VARCHAR(255)) Review Comment: The procedure name has an inconsistent naming convention. Line 19 uses 'INSERT_CATEGORY_IF_NOT_EXIST' (singular) while the filename and other similar procedures use 'INSERT_..._IF_NOT_EXISTS' (plural). This inconsistency should be corrected to match the pattern used in other procedures. ```suggestion DROP PROCEDURE IF EXISTS `cloud`.`INSERT_CATEGORY_IF_NOT_EXISTS`; CREATE PROCEDURE `cloud`.`INSERT_CATEGORY_IF_NOT_EXISTS`(IN os_name VARCHAR(255)) ``` ########## engine/schema/src/main/resources/META-INF/db/procedures/cloud.insert_extension_custom_action_if_not_exists.sql: ########## @@ -0,0 +1,46 @@ +-- 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. + +DROP PROCEDURE IF EXISTS `cloud`.`INSERT_EXTENSION_CUSTOM_ACTION_IF_NOT_EXISTS`; +CREATE PROCEDURE `cloud`.`INSERT_EXTENSION_CUSTOM_ACTION_IF_NOT_EXISTS`( + IN ext_name VARCHAR(255), + IN action_name VARCHAR(255), + IN action_desc VARCHAR(4096), + IN resource_type VARCHAR(255), + IN allowed_roles INT UNSIGNED, + IN success_msg VARCHAR(4096), + IN error_msg VARCHAR(4096), + IN timeout_seconds INT UNSIGNED +) +BEGIN + DECLARE ext_id BIGINT +; SELECT `id` INTO ext_id FROM `cloud`.`extension` WHERE `name` = ext_name LIMIT 1 Review Comment: The procedure does not validate whether ext_id is NULL after the SELECT statement. If the extension with the given name does not exist, ext_id will be NULL, and the subsequent operations will fail or produce incorrect results. Add a check to signal an error if the extension is not found, similar to the pattern used in other procedures. ```suggestion ; SELECT `id` INTO ext_id FROM `cloud`.`extension` WHERE `name` = ext_name LIMIT 1 ; IF ext_id IS NULL THEN ; SIGNAL SQLSTATE '45000' ; SET MESSAGE_TEXT = CONCAT('Extension not found: ', ext_name) ; END IF ``` ########## engine/schema/src/main/resources/META-INF/db/procedures/cloud.insert_extension_detail_if_not_exists.sql: ########## @@ -0,0 +1,39 @@ +-- 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. + +ROP PROCEDURE IF EXISTS `cloud`.`INSERT_EXTENSION_DETAIL_IF_NOT_EXISTS`; +CREATE PROCEDURE `cloud`.`INSERT_EXTENSION_DETAIL_IF_NOT_EXISTS`( + IN ext_name VARCHAR(255), + IN detail_key VARCHAR(255), + IN detail_value TEXT, + IN display TINYINT(1) +) +BEGIN + DECLARE ext_id BIGINT +; SELECT `id` INTO ext_id FROM `cloud`.`extension` WHERE `name` = ext_name LIMIT 1 Review Comment: The procedure does not validate whether ext_id is NULL after the SELECT statement. If the extension with the given name does not exist, ext_id will be NULL, and the subsequent INSERT will fail or insert invalid data. Add a check to signal an error if the extension is not found, similar to the pattern used in other procedures. ```suggestion ; SELECT `id` INTO ext_id FROM `cloud`.`extension` WHERE `name` = ext_name LIMIT 1 ; IF ext_id IS NULL THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Extension not found for given name'; END IF ``` ########## engine/schema/src/main/resources/META-INF/db/procedures/cloud.insert_extension_detail_if_not_exists.sql: ########## @@ -0,0 +1,39 @@ +-- 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. + +ROP PROCEDURE IF EXISTS `cloud`.`INSERT_EXTENSION_DETAIL_IF_NOT_EXISTS`; Review Comment: The keyword 'DROP' is misspelled as 'ROP'. This will cause a syntax error when executing the procedure. ```suggestion DROP PROCEDURE IF EXISTS `cloud`.`INSERT_EXTENSION_DETAIL_IF_NOT_EXISTS`; ``` ########## engine/schema/src/main/resources/META-INF/db/procedures/cloud.update_new_and_delete_old_category_for_guest_os.sql: ########## @@ -0,0 +1,35 @@ +-- 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. + +-- Move existing guest OS whose category will be deleted to Other category +DROP PROCEDURE IF EXISTS `cloud`.`UPDATE_NEW_AND_DELETE_OLD_CATEGORY_FOR_GUEST_OS`; +CREATE PROCEDURE `cloud`.`UPDATE_NEW_AND_DELETE_OLD_CATEGORY_FOR_GUEST_OS`(IN to_category_name VARCHAR(255), IN from_category_name VARCHAR(255)) +BEGIN + DECLARE done INT DEFAULT 0 +; DECLARE to_category_id BIGINT Review Comment: There is an unused variable 'done' declared that is never used in the procedure. This should be removed to avoid confusion. ```suggestion DECLARE to_category_id BIGINT ``` -- 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]
