Copilot commented on code in PR #12737: URL: https://github.com/apache/cloudstack/pull/12737#discussion_r3459262187
########## api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java: ########## @@ -0,0 +1,150 @@ +// 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.user.dns; + +import java.util.List; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.acl.SecurityChecker; +import org.apache.cloudstack.api.ACL; +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.DnsServerResponse; +import org.apache.cloudstack.dns.DnsServer; +import org.apache.commons.lang3.StringUtils; + +import com.cloud.user.Account; +import com.cloud.utils.EnumUtils; + +@APICommand(name = "updateDnsServer", + description = "Update DNS server", + responseObject = DnsServerResponse.class, + entityType = {DnsServer.class}, + requestHasSensitiveInfo = true, responseHasSensitiveInfo = false, + since = "4.23.0", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) +public class UpdateDnsServerCmd extends BaseCmd { + + ///////////////////////////////////////////////////// + //////////////// API parameters ///////////////////// + ///////////////////////////////////////////////////// + + @ACL(accessType = SecurityChecker.AccessType.OperateEntry) + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = DnsServerResponse.class, + required = true, description = "The ID of the DNS server to update") + private Long id; + + @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "Name of the DNS server") + private String name; + + @Parameter(name = ApiConstants.URL, type = CommandType.STRING, description = "API URL of the provider") + private String url; + + @Parameter(name = ApiConstants.DNS_API_KEY, type = CommandType.STRING, description = "API Key or Credentials for the external provider") + private String dnsApiKey; + + @Parameter(name = ApiConstants.PORT, type = CommandType.INTEGER, description = "Port number of the external DNS server") + private Integer port; + + @Parameter(name = ApiConstants.IS_PUBLIC, type = CommandType.BOOLEAN, + description = "Whether this DNS server can be used by accounts other than the owner to create and manage DNS zones") + private Boolean isPublic; + + @Parameter(name = ApiConstants.PUBLIC_DOMAIN_SUFFIX, type = CommandType.STRING, + description = "Domain suffix that restricts DNS zones created by non-owner accounts to subdomains of this " + + "suffix (for example, sub.example.com under example.com)") + private String publicDomainSuffix; + + @Parameter(name = ApiConstants.NAME_SERVERS, type = CommandType.LIST, collectionType = CommandType.STRING, + description = "Comma separated list of name servers; used to create NS records for the DNS Zone (for example, ns1.example.com, ns2.example.com)") + private List<String> nameServers; + + @Parameter(name = ApiConstants.STATE, type = CommandType.STRING, description = "Update state for the DNS server (Enabled, Disabled)") + private String state; + + ///////////////////////////////////////////////////// + /////////////////// Accessors /////////////////////// + ///////////////////////////////////////////////////// + + public Long getId() { return id; } + public String getName() { return name; } + public String getUrl() { return url; } + public String getDnsApiKey() { + return dnsApiKey; + } + public Integer getPort() { + return port; + } + public Boolean isPublic() { + return isPublic; + } + public String getPublicDomainSuffix() { + return publicDomainSuffix; + } + public String getNameServers() { + if (nameServers == null) { + return null; + } + return StringUtils.join(nameServers.stream() + .filter(StringUtils::isNotBlank) + .map(StringUtils::trim) + .toArray(String[]::new), ","); + } + + @Override + public long getEntityOwnerId() { + DnsServer server = _entityMgr.findById(DnsServer.class, id); + if (server != null) { + return server.getAccountId(); + } + // If server not found, return System to fail safely (or let manager handle 404) + return Account.ACCOUNT_ID_SYSTEM; + } + + @Override + public void execute() { + try { + DnsServer server = dnsProviderManager.updateDnsServer(this); + if (server != null) { + DnsServerResponse response = dnsProviderManager.createDnsServerResponse(server); + response.setResponseName(getCommandName()); + setResponseObject(response); + } else { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update DNS server"); + } + } catch (Exception ex) { + logger.error("Failed to add update server", ex); Review Comment: This log message is confusing/incorrect ('add update server'). Update it to accurately describe the operation (e.g., 'Failed to update DNS server') so operational logs are clearer during incident triage. ########## api/src/main/java/org/apache/cloudstack/api/response/DnsServerResponse.java: ########## @@ -0,0 +1,133 @@ +// 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.response; + +import java.util.List; + +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.BaseResponse; +import org.apache.cloudstack.api.EntityReference; +import org.apache.cloudstack.dns.DnsServer; + +import com.cloud.serializer.Param; +import com.google.gson.annotations.SerializedName; + +@EntityReference(value = DnsServer.class) +public class DnsServerResponse extends BaseResponse { + + @SerializedName(ApiConstants.ID) + @Param(description = "ID of the DNS server") + private String id; + + @SerializedName(ApiConstants.NAME) + @Param(description = "Name of the DNS server") + private String name; + + @SerializedName(ApiConstants.URL) + @Param(description = "URL of the DNS server API") + private String url; + + @SerializedName(ApiConstants.PORT) + @Param(description = "The port of the DNS server") + private Integer port; + + @SerializedName(ApiConstants.PROVIDER) + @Param(description = "The provider type of the DNS server") + private String provider; + + @SerializedName(ApiConstants.IS_PUBLIC) + @Param(description = "Is the DNS server publicly available") + private Boolean isPublic; + + @SerializedName(ApiConstants.PUBLIC_DOMAIN_SUFFIX) + @Param(description = "The public domain suffix for the DNS server") + private String publicDomainSuffix; + + @SerializedName(ApiConstants.NAME_SERVERS) + @Param(description = "Name servers entries associated to DNS server") + private List<String> nameServers; + + @SerializedName(ApiConstants.ACCOUNT) + @Param(description = "the account associated with the DNS server") + private String accountName; + + @SerializedName(ApiConstants.DOMAIN_ID) + @Param(description = "the ID of the domain associated with the DNS server") + private String domainId; + + @SerializedName(ApiConstants.DOMAIN) + @Param(description = "the name of the domain associated with the DNS server") + private String domainName; + + @SerializedName(ApiConstants.STATE) + @Param(description = "The state of the account") + private String state; + + public DnsServerResponse() { + super(); + + } Review Comment: Unlike `DnsZoneResponse`/`DnsRecordResponse`, this response constructor does not set an `objectName` (e.g., `dnsserver`). Several parts of CloudStack rely on object names for consistent API serialization/wrapping; consider calling `setObjectName(\"dnsserver\")` in the constructor to keep responses consistent across list and non-list APIs. -- 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]
