Copilot commented on code in PR #12144: URL: https://github.com/apache/cloudstack/pull/12144#discussion_r3438018920
########## engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql: ########## @@ -19,6 +19,20 @@ -- 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 FK constraint is added before the default category row is inserted. During upgrade, existing `service_offering` rows will get `category_id=1` from the column default, so adding the FK will fail because `service_offering_category(id=1)` does not exist yet. Insert the default category row before adding the FK (or delay the FK until after the insert). ########## 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: `getEntityOwnerId()` should return an account ID (typically `Account.ACCOUNT_ID_SYSTEM` for admin/system actions). Returning `Account.Type.ADMIN.ordinal()` is not a valid account ID and can break event/resource ownership tracking. ########## server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java: ########## @@ -9546,4 +9574,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"); + } + + boolean result = _serviceOfferingCategoryDao.remove(categoryId); + if (result) { + CallContext.current().setEventDetails("Service offering category id=" + categoryId); + } + return result; + } Review Comment: `service_offering.category_id` is protected by an FK with `ON DELETE RESTRICT`. If a category is in use, `_serviceOfferingCategoryDao.remove(categoryId)` will throw and likely return a 500 to the API caller. This should be handled gracefully (either pre-check for usage or catch the DB exception and return a clear `InvalidParameterValueException`). ########## ui/src/views/compute/wizard/ComputeOfferingSelection.vue: ########## @@ -402,4 +437,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 { + font-size: 2rem; + margin-bottom: 0.5rem; + } Review Comment: Typo in the CSS class name: `.radio-opion__icon` likely intends to be `.radio-option__icon`. As written, any markup using the correct spelling won't get styled. ########## ui/src/views/offering/CloneComputeOffering.vue: ########## @@ -397,6 +400,15 @@ export default { this.gpuCardLoading = false }) }, + fetchCategories () { + this.categoryLoading = true + getAPI('listServiceOfferingCategories', { + }).then(json => { + this.categories = json.listserviceofferingcategoriesresponse.serviceofferingcategory || [] + }).finally(() => { + this.categoryLoading = false + }) + }, Review Comment: After removing the call to `fetchCategories()`, this helper method becomes unused in this component (category fetching is already done inside `ComputeOfferingForm`). It should be removed to avoid dead code. ########## server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java: ########## @@ -147,6 +151,16 @@ public ServiceOfferingResponse newServiceOfferingResponse(ServiceOfferingJoinVO offeringResponse.setMaxResolutionY(offering.getMaxResolutionY()); offeringResponse.setGpuCount(offering.getGpuCount()); offeringResponse.setGpuDisplay(offering.getGpuDisplay()); + + // Set category information if available + if (offering.getCategoryId() != null) { + ServiceOfferingCategoryVO category = _serviceOfferingCategoryDao.findById(offering.getCategoryId()); + if (category != null) { + offeringResponse.setCategoryId(category.getUuid()); + offeringResponse.setCategoryName(category.getName()); + } + } Review Comment: This adds a per-row lookup (`_serviceOfferingCategoryDao.findById`) while building each `ServiceOfferingResponse`. Listing service offerings can return many rows, so this becomes an N+1 query pattern and can significantly slow down `listServiceOfferings`. ########## ui/src/views/compute/DeployVM.vue: ########## @@ -2277,6 +2280,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` will still serialize the parameter into the API request (axios includes null params), which can lead to CloudStack rejecting the request as an invalid UUID. To clear the filter, omit `categoryid` from the params instead of sending null. ########## 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: `getEntityOwnerId()` should return an account ID (typically `Account.ACCOUNT_ID_SYSTEM` for admin/system actions). Returning `Account.Type.ADMIN.ordinal()` is not a valid account ID and can break event/resource ownership tracking. ########## ui/src/views/offering/AddComputeOffering.vue: ########## @@ -254,6 +257,15 @@ export default { this.gpuCardLoading = false }) }, + fetchCategories () { + this.categoryLoading = true + getAPI('listServiceOfferingCategories', { + }).then(json => { + this.categories = json.listserviceofferingcategoriesresponse.serviceofferingcategory || [] + }).finally(() => { + this.categoryLoading = false + }) + }, Review Comment: After removing the call to `fetchCategories()`, this helper method becomes unused in this component (category fetching is already done inside `ComputeOfferingForm`). It should be removed to avoid dead code. ########## server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java: ########## @@ -9546,4 +9574,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) { Review Comment: These category CRUD actions are emitting `EVENT_SERVICE_OFFERING_*` events, which are mapped to `ServiceOffering` in `EventTypes`. This will misclassify events and resource types in the event log/audit trail. Consider introducing dedicated event types for service offering categories (similar to `EVENT_GUEST_OS_CATEGORY_*`) and using them here, along with setting `CallContext` resource id/type. ########## ui/src/views/offering/AddComputeOffering.vue: ########## @@ -117,7 +117,9 @@ export default { leaseexpiryaction: undefined, vgpuProfiles: [], vgpuProfileLoading: false, - externalDetailsEnabled: false + externalDetailsEnabled: false, + categories: [], + categoryLoading: false Review Comment: `categories`/`categoryLoading` are not used in this component (the category selector and category fetch are handled inside `ComputeOfferingForm`). Keeping them here adds dead state and can lead to confusion. ########## ui/src/views/offering/CloneComputeOffering.vue: ########## @@ -140,7 +140,9 @@ export default { leaseexpiryaction: undefined, vgpuProfiles: [], vgpuProfileLoading: false, - externalDetailsEnabled: false + externalDetailsEnabled: false, + categories: [], + categoryLoading: false Review Comment: `categories`/`categoryLoading` are not used in this component (the category selector and category fetch are handled inside `ComputeOfferingForm`). Keeping them here adds dead state and can lead to confusion. ########## ui/src/views/offering/CloneComputeOffering.vue: ########## @@ -252,6 +254,7 @@ export default { this.fetchDomainData() this.fetchZoneData() this.fetchGPUCards() + this.fetchCategories() if (isAdmin()) { this.fetchStorageTagData() Review Comment: This triggers an extra `listServiceOfferingCategories` API call from `CloneComputeOffering`, but the fetched data is not used here (category selection is handled in `ComputeOfferingForm`). Removing this avoids an unnecessary request during page load. ########## ui/src/views/offering/AddComputeOffering.vue: ########## @@ -229,6 +231,7 @@ export default { this.fetchDomainData() this.fetchZoneData() this.fetchGPUCards() + this.fetchCategories() if (isAdmin()) { this.fetchStorageTagData() Review Comment: This triggers an extra `listServiceOfferingCategories` API call from `AddComputeOffering`, but the fetched data is not used here (category selection is handled in `ComputeOfferingForm`). Removing this avoids an unnecessary request during page load. -- 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]
