Fix three null-safety bugs introduced with the map-based endpoints refactor

commons/src/main/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtil.java

@@ -79,7 +79,7 @@ public static ApplicationLinkDetails toApplicationLinkDetails(
                 .name(applicationLinkModel.getName())
                 .displayUrl(applicationLinkModel.getDisplayUrl())
                 .rpcUrl(applicationLinkModel.getRpcUrl())
-                .isPrimary(applicationLinkModel.getPrimary())
+                .isPrimary(Boolean.TRUE.equals(applicationLinkModel.getPrimary()))
                 .build();
     }
 

commons/src/test/java/com/deftdevs/bootstrapi/commons/model/util/ApplicationLinkModelUtilTest.java

@@ -59,6 +59,20 @@ void testToApplicationLinkDetails() throws Exception {
         assertEquals(bean.getPrimary(), linkDetails.isPrimary());
     }
 
+    @Test
+    void testToApplicationLinkDetailsWithNullPrimary() throws Exception {
+        final ApplicationLinkModel bean = ApplicationLinkModel.builder()
+                .name(ApplicationLinkModel.EXAMPLE_1.getName())
+                .displayUrl(ApplicationLinkModel.EXAMPLE_1.getDisplayUrl())
+                .rpcUrl(ApplicationLinkModel.EXAMPLE_1.getRpcUrl())
+                .primary(null)
+                .build();
+        final ApplicationLinkDetails linkDetails = ApplicationLinkModelUtil.toApplicationLinkDetails(bean);
+
+        assertNotNull(linkDetails);
+        assertFalse(linkDetails.isPrimary());
+    }
+
     @Test
     void testToApplicationLinkAuthTypeDisabled() {
         assertEquals(ApplicationLinkModel.ApplicationLinkAuthType.DISABLED,

crowd/src/main/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceImpl.java

@@ -66,10 +66,22 @@ public UserModel setUser(
             final long directoryId,
             final UserModel userModel) {
 
-        final User user = findUser(directoryId, userModel.getUsername());
+        if (userModel.getUsername() == null) {
+            throw new BadRequestException("Cannot set user, username is required");
+        }
+
+        return setUser(directoryId, userModel.getUsername(), userModel);
+    }
+
+    UserModel setUser(
+            final long directoryId,
+            final String username,
+            final UserModel userModel) {
+
+        final User user = findUser(directoryId, username);
 
         if (user == null) {
-            return addUser(directoryId, userModel);
+            return addUser(directoryId, username, userModel);
         }
 
         return updateUser(directoryId, user.getName(), userModel);
@@ -86,7 +98,7 @@ public Map<String, UserModel> setUsers(
 
         final Map<String, UserModel> resultUserModels = new LinkedHashMap<>();
         for (Map.Entry<String, UserModel> entry : userModels.entrySet()) {
-            final UserModel resultUserModel = setUser(directoryId, entry.getValue());
+            final UserModel resultUserModel = setUser(directoryId, entry.getKey(), entry.getValue());
             resultUserModels.put(resultUserModel.getUsername(), resultUserModel);
         }
         return resultUserModels;
@@ -415,7 +427,7 @@ Map<String, GroupModel> addUserToGroups(
                          OperationFailedException e) {
                     throw new InternalServerErrorException(e);
                 } catch (MembershipAlreadyExistsException e) {
-                    // ignore
+                    resultGroupModels.add(resultGroupModel);
                 }
             }
         }

crowd/src/test/java/com/deftdevs/bootstrapi/crowd/service/UsersServiceTest.java

@@ -100,15 +100,21 @@ public void testGetUserNotFoundAnyDirectory() throws Exception {
         });
     }
 
+    @Test
+    public void testSetUserNoUsername() {
+        assertThrows(BadRequestException.class, () ->
+                usersService.setUser(1L, UserModel.builder().build()));
+    }
+
     @Test
     public void testSetUserAddNew() {
         final User user = getTestUser();
         final UserModel userModel = UserModelUtil.toUserModel(user);
         final UsersServiceImpl spy = spy(usersService);
-        doReturn(userModel).when(spy).addUser(anyLong(), any(UserModel.class));
+        doReturn(userModel).when(spy).addUser(anyLong(), anyString(), any(UserModel.class));
 
         spy.setUser(user.getDirectoryId(), userModel);
-        verify(spy).addUser(anyLong(), any(UserModel.class));
+        verify(spy).addUser(anyLong(), anyString(), any(UserModel.class));
     }
 
     @Test
