Copilot commented on code in PR #12144:
URL: https://github.com/apache/cloudstack/pull/12144#discussion_r3949019545
##########
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:
##########
@@ -19,6 +19,21 @@
-- Schema upgrade from 4.22.1.0 to 4.23.0.0
--;
+CREATE TABLE IF NOT EXISTS `cloud`.`service_offering_category` (
+ `id` bigint unsigned NOT NULL auto_increment,
+ `name` varchar(255) NOT NULL,
+ `uuid` varchar(40),
+ `sort_key` int NOT NULL DEFAULT 0,
+ PRIMARY KEY (`id`),
+ CONSTRAINT `uc_service_offering_category__uuid` UNIQUE (`uuid`),
+ CONSTRAINT `uc_service_offering_category__name` UNIQUE (`name`)
+) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8;
+
+INSERT INTO `cloud`.`service_offering_category` (id, name, uuid) VALUES (1,
'Default', UUID());
+
+ALTER TABLE `cloud`.`service_offering` ADD COLUMN `category_id` bigint
unsigned NOT NULL DEFAULT 1;
+ALTER TABLE `cloud`.`service_offering` ADD CONSTRAINT
`fk_service_offering__category_id` FOREIGN KEY (`category_id`) REFERENCES
`cloud`.`service_offering_category` (`id`) ON DELETE RESTRICT ON UPDATE CASCADE;
Review Comment:
This upgrade adds DDL/DML directly (CREATE/ALTER/INSERT) in a schema upgrade
file that already uses idempotent helpers below. The INSERT with a fixed id=1
and the ALTER statements are not idempotent and can fail on re-runs/partial
upgrades (e.g., repeated execution in CI or recovery). Prefer the project’s
idempotent helpers (e.g., IDEMPOTENT_ADD_COLUMN / IDEMPOTENT_ADD_FOREIGN_KEY,
and an idempotent seed insert guarded by existence check) to make upgrades
resilient.
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -9555,4 +9583,97 @@ public void setScope(String scope) {
this.scope = scope;
}
}
+
+ @Override
+ @ActionEvent(eventType =
EventTypes.EVENT_SERVICE_OFFERING_CATEGORY_CREATE, eventDescription = "creating
service offering category")
+ public ServiceOfferingCategory
createServiceOfferingCategory(CreateServiceOfferingCategoryCmd cmd) {
+ String name = cmd.getName();
+ Integer sortKey = cmd.getSortKey();
+
+ // Check if category with same name already exists
+ ServiceOfferingCategoryVO existingCategory =
_serviceOfferingCategoryDao.findByName(name);
+ if (existingCategory != null) {
+ throw new InvalidParameterValueException("Service offering
category with name " + name + " already exists");
+ }
+
+ ServiceOfferingCategoryVO category = new
ServiceOfferingCategoryVO(name);
+ if (sortKey != null) {
+ category.setSortKey(sortKey);
+ }
+
+ category = _serviceOfferingCategoryDao.persist(category);
+ CallContext.current().setEventDetails("Service offering category id="
+ category.getId());
+ return category;
+ }
+
+ @Override
+ @ActionEvent(eventType =
EventTypes.EVENT_SERVICE_OFFERING_CATEGORY_DELETE, eventDescription = "deleting
service offering category")
+ public boolean
deleteServiceOfferingCategory(DeleteServiceOfferingCategoryCmd cmd) {
+ Long categoryId = cmd.getId();
+
+ ServiceOfferingCategoryVO category =
_serviceOfferingCategoryDao.findById(categoryId);
+ if (category == null) {
+ throw new InvalidParameterValueException("Unable to find service
offering category with id " + categoryId);
+ }
+
+ // Check if any service offering is using this category
+ // For now we'll just check if it's the default category (id=1)
+ if (categoryId == 1L) {
+ throw new InvalidParameterValueException("Cannot delete the
default service offering category");
+ }
Review Comment:
Hard-coding the default category id as `1L` bakes a schema assumption into
application logic. A more robust approach is to define a constant (shared with
schema seed), or determine the default by a stable attribute (e.g., a dedicated
`is_default` column, or a lookup by reserved name) so the behavior remains
correct if seeding/migrations change.
--
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]
