Address review comments

Author: sudo87

api/src/main/java/org/apache/cloudstack/api/ApiConstants.java

@@ -524,7 +524,7 @@ public class ApiConstants {
     public static final String INSTANCE_LEASE_DURATION = "leaseduration";
     public static final String INSTANCE_LEASE_EXPIRY_DATE= "leaseexpirydate";
     public static final String INSTANCE_LEASE_EXPIRY_ACTION = "leaseexpiryaction";
-    public static final String ONLY_LEASED_INSTANCES = "onlyleasedinstances";
+    public static final String LEASED = "leased";
 
     public static final String USER_DATA_NAME = "userdataname";
     public static final String USER_DATA_ID = "userdataid";

api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java

@@ -257,7 +257,7 @@ public class CreateServiceOfferingCmd extends BaseCmd {
     private Long leaseDuration;
 
     @Parameter(name = ApiConstants.INSTANCE_LEASE_EXPIRY_ACTION, type = CommandType.STRING, since = "4.21.0",
-            description = "Action to be taken on lease expiration, valid values are STOP and DESTROY")
+            description = "Lease expiry action, valid values are STOP and DESTROY")
     private String leaseExpiryAction;
 
     /////////////////////////////////////////////////////

api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java

@@ -283,7 +283,7 @@ public class DeployVMCmd extends BaseAsyncCreateCustomIdCmd implements SecurityG
     private Long leaseDuration;
 
     @Parameter(name = ApiConstants.INSTANCE_LEASE_EXPIRY_ACTION, type = CommandType.STRING, since = "4.21.0",
-            description = "Lease expiry action")
+            description = "Lease expiry action, valid values are STOP and DESTROY")
     private String leaseExpiryAction;
 
     /////////////////////////////////////////////////////

api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java

@@ -149,8 +149,8 @@ public class ListVMsCmd extends BaseListRetrieveOnlyResourceCountCmd implements
     @Parameter(name = ApiConstants.USER_DATA, type = CommandType.BOOLEAN, description = "Whether to return the VMs' user data or not. By default, user data will not be returned.", since = "4.18.0.0")
     private Boolean showUserData;
 
-    @Parameter(name = ApiConstants.ONLY_LEASED_INSTANCES, type = CommandType.BOOLEAN,
-            description = "Whether to return the Leased instances",
+    @Parameter(name = ApiConstants.LEASED, type = CommandType.BOOLEAN,
+            description = "Whether to return only leased instances",
             since = "4.21.0")
     private Boolean onlyLeasedInstances = false;
 
@@ -328,9 +328,7 @@ protected void updateVMResponse(List<UserVmResponse> response) {
         }
     }
 
-
     public Boolean getOnlyLeasedInstances() {
         return onlyLeasedInstances;
     }
-
 }

api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java

@@ -159,7 +159,7 @@ public class UpdateVMCmd extends BaseCustomIdCmd implements SecurityGroupAction,
     private Long leaseDuration;
 
     @Parameter(name = ApiConstants.INSTANCE_LEASE_EXPIRY_ACTION, type = CommandType.STRING, since = "4.21.0",
-            description = "Lease expiry action")
+            description = "Lease expiry action, valid values are STOP and DESTROY")
     private String leaseExpiryAction;
 
     /////////////////////////////////////////////////////

api/src/main/java/org/apache/cloudstack/api/response/UserVmResponse.java

@@ -396,7 +396,7 @@ public class UserVmResponse extends BaseResponseWithTagInformation implements Co
     private String vmType;
 
     @SerializedName(ApiConstants.INSTANCE_LEASE_DURATION)
-    @Param(description = "Instance lease duration", since = "4.21.0")
+    @Param(description = "Instance lease duration in days", since = "4.21.0")
     private Long leaseDuration;
 
     @SerializedName(ApiConstants.INSTANCE_LEASE_EXPIRY_DATE)

engine/schema/src/main/resources/META-INF/db/schema-42010to42100.sql

@@ -359,6 +359,24 @@ public J and() {
         return (J)this;
     }
 
+    /**
+     * Adds an AND NOT condition to the search.  Normally you should use this to
+     * perform an 'AND NOT' with a big conditional in parenthesis.  For example,
+     *
+     * search.andNot().op(entity.getId(), Op.Eq, "abc").cp()
+     *
+     * The above fragment produces something similar to
+     *
+     * "AND NOT (id = $abc) where abc is the token to be replaced by a value later.
+     *
+     * @return this
+     */
+    @SuppressWarnings("unchecked")
+    public J andNot() {
+        constructCondition(null, " AND NOT ", null, null);
+        return (J)this;
+    }
+
     /**
      * Closes a parenthesis that's started by op()
      * @return this

framework/db/src/main/java/com/cloud/utils/db/SearchBase.java

@@ -1329,6 +1329,11 @@ private Pair<List<Long>, Integer> searchForUserVMIdsAndCount(ListVMsCmd cmd) {
             }
         }
 
+        boolean requestingOnlyLeasedInstances = cmd.getOnlyLeasedInstances() != null && cmd.getOnlyLeasedInstances();
+        if (!VMLeaseManagerImpl.InstanceLeaseEnabled.value() && requestingOnlyLeasedInstances) {
+            throw new InvalidParameterValueException("Enable lease feature to use leased=true");
+        }
+
         Ternary<Long, Boolean, ListProjectResourcesCriteria> domainIdRecursiveListProject = new Ternary<>(cmd.getDomainId(), cmd.isRecursive(), null);
         accountMgr.buildACLSearchParameters(caller, id, cmd.getAccountName(), cmd.getProjectId(), permittedAccounts, domainIdRecursiveListProject, listAll, false);
         Long domainId = domainIdRecursiveListProject.first();
@@ -1478,14 +1483,11 @@ private Pair<List<Long>, Integer> searchForUserVMIdsAndCount(ListVMsCmd cmd) {
             userVmSearchBuilder.join("tags", resourceTagSearch, resourceTagSearch.entity().getResourceId(), userVmSearchBuilder.entity().getId(), JoinBuilder.JoinType.INNER);
         }
 
-        if (cmd.getOnlyLeasedInstances() != null && cmd.getOnlyLeasedInstances()) {
-            if (VMLeaseManagerImpl.InstanceLeaseEnabled.value()) {
-                SearchBuilder<UserVmDetailVO> leasedInstancesSearch = userVmDetailsDao.createSearchBuilder();
-                leasedInstancesSearch.and(leasedInstancesSearch.entity().getName(), SearchCriteria.Op.EQ).values(VmDetailConstants.INSTANCE_LEASE_EXPIRY_DATE);
-                userVmSearchBuilder.join("userVmToLeased", leasedInstancesSearch, leasedInstancesSearch.entity().getResourceId(), userVmSearchBuilder.entity().getId(), JoinBuilder.JoinType.INNER);
-            } else {
-                logger.warn("Lease feature is not enabled, onlyleasedinstances will be considered as false");
-            }
+        if (requestingOnlyLeasedInstances) {
+            SearchBuilder<UserVmDetailVO> leasedInstancesSearch = userVmDetailsDao.createSearchBuilder();
+            leasedInstancesSearch.and(leasedInstancesSearch.entity().getName(), SearchCriteria.Op.EQ).values(VmDetailConstants.INSTANCE_LEASE_EXPIRY_DATE);
+            userVmSearchBuilder.join("userVmToLeased", leasedInstancesSearch, leasedInstancesSearch.entity().getResourceId(),
+                    userVmSearchBuilder.entity().getId(), JoinBuilder.JoinType.INNER);
         }
 
         if (keyPairName != null) {

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

@@ -177,7 +177,7 @@ public ServiceOfferingResponse newServiceOfferingResponse(ServiceOfferingJoinVO
             }
         }
 
-        if (VMLeaseManagerImpl.InstanceLeaseEnabled.value() && offering.getLeaseDuration() != null && offering.getLeaseDuration() >= -1L) {
+        if (VMLeaseManagerImpl.InstanceLeaseEnabled.value() && offering.getLeaseDuration() != null && offering.getLeaseDuration() > 0L) {
             offeringResponse.setLeaseDuration(offering.getLeaseDuration());
             offeringResponse.setLeaseExpiryAction(offering.getLeaseExpiryAction());
         }