[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #5753: server: Fix NPE while deleting a domain

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

GabrielBrascher commented on a change in pull request #5753:
URL: https://github.com/apache/cloudstack/pull/5753#discussion_r766766745



##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -4922,7 +4922,9 @@ public boolean releasePublicIpRange(final ReleasePublicIpRangeCmd cmd) {
     public boolean releasePublicIpRange(final long vlanDbId, final long userId, final Account caller) {
         VlanVO vlan = _vlanDao.findById(vlanDbId);
         if(vlan == null) {
-            s_logger.warn("VLAN information for Account '" + caller + "', User '" + userId + "' VLAN '" + vlanDbId + "' is null. This is NPE situation.");
+            // Nothing to do if vlan can't be found
+            s_logger.warn("VLAN information for Account '" + caller + "', User '" + userId + "' VLAN '" + vlanDbId + "' is null.");
+            return true;

Review comment:
       @DaanHoogland I see your [comment](https://github.com/apache/cloudstack/pull/5753#issuecomment-990648272).
   
   > @ravening code looks good but in the diff I don't see the NPE being prevented, does it make sense to have a (unit-)test for this?
   
   I think that the null pointer is avoided by returning `true` in case VLAN is null, instead of proceeding with the `releasePublicUpRange` method. So it looks good at this point.
   
   I would recommend something such as `Skipping the process for releasing public IP range as could not find a VLAN with ID '843' for Account 'Acct[ef5d57e5-4b9d-11e3-8346-2c44fd7a3378-system]' and User '1'.`.
   
   ```
   s_logger.warn(String.format("Skipping the process for releasing public IP range as could not find a VLAN with ID '%s' for Account '%s' and User '%s'.",vlanDbId, caller, userId));
   ```




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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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