From 9c61d493d4935c7d04c4c3b3d07ecec47a7c785a Mon Sep 17 00:00:00 2001 From: Hasini Samarathunga Date: Mon, 15 Jul 2024 01:02:08 +0530 Subject: [PATCH 1/2] Fixed issue of getting 0 results when filtering with 2+ meta attributes --- .../OrganizationManagementConstants.java | 1 + .../service/constant/SQLConstants.java | 2 +- .../impl/OrganizationManagementDAOImpl.java | 26 ++++++++------ .../service/model/FilterQueryBuilder.java | 34 ++++++++++++++----- 4 files changed, 42 insertions(+), 21 deletions(-) diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/OrganizationManagementConstants.java b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/OrganizationManagementConstants.java index 75c15c48..bbb8648b 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/OrganizationManagementConstants.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/OrganizationManagementConstants.java @@ -93,6 +93,7 @@ public class OrganizationManagementConstants { public static final String AND = "and"; public static final String FILTER_PLACEHOLDER_PREFIX = "FILTER_ID_"; public static final String PARENT_ID_FILTER_PLACEHOLDER_PREFIX = "FILTER_PARENT_ID_"; + public static final String META_ATTRIBUTE_PLACEHOLDER_PREFIX = "UM_ORG_ATTRIBUTE_"; private static final String ORGANIZATION_MANAGEMENT_ERROR_CODE_PREFIX = "ORG-"; private static final Map attributeColumnMap = new HashMap<>(); public static final Map ATTRIBUTE_COLUMN_MAP = Collections.unmodifiableMap(attributeColumnMap); diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/SQLConstants.java b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/SQLConstants.java index c41756a6..dc57c0c2 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/SQLConstants.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/SQLConstants.java @@ -352,7 +352,7 @@ public class SQLConstants { "FETCH NEXT :" + SQLPlaceholders.DB_SCHEMA_LIMIT + "; ROWS ONLY"; public static final String INNER_JOIN_UM_ORG_ATTRIBUTE = - "INNER JOIN UM_ORG_ATTRIBUTE ON UM_ORG.UM_ID = UM_ORG_ATTRIBUTE.UM_ORG_ID WHERE"; + "INNER JOIN UM_ORG_ATTRIBUTE %s ON UM_ORG.UM_ID = %s.UM_ORG_ID WHERE"; public static final String GET_ALL_UM_ORG_ATTRIBUTES = "SELECT UM_ORG.UM_ID, UM_ORG.UM_ORG_NAME, " + "UM_ORG.UM_CREATED_TIME, UM_ORG.UM_STATUS, UM_ORG_ATTRIBUTE.UM_ATTRIBUTE_KEY, " + diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java index 9b8257a8..438b14b9 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java @@ -121,7 +121,6 @@ import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_ID_COLUMN; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_LAST_MODIFIED_COLUMN; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_NAME_COLUMN; -import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_ORGANIZATION_ATTRIBUTES_TABLE; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_PARENT_ID_COLUMN; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_STATUS_COLUMN; import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.VIEW_TENANT_UUID_COLUMN; @@ -731,8 +730,9 @@ private List getOrganizationsList(boolean authorizedSubOrgsOnly, b String userID = getUserId(); String sqlStmt = prepareGetOrganizationQuery(authorizedSubOrgsOnly, recursive, sortOrder, applicationAudience, filterQueryBuilder, parentIdFilterQueryBuilder, userID); + boolean isFilteringMetaAttributes = filterQueryBuilder.getMetaAttributeCount() > 1; - if (filterQueryBuilder.getHasSubAttribute()) { + if (isFilteringMetaAttributes) { sqlStmt = String.format(GET_ALL_UM_ORG_ATTRIBUTES, sqlStmt); } List organizations; @@ -741,7 +741,7 @@ private List getOrganizationsList(boolean authorizedSubOrgsOnly, b try { organizations = namedJdbcTemplate.executeQuery(sqlStmt, (resultSet, rowNumber) -> { - if (filterQueryBuilder.getHasSubAttribute()) { + if (isFilteringMetaAttributes) { String orgId = resultSet.getString(1); Organization organization = organizationMap.get(orgId); if (organization == null) { @@ -761,7 +761,7 @@ private List getOrganizationsList(boolean authorizedSubOrgsOnly, b } catch (DataAccessException e) { throw handleServerException(ERROR_CODE_ERROR_RETRIEVING_ORGANIZATIONS, e); } - if (filterQueryBuilder.getHasSubAttribute()) { + if (isFilteringMetaAttributes) { return new ArrayList<>(organizationMap.values()); } return organizations; @@ -1509,8 +1509,12 @@ private String prepareGetOrganizationQuery(boolean authorizedSubOrgsOnly, boolea String sqlStmt = getOrgSqlStatement(authorizedSubOrgsOnly, applicationAudience); String getOrgSqlStmtTail = getOrgSqlStmtTail(authorizedSubOrgsOnly, applicationAudience); - if (filterQueryBuilder.getHasSubAttribute()) { - sqlStmt = sqlStmt.replace("WHERE", INNER_JOIN_UM_ORG_ATTRIBUTE); + + if (filterQueryBuilder.getMetaAttributeCount() > 1) { + for (String placeholder : filterQueryBuilder.getMetaAttributePlaceholders()) { + sqlStmt = sqlStmt.replace("WHERE", + String.format(INNER_JOIN_UM_ORG_ATTRIBUTE, placeholder, placeholder)); + } } sqlStmt = appendFilterQueries(sqlStmt, filterQueryBuilder, parentIdFilterQueryBuilder, getOrgSqlStmtTail, recursive, sortOrder); @@ -1614,11 +1618,11 @@ private void setFilterAttributes(NamedPreparedStatement namedPreparedStatement, private String handleViewAttrKeyColumn(ExpressionNode expressionNode, FilterQueryBuilder filterQueryBuilder) { - filterQueryBuilder.setHasSubAttribute(true); + String placeholder = filterQueryBuilder.generateMetaAttributePlaceholder(); String attributeValue = expressionNode.getAttributeValue(); - String subAttributeName = StringUtils.substringAfter(attributeValue, - ORGANIZATION_ATTRIBUTES_FIELD + "."); - return VIEW_ORGANIZATION_ATTRIBUTES_TABLE + "." + VIEW_ATTR_KEY_COLUMN + " = '" + subAttributeName + "' AND " + - VIEW_ORGANIZATION_ATTRIBUTES_TABLE + "." + VIEW_ATTR_VALUE_COLUMN; + String subAttributeName = StringUtils.substringAfter(attributeValue, ORGANIZATION_ATTRIBUTES_FIELD + "."); + + return placeholder + "." + VIEW_ATTR_KEY_COLUMN + " = '" + subAttributeName + "' AND " + placeholder + "." + + VIEW_ATTR_VALUE_COLUMN; } } diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java index 6460d74e..0018a593 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java @@ -23,6 +23,8 @@ import java.util.List; import java.util.Map; +import static org.wso2.carbon.identity.organization.management.service.constant.OrganizationManagementConstants.META_ATTRIBUTE_PLACEHOLDER_PREFIX; + /** * Create filter query builder. */ @@ -32,7 +34,8 @@ public class FilterQueryBuilder { private List timestampParameters = new ArrayList<>(); private int count = 1; private String filter; - private boolean hasSubAttribute; + private List metaAttributePlaceholders = new ArrayList<>(); + private int metaAttributeCount = 1; /** * Get filter query builder attributes. @@ -97,22 +100,35 @@ public String getFilterQuery() { } /** - * Get the value of hasSubAttribute. + * Get filter query builder meta attribute placeholders. + * + * @return List of filter query builder meta attribute placeholders. + */ + public List getMetaAttributePlaceholders() { + + return metaAttributePlaceholders; + } + + /** + * Generate and add a filter query builder meta attribute placeholder. * - * @return boolean value indicating if the filter contains a sub attribute. + * @return Next meta attribute placeholder. */ - public boolean getHasSubAttribute() { + public String generateMetaAttributePlaceholder() { - return hasSubAttribute; + String placeholder = META_ATTRIBUTE_PLACEHOLDER_PREFIX + metaAttributeCount; + metaAttributePlaceholders.add(placeholder); + metaAttributeCount++; + return placeholder; } /** - * Set the value of hasSubAttribute. + * Get the current count of meta attribute placeholders. * - * @param hasSubAttribute boolean value indicating if the filter contains a sub attributes. + * @return Meta attribute count. */ - public void setHasSubAttribute(boolean hasSubAttribute) { + public int getMetaAttributeCount() { - this.hasSubAttribute = hasSubAttribute; + return metaAttributeCount; } } From deb798e6479ffd591511a2f5e045efcaae9be4dd Mon Sep 17 00:00:00 2001 From: Hasini Samarathunga Date: Mon, 22 Jul 2024 10:20:47 +0530 Subject: [PATCH 2/2] Added unit tests for multiattribute filtering --- .../OrganizationManagementConstants.java | 2 +- .../impl/OrganizationManagementDAOImpl.java | 4 +-- .../service/model/FilterQueryBuilder.java | 4 +-- .../service/OrganizationManagerImplTest.java | 2 ++ .../OrganizationManagementDAOImplTest.java | 35 +++++++++++++++++++ 5 files changed, 42 insertions(+), 5 deletions(-) diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/OrganizationManagementConstants.java b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/OrganizationManagementConstants.java index 1efe9bd9..f86c0411 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/OrganizationManagementConstants.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/constant/OrganizationManagementConstants.java @@ -422,7 +422,7 @@ public enum ErrorMessages { ERROR_CODE_ERROR_EVALUATING_ADD_ORGANIZATION_AUTHORIZATION("65019", "Unable to create the organization.", "Server encountered an error while evaluating authorization of user to create the " + "organization in parent organization with ID: %s."), - ERROR_CODE_ERROR_BUILDING_PAGINATED_RESPONSE_URL("65020", "Unable to retrieve the organizations.", + ERROR_CODE_ERROR_BUILDING_PAGINATED_RESPONSE_URL("65020", "Unable to retrieve the paginated results.", "Server encountered an error while building paginated response URL."), ERROR_CODE_ERROR_MISSING_SUPER("65021", "Unable to create the organization.", "Server encountered an error while retrieving the super organization."), diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java index 0aff13d1..52cf3654 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImpl.java @@ -738,7 +738,7 @@ private List getOrganizationsList(boolean authorizedSubOrgsOnly, b String userID = getUserId(); String sqlStmt = prepareGetOrganizationQuery(authorizedSubOrgsOnly, recursive, sortOrder, applicationAudience, filterQueryBuilder, parentIdFilterQueryBuilder, userID); - boolean isFilteringMetaAttributes = filterQueryBuilder.getMetaAttributeCount() > 1; + boolean isFilteringMetaAttributes = filterQueryBuilder.getMetaAttributeCount() > 0; if (isFilteringMetaAttributes) { sqlStmt = String.format(GET_ALL_UM_ORG_ATTRIBUTES, sqlStmt); @@ -1558,7 +1558,7 @@ private String prepareGetOrganizationQuery(boolean authorizedSubOrgsOnly, boolea String sqlStmt = getOrgSqlStatement(authorizedSubOrgsOnly, applicationAudience); String getOrgSqlStmtTail = getOrgSqlStmtTail(authorizedSubOrgsOnly, applicationAudience); - if (filterQueryBuilder.getMetaAttributeCount() > 1) { + if (filterQueryBuilder.getMetaAttributeCount() > 0) { for (String placeholder : filterQueryBuilder.getMetaAttributePlaceholders()) { sqlStmt = sqlStmt.replace("WHERE", String.format(INNER_JOIN_UM_ORG_ATTRIBUTE, placeholder, placeholder)); diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java index 0018a593..9ffca585 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/main/java/org/wso2/carbon/identity/organization/management/service/model/FilterQueryBuilder.java @@ -35,7 +35,7 @@ public class FilterQueryBuilder { private int count = 1; private String filter; private List metaAttributePlaceholders = new ArrayList<>(); - private int metaAttributeCount = 1; + private int metaAttributeCount; /** * Get filter query builder attributes. @@ -116,9 +116,9 @@ public List getMetaAttributePlaceholders() { */ public String generateMetaAttributePlaceholder() { + metaAttributeCount++; String placeholder = META_ATTRIBUTE_PLACEHOLDER_PREFIX + metaAttributeCount; metaAttributePlaceholders.add(placeholder); - metaAttributeCount++; return placeholder; } diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/OrganizationManagerImplTest.java b/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/OrganizationManagerImplTest.java index 50428d03..c3d1f73e 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/OrganizationManagerImplTest.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/OrganizationManagerImplTest.java @@ -387,6 +387,8 @@ public Object[][] dataForFilterOrganizationsByMetaAttributes() { return new Object[][]{ {"attributes.country co S", false}, + {"attributes.country co S and name eq Greater", false}, + {"attributes.country sw S and attributes.city ew o", false}, {"attributes.country co Z", true}, {"attributes.invalid co S", true} }; diff --git a/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImplTest.java b/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImplTest.java index 3f2389f4..f90dcea3 100644 --- a/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImplTest.java +++ b/components/org.wso2.carbon.identity.organization.management.service/src/test/java/org/wso2/carbon/identity/organization/management/service/dao/impl/OrganizationManagementDAOImplTest.java @@ -59,6 +59,8 @@ public class OrganizationManagementDAOImplTest { private static final String ATTRIBUTE_KEY = "country"; private static final String ATTRIBUTE_KEY_REGION = "region"; private static final String ATTRIBUTE_VALUE = "Sri Lanka"; + private static final String ATTRIBUTE_KEY_CITY = "city"; + private static final String ATTRIBUTE_VALUE_CITY = "Colombo"; private static final String ORG_NAME = "XYZ builders"; private static final String CHILD_ORG_NAME = "ABC builders"; private static final String ORG_DESCRIPTION = "This is a construction company."; @@ -76,6 +78,7 @@ public void setUp() throws Exception { orgId = generateUniqueID(); childOrgId = generateUniqueID(); storeChildOrganization(orgId, ORG_NAME, ORG_DESCRIPTION, SUPER_ORG_ID); + storeOrganizationAttributes(orgId, ATTRIBUTE_KEY_CITY, ATTRIBUTE_VALUE_CITY); storeChildOrganization(childOrgId, CHILD_ORG_NAME, ORG_DESCRIPTION, orgId); storeOrganizationAttributes(childOrgId, ATTRIBUTE_KEY_REGION, ATTRIBUTE_VALUE); } @@ -213,6 +216,38 @@ public void testFilterOrganizationsByMetaAttributes(String attributeValue, Strin SUPER_ORG_ID, "DESC", expressionNodes, new ArrayList<>()); Assert.assertEquals(organizations.get(0).getName(), ORG_NAME); + } + + @DataProvider(name = "dataForFilterOrganizationsByMultipleAttributes") + public Object[][] dataForFilterOrganizationsByMultipleAttributes() { + + return new Object[][]{ + {"attributes.country co S and name co XYZ"}, + {"attributes.country sw S and attributes.country ew a and attributes.city eq Colombo"}, + }; + } + + @Test(dataProvider = "dataForFilterOrganizationsByMultipleAttributes") + public void testFilterOrganizationsByMultipleAttributes(String filter) + throws OrganizationManagementServerException { + + TestUtils.mockCarbonContext(SUPER_ORG_ID); + List expressionNodes = new ArrayList<>(); + String[] filters = filter.split(" and "); + + for (String singleFilter : filters) { + String[] parts = singleFilter.split(" "); + String attributeValue = parts[0]; + String operation = parts[1]; + String value = parts[2]; + + ExpressionNode expressionNode = getExpressionNode(attributeValue, operation, value); + expressionNodes.add(expressionNode); + } + List organizations = organizationManagementDAO.getOrganizationsList(false, 10, + SUPER_ORG_ID, "DESC", expressionNodes, new ArrayList<>()); + + Assert.assertEquals(organizations.get(0).getName(), ORG_NAME); Assert.assertEquals(organizations.get(0).getAttributes().get(0).getKey(), ATTRIBUTE_KEY); Assert.assertEquals(organizations.get(0).getAttributes().get(0).getValue(), ATTRIBUTE_VALUE); }