ravening commented on a change in pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#discussion_r599400282
##########
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 😄) 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);
> }
> ```
I dont think we need exception when `allocatedonly=false` and
`state=allocated`. since `allocatedonly=false` indicates it can return both
allocated and non allocated and in the returned list we are filtering by
allocated state.
If `allocatedonly=true` and `state=free`, we can throw exception since we
know that the list wont contain free ip's anyway
--
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:
[email protected]
