Copilot commented on code in PR #12144:
URL: https://github.com/apache/cloudstack/pull/12144#discussion_r3951421396
##########
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:
##########
@@ -6431,6 +6447,46 @@ private List<BucketVO>
searchForBucketsInternal(ListBucketsCmd cmd) {
return bucketDao.searchByIds(bktIds);
}
+ @Override
+ public ListResponse<ServiceOfferingCategoryResponse>
listServiceOfferingCategories(ListServiceOfferingCategoriesCmd cmd) {
+ Long id = cmd.getId();
+ String name = cmd.getName();
+
+ Filter searchFilter = new Filter(ServiceOfferingCategoryVO.class,
"sortKey", true, cmd.getStartIndex(), cmd.getPageSizeVal());
Review Comment:
The `Filter` order-by field is set to `\"sortKey\"`, but the underlying DB
column is `sort_key` (per schema/VO mapping). If `Filter` expects a DB column
name (common in CloudStack DAOs), this will generate an invalid ORDER BY and
break listServiceOfferingCategories. Use the correct order-by identifier (e.g.,
`sort_key`) or whatever the project convention is for `Filter` order-by strings
for VO-backed queries.
##########
api/src/main/java/com/cloud/offering/ServiceOffering.java:
##########
@@ -146,4 +146,6 @@ enum StorageType {
Long getVgpuProfileId();
Integer getGpuCount();
+
+ long getCategoryId();
Review Comment:
`getCategoryId()` is defined as a primitive `long`, but the category
association is treated as optional in create/clone/update flows (nullable `Long
categoryId` params). Using a primitive in the interface forces all
implementations to always provide a value and makes ‘unset/unknown’ impossible
to represent. Consider changing the interface method to `Long getCategoryId()`
(and adjust implementations accordingly) or make category mandatory at the API
level to match the non-nullable contract.
##########
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;
+
+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 FK creation uses a raw `ALTER TABLE ... ADD CONSTRAINT`, which is not
idempotent. Since the script already uses idempotent helpers, this should use
the project’s `IDEMPOTENT_ADD_FOREIGN_KEY` helper (or equivalent) to avoid
upgrade failures if the migration is re-applied or partially applied/retried.
##########
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 introducing both category ID and category name
constants (`SERVICE_OFFERING_CATEGORY_ID`, `SERVICE_OFFERING_CATEGORY_NAME`),
but only `SERVICE_OFFERING_CATEGORY_ID` appears in code. Either add the missing
constant (if intended for command/response consistency) or update the PR
description to reflect the actual implementation.
--
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]
