Copilot commented on code in PR #12336:
URL: https://github.com/apache/cloudstack/pull/12336#discussion_r3528457716
##########
api/src/main/java/org/apache/cloudstack/context/CallContext.java:
##########
@@ -135,6 +141,25 @@ public Account getCallingAccount() {
return account;
}
+ public boolean isCallingAccountRootAdmin() {
+ if (isAccountRootAdmin != null) {
+ return isAccountRootAdmin;
+ }
+ if (account == null && s_entityMgr == null) {
+ return false;
+ }
+ Account caller = getCallingAccount();
+ AccountService accountService;
+ try {
+ accountService =
ComponentContext.getDelegateComponentOfType(AccountService.class);
+ } catch (NoSuchBeanDefinitionException e) {
+ LOGGER.warn("Falling back to account type check for isRootAdmin
for account ID: {} as no AccountService bean found: {}", accountId,
e.getMessage());
+ return caller != null && caller.getType() == Account.Type.ADMIN;
+ }
Review Comment:
In the fallback path (when AccountService bean is missing), the computed
result is not cached into isAccountRootAdmin. This can cause repeated WARN logs
and repeated computation for the same request/thread each time
isCallingAccountRootAdmin() is called.
##########
test/integration/smoke/test_deploy_vm_root_resize.py:
##########
@@ -395,11 +395,10 @@ def test_02_deploy_vm_root_resize(self):
rootdisksize=newrootsize
)
except Exception as ex:
- if "rootdisksize override (" + str(newrootsize) + " GB) is
smaller than template size" in str(ex):
- success = True
- else:
- self.debug("Virtual machine create did not fail
appropriately. Error was actually : " + str(ex));
-
+ if "Root disk size override (" + str(newrootsize) + " GB) is
smaller than the template size" in str(ex):
+ success = True
+ else:
+ self.debug("Virtual machine create did not fail appropriately.
Error was actually : " + str(ex))
Review Comment:
This section has invalid indentation (the `if` under `except` is not
indented) and the `VirtualMachine.create(...)` call is not properly
closed/indented, which makes the test file fail to parse (SyntaxError).
##########
api/src/main/java/org/apache/cloudstack/context/ResponseMessageResolver.java:
##########
@@ -0,0 +1,406 @@
+// 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.context;
+
+import java.io.File;
+import java.io.InputStream;
+import java.lang.reflect.InvocationTargetException;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.apache.cloudstack.api.Identity;
+import org.apache.cloudstack.api.InternalIdentity;
+import org.apache.cloudstack.api.response.ExceptionResponse;
+import org.apache.commons.collections.MapUtils;
+import org.apache.commons.lang3.ObjectUtils;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import com.cloud.utils.PropertiesUtil;
+import com.cloud.utils.Ternary;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+public class ResponseMessageResolver {
+ private static final Logger LOG =
+ LogManager.getLogger(ResponseMessageResolver.class);
+
+ protected static final String ERROR_MESSAGES_FILENAME =
"error-messages.json";
+ protected static final String ERROR_KEY_ADMIN_SUFFIX = ".admin";
+ protected static final boolean USE_RESOURCE_TO_STRING_IN_METADATA = false;
+ protected static final boolean INCLUDE_RESOURCE_ID_FOR_ADMINS_IN_METADATA
= true;
+
+ private static final Pattern VARIABLE_PATTERN =
Pattern.compile("\\{\\{\\s*([A-Za-z0-9_]+)\\s*}}");
+ private static final List<String> RESOURCE_NAME_GETTERS =
+ Arrays.asList("getDisplayText", "getDisplayName", "getName");
+
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+
+ // volatile for safe publication
+ private static volatile Map<String, String> templates =
+ Collections.emptyMap();
+
+ private static volatile long lastModified = -1;
+
+ private ResponseMessageResolver() {
+ }
+
+ /**
+ * Clears the cached templates and last modified timestamp.
+ * Useful for testing to ensure cache isolation between tests.
+ */
+ protected static synchronized void clearCache() {
+ templates = Collections.emptyMap();
+ lastModified = -1;
+ }
+
+ protected static List<String> getVariableNamesInErrorKey(String template) {
+ if (template == null || template.isEmpty()) {
+ return Collections.emptyList();
+ }
+ List<String> variables = new ArrayList<>();
+ Matcher matcher = VARIABLE_PATTERN.matcher(template);
+ while (matcher.find()) {
+ String name = matcher.group(1);
+ if (name != null && !name.isEmpty()) {
+ variables.add(name);
+ }
+ }
+ return variables;
+ }
+
+ protected static Map<String, Object>
getCombinedMetadataFromErrorTemplate(String template,
+
Map<String, Object> metadata) {
+ return getCombinedMetadataFromErrorTemplate(template, metadata, false);
+ }
+
+ protected static Map<String, Object>
getCombinedMetadataFromErrorTemplate(String template,
+
Map<String, Object> metadata,
+
boolean onlyForTemplateVariables) {
+ List<String> variableNames = getVariableNamesInErrorKey(template);
+ if (variableNames.isEmpty()) {
+ return onlyForTemplateVariables ? Collections.emptyMap() :
metadata;
+ }
+ Map<String, Object> contextMetadata =
CallContext.current().getErrorContextParameters();
+ Map<String, Object> combinedMetadata = new LinkedHashMap<>();
+ if (MapUtils.isNotEmpty(contextMetadata)) {
+ for (String varName : variableNames) {
+ if (contextMetadata.containsKey(varName)) {
+ combinedMetadata.put(varName,
contextMetadata.get(varName));
+ }
+ }
+ }
+ if (MapUtils.isNotEmpty(metadata)) {
+ combinedMetadata.putAll(metadata);
+ }
+ if (onlyForTemplateVariables) {
+ combinedMetadata.entrySet().removeIf(x ->
!variableNames.contains(x.getKey()));
+ }
+ return combinedMetadata;
+ }
+
+ protected static String getTemplateForKey(String errorKey) {
+ if (errorKey == null) {
+ return null;
+ }
+ reloadIfRequired();
+ if (!errorKey.endsWith(ERROR_KEY_ADMIN_SUFFIX) &&
CallContext.current().isCallingAccountRootAdmin()) {
+ String template = templates.get(errorKey + ERROR_KEY_ADMIN_SUFFIX);
+ if (template != null) {
+ return template;
+ }
+ }
+ return templates.get(errorKey);
+ }
+
+ protected static boolean useResourceToStringInMetadata() {
+ // ToDo: Add a global config
+ return USE_RESOURCE_TO_STRING_IN_METADATA;
+ }
+
+ protected static Map<String, String> getStringMap(Map<String, Object>
metadata) {
+ boolean isAdmin = CallContext.current().isCallingAccountRootAdmin();
+ return getStringMap(metadata, isAdmin);
+ }
+
+ protected static Map<String, String> getStringMap(Map<String, Object>
metadata, boolean isAdmin) {
+ Map<String, String> stringMap = new LinkedHashMap<>();
+ if (MapUtils.isEmpty(metadata)) {
+ return stringMap;
+ }
+ for (Map.Entry<String, Object> entry : metadata.entrySet()) {
+ Object value = entry.getValue();
+ stringMap.put(entry.getKey(),
+ useResourceToStringInMetadata() ?
+ getMetadataObjectStringValueAlt(value, isAdmin) :
+ getMetadataObjectStringValue(value, isAdmin));
+ }
+ return stringMap;
+ }
+
+ public static Map<String, Object> convertToStringMap(Map<String, Object>
metadata) {
+ Map<String, String> stringMap = getStringMap(metadata, false);
+ return new LinkedHashMap<>(stringMap);
+ }
+
+
+
+ /**
+ * Converts a metadata object to a human-readable string for error
messages.
+ *
+ * <p>Behavior:
+ * <ul>
+ * <li>If {@code obj} is {@code null}, returns {@code null}.</li>
+ * <li>Attempts to obtain a display name by invoking one of the getters
+ * {@code getDisplayText()}, {@code getDisplayName()}, or {@code
getName()} via reflection.
+ * If a name is found, returns it quoted as {@code 'NAME'}.</li>
+ * <li>When the current calling account is a root admin, the returned
value will include
+ * an identifier suffix in the form {@code (ID: id, UUID: uuid)}
when available.
+ * The ID is included only if {@code INCLUDE_METADATA_ID_IN_MESSAGE}
is {@code true}
+ * and {@code obj} implements {@link InternalIdentity}. The UUID is
included when
+ * {@code obj} implements {@link
org.apache.cloudstack.api.Identity}.</li>
+ * <li>If no display name is available, returns the UUID (if {@code obj}
implements
+ * {@code Identity}); otherwise returns {@code obj.toString()}.</li>
+ * </ul>
+ *
+ * <p>Reflection is used to call getters; invocation failures are silently
ignored and treated as
+ * absence of the corresponding value.
+ *
+ * @param obj metadata object
+ * @param isAdmin true when the caller is a root admin and admin-only
details may be included
+ * @return formatted metadata string suitable for inclusion in error
messages, or {@code null}
+ * if {@code obj} is {@code null}
+ */
+ protected static String getMetadataObjectStringValue(Object obj, boolean
isAdmin) {
+ if (obj == null) {
+ return null;
+ }
+ String uuid = null;
+ if (obj instanceof Identity) {
+ uuid = ((Identity) obj).getUuid();
+ }
+ String name = null;
+ for (String getter : RESOURCE_NAME_GETTERS) {
+ name = invokeStringGetter(obj, getter);
+ if (name != null) {
+ break;
+ }
+ }
+ if (StringUtils.isEmpty(name)) {
+ if (StringUtils.isNotEmpty(uuid)) {
+ return uuid;
+ }
+ return obj.toString();
+ }
+
+ StringBuilder sb = new StringBuilder();
+ sb.append(name);
+
+ Long id = null;
+ if (obj instanceof InternalIdentity && isAdmin) {
+ id = ((InternalIdentity) obj).getId();
+ }
Review Comment:
INCLUDE_RESOURCE_ID_FOR_ADMINS_IN_METADATA is currently unused: admin
callers will always get the internal DB id included in metadata whenever the
object implements InternalIdentity. Either remove the constant or (preferably)
apply it to the id-inclusion condition so the behavior is actually
configurable/documentable.
--
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]
