Copilot commented on code in PR #12144:
URL: https://github.com/apache/cloudstack/pull/12144#discussion_r3959254726
##########
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:
##########
@@ -19,6 +19,22 @@
-- 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) NOT NULL,
+ `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 IGNORE INTO `cloud`.`service_offering_category` (id, name, uuid) VALUES
(1, 'Default', UUID());
+
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.service_offering', 'category_id',
'bigint unsigned NOT NULL DEFAULT 1');
+CALL `cloud`.`IDEMPOTENT_DROP_FOREIGN_KEY`('cloud.service_offering',
'fk_service_offering__category_id');
+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:
The migration uses a raw `ALTER TABLE ... ADD CONSTRAINT` which is not
idempotent; re-running the upgrade (or running on a partially-upgraded DB) can
fail if the FK already exists. Prefer using the project’s idempotent helper
(e.g., `IDEMPOTENT_ADD_FOREIGN_KEY`) or an `IF NOT EXISTS`-style mechanism
consistent with other schema migrations.
##########
api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java:
##########
@@ -286,6 +286,14 @@ public class ServiceOfferingResponse extends
BaseResponseWithAnnotations {
@Param(description = "Action to be taken once lease is over", since =
"4.21.0")
private String leaseExpiryAction;
+ @SerializedName("categoryid")
+ @Param(description = "the ID of the service offering category", since =
"24.0")
+ private String categoryId;
+
+ @SerializedName("category")
+ @Param(description = "the name of the service offering category", since =
"24.0")
Review Comment:
The `since = "24.0"` metadata looks inconsistent with the existing
versioning scheme in the codebase (e.g., many params use `"4.xx.x"`). If the
project expects CloudStack-style `4.x` versions, this `since` value should be
updated to the correct release version for this feature to avoid inaccurate API
documentation and client generation artifacts.
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -9555,4 +9590,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:
The hard-coded default category ID (`1L`) is a fragile “magic number” that
tightly couples runtime behavior to an assumption about DB contents. If the
default row’s ID ever differs (e.g., different seed strategy or pre-existing
table), deletion protection will be incorrect. A more robust approach is to
explicitly model “default” (e.g., `is_default` column), or resolve the
protected category by a stable attribute (like reserved UUID/name) and enforce
that at the DB/application level.
--
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]