@@ -125,6 +131,17 @@ public void testSetUserUpdateExisting() throws CrowdException {
         verify(spy).updateUser(anyLong(), anyString(), any());
     }
 
+    @Test
+    public void testSetUserWithMapKeyUsernameWhenModelUsernameIsNull() {
+        final User user = getTestUser();
+        final UserModel userModel = UserModel.builder().build(); // no username set
+        final UsersServiceImpl spy = spy(usersService);
+        doReturn(userModel).when(spy).addUser(anyLong(), anyString(), any(UserModel.class));
+
+        spy.setUser(user.getDirectoryId(), user.getName(), userModel);
+        verify(spy).addUser(anyLong(), eq(user.getName()), any(UserModel.class));
+    }
+
     @Test
     public void testSetUsers() {
         final User user = getTestUser();
@@ -134,10 +151,22 @@ public void testSetUsers() {
         final Map<String, UserModel> userModels = Stream.of(userModel, UserModel.EXAMPLE_1)
                 .collect(Collectors.toMap(UserModel::getUsername, Function.identity()));
         final UsersServiceImpl spy = spy(usersService);
-        doAnswer(invocation -> invocation.getArguments()[1]).when(spy).setUser(anyLong(), any());
+        doAnswer(invocation -> invocation.getArguments()[2]).when(spy).setUser(anyLong(), anyString(), any());
+
+        spy.setUsers(user.getDirectoryId(), userModels);
+        verify(spy, times(userModels.size())).setUser(anyLong(), anyString(), any());
+    }
+
+    @Test
+    public void testSetUsersWithNullUsernameInModelUsesMapKey() {
+        final User user = getTestUser();
+        final UserModel userModel = UserModel.builder().build(); // no username set
+        final Map<String, UserModel> userModels = Collections.singletonMap(user.getName(), userModel);
+        final UsersServiceImpl spy = spy(usersService);
+        doAnswer(invocation -> UserModelUtil.toUserModel(user)).when(spy).setUser(anyLong(), anyString(), any());
 
         spy.setUsers(user.getDirectoryId(), userModels);
-        verify(spy, times(userModels.size())).setUser(anyLong(), any());
+        verify(spy).setUser(anyLong(), eq(user.getName()), eq(userModel));
     }
 
     @Test
@@ -192,6 +221,24 @@ public void testAddUserWithGroups() throws CrowdException, DirectoryPermissionEx
         verify(groupsService, times(groupModels.size())).setGroup(anyLong(), anyString(), any());
     }
 
+    @Test
+    public void testAddUserWithGroupsAlreadyMember() throws CrowdException, DirectoryPermissionException {
+        // return the same user as the one we are adding
+        doAnswer(invocation -> invocation.getArguments()[1]).when(directoryManager).addUser(anyLong(), any(), any());
+        doAnswer(invocation -> invocation.getArguments()[2]).when(groupsService).setGroup(anyLong(), anyString(), any());
+        doThrow(new MembershipAlreadyExistsException(1L, "test", GroupModel.EXAMPLE_1.getName()))
+                .when(directoryManager).addUserToGroup(anyLong(), anyString(), anyString());
+
+        final UserModel userModel = UserModelUtil.toUserModel(getTestUser());
+        final Map<String, GroupModel> groupModels = Stream.of(GroupModel.EXAMPLE_1).collect(Collectors.toMap(GroupModel::getName, Function.identity()));
+        userModel.setPassword("12345");
+        userModel.setGroups(groupModels);
+
+        final UserModel result = usersService.addUser(1L, userModel);
+        assertNotNull(result.getGroups());
+        assertEquals(groupModels.size(), result.getGroups().size());
+    }
+
     @Test
     public void testAddUserAlreadyExists() throws CrowdException {
         final User user = getTestUser();