remove instanceof checks and use reflection for admin cmd invocations

apache / cloudstack

Apache CloudStack is an opensource Infrastructure as a Service (IaaS) cloud computing platform

https://cloudstack.apache.org/

Apache License 2.0

2.06k stars 1.1k forks source link

remove instanceof checks and use reflection for admin cmd invocations #6966

Open DaanHoogland opened 1 year ago

DaanHoogland commented 1 year ago

ISSUE TYPE

cleanup

COMPONENT NAME

API

CLOUDSTACK VERSION

4.17

CONFIGURATION

OS / ENVIRONMENT

SUMMARY

the API classes have instances that are for root admin only, these are checked for using instanceof in the service layer. This should be cleaned up in favour for reflection invocations as in #6951. It would be better to do all in one go but the fix in #6951 really needs it and is for a point release, so postponing.

STEPS TO REPRODUCE

EXPECTED RESULTS

ACTUAL RESULTS

rohityadavcloud commented 1 year ago

This may not be necessary generally; the metrics API extend relevant list APIs (for example, listVirtualMachines and listVirtualMachinesMetrics) and because of this the reflections-based pattern was used.

Ideally one could have implemented that by implementing a interface that both the listVirtualMachines and listVirtualMachinesMetrics implements, and checking using instanceof on that interface; this would have introduced a static type checking and avoided the dynamic/runtime behaviour that reflection based code (generally) introduces.

shwstppr commented 1 year ago

@DaanHoogland do you plan to work on this for 4.19 or can this moved to the next milestone?

weizhouapache commented 2 months ago

@DaanHoogland is the issue related to #6951 ? (according to the summary)

DaanHoogland commented 2 months ago

@DaanHoogland is the issue related to #6951 ? (according to the summary)

by the looks of it this is a mis-reference . This is about using if (cmd isinstance of ..CmdForAdmin) conditions in the service layer. extremely low priority, but a cleanup we may have to consider sometime.