Merge pull request #2420 from mposolda/master

Fixes and performance improvements for "create user"

model/jpa/src/main/java/org/keycloak/connections/jpa/HibernateStatsReporter.java

@@ -21,6 +21,8 @@ import javax.persistence.EntityManagerFactory;
 
 import org.hibernate.SessionFactory;
 import org.hibernate.jpa.internal.EntityManagerFactoryImpl;
+import org.hibernate.stat.CollectionStatistics;
+import org.hibernate.stat.EntityStatistics;
 import org.hibernate.stat.QueryStatistics;
 import org.hibernate.stat.Statistics;
 import org.jboss.logging.Logger;
@@ -32,6 +34,8 @@ import org.keycloak.services.scheduled.ScheduledTask;
  */
 public class HibernateStatsReporter implements ScheduledTask {
 
+    private static final int LIMIT = 100; // Just hardcoded for now
+
     private final EntityManagerFactory emf;
     private static final Logger logger = Logger.getLogger(HibernateStatsReporter.class);
 
@@ -53,21 +57,66 @@ public class HibernateStatsReporter implements ScheduledTask {
 
     protected void logStats(Statistics stats) {
         String lineSep = System.getProperty("line.separator");
-        StringBuilder builder = new StringBuilder(lineSep).append(stats.toString()).append(lineSep);
+        StringBuilder builder = new StringBuilder(lineSep).append(stats.toString()).append(lineSep).append(lineSep);
-        builder.append(lineSep).append("Queries statistics: ").append(lineSep).append(lineSep);
 
-        for (String query : stats.getQueries()) {
+        logEntities(builder, lineSep, stats);
-            QueryStatistics queryStats = stats.getQueryStatistics(query);
+        logCollections(builder, lineSep, stats);
-
+        logQueries(builder, lineSep, stats);
-            builder.append(query).append(lineSep)
-                    .append("executionCount=" + queryStats.getExecutionCount()).append(lineSep)
-                    .append("executionAvgTime=" + queryStats.getExecutionAvgTime()).append(" ms").append(lineSep)
-                    .append(lineSep);
-
-            builder.append(lineSep);
-        }
 
         logger.infof(builder.toString());
     }
 
+
+    protected void logEntities(StringBuilder builder, String lineSep, Statistics stats) {
+        builder.append("Important entities statistics: ").append(lineSep);
+        for (String entity : stats.getEntityNames()) {
+            EntityStatistics entityStats = stats.getEntityStatistics(entity);
+            if (entityStats.getInsertCount() > LIMIT || entityStats.getDeleteCount() > LIMIT || entityStats.getUpdateCount() > LIMIT || entityStats.getLoadCount() > LIMIT || entityStats.getFetchCount() > LIMIT) {
+                builder.append(entity + " - ")
+                        .append("inserted: " + entityStats.getInsertCount())
+                        .append(", updated: " + entityStats.getUpdateCount())
+                        .append(", removed: " + entityStats.getDeleteCount())
+                        .append(", loaded: " + entityStats.getLoadCount())
+                        .append(", fetched: " + entityStats.getFetchCount())
+                        .append(lineSep);
+            }
+        }
+        builder.append(lineSep);
+    }
+
+
+    protected void logCollections(StringBuilder builder, String lineSep, Statistics stats) {
+        builder.append("Important collections statistics: ").append(lineSep);
+        for (String col : stats.getCollectionRoleNames()) {
+            CollectionStatistics collectionStats = stats.getCollectionStatistics(col);
+            if (collectionStats.getRecreateCount() > LIMIT || collectionStats.getUpdateCount() > LIMIT || collectionStats.getRemoveCount() > LIMIT ||
+                    collectionStats.getLoadCount() > LIMIT || collectionStats.getFetchCount() > LIMIT) {
+                builder.append(col + " - ")
+                        .append("recreated: " + collectionStats.getRecreateCount())
+                        .append(", updated: " + collectionStats.getUpdateCount())
+                        .append(", removed: " + collectionStats.getRemoveCount())
+                        .append(", loaded: " + collectionStats.getLoadCount())
+                        .append(", fetched: " + collectionStats.getFetchCount())
+                        .append(lineSep);
+            }
+        }
+        builder.append(lineSep);
+    }
+
+
+    protected void logQueries(StringBuilder builder, String lineSep, Statistics stats) {
+        builder.append("Important queries statistics: ").append(lineSep).append(lineSep);
+        for (String query : stats.getQueries()) {
+            QueryStatistics queryStats = stats.getQueryStatistics(query);
+
+            if (queryStats.getExecutionCount() > LIMIT || (queryStats.getExecutionCount() * queryStats.getExecutionAvgTime() > LIMIT)) {
+                builder.append(query).append(lineSep)
+                        .append("executionCount=" + queryStats.getExecutionCount()).append(lineSep)
+                        .append("executionAvgTime=" + queryStats.getExecutionAvgTime()).append(" ms").append(lineSep)
+                        .append(lineSep)
+                        .append(lineSep);
+            }
+        }
+    }
+
 }

model/jpa/src/main/java/org/keycloak/models/jpa/JpaUserProvider.java

@@ -76,21 +76,21 @@ public class JpaUserProvider implements UserProvider {
         entity.setRealmId(realm.getId());
         em.persist(entity);
         em.flush();
-        UserModel userModel = new UserAdapter(session, realm, em, entity);
+        UserAdapter userModel = new UserAdapter(session, realm, em, entity);
 
         if (addDefaultRoles) {
             for (String r : realm.getDefaultRoles()) {
-                userModel.grantRole(realm.getRole(r));
+                userModel.grantRoleImpl(realm.getRole(r)); // No need to check if user has role as it's new user
             }
 
             for (ClientModel application : realm.getClients()) {
                 for (String r : application.getDefaultRoles()) {
-                    userModel.grantRole(application.getRole(r));
+                    userModel.grantRoleImpl(application.getRole(r)); // No need to check if user has role as it's new user
                 }
             }
 
             for (GroupModel g : realm.getDefaultGroups()) {
-                userModel.joinGroup(g);
+                userModel.joinGroupImpl(g); // No need to check if user has group as it's new user
             }
         }
 

model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java

@@ -517,6 +517,11 @@ public class UserAdapter implements UserModel, JpaModel<UserEntity> {
     @Override
     public void joinGroup(GroupModel group) {
         if (isMemberOf(group)) return;
+        joinGroupImpl(group);
+
+    }
+
+    protected void joinGroupImpl(GroupModel group) {
         UserGroupMembershipEntity entity = new UserGroupMembershipEntity();
         entity.setUser(getEntity());
         entity.setGroupId(group.getId());
@@ -570,6 +575,10 @@ public class UserAdapter implements UserModel, JpaModel<UserEntity> {
     @Override
     public void grantRole(RoleModel role) {
         if (hasRole(role)) return;
+        grantRoleImpl(role);
+    }
+
+    public void grantRoleImpl(RoleModel role) {
         UserRoleMappingEntity entity = new UserRoleMappingEntity();
         entity.setUser(getEntity());
         entity.setRoleId(role.getId());

services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java

@@ -173,7 +173,7 @@ public class UsersResource {
                 }
             }
 
-            updateUserFromRep(user, rep, attrsToRemove, realm, session);
+            updateUserFromRep(user, rep, attrsToRemove, realm, session, true);
             adminEvent.operation(OperationType.UPDATE).resourcePath(uriInfo).representation(rep).success();
 
             if (session.getTransaction().isActive()) {
@@ -212,7 +212,7 @@ public class UsersResource {
         try {
             UserModel user = session.users().addUser(realm, rep.getUsername());
             Set<String> emptySet = Collections.emptySet();
-            updateUserFromRep(user, rep, emptySet, realm, session);
+            updateUserFromRep(user, rep, emptySet, realm, session, false);
 
             adminEvent.operation(OperationType.CREATE).resourcePath(uriInfo, user.getId()).representation(rep).success();
 
@@ -229,7 +229,7 @@ public class UsersResource {
         }
     }
 
-    public static void updateUserFromRep(UserModel user, UserRepresentation rep, Set<String> attrsToRemove, RealmModel realm, KeycloakSession session) {
+    public static void updateUserFromRep(UserModel user, UserRepresentation rep, Set<String> attrsToRemove, RealmModel realm, KeycloakSession session, boolean removeMissingRequiredActions) {
         if (rep.getUsername() != null && realm.isEditUsernameAllowed()) {
             user.setUsername(rep.getUsername());
         }
@@ -251,7 +251,7 @@ public class UsersResource {
             for (String action : allActions) {
                 if (reqActions.contains(action)) {
                     user.addRequiredAction(action);
-                } else {
+                } else if (removeMissingRequiredActions) {
                     user.removeRequiredAction(action);
                 }
             }

testsuite/integration/src/test/java/org/keycloak/testsuite/admin/UserTest.java

@@ -42,6 +42,7 @@ import javax.ws.rs.core.UriBuilder;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
 
@@ -81,6 +82,7 @@ public class UserTest extends AbstractClientTest {
         UserRepresentation user = new UserRepresentation();
         user.setUsername(username);
         user.setEmail(email);
+        user.setRequiredActions(Collections.<String>emptyList());
         user.setEnabled(true);
 
         Response response = realm.users().create(user);
@@ -663,6 +665,27 @@ public class UserTest extends AbstractClientTest {
         }
     }
 
+    @Test
+    public void testDefaultRequiredActionAdded() {
+        // Add UPDATE_PASSWORD as default required action
+        RequiredActionProviderRepresentation updatePasswordReqAction = realm.flows().getRequiredAction(UserModel.RequiredAction.UPDATE_PASSWORD.toString());
+        updatePasswordReqAction.setDefaultAction(true);
+        realm.flows().updateRequiredAction(UserModel.RequiredAction.UPDATE_PASSWORD.toString(), updatePasswordReqAction);
+
+        // Create user
+        String userId = createUser("user1", "user1@localhost");
+
+        UserRepresentation userRep = realm.users().get(userId).toRepresentation();
+        Assert.assertEquals(1, userRep.getRequiredActions().size());
+        Assert.assertEquals(UserModel.RequiredAction.UPDATE_PASSWORD.toString(), userRep.getRequiredActions().get(0));
+
+        // Remove UPDATE_PASSWORD default action
+        updatePasswordReqAction = realm.flows().getRequiredAction(UserModel.RequiredAction.UPDATE_PASSWORD.toString());
+        updatePasswordReqAction.setDefaultAction(true);
+        realm.flows().updateRequiredAction(UserModel.RequiredAction.UPDATE_PASSWORD.toString(), updatePasswordReqAction);
+    }
+
+
     private void switchEditUsernameAllowedOn() {
         RealmRepresentation rep = realm.toRepresentation();
         rep.setEditUsernameAllowed(true);