Copilot commented on code in PR #12854:
URL: https://github.com/apache/cloudstack/pull/12854#discussion_r2989373346
##########
engine/schema/src/main/resources/META-INF/db/views/cloud.account_view.sql:
##########
@@ -89,8 +89,15 @@ from
`cloud`.`domain` ON account.domain_id = domain.id
left join
`cloud`.`data_center` ON account.default_zone_id = data_center.id
- left join
- `cloud`.`account_netstats_view` ON account.id =
account_netstats_view.account_id
+ left join lateral (
+ select
+ coalesce(sum(`user_statistics`.`net_bytes_received` +
`user_statistics`.`current_bytes_received`), 0) AS `bytesReceived`,
+ coalesce(sum(`user_statistics`.`net_bytes_sent` +
`user_statistics`.`current_bytes_sent`), 0) AS `bytesSent`
+ from
+ `cloud`.`user_statistics`
+ where
+ `user_statistics`.`account_id` = `account`.`id`
+ ) AS `account_netstats` ON TRUE
Review Comment:
The aggregation can produce incorrect totals when either `net_bytes_*` or
`current_bytes_*` is NULL: the expression `(net + current)` becomes NULL for
that row and SUM will skip it, losing non-null values. Use COALESCE per column
inside the SUM (e.g., `SUM(COALESCE(net,0) + COALESCE(current,0))`) to match
the previous semantics. Also consider avoiding `LATERAL` (use a grouped derived
table joined on `account_id`) to reduce per-row correlation overhead and
improve compatibility across supported MySQL/MariaDB versions.
```suggestion
left join (
select
`user_statistics`.`account_id` AS `account_id`,
coalesce(sum(coalesce(`user_statistics`.`net_bytes_received`, 0)
+ coalesce(`user_statistics`.`current_bytes_received`, 0)), 0) AS
`bytesReceived`,
coalesce(sum(coalesce(`user_statistics`.`net_bytes_sent`, 0) +
coalesce(`user_statistics`.`current_bytes_sent`, 0)), 0) AS `bytesSent`
from
`cloud`.`user_statistics`
group by
`user_statistics`.`account_id`
) AS `account_netstats` ON `account_netstats`.`account_id` =
`account`.`id`
```
##########
ui/src/views/tools/ImportUnmanagedInstance.vue:
##########
@@ -1,19 +1,10 @@
-// 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.
+// 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.
Review Comment:
The license header appears to have been auto-joined into invalid syntax:
lines 2–3 are not comments, and line 5 starts with raw text (`Unless
required...`) which will break parsing/compilation of the SFC. Please restore
the header to valid comment lines (each line starting with `//`) without
embedded `file //` fragments.
```suggestion
// 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.
```
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vpc/CreateStaticRouteCmd.java:
##########
@@ -46,7 +46,6 @@ public class CreateStaticRouteCmd extends BaseAsyncCreateCmd {
@Parameter(name = ApiConstants.GATEWAY_ID,
type = CommandType.UUID,
entityType = PrivateGatewayResponse.class,
- required = true,
description = "The gateway ID we are creating static route for.
Mutually exclusive with the nexthop parameter")
private Long gatewayId;
Review Comment:
Removing `required=true` changes the API contract. If the intent is to allow
either `gatewayId` or `nexthop`, the command should enforce (and clearly
document) that exactly one of the two is provided (and fail with a clear error
otherwise). As-is, clients may be able to submit requests without either
parameter depending on other validations.
##########
ui/src/views/compute/wizard/MultiNetworkSelection.vue:
##########
@@ -1,31 +1,22 @@
-// 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.
+// 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.
Review Comment:
Same issue as in ImportUnmanagedInstance.vue: the license header was
reformatted into invalid non-comment text (notably line 5 begins with `Unless
required...` without `//`). This will break compilation; please revert/restore
the standard multi-line `//` header.
```suggestion
// 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.
```
##########
ui/src/views/compute/wizard/MultiNetworkSelection.vue:
##########
@@ -107,6 +103,10 @@ export default {
type: String,
default: ''
},
+ projectid: {
+ type: String,
+ default: ''
+ },
Review Comment:
The PR description focuses on project-based network filtering for unmanaged
instance import, but this PR also includes broad unrelated changes across
backend Java classes, schema views, API annotations, multiple unrelated UI
pages, tests, and packaging files. This makes review and rollback riskier;
please consider splitting these unrelated changes into separate PRs (or justify
why they’re coupled to this fix).
##########
ui/src/views/compute/wizard/MultiNetworkSelection.vue:
##########
@@ -221,28 +224,37 @@ export default {
params.domainid = this.domainid
params.account = this.account
}
- getAPI('listNetworks', params).then(response => {
- this.networks = response.listnetworksresponse.network || []
- }).catch(() => {
- this.networks = []
- }).finally(() => {
- this.orderNetworks()
- this.loading = false
- })
+ if (this.projectid) {
+ params.projectid = this.projectid
+ }
Review Comment:
When `projectid` is provided, `domainid/account` are still sent if they’re
populated. CloudStack list APIs commonly treat `projectid` as mutually
exclusive with `domainid/account`, and sending both can lead to incorrect
filtering or API errors. Fix by building params so that when `projectid` is set
you omit `domainid` and `account` (and conversely only send `domainid/account`
when `projectid` is empty).
##########
ui/src/views/tools/ImportUnmanagedInstance.vue:
##########
@@ -1088,226 +1207,253 @@ export default {
var rootDisk = this.resource.disk[this.selectedRootDiskIndex]
rootDisk.size = rootDisk.capacity / (1024 * 1024 * 1024)
rootDisk.name = `${rootDisk.label} (${rootDisk.size} GB)`
- rootDisk.meta = this.getMeta(rootDisk, { controller: 'controller',
datastorename: 'datastore', position: 'position' })
+ rootDisk.meta = this.getMeta(rootDisk, {
+ controller: 'controller',
+ datastorename: 'datastore',
+ position: 'position'
+ })
this.selectedRootDiskSources = [rootDisk]
},
handleSubmit (e) {
e.preventDefault()
if (this.loading) return
- this.formRef.value.validate().then(() => {
- const values = toRaw(this.form)
- const params = {
- name: this.resource.name,
- clusterid: this.cluster.id,
- displayname: values.displayname,
- zoneid: this.zoneid,
- importsource: this.importsource,
- hypervisor: this.hypervisor,
- host: this.exthost,
- hostname: values.hostname,
- username: this.username,
- password: this.password,
- hostid: this.host.id,
- storageid: this.pool.id,
- diskpath: this.diskpath,
- temppath: this.tmppath
- }
- var importapi = 'importUnmanagedInstance'
- if (this.isExternalImport || this.isDiskImport ||
this.selectedVmwareVcenter) {
- importapi = 'importVm'
- if (this.isDiskImport) {
- if (!values.networkid) {
+ this.formRef.value
+ .validate()
+ .then(() => {
+ const values = toRaw(this.form)
+ const params = {
+ name: this.resource.name,
+ clusterid: this.cluster.id,
+ displayname: values.displayname,
+ zoneid: this.zoneid,
+ importsource: this.importsource,
+ hypervisor: this.hypervisor,
+ host: this.exthost,
+ hostname: values.hostname,
+ username: this.username,
+ password: this.password,
+ hostid: this.host.id,
+ storageid: this.pool.id,
+ diskpath: this.diskpath,
+ temppath: this.tmppath
+ }
+ var importapi = 'importUnmanagedInstance'
+ if (this.isExternalImport || this.isDiskImport ||
this.selectedVmwareVcenter) {
+ importapi = 'importVm'
+ if (this.isDiskImport) {
+ if (!values.networkid) {
+ this.$notification.error({
+ message: this.$t('message.request.failed'),
+ description: this.$t('message.please.enter.valid.value') +
': ' + this.$t('label.network')
+ })
+ return
+ }
+ params.name = values.displayname
+ params.networkid = values.networkid
+ }
+ }
+ if (!this.computeOffering || !this.computeOffering.id) {
+ this.$notification.error({
+ message: this.$t('message.request.failed'),
+ description: this.$t('message.step.2.continue')
+ })
+ return
+ }
+ params.serviceofferingid = values.computeofferingid
+ if (this.computeOffering.iscustomized) {
+ var details = [this.cpuNumberKey, this.cpuSpeedKey, this.memoryKey]
+ for (var detail of details) {
+ if (!(values[detail] || this.computeOffering[detail])) {
+ this.$notification.error({
+ message: this.$t('message.request.failed'),
+ description:
+ this.$t('message.please.enter.valid.value') + ': ' +
this.$t('label.' + detail.toLowerCase())
+ })
+ return
+ }
+ if (values[detail]) {
+ params['details[0].' + detail] = values[detail]
+ }
+ }
+ }
+ if (this.computeOffering.iscustomizediops) {
+ var iopsDetails = [this.minIopsKey, this.maxIopsKey]
+ for (var iopsDetail of iopsDetails) {
+ if (!values[iopsDetail] || values[iopsDetail] < 0) {
+ this.$notification.error({
+ message: this.$t('message.request.failed'),
+ description:
+ this.$t('message.please.enter.valid.value') + ': ' +
this.$t('label.' + iopsDetail.toLowerCase())
+ })
+ return
+ }
+ params['details[0].' + iopsDetail] = values[iopsDetail]
+ }
+ if (values[this.minIopsKey] > values[this.maxIopsKey]) {
this.$notification.error({
message: this.$t('message.request.failed'),
- description: this.$t('message.please.enter.valid.value') + ':
' + this.$t('label.network')
+ description: this.$t('error.form.message')
})
Review Comment:
The validation error for `minIops > maxIops` no longer stops submission.
After showing the notification, the flow continues and will still call the
import API. Add an early `return` after the notification so invalid IOPS values
cannot proceed.
```suggestion
})
return
```
##########
ui/src/views/compute/wizard/SecurityGroupSelection.vue:
##########
@@ -140,9 +160,9 @@ export default {
methods: {
fetchData () {
const params = {
- projectid: this.$store.getters.project ?
this.$store.getters.project.id : null,
- domainid: this.$store.getters.project &&
this.$store.getters.project.id ? null : this.$store.getters.userInfo.domainid,
- account: this.$store.getters.project && this.$store.getters.project.id
? null : this.$store.getters.userInfo.account,
+ projectid: this.projectId || (this.$store.getters.project ?
this.$store.getters.project.id : null),
+ domainid: this.projectId || (this.$store.getters.project &&
this.$store.getters.project.id) ? null : (this.domainId ||
this.$store.getters.userInfo.domainid),
+ account: this.projectId || (this.$store.getters.project &&
this.$store.getters.project.id) ? null : (this.account ||
this.$store.getters.userInfo.account),
Review Comment:
The `domainid`/`account` ternary expressions rely on operator precedence
(`||` with `?:`) and are hard to read/verify. Refactor to compute a boolean
like `usingProject = Boolean(this.projectId || (this.$store.getters.project &&
this.$store.getters.project.id))` and then set `domainid/account` explicitly
based on that. This will prevent subtle precedence mistakes and make future
changes safer.
--
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]
