Copilot commented on code in PR #12144:
URL: https://github.com/apache/cloudstack/pull/12144#discussion_r3951517921
##########
api/src/main/java/org/apache/cloudstack/api/ApiConstants.java:
##########
@@ -550,6 +550,7 @@ public class ApiConstants {
public static final String SENT_BYTES = "sentbytes";
public static final String SERIAL = "serial";
public static final String SERVICE_IP = "serviceip";
+ public static final String SERVICE_OFFERING_CATEGORY_ID = "categoryid";
Review Comment:
The PR description mentions adding both category ID and category name
constants (e.g., `SERVICE_OFFERING_CATEGORY_NAME`), but this diff only
introduces `SERVICE_OFFERING_CATEGORY_ID`. Either update the description or add
the missing constant if it’s intended to be used by commands/responses.
##########
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),
+ `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;
Review Comment:
The `uuid` column is nullable while also being declared UNIQUE. This allows
multiple rows with `NULL` UUIDs, which weakens the expectation that every
category has a stable API identity. Make `uuid` `NOT NULL` (and ensure backfill
for any existing rows) to match how CloudStack typically models UUID-bearing
entities.
##########
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:
Hardcoding the default category as `id=1` is brittle (schema or deployments
could evolve, and it’s not self-documenting). Prefer deriving this from a
well-defined invariant (e.g., lookup by a constant name like 'Default', or a
dedicated flag/column), and centralize that logic so it’s reused consistently
across create/update/delete validations.
--
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]
