-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Create new generic method for resource UUID obtention in event's descriptions #12502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Create new generic method for resource UUID obtention in event's descriptions #12502
Conversation
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
3e77924 to
6f16482
Compare
|
@erikbocks, it seems that there are some checkstyle errors: Error: Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0:check (cloudstack-checkstyle) on project cloud-api: Failed during checkstyle execution: There is 1 error reported by Checkstyle 8.18 with cloud-style.xml ruleset. -> [Help 1] |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12502 +/- ##
=========================================
Coverage 17.85% 17.85%
- Complexity 15993 15996 +3
=========================================
Files 5929 5929
Lines 531155 531176 +21
Branches 64921 64929 +8
=========================================
+ Hits 94830 94838 +8
- Misses 425709 425720 +11
- Partials 10616 10618 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the hard work and description. It seems like a good solution. I will try to get through all the small changes. |
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small remarks, clgtm generally, needs a lot of trivial testing.
api/src/main/java/org/apache/cloudstack/api/command/admin/account/DisableAccountCmd.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/command/admin/account/DisableAccountCmd.java
Outdated
Show resolved
Hide resolved
...rk-elements/palo-alto/src/main/java/com/cloud/api/commands/ConfigurePaloAltoFirewallCmd.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/command/user/vpn/DeleteRemoteAccessVpnCmd.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/command/user/vpc/RestartVPCCmd.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/command/user/vpc/DeleteStaticRouteCmd.java
Outdated
Show resolved
Hide resolved
| public String getResourceUuid(String parameterName) { | ||
| String resourceUuid = CallContext.current().getApiResourceUuid(parameterName); | ||
|
|
||
| if (resourceUuid != null && UuidUtils.isUuid(resourceUuid)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can check isUuid() while putting in the map or use Map<String, UUID> to keep them?
| private User user; | ||
| private long userId; | ||
| private final Map<Object, Object> context = new HashMap<Object, Object>(); | ||
| private final Map<String, String > apiResourcesUuids = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private final Map<String, String > apiResourcesUuids = new HashMap<>(); | |
| private final Map<String, String> apiResourcesUuids = new HashMap<>(); |
can use Map<String, UUID> ?
| CallContext.current().putContextParameter(entity, internalId); | ||
| final Object objVO = _entityMgr.findByIdIncludingRemoved(entity, internalId); | ||
| if (!(objVO instanceof Identity)) { | ||
| continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What event description is logged for create/add calls (UUID might not exist)?
| // Populate CallContext for each of the entity. | ||
| for (final Class<?> entity : entities) { | ||
| CallContext.current().putContextParameter(entity, internalId); | ||
| final Object objVO = _entityMgr.findByIdIncludingRemoved(entity, internalId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can add to Map here, instead of querying db again.
cloudstack/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java
Lines 264 to 268 in 420bf6d
| final Object entityObj = _entityMgr.findById(entity, entityId); | |
| if(entityObj != null){ | |
| entitiesToAccess.put(entityObj, checkAccess.accessType()); | |
| break; | |
| } |
cloudstack/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java
Lines 287 to 291 in 420bf6d
| final Object entityObj = _entityMgr.findById(entity, (Long) field.get(cmd)); | |
| if(entityObj != null){ | |
| entitiesToAccess.put(entityObj, checkAccess.accessType()); | |
| break; | |
| } |
Description
Currently, API parameters that have
UUIDas their type either accept the resource's UUID, or their internal ID directly. If the resource UUID was provided, a workflow responsible for processing the parameters of the APIs automatically translates it to the resource's internal ID before it reaches the API level. However, some APIs events descriptions are created after the translation process. If the command wishes to present which resource is being interacted with, the translation process causes API events descriptions to expose the resource's internal ID to the final user.As a workaround, it is possible to use the
UuidMgr.getUuid(Class<T> entityType, Long customId)method to get the resource's UUID. Even though this method can be used, it is very verbose and makes an additional call to the database. Thus, in order to solve this issue, thegetResourceUuid(String parameterName)method was created.This method belongs to the
BaseCmdclass, allowing all current and future API classes to inherit it. To store the resources UUIDs, theapiResourcesUuidsHashMap was added to theCallContextclass. This map is populated during the parameter processing, that occurs at theParamProcessWorker#translateUuidToInternalId(final String uuid, final Parameter annotation)method, leveraging the UUID translation process to eliminate the additional database call. For each UUID type parameter sent, the map is populated with the parameter's name as the map key, and the resource's UUID as its value. If the provided key is not found in the map, or the value found is not a UUID, the method returns anullvalue.This PR also refactors events descriptions, aiming to enhance them to be more descriptive or only to implement the new
getResourceUuid()method. The method calling was standardized by using theApiConstantsattributes as theparameterNamevalue.Fixes: #11859
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
After applying the updated packages to my local environment, I called the
disable accountAPI, informing theidparameter as the account's UUID, and validated that the created event contained the resource UUID instead of the database ID. The same process was repeated informing the resource's database ID as the parameter value, and the correct behavior was observed. By using a debugger, I also validated that the parameter processing flow as executing the correct validations and storing the values correctly in the HashMap.The codebase tests were also successfully executed.