Uploaded image for project: 'Kuali Rice Development'
  1. Kuali Rice Development
  2. KULRICE-9153

Analyze IU patch for preventing role member lookup from causing out of memory exceptions

    Details

    • Type: Task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.3, 2.2.1
    • Fix Version/s: 2.1.6, 2.2.4
    • Component/s: Development
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Rice Module:
      KIM
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required
    • Include in Release Notes?:
      Yes

      Description

      See the following code for the IU modification to prevent the role member lookup from loading all people in the DB into memory.

      From Eric: "the reason that shannon didn't contribute this back was that it changes the behavior of the role lookup such that now you have to enter a principal name that matches exactly instead of being able to use wildcards but if you look at the original commented out code in there you can see why it causes problems for those with large identity databases"

      org.kuali.rice.kim.lookup.RoleMemberLookupableHelperServiceImpl
         @SuppressWarnings({ "unchecked" })
      	protected Map<String, String> buildRoleSearchCriteria(Map<String, String> fieldValues){
             	String assignedToPrincipalName = fieldValues.get(ASSIGNED_TO_PRINCIPAL_NAME);
          	Map<String, String> searchCriteria;
          	List<Principal> principals = new ArrayList<Principal>();
              if(StringUtils.isNotEmpty(assignedToPrincipalName)){
      	    	/**
      	    	 * Begin IU Customization
      	    	 * 06/02/2011 Shannon Hess
      	    	 * EN-2182:  Commented code causes GC out of memory exceptions and eventually brings down the server.
      	    	 * Not sure why the principal name was wild-carded when doing a role search.  If that functionality is 
      	    	 * needed, this fix will not be adequate.
      	    	 */
              	Principal principal= KimApiServiceLocator.getIdentityService().getPrincipalByPrincipalName(assignedToPrincipalName);
      			principals.add(principal);
      
              	//QueryByCriteria.Builder query = QueryByCriteria.Builder.create();
              	//query.setPredicates(like("principals.principalName", WILDCARD+assignedToPrincipalName+WILDCARD));
              	//EntityQueryResults qr = KimApiServiceLocator.getIdentityService().findEntities(query.build());
      			//if(qr.getResults() == null || qr.getResults().isEmpty()) {
              	//	return null;
              	//} else {
              	//	for (Entity kimEntityInfo : qr.getResults()) {
              	//		if(kimEntityInfo.getPrincipals() != null){
              	//			principals.addAll(kimEntityInfo.getPrincipals());
              	//		}
              	//	}
              	//}
      	    	/**
      	    	 * End IU Customization
      	    	 */
              }
              String assignedToGroupNamespaceCode = fieldValues.get(ASSIGNED_TO_GROUP_NAMESPACE_CODE);
              String assignedToGroupName = fieldValues.get(ASSIGNED_TO_GROUP_NAME);
              List<Group> groups = null;
              if(StringUtils.isNotEmpty(assignedToGroupNamespaceCode) && StringUtils.isEmpty(assignedToGroupName) ||
              		StringUtils.isEmpty(assignedToGroupNamespaceCode) && StringUtils.isNotEmpty(assignedToGroupName) ||
              		StringUtils.isNotEmpty(assignedToGroupNamespaceCode) && StringUtils.isNotEmpty(assignedToGroupName)){
                  QueryByCriteria.Builder builder = QueryByCriteria.Builder.create();
              	builder.setPredicates(and(
                                  like(NAMESPACE_CODE, getQueryString(assignedToGroupNamespaceCode)),
                                  like(GROUP_NAME, getQueryString(assignedToGroupName))));
              	GroupQueryResults qr = KimApiServiceLocator.getGroupService().findGroups(builder.build());
              	if (qr.getResults() == null || qr.getResults().isEmpty()) {
              		return null;
                  }
                  groups = qr.getResults();
              }
      
              String assignedToRoleNamespaceCode = fieldValues.get(ASSIGNED_TO_NAMESPACE_FOR_LOOKUP);
              String assignedToRoleName = fieldValues.get(ASSIGNED_TO_ROLE_NAME);
      
          	searchCriteria = new HashMap<String, String>();
              if (StringUtils.isNotEmpty(assignedToRoleNamespaceCode)) {
              	searchCriteria.put(ASSIGNED_TO_ROLE_NAMESPACE_CODE, WILDCARD+assignedToRoleNamespaceCode+WILDCARD);
              }
              if(StringUtils.isNotEmpty(assignedToRoleName)) {
              	searchCriteria.put(ASSIGNED_TO_ROLE_ROLE_NAME, WILDCARD+assignedToRoleName+WILDCARD);
              }
      
          	StringBuffer memberQueryString = null;
              if(!principals.isEmpty()) {
              	memberQueryString = new StringBuffer();
              	for(Principal principal: principals){
              		memberQueryString.append(principal.getPrincipalId()+KimConstants.KimUIConstants.OR_OPERATOR);
              	}
                  if(memberQueryString.toString().endsWith(KimConstants.KimUIConstants.OR_OPERATOR)) {
                  	memberQueryString.delete(memberQueryString.length()-KimConstants.KimUIConstants.OR_OPERATOR.length(), memberQueryString.length());
                  }
              }
              if(groups!=null){
              	if(memberQueryString==null) {
              		memberQueryString = new StringBuffer();
                  }
              	else if(StringUtils.isNotEmpty(memberQueryString.toString())) {
              		memberQueryString.append(KimConstants.KimUIConstants.OR_OPERATOR);
                  }
              	for(Group group: groups){
              		memberQueryString.append(group.getId()+KimConstants.KimUIConstants.OR_OPERATOR);
              	}
                  if(memberQueryString.toString().endsWith(KimConstants.KimUIConstants.OR_OPERATOR)) {
                  	memberQueryString.delete(memberQueryString.length()-KimConstants.KimUIConstants.OR_OPERATOR.length(), memberQueryString.length());
                  }
              	searchCriteria.put(ASSIGNED_TO_ROLE_MEMBER_ID, memberQueryString.toString());
              }
              if (memberQueryString!=null && StringUtils.isNotEmpty(memberQueryString.toString())) {
              	searchCriteria.put(ASSIGNED_TO_ROLE_MEMBER_ID, memberQueryString.toString());
              }
      
              return searchCriteria;
          }
      

        Attachments

          Issue Links

            Activity

            Hide
            gilesp Peter Giles (Inactive) added a comment -

            I found that this code gets hit from the responsibility and permission lookups. Anywhere else noteworthy that you know of Eric?

            Show
            gilesp Peter Giles (Inactive) added a comment - I found that this code gets hit from the responsibility and permission lookups. Anywhere else noteworthy that you know of Eric?
            Hide
            gilesp Peter Giles (Inactive) added a comment - - edited

            I wonder if IU modified person lookup to limit results too? I know this role member lookup is particulary bad because it automatically wildcards both ends of the search but there is nothing preventing someone from searching for Principal Name = '*e*' on person search, at least not by default.

            Show
            gilesp Peter Giles (Inactive) added a comment - - edited I wonder if IU modified person lookup to limit results too? I know this role member lookup is particulary bad because it automatically wildcards both ends of the search but there is nothing preventing someone from searching for Principal Name = '*e*' on person search, at least not by default.
            Hide
            shahess Shannon Hess added a comment -

            IU did not limit the person search in this way, I think mostly because people sometimes do want to do a person search with the wildcards. I just did a search for hess in production here at IU and it came back within 10 seconds with 85 results (shahess included). One thing we did do here at IU is restrict the person search to a maximum of 200 results. Without that limit, a no criteria search would bring down the server.

            Show
            shahess Shannon Hess added a comment - IU did not limit the person search in this way, I think mostly because people sometimes do want to do a person search with the wildcards. I just did a search for hess in production here at IU and it came back within 10 seconds with 85 results (shahess included). One thing we did do here at IU is restrict the person search to a maximum of 200 results. Without that limit, a no criteria search would bring down the server.
            Hide
            cpedersen Corey Pedersen (Inactive) added a comment -

            The fix as shown above results in a NPE if a principal name is specified and does not exist.

            java.lang.NullPointerException
            	at org.kuali.rice.kim.lookup.RoleMemberLookupableHelperServiceImpl.buildRoleSearchCriteria(RoleMemberLookupableHelperServiceImpl.java:176)
            	at org.kuali.rice.kim.lookup.RoleMemberLookupableHelperServiceImpl.getSearchResultsHelper(RoleMemberLookupableHelperServiceImpl.java:73)
            	at org.kuali.rice.kns.lookup.KualiLookupableHelperServiceImpl.getSearchResults(KualiLookupableHelperServiceImpl.java:61)
            	at org.kuali.rice.kns.lookup.AbstractLookupableHelperServiceImpl.performLookup(AbstractLookupableHelperServiceImpl.java:1104)
            	at org.kuali.rice.kns.lookup.KualiLookupableImpl.performLookup(KualiLookupableImpl.java:307)
            

            A possible solution for this would be:

                    if(!principals.isEmpty()) {
                        memberQueryString = new StringBuffer();
                        for(Principal principal: principals){
                            if(principal != null) {
                    	        memberQueryString.append(principal.getPrincipalId()+KimConstants.KimUIConstants.OR_OPERATOR);
                            } else {
                                memberQueryString.append("NOMATCH" + KimConstants.KimUIConstants.OR_OPERATOR);
                            }
                        }
                        if(memberQueryString.toString().endsWith(KimConstants.KimUIConstants.OR_OPERATOR)) {
                            memberQueryString.delete(memberQueryString.length()-KimConstants.KimUIConstants.OR_OPERATOR.length(), memberQueryString.length());
                        }
                    }
            
            Show
            cpedersen Corey Pedersen (Inactive) added a comment - The fix as shown above results in a NPE if a principal name is specified and does not exist. java.lang.NullPointerException at org.kuali.rice.kim.lookup.RoleMemberLookupableHelperServiceImpl.buildRoleSearchCriteria(RoleMemberLookupableHelperServiceImpl.java:176) at org.kuali.rice.kim.lookup.RoleMemberLookupableHelperServiceImpl.getSearchResultsHelper(RoleMemberLookupableHelperServiceImpl.java:73) at org.kuali.rice.kns.lookup.KualiLookupableHelperServiceImpl.getSearchResults(KualiLookupableHelperServiceImpl.java:61) at org.kuali.rice.kns.lookup.AbstractLookupableHelperServiceImpl.performLookup(AbstractLookupableHelperServiceImpl.java:1104) at org.kuali.rice.kns.lookup.KualiLookupableImpl.performLookup(KualiLookupableImpl.java:307) A possible solution for this would be: if (!principals.isEmpty()) { memberQueryString = new StringBuffer (); for (Principal principal: principals){ if (principal != null ) { memberQueryString.append(principal.getPrincipalId()+KimConstants.KimUIConstants.OR_OPERATOR); } else { memberQueryString.append( "NOMATCH" + KimConstants.KimUIConstants.OR_OPERATOR); } } if (memberQueryString.toString().endsWith(KimConstants.KimUIConstants.OR_OPERATOR)) { memberQueryString.delete(memberQueryString.length()-KimConstants.KimUIConstants.OR_OPERATOR.length(), memberQueryString.length()); } }
            Hide
            cpedersen Corey Pedersen (Inactive) added a comment -

            Another possible solutions would be to use the current code, but remove the "automatic" wildcards from the start and end of the search string. This may provide some of base performance enhancements and still allow for wildcard search.

                    if(StringUtils.isNotEmpty(assignedToPrincipalName)){
                        QueryByCriteria.Builder query = QueryByCriteria.Builder.create();
                        // query.setPredicates(like("principals.principalName", WILDCARD+assignedToPrincipalName+WILDCARD));
                        query.setPredicates(like("principals.principalName", assignedToPrincipalName));
                    	EntityQueryResults qr = KimApiServiceLocator.getIdentityService().findEntities(query.build());
                    	if(qr.getResults() == null || qr.getResults().isEmpty()) {
                    	    return null;
                    	} else {
                    	    for (Entity kimEntityInfo : qr.getResults()) {
                    	        if(kimEntityInfo.getPrincipals() != null){
                    		    principals.addAll(kimEntityInfo.getPrincipals());
                    	        }
                    	    }
                    	}
                    }
            
            Show
            cpedersen Corey Pedersen (Inactive) added a comment - Another possible solutions would be to use the current code, but remove the "automatic" wildcards from the start and end of the search string. This may provide some of base performance enhancements and still allow for wildcard search. if (StringUtils.isNotEmpty(assignedToPrincipalName)){ QueryByCriteria.Builder query = QueryByCriteria.Builder.create(); // query.setPredicates(like( "principals.principalName" , WILDCARD+assignedToPrincipalName+WILDCARD)); query.setPredicates(like( "principals.principalName" , assignedToPrincipalName)); EntityQueryResults qr = KimApiServiceLocator.getIdentityService().findEntities(query.build()); if (qr.getResults() == null || qr.getResults().isEmpty()) { return null ; } else { for (Entity kimEntityInfo : qr.getResults()) { if (kimEntityInfo.getPrincipals() != null ){ principals.addAll(kimEntityInfo.getPrincipals()); } } } }
            Hide
            gilesp Peter Giles (Inactive) added a comment -

            When looking over this code with Corey, we noticed something else strange. The IdentityService method getPrincipalByPrincipalName has some funny and undesirable behavior around wildcards, which stems from its use of BusinessObjectService.findMatching to retrieve its result. That method will detect wildcards and use a LIKE predicate, and if it finds more than one matchit will return null. If exactly one match is found it will return that result. So it kind of does what its supposed to, but has some surprising corner cases.

            Show
            gilesp Peter Giles (Inactive) added a comment - When looking over this code with Corey, we noticed something else strange. The IdentityService method getPrincipalByPrincipalName has some funny and undesirable behavior around wildcards, which stems from its use of BusinessObjectService.findMatching to retrieve its result. That method will detect wildcards and use a LIKE predicate, and if it finds more than one matchit will return null. If exactly one match is found it will return that result. So it kind of does what its supposed to, but has some surprising corner cases.
            Hide
            gilesp Peter Giles (Inactive) added a comment -

            Modified the IU patch to avoid the possible NPE and also the strange wildcard processing behavior:

                protected Map<String, String> buildRoleSearchCriteria(Map<String, String> fieldValues){
                   	String assignedToPrincipalName = fieldValues.get(ASSIGNED_TO_PRINCIPAL_NAME);
                	Map<String, String> searchCriteria;
                	List<Principal> principals = new ArrayList<Principal>();
            
                    if(StringUtils.isNotEmpty(assignedToPrincipalName)) { // if a principal name is specified in the search
                        // KULRICE-9153: Analyze IU patch for preventing role member lookup from causing out of memory exceptions
                        // Changing to exact match on principal name to prevent blowing up Rice by loading every user into memory
                        if (assignedToPrincipalName.contains("*")) {
                            return null; // bail out, wild cards are not allowed since
                                         // IdentityServiceImpl.getPrincipalByPrincipalName has weird behavior around wildcards
                        }
            
                        Principal principal = KimApiServiceLocator.getIdentityService().getPrincipalByPrincipalName(assignedToPrincipalName);
            
                        if (principal == null) {
                            return null; // bail out, if no principal matched and a principal name was supplied, then there will be
                                         // no valid matching roles.
                        }
            
                        principals.add(principal);
                    }
                    ...
            
            Show
            gilesp Peter Giles (Inactive) added a comment - Modified the IU patch to avoid the possible NPE and also the strange wildcard processing behavior: protected Map< String , String > buildRoleSearchCriteria(Map< String , String > fieldValues){ String assignedToPrincipalName = fieldValues.get(ASSIGNED_TO_PRINCIPAL_NAME); Map< String , String > searchCriteria; List<Principal> principals = new ArrayList<Principal>(); if (StringUtils.isNotEmpty(assignedToPrincipalName)) { // if a principal name is specified in the search // KULRICE-9153: Analyze IU patch for preventing role member lookup from causing out of memory exceptions // Changing to exact match on principal name to prevent blowing up Rice by loading every user into memory if (assignedToPrincipalName.contains( "*" )) { return null ; // bail out, wild cards are not allowed since // IdentityServiceImpl.getPrincipalByPrincipalName has weird behavior around wildcards } Principal principal = KimApiServiceLocator.getIdentityService().getPrincipalByPrincipalName(assignedToPrincipalName); if (principal == null ) { return null ; // bail out, if no principal matched and a principal name was supplied, then there will be // no valid matching roles. } principals.add(principal); } ...

              People

              • Assignee:
                gilesp Peter Giles (Inactive)
                Reporter:
                ewestfal Eric Westfall
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: