[GitHub] [cloudstack] shwstppr commented on a change in pull request #4675: Bug fix in displaying public IP address of shared networks

[GitBox <gi...@apache.org>]

shwstppr commented on a change in pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#discussion_r596611620



##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -2004,42 +2023,101 @@ private void abstractDataStoreClustersList(List<StoragePool> storagePools, List<
 
     @Override
     public Pair<List<? extends IpAddress>, Integer> searchForIPAddresses(final ListPublicIpAddressesCmd cmd) {
-        final Object keyword = cmd.getKeyword();
-        final Long physicalNetworkId = cmd.getPhysicalNetworkId();
         final Long associatedNetworkId = cmd.getAssociatedNetworkId();
-        final Long sourceNetworkId = cmd.getNetworkId();
         final Long zone = cmd.getZoneId();
-        final String address = cmd.getIpAddress();
         final Long vlan = cmd.getVlanId();
         final Boolean forVirtualNetwork = cmd.isForVirtualNetwork();
-        final Boolean forLoadBalancing = cmd.isForLoadBalancing();
         final Long ipId = cmd.getId();
-        final Boolean sourceNat = cmd.isSourceNat();
-        final Boolean staticNat = cmd.isStaticNat();
+        final Long networkId = cmd.getNetworkId();
         final Long vpcId = cmd.getVpcId();
-        final Boolean forDisplay = cmd.getDisplay();
-        final Map<String, String> tags = cmd.getTags();
 
         final String state = cmd.getState();
         Boolean isAllocated = cmd.isAllocatedOnly();
         if (isAllocated == null) {
-            isAllocated = Boolean.TRUE;
-
-            if (state != null) {
+            if (state != null && state.equalsIgnoreCase(IpAddress.State.Free.name())) {
                 isAllocated = Boolean.FALSE;
+            } else {
+                isAllocated = Boolean.TRUE; // default
+            }
+        } else {
+            if (state != null && state.equalsIgnoreCase(IpAddress.State.Free.name())) {
+                if (isAllocated) {
+                    throw new InvalidParameterValueException("Conflict: allocatedonly is true but state is Free");
+                }
+            } else if (state != null && state.equalsIgnoreCase(IpAddress.State.Allocated.name())) {
+                isAllocated = Boolean.TRUE;

Review comment:
       @ravening what if API was passed `allocatedonly=False` and `state=Allocated`? Shouldn't we raise exception like above case (allocatedonly=true & state=Free)
   
   This block can be simplified (or maybe complicated :smile:) with something like,
   ```
           if (Boolean.TRUE.equals(isAllocated) && IpAddress.State.Free.name().equals(state)) {
               throw new InvalidParameterValueException("Conflict: allocatedonly is true but state is Free");
           }
           if (Boolean.FALSE.equals(isAllocated) && IpAddress.State.Allocated.name().equals(state)) {
               throw new InvalidParameterValueException("Conflict: allocatedonly is false but state is Allocated");
           }
           if (isAllocated == null) {
               isAllocated = !IpAddress.State.Free.name().equals(state);
           }
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org