Copilot commented on code in PR #12144:
URL: https://github.com/apache/cloudstack/pull/12144#discussion_r2591984382
##########
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:
##########
@@ -18,3 +18,18 @@
--;
-- 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,
Review Comment:
The `name` column should have a UNIQUE constraint to prevent duplicate
category names, as the application logic in `ConfigurationManagerImpl.java`
checks for duplicate names when creating/updating categories (lines 8634-8636
and 8692-8695).
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -8595,4 +8623,91 @@ public void setScope(String scope) {
this.scope = scope;
}
}
+
+ @Override
+ @ActionEvent(eventType = EventTypes.EVENT_SERVICE_OFFERING_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_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 delete validation is incomplete. The comment on line 8659 states "Check
if any service offering is using this category", but the actual implementation
only checks if it's the default category (id=1). Due to the foreign key
constraint with `ON DELETE RESTRICT` in the schema, attempting to delete a
category that's in use will cause a database error. The code should query for
service offerings using this category and provide a clear error message before
attempting deletion.
```suggestion
// Check if any service offering is using this category
List<ServiceOfferingVO> offeringsUsingCategory =
_serviceOfferingDao.listByCategoryId(categoryId);
if (offeringsUsingCategory != null &&
!offeringsUsingCategory.isEmpty()) {
throw new InvalidParameterValueException("Cannot delete service
offering category with id " + categoryId +
" because it is in use by one or more service offerings.");
}
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -289,6 +290,14 @@ public class CreateServiceOfferingCmd extends BaseCmd {
since = "4.21.0")
private Map externalDetails;
+ @Parameter(name = ApiConstants.SERVICE_OFFERING_CATEGORY_ID,
+ type = CommandType.UUID,
+ entityType = ServiceOfferingCategoryResponse.class,
+ required = false,
+ description = "the ID of the service offering category to
associate with this offering",
+ since = "4.23")
Review Comment:
The version format should be consistent. This parameter uses "4.23" while
most other API commands use "4.23.0" format. Please use "4.23.0" for
consistency.
```suggestion
since = "4.23.0")
```
##########
ui/src/views/compute/wizard/ComputeOfferingSelection.vue:
##########
@@ -381,4 +416,26 @@ export default {
:deep(.ant-table-tbody) > tr > td {
cursor: pointer;
}
+
+ .radio-option {
+ display: flex;
+ flex-direction: column;
+ justify-content: center;
+ align-items: center;
+ width: 100%;
+ height: 100%;
+
+ .radio-opion__icon {
Review Comment:
There is a typo in the CSS class name: 'radio-opion__icon' should be
'radio-option__icon' (missing 't' in 'option').
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCategoryCmd.java:
##########
@@ -0,0 +1,85 @@
+// 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.
+package org.apache.cloudstack.api.command.admin.offering;
+
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ServiceOfferingCategoryResponse;
+
+import com.cloud.offering.ServiceOfferingCategory;
+import com.cloud.user.Account;
+
+@APICommand(name = "createServiceOfferingCategory",
+ description = "Creates a service offering category.",
+ responseObject = ServiceOfferingCategoryResponse.class,
+ since = "4.23.0",
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false)
+public class CreateServiceOfferingCategoryCmd extends BaseCmd {
+
+ /////////////////////////////////////////////////////
+ //////////////// API parameters /////////////////////
+ /////////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.NAME,
+ type = CommandType.STRING,
+ required = true,
+ description = "the name of the service offering category")
+ private String name;
+
+ @Parameter(name = ApiConstants.SORT_KEY,
+ type = CommandType.INTEGER,
+ description = "sort key of the service offering category, default
is 0")
+ private Integer sortKey;
+
+ /////////////////////////////////////////////////////
+ /////////////////// Accessors ///////////////////////
+ /////////////////////////////////////////////////////
+
+ public String getName() {
+ return name;
+ }
+
+ public Integer getSortKey() {
+ return sortKey;
+ }
+
+ /////////////////////////////////////////////////////
+ /////////////// API Implementation///////////////////
+ /////////////////////////////////////////////////////
+
+ @Override
+ public void execute() {
+ ServiceOfferingCategory result =
_configService.createServiceOfferingCategory(this);
+ if (result != null) {
+ ServiceOfferingCategoryResponse response =
_responseGenerator.createServiceOfferingCategoryResponse(result);
+ response.setResponseName(getCommandName());
+ setResponseObject(response);
+ } else {
+ throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed
to create service offering category");
+ }
+ }
+
+ @Override
+ public long getEntityOwnerId() {
+ return Account.Type.ADMIN.ordinal();
Review Comment:
The `getEntityOwnerId()` method returns `Account.Type.ADMIN.ordinal()`,
which is incorrect. The ordinal() of an enum returns its position (0 for
ADMIN), not an account ID. This should return `Account.ACCOUNT_ID_SYSTEM` to
match the pattern used in similar commands like
`UpdateServiceOfferingCategoryCmd` (line 80-82).
```suggestion
return Account.ACCOUNT_ID_SYSTEM;
```
##########
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 =
"4.23")
+ private String categoryId;
+
+ @SerializedName("category")
+ @Param(description = "the name of the service offering category", since =
"4.23")
Review Comment:
The version format should be consistent across the codebase. Most new
features use "4.23.0" format (as seen in the API commands), but this uses
"4.23". Please use "4.23.0" to maintain consistency.
```suggestion
@Param(description = "the ID of the service offering category", since =
"4.23.0")
private String categoryId;
@SerializedName("category")
@Param(description = "the name of the service offering category", since
= "4.23.0")
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/UpdateServiceOfferingCmd.java:
##########
@@ -109,6 +110,14 @@ public class UpdateServiceOfferingCmd extends BaseCmd {
since = "4.22.0")
protected Boolean cleanupExternalDetails;
+ @Parameter(name = ApiConstants.SERVICE_OFFERING_CATEGORY_ID,
+ type = CommandType.UUID,
+ entityType = ServiceOfferingCategoryResponse.class,
+ required = false,
+ description = "the ID of the service offering category to
associate",
+ since = "4.23")
Review Comment:
The version format should be consistent. This parameter uses "4.23" while
most other API commands use "4.23.0" format. Please use "4.23.0" for
consistency.
```suggestion
since = "4.23.0")
```
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/DeleteServiceOfferingCategoryCmd.java:
##########
@@ -0,0 +1,76 @@
+// 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.
+package org.apache.cloudstack.api.command.admin.offering;
+
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ServiceOfferingCategoryResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+
+import com.cloud.user.Account;
+
+@APICommand(name = "deleteServiceOfferingCategory",
+ description = "Deletes a service offering category.",
+ responseObject = SuccessResponse.class,
+ since = "4.23.0",
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false)
+public class DeleteServiceOfferingCategoryCmd extends BaseCmd {
+
+ /////////////////////////////////////////////////////
+ //////////////// API parameters /////////////////////
+ /////////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.ID,
+ type = CommandType.UUID,
+ entityType = ServiceOfferingCategoryResponse.class,
+ required = true,
+ description = "the ID of the service offering category")
+ private Long id;
+
+ /////////////////////////////////////////////////////
+ /////////////////// Accessors ///////////////////////
+ /////////////////////////////////////////////////////
+
+ public Long getId() {
+ return id;
+ }
+
+ /////////////////////////////////////////////////////
+ /////////////// API Implementation///////////////////
+ /////////////////////////////////////////////////////
+
+ @Override
+ public void execute() {
+ boolean result = _configService.deleteServiceOfferingCategory(this);
+ if (result) {
+ SuccessResponse response = new SuccessResponse(getCommandName());
+ setResponseObject(response);
+ } else {
+ throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed
to delete service offering category");
+ }
+ }
+
+ @Override
+ public long getEntityOwnerId() {
+ return Account.Type.ADMIN.ordinal();
Review Comment:
The `getEntityOwnerId()` method returns `Account.Type.ADMIN.ordinal()`,
which is incorrect. The ordinal() of an enum returns its position (0 for
ADMIN), not an account ID. This should return `Account.ACCOUNT_ID_SYSTEM` to
match the pattern used in other admin commands, such as
`CreateServiceOfferingCategoryCmd` (line 82-83) and
`UpdateServiceOfferingCategoryCmd` (line 80-82).
```suggestion
return Account.ACCOUNT_ID_SYSTEM;
```
##########
api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListServiceOfferingsCmd.java:
##########
@@ -124,6 +125,13 @@ public class ListServiceOfferingsCmd extends
BaseListProjectAndAccountResourcesC
since = "4.21.0")
private Boolean gpuEnabled;
+ @Parameter(name = ApiConstants.SERVICE_OFFERING_CATEGORY_ID,
+ type = CommandType.UUID,
+ entityType = ServiceOfferingCategoryResponse .class,
Review Comment:
There is an extra space before `.class` in the entityType parameter:
'ServiceOfferingCategoryResponse .class' should be
'ServiceOfferingCategoryResponse.class'.
```suggestion
entityType = ServiceOfferingCategoryResponse.class,
```
##########
ui/src/components/view/ListView.vue:
##########
@@ -1027,6 +1027,9 @@
<template v-if="column.key === 'vgpuActions'">
<slot name="actionButtons" :record="record" :actions="actions"></slot>
</template>
+ <template v-if="column.key === 'category' && $route.path.split('/')[1]
=== 'computeoffering'">
+ <router-link :to="{ path: '/serviceofferingcategory/' +
record.categoryid }">{{ text }}</router-link>
Review Comment:
The router-link uses `record.categoryid` which is the internal database ID,
not the UUID. The route should use `record.categoryuuid` or similar UUID field
to maintain consistency with how CloudStack typically exposes resource IDs in
the API and UI URLs.
##########
ui/src/components/view/ListView.vue:
##########
@@ -1027,6 +1027,9 @@
<template v-if="column.key === 'vgpuActions'">
<slot name="actionButtons" :record="record" :actions="actions"></slot>
</template>
+ <template v-if="column.key === 'category' && $route.path.split('/')[1]
=== 'computeoffering'">
Review Comment:
There is an extra space in the condition: `column.key === 'category' &&`
has two spaces before `&&`. Should be a single space for consistency.
##########
ui/src/views/compute/DeployVM.vue:
##########
@@ -2271,6 +2274,46 @@ export default {
console.error('Error fetching guestOsCategories:', e)
})
},
+ fetchServiceOfferingCategories () {
+ this.loading.serviceOfferingCategories = true
+ return new Promise((resolve, reject) => {
+ getAPI('listServiceOfferingCategories').then(json => {
+ const categories =
json.listserviceofferingcategoriesresponse.serviceofferingcategory || []
+ this.options.serviceOfferingCategories = [
+ {
+ id: '-1',
+ name: this.$t('label.all')
+ },
+ ...categories
+ ]
+ resolve()
+ }).catch(error => {
+ console.error('Error fetching service offering categories:', error)
+ this.options.serviceOfferingCategories = [
+ {
+ id: '-1',
+ name: this.$t('label.all')
+ }
+ ]
+ reject(error)
+ }).finally(() => {
+ this.loading.serviceOfferingCategories = false
+ })
+ })
+ },
+ handleServiceOfferingCategoryChange (categoryId) {
+ this.selectedServiceOfferingCategoryId = categoryId
+ const params = {
+ page: 1,
+ pageSize: 10
+ }
+ if (categoryId && categoryId !== '-1') {
+ params.categoryid = categoryId
+ } else {
+ params.categoryid = null
Review Comment:
Setting `params.categoryid = null` may not properly remove the filter
parameter. Instead, consider using `delete params.categoryid` or simply not
setting it when categoryId is '-1', to avoid sending `categoryid=null` in the
API request which may not be handled as "no filter".
##########
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:
##########
@@ -18,3 +18,18 @@
--;
-- 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`)
+) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8;
+
+
+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;
+INSERT INTO `cloud`.`service_offering_category` (id, name, uuid) VALUES (1,
'Default', UUID());
Review Comment:
The database schema has a potential issue with the order of operations. The
`ALTER TABLE service_offering ADD COLUMN category_id` statement sets a default
value of 1, but the `INSERT INTO service_offering_category` statement that
creates the category with id=1 comes after. If the ALTER TABLE executes first
and there are existing service offerings, the default value will reference a
non-existent category temporarily. The INSERT statement should come before the
ALTER TABLE to ensure the foreign key constraint is satisfied.
```suggestion
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;
```
--
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]
