Repository: incubator-falcon Updated Branches: refs/heads/master 35506d528 -> 20a13e3b4
FALCON-840 Possible NPE in filteredInstanceSet method of AbstractInstanceManager. Contributed by Balu Vellanki Project: http://git-wip-us.apache.org/repos/asf/incubator-falcon/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-falcon/commit/20a13e3b Tree: http://git-wip-us.apache.org/repos/asf/incubator-falcon/tree/20a13e3b Diff: http://git-wip-us.apache.org/repos/asf/incubator-falcon/diff/20a13e3b Branch: refs/heads/master Commit: 20a13e3b458526153a884ce6135162245ab016cd Parents: b49360e Author: Venkatesh Seetharam <venkat...@apache.org> Authored: Tue Oct 28 22:22:00 2014 -0700 Committer: Venkatesh Seetharam <venkat...@apache.org> Committed: Tue Oct 28 22:22:32 2014 -0700 ---------------------------------------------------------------------- CHANGES.txt | 3 + .../falcon/resource/AbstractEntityManager.java | 106 +++++++-------- .../resource/AbstractInstanceManager.java | 133 +++++++++++-------- .../AbstractSchedulableEntityManager.java | 1 + .../java/org/apache/falcon/cli/FalconCLIIT.java | 20 ++- 5 files changed, 140 insertions(+), 123 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/20a13e3b/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 625dd43..f83e4a8 100755 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -125,6 +125,9 @@ Trunk (Unreleased) OPTIMIZATIONS BUG FIXES + FALCON-840 Possible NPE in filteredInstanceSet method of + AbstractInstanceManager (Balu Vellanki via Venkatesh Seetharam) + FALCON-839 Authorization succeeds with invalid acl owner based on group membership (Venkatesh Seetharam) http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/20a13e3b/prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java ---------------------------------------------------------------------- diff --git a/prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java b/prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java index 23e0535..c4c6493 100644 --- a/prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java +++ b/prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java @@ -513,6 +513,7 @@ public abstract class AbstractEntityManager { String orderBy, String sortOrder, Integer offset, Integer resultsPerPage) { HashSet<String> fields = new HashSet<String>(Arrays.asList(fieldStr.toLowerCase().split(","))); + validateEntityFilterByClause(filterBy); List<Entity> entities; try { entities = getEntities(type, "", "", "", filterBy, filterTags, orderBy, sortOrder, offset, resultsPerPage); @@ -526,11 +527,23 @@ public abstract class AbstractEntityManager { : new EntityList(buildEntityElements(fields, entities)); } + protected void validateEntityFilterByClause(String entityFilterByClause) { + Map<String, String> filterByFieldsValues = getFilterByFieldsValues(entityFilterByClause); + for (Map.Entry<String, String> entry : filterByFieldsValues.entrySet()) { + try { + EntityList.EntityFilterByFields.valueOf(entry.getKey().toUpperCase()); + } catch (IllegalArgumentException e) { + throw FalconWebException.newInstanceException( + "Invalid filter key: " + entry.getKey(), Response.Status.BAD_REQUEST); + } + } + } + protected List<Entity> getEntities(String type, String startDate, String endDate, String cluster, String filterBy, String filterTags, String orderBy, String sortOrder, int offset, int resultsPerPage) throws FalconException { - final HashMap<String, String> filterByFieldsValues = getFilterByFieldsValues(filterBy); - final ArrayList<String> filterByTags = getFilterByTags(filterTags); + final Map<String, String> filterByFieldsValues = getFilterByFieldsValues(filterBy); + final List<String> filterByTags = getFilterByTags(filterTags); EntityType entityType = EntityType.valueOf(type.toUpperCase()); Collection<String> entityNames = configStore.getEntities(entityType); @@ -538,7 +551,7 @@ public abstract class AbstractEntityManager { return Collections.emptyList(); } - ArrayList<Entity> entities = new ArrayList<Entity>(); + List<Entity> entities = new ArrayList<Entity>(); for (String entityName : entityNames) { Entity entity; try { @@ -603,25 +616,24 @@ public abstract class AbstractEntityManager { return false; } - protected static HashMap<String, String> getFilterByFieldsValues(String filterBy) { - //Filter the results by specific field:value - HashMap<String, String> filterByFieldValues = new HashMap<String, String>(); + protected static Map<String, String> getFilterByFieldsValues(String filterBy) { + // Filter the results by specific field:value, eliminate empty values + Map<String, String> filterByFieldValues = new HashMap<String, String>(); if (!StringUtils.isEmpty(filterBy)) { String[] fieldValueArray = filterBy.split(","); for (String fieldValue : fieldValueArray) { String[] splits = fieldValue.split(":", 2); String filterByField = splits[0]; - if (splits.length == 2) { + if (splits.length == 2 && !splits[1].equals("")) { filterByFieldValues.put(filterByField, splits[1]); - } else { - filterByFieldValues.put(filterByField, ""); } } } + return filterByFieldValues; } - private static ArrayList<String> getFilterByTags(String filterTags) { + private static List<String> getFilterByTags(String filterTags) { ArrayList<String> filterTagsList = new ArrayList<String>(); if (!StringUtils.isEmpty(filterTags)) { String[] splits = filterTags.split(","); @@ -644,7 +656,7 @@ public abstract class AbstractEntityManager { } private boolean filterEntity(Entity entity, String entityStatus, - HashMap<String, String> filterByFieldsValues, ArrayList<String> filterByTags, + Map<String, String> filterByFieldsValues, List<String> filterByTags, List<String> tags, List<String> pipelines) { if (SecurityUtil.isAuthorizationEnabled() && !isEntityAuthorized(entity)) { // the user who requested list query has no permission to access this entity. Skip this entity @@ -670,7 +682,7 @@ public abstract class AbstractEntityManager { return true; } - private boolean filterEntityByTags(ArrayList<String> filterTagsList, List<String> tags) { + private boolean filterEntityByTags(List<String> filterTagsList, List<String> tags) { boolean filterEntity = false; for (String tag : filterTagsList) { if (!tags.contains(tag)) { @@ -682,64 +694,40 @@ public abstract class AbstractEntityManager { return filterEntity; } - private boolean filterEntityByFields(Entity entity, HashMap<String, String> filterKeyVals, + private boolean filterEntityByFields(Entity entity, Map<String, String> filterKeyVals, String status, List<String> pipelines) { - boolean filterEntity = false; - - if (filterKeyVals.size() != 0) { - String filterValue; - for (Map.Entry<String, String> pair : filterKeyVals.entrySet()) { - filterValue = pair.getValue(); - if (StringUtils.isEmpty(filterValue)) { - continue; // nothing to filter - } - EntityList.EntityFilterByFields filter = - EntityList.EntityFilterByFields.valueOf(pair.getKey().toUpperCase()); - switch (filter) { + for (Map.Entry<String, String> pair : filterKeyVals.entrySet()) { + String filterValue = pair.getValue(); + if (StringUtils.isEmpty(filterValue)) { + continue; // nothing to filter + } + EntityList.EntityFilterByFields filter = + EntityList.EntityFilterByFields.valueOf(pair.getKey().toUpperCase()); + switch (filter) { - case TYPE: - if (!entity.getEntityType().toString().equalsIgnoreCase(filterValue)) { - filterEntity = true; - } - break; + case TYPE: + return !entity.getEntityType().toString().equalsIgnoreCase(filterValue); - case NAME: - if (!entity.getName().equalsIgnoreCase(filterValue)) { - filterEntity = true; - } - break; + case NAME: + return !entity.getName().equalsIgnoreCase(filterValue); - case STATUS: - if (!status.equalsIgnoreCase(filterValue)) { - filterEntity = true; - } - break; + case STATUS: + return !status.equalsIgnoreCase(filterValue); - case PIPELINES: - if (entity.getEntityType().equals(EntityType.PROCESS) - && !pipelines.contains(filterValue)) { - filterEntity = true; - } - break; + case PIPELINES: + return entity.getEntityType().equals(EntityType.PROCESS) && !pipelines.contains(filterValue); - case CLUSTER: - Set<String> clusters = EntityUtil.getClustersDefined(entity); - if (!clusters.contains(filterValue)) { - filterEntity = true; - } + case CLUSTER: + return !EntityUtil.getClustersDefined(entity).contains(filterValue); - default: - break; - } - if (filterEntity) { - break; - } + default: + break; } } - return filterEntity; + return false; } - private ArrayList<Entity> sortEntities(ArrayList<Entity> entities, String orderBy, String sortOrder) { + private List<Entity> sortEntities(List<Entity> entities, String orderBy, String sortOrder) { // Sort the ArrayList using orderBy param if (!StringUtils.isEmpty(orderBy)) { EntityList.EntityFieldList orderByField = EntityList.EntityFieldList.valueOf(orderBy.toUpperCase()); http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/20a13e3b/prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java ---------------------------------------------------------------------- diff --git a/prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java b/prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java index fce28ad..8dd57ed 100644 --- a/prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java +++ b/prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java @@ -95,6 +95,7 @@ public abstract class AbstractInstanceManager extends AbstractEntityManager { try { lifeCycles = checkAndUpdateLifeCycle(lifeCycles, type); validateNotEmpty("entityName", entity); + validateInstanceFilterByClause(filterBy); AbstractWorkflowEngine wfEngine = getWorkflowEngine(); Entity entityObject = EntityUtil.getEntity(type, entity); return getInstanceResultSubset(wfEngine.getRunningInstances(entityObject, lifeCycles), @@ -105,6 +106,25 @@ public abstract class AbstractInstanceManager extends AbstractEntityManager { } } + protected void validateInstanceFilterByClause(String entityFilterByClause) { + Map<String, String> filterByFieldsValues = getFilterByFieldsValues(entityFilterByClause); + for (Map.Entry<String, String> entry : filterByFieldsValues.entrySet()) { + try { + InstancesResult.InstanceFilterFields filterKey = + InstancesResult.InstanceFilterFields .valueOf(entry.getKey()); + if (filterKey == InstancesResult.InstanceFilterFields.STARTEDAFTER) { + EntityUtil.parseDateUTC(entry.getValue()); + } + } catch (IllegalArgumentException e) { + throw FalconWebException.newInstanceException( + "Invalid filter key: " + entry.getKey(), Response.Status.BAD_REQUEST); + } catch (FalconException e) { + throw FalconWebException.newInstanceException( + "Invalid date value for key: " + entry.getKey(), Response.Status.BAD_REQUEST); + } + } + } + //SUSPEND CHECKSTYLE CHECK ParameterNumberCheck public InstancesResult getInstances(String type, String entity, String startStr, String endStr, String colo, List<LifeCycle> lifeCycles, @@ -123,6 +143,7 @@ public abstract class AbstractInstanceManager extends AbstractEntityManager { try { lifeCycles = checkAndUpdateLifeCycle(lifeCycles, type); validateParams(type, entity); + validateInstanceFilterByClause(filterBy); Entity entityObject = EntityUtil.getEntity(type, entity); Pair<Date, Date> startAndEndDate = getStartAndEndDate(entityObject, startStr, endStr); @@ -149,7 +170,8 @@ public abstract class AbstractInstanceManager extends AbstractEntityManager { Pair<Date, Date> startAndEndDate = getStartAndEndDate(entityObject, startStr, endStr); AbstractWorkflowEngine wfEngine = getWorkflowEngine(); - return wfEngine.getSummary(entityObject, startAndEndDate.first, startAndEndDate.second, lifeCycles); + return wfEngine.getSummary(entityObject, startAndEndDate.first, startAndEndDate.second, + lifeCycles); } catch (Throwable e) { LOG.error("Failed to get instances status", e); throw FalconWebException.newInstanceSummaryException(e, Response.Status.BAD_REQUEST); @@ -179,10 +201,8 @@ public abstract class AbstractInstanceManager extends AbstractEntityManager { } private InstancesResult getInstanceResultSubset(InstancesResult resultSet, String filterBy, - String orderBy, String sortOrder, - Integer offset, Integer numResults) { - - ArrayList<Instance> instanceSet = new ArrayList<Instance>(); + String orderBy, String sortOrder, Integer offset, + Integer numResults) throws FalconException { if (resultSet.getInstances() == null) { // return the empty resultSet resultSet.setInstances(new Instance[0]); @@ -190,7 +210,8 @@ public abstract class AbstractInstanceManager extends AbstractEntityManager { } // Filter instances - instanceSet = filteredInstanceSet(resultSet, instanceSet, getFilterByFieldsValues(filterBy)); + Map<String, String> filterByFieldsValues = getFilterByFieldsValues(filterBy); + List<Instance> instanceSet = getFilteredInstanceSet(resultSet, filterByFieldsValues); int pageCount = super.getRequiredNumberOfResults(instanceSet.size(), offset, numResults); InstancesResult result = new InstancesResult(resultSet.getStatus(), resultSet.getMessage()); @@ -201,69 +222,65 @@ public abstract class AbstractInstanceManager extends AbstractEntityManager { } // Sort the ArrayList using orderBy instanceSet = sortInstances(instanceSet, orderBy, sortOrder); - result.setCollection(instanceSet.subList(offset, (offset+pageCount)).toArray(new Instance[pageCount])); + result.setCollection(instanceSet.subList( + offset, (offset+pageCount)).toArray(new Instance[pageCount])); return result; } - private ArrayList<Instance> filteredInstanceSet(InstancesResult resultSet, ArrayList<Instance> instanceSet, - HashMap<String, String> filterByFieldsValues) { - - for (Instance instance : resultSet.getInstances()) { - boolean addInstance = true; - // If filterBy is empty, return all instances. Else return instances with matching filter. - if (filterByFieldsValues.size() > 0) { - String filterValue; - for (Map.Entry<String, String> pair : filterByFieldsValues.entrySet()) { - filterValue = pair.getValue(); - if (filterValue.equals("")) { - continue; - } - try { - switch (InstancesResult.InstanceFilterFields.valueOf(pair.getKey().toUpperCase())) { - case STATUS: - String status = ""; - if (instance.getStatus() != null) { - status = instance.getStatus().toString(); - } - if (!status.equalsIgnoreCase(filterValue)) { - addInstance = false; - } - break; - case CLUSTER: - if (!instance.getCluster().equalsIgnoreCase(filterValue)) { - addInstance = false; - } - break; - case SOURCECLUSTER: - if (!instance.getSourceCluster().equalsIgnoreCase(filterValue)) { - addInstance = false; - } - break; - case STARTEDAFTER: - if (instance.getStartTime().before(EntityUtil.parseDateUTC(filterValue))) { - addInstance = false; - } - break; - default: - break; - } - } catch (Exception e) { - LOG.error("Invalid entry for filterBy field", e); - throw FalconWebException.newInstanceException(e, Response.Status.BAD_REQUEST); - } - if (!addInstance) { - break; - } + private List<Instance> getFilteredInstanceSet(InstancesResult resultSet, + Map<String, String> filterByFieldsValues) + throws FalconException { + // If filterBy is empty, return all instances. Else return instances with matching filter. + if (filterByFieldsValues.size() == 0) { + return Arrays.asList(resultSet.getInstances()); + } + + List<Instance> instanceSet = new ArrayList<Instance>(); + for (Instance instance : resultSet.getInstances()) { // for each instance + boolean isInstanceFiltered = false; + + // for each filter + for (Map.Entry<String, String> pair : filterByFieldsValues.entrySet()) { + if (isInstanceFiltered(instance, pair)) { // wait until all filters are applied + isInstanceFiltered = true; + break; // no use to continue other filters as the current one filtered this } } - if (addInstance) { + + if (!isInstanceFiltered) { // survived all filters instanceSet.add(instance); } } + return instanceSet; } - private ArrayList<Instance> sortInstances(ArrayList<Instance> instanceSet, + private boolean isInstanceFiltered(Instance instance, + Map.Entry<String, String> pair) throws FalconException { + final String filterValue = pair.getValue(); + switch (InstancesResult.InstanceFilterFields.valueOf(pair.getKey().toUpperCase())) { + case STATUS: + return instance.getStatus() == null + || !instance.getStatus().toString().equalsIgnoreCase(filterValue); + + case CLUSTER: + return instance.getCluster() == null + || !instance.getCluster().equalsIgnoreCase(filterValue); + + case SOURCECLUSTER: + return instance.getSourceCluster() == null + || !instance.getSourceCluster().equalsIgnoreCase(filterValue); + + case STARTEDAFTER: + return instance.getStartTime() == null + || instance.getStartTime().before(EntityUtil.parseDateUTC(filterValue)); + + default: + return true; + } + } + + private List<Instance> sortInstances(List<Instance> instanceSet, String orderBy, String sortOrder) { final String order = getValidSortOrder(sortOrder, orderBy); if (orderBy.equals("status")) { http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/20a13e3b/prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java ---------------------------------------------------------------------- diff --git a/prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java b/prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java index eb89f5f..687fde9 100644 --- a/prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java +++ b/prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java @@ -176,6 +176,7 @@ public abstract class AbstractSchedulableEntityManager extends AbstractInstanceM Integer resultsPerPage, Integer numInstances) { HashSet<String> fieldSet = new HashSet<String>(Arrays.asList(fields.toLowerCase().split(","))); Pair<Date, Date> startAndEndDates = getStartEndDatesForSummary(startDate, endDate); + validateEntityFilterByClause(filterBy); List<Entity> entities; String colo; http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/20a13e3b/webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java ---------------------------------------------------------------------- diff --git a/webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java b/webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java index c9b03c5..88aee0c 100644 --- a/webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java +++ b/webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java @@ -29,10 +29,10 @@ import org.testng.annotations.Test; import java.io.File; import java.io.FileOutputStream; +import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.io.PrintStream; -import java.io.IOException; import java.util.Date; import java.util.Map; import java.util.Properties; @@ -483,10 +483,23 @@ public class FalconCLIIT { + " -filterBy STATUS:SUCCEEDED,STARTEDAFTER:"+START_INSTANCE + " -orderBy startTime -sortOrder desc -offset 0 -numResults 1")); Assert.assertEquals(0, + executeWithURL("instance -status -type process -name " + + overlay.get("processName") + + " -start "+ START_INSTANCE + + " -filterBy SOURCECLUSTER:"+ overlay.get("cluster") + + " -orderBy startTime -sortOrder desc -offset 0 -numResults 1")); + + Assert.assertEquals(0, executeWithURL("instance -list -type feed -lifecycle eviction -name " + overlay.get("outputFeedName") + " -start "+ SchemaHelper.getDateFormat().format(new Date()) +" -filterBy STATUS:SUCCEEDED -orderBy startTime -offset 0 -numResults 1")); + Assert.assertEquals(0, + executeWithURL("instance -list -type feed -lifecycle eviction -name " + + overlay.get("outputFeedName") + + " -start "+ SchemaHelper.getDateFormat().format(new Date()) + +" -filterBy SOURCECLUSTER:" + overlay.get("src.cluster.name") + + " -orderBy startTime -offset 0 -numResults 1")); Assert.assertEquals(-1, executeWithURL("instance -status -type feed -lifecycle eviction -name " + overlay.get("outputFeedName") @@ -502,11 +515,6 @@ public class FalconCLIIT { + overlay.get("outputFeedName") + " -start "+ SchemaHelper.getDateFormat().format(new Date()) +" -filterBy STATUS:SUCCEEDED -orderBy startTime -offset 1 -numResults 1")); - Assert.assertEquals(0, - executeWithURL("instance -list -type feed -lifecycle eviction -name " - + overlay.get("outputFeedName") - + " -start "+ SchemaHelper.getDateFormat().format(new Date()) - +" -filterBy STATUS:SUCCEEDED -offset 0 -numResults 1")); // When you get a cluster for which there are no feed entities, Assert.assertEquals(0, executeWithURL("entity -summary -type feed -cluster " + overlay.get("cluster") + " -fields status,tags"
