address review comments

CHANGELOG.md

@@ -12,7 +12,7 @@ Current
 - [Enrich the ApiJobStore interface] (https://github.com/yahoo/fili/pull/23)
     * `ApiJobStore` Interface now supports filtering `JobRows` in the store
     * Added support for filtering JobRows in `HashJobStore`
-    * Added `ApiJobStoreFilter` to hold filter information
+    * Added `JobRowFilter` to hold filter information
 
 ### Deprecated:
 

fili-core/src/main/java/com/yahoo/bard/webservice/async/jobs/stores/ApiJobStore.java

@@ -55,19 +55,19 @@ public interface ApiJobStore {
     Observable<JobRow> getAllRows();
 
     /**
-     * This method takes a Set of ApiJobStoreFilters, ANDS them by default, and returns a cold observable that emits a
+     * This method takes a Set of JobRowFilters, ANDS them by default, and returns a cold observable that emits a
      * stream of JobRows which satisfy the given filter.
      * <p>
-     * Every field may not be filterable for every implementation of the `ApiJobStore` as the efficiency of filtering is
-     * dependent on the backing store. An IllegalArgumentException is thrown if filtering on any given field is not
-     * supported.
+     * Every field may not be filterable for every implementation of the {@code ApiJobStore} as the efficiency of
+     * filtering is dependent on the backing store. An IllegalArgumentException is thrown if filtering on any given
+     * field is not supported.
      *
-     * @param apiJobStoreFilters  A List of ApiJobStoreFilters where each ApiJobStoreFilter contains the JobField to be
+     * @param jobRowFilters  A List of JobRowFilters where each JobRowFilter contains the JobField to be
      * filtered on, the filter operation and the values to be compared to.
      *
      * @return An Observable that emits a stream of JobRows that satisfy the given filters
      *
      * @throws IllegalArgumentException if filtering on any field is not supported
      */
-    Observable<JobRow> getFilteredRows(Set<ApiJobStoreFilter> apiJobStoreFilters) throws IllegalArgumentException;
+    Observable<JobRow> getFilteredRows(Set<JobRowFilter> jobRowFilters) throws IllegalArgumentException;
 }

fili-core/src/main/java/com/yahoo/bard/webservice/async/jobs/stores/HashJobStore.java

@@ -2,10 +2,12 @@
 // Licensed under the terms of the Apache license. Please see LICENSE file distributed with this work for terms.
 package com.yahoo.bard.webservice.async.jobs.stores;
 
+import static com.yahoo.bard.webservice.web.ErrorMessageFormat.FILTER_OPERATOR_INVALID;
+import static com.yahoo.bard.webservice.web.ErrorMessageFormat.JOBFIELD_NOT_PRESENT_IN_JOB_META_DATA;
+
 import com.yahoo.bard.webservice.async.jobs.jobrows.JobField;
 import com.yahoo.bard.webservice.async.jobs.jobrows.JobRow;
-
-import static com.yahoo.bard.webservice.web.ErrorMessageFormat.JOBFIELD_NOT_PRESENT_IN_JOB_META_DATA;
+import com.yahoo.bard.webservice.web.FilterOperation;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -15,7 +17,6 @@
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Set;
-import java.util.stream.Collectors;
 
 /**
  * An ApiJobStore backed by an in-memory map. This is meant as a stub implementation for
@@ -63,40 +64,55 @@ public Observable<JobRow> getAllRows() {
     }
 
     @Override
-    public Observable<JobRow> getFilteredRows(Set<ApiJobStoreFilter> apiJobStoreFilters)
+    public Observable<JobRow> getFilteredRows(Set<JobRowFilter> jobRowFilters)
             throws IllegalArgumentException {
-        return Observable.from(
-                store.entrySet().stream()
-                        .filter(entry -> satisfiesFilters(apiJobStoreFilters, entry.getValue()))
-                        .map(entry -> entry.getValue())
-                        .collect(Collectors.toSet())
-        );
+        return getAllRows().filter(jobRow -> satisfiesFilters(jobRowFilters, jobRow));
     }
 
     /**
-     * This method checks if the given JobRow satisfies all the ApiJobStoreFilters and returns true if it does.
+     * This method checks if the given JobRow satisfies all the JobRowFilters and returns true if it does.
      * If a JobField in any of the filters is not a part the JobRow, this method throws a IllegalArgumentException.
      *
-     * @param apiJobStoreFilters  A Set of ApiJobStoreFilters specifying the different conditions to be satisfied
+     * @param jobRowFilters  A Set of JobRowFilters specifying the different conditions to be satisfied
      * @param jobRow  The JobRow which needs to be inspected
      *
      * @return true if the JobRow satisfies all the filters, false otherwise
      *
      * @throws IllegalArgumentException if a JobField in any of the filters is not a part the JobRow
      */
-    private boolean satisfiesFilters(Set<ApiJobStoreFilter> apiJobStoreFilters, JobRow jobRow)
+    private boolean satisfiesFilters(Set<JobRowFilter> jobRowFilters, JobRow jobRow)
             throws IllegalArgumentException {
-        for (ApiJobStoreFilter filter : apiJobStoreFilters) {
-            JobField jobField = filter.getJobField();
-            if (!jobRow.containsKey(jobField)) {
-                Set<JobField> jobFields = jobRow.keySet();
-                LOG.debug(JOBFIELD_NOT_PRESENT_IN_JOB_META_DATA.logFormat(jobField, jobFields));
-                throw new IllegalArgumentException(JOBFIELD_NOT_PRESENT_IN_JOB_META_DATA.format(jobField, jobFields));
-            }
-
-            if (!filter.getValues().contains(jobRow.get(jobField))) {
-                return false;
-            }
+        return jobRowFilters.stream().allMatch(filter -> satisfiesFilter(jobRow, filter));
+    }
+
+    /**
+     * This method checks if the given JobRow satisfies the given JobRowFilter and returnd true if it does.
+     * If a JobField in the filter is not a part the JobRow, this method throws a IllegalArgumentException.
+     *
+     * @param jobRow  The JobRow which needs to be inspected
+     * @param jobRowFilter  A JobRowFilter specifying the filter condition
+     *
+     * @return true if the JobRow satisfies the filter, false otherwise
+     *
+     * @throws IllegalArgumentException if a JobField in the filter is not a part the JobRow
+     */
+    private boolean satisfiesFilter(JobRow jobRow, JobRowFilter jobRowFilter) throws IllegalArgumentException {
+        JobField filterJobField = jobRowFilter.getJobField();
+        FilterOperation filterOperation = jobRowFilter.getOperation();
+        if (!filterOperation.equals(FilterOperation.eq)) {
+            LOG.debug(FILTER_OPERATOR_INVALID.logFormat(filterOperation));
+            throw new IllegalArgumentException(FILTER_OPERATOR_INVALID.format(filterOperation));
+        }
+        if (!jobRow.containsKey(filterJobField)) {
+            Set<JobField> actualJobFields = jobRow.keySet();
+            LOG.debug(JOBFIELD_NOT_PRESENT_IN_JOB_META_DATA.logFormat(filterJobField, actualJobFields));
+            throw new IllegalArgumentException(
+                    JOBFIELD_NOT_PRESENT_IN_JOB_META_DATA.format(filterJobField, actualJobFields)
+            );
+        }
+
+        if (!jobRowFilter.getValues().contains(jobRow.get(filterJobField))) {
+            return false;
         }
         return true;
     }

fili-core/src/main/java/com/yahoo/bard/webservice/async/jobs/stores/ApiJobStoreFilter.java → fili-core/src/main/java/com/yahoo/bard/webservice/async/jobs/stores/JobRowFilter.java

@@ -11,10 +11,12 @@
 import com.yahoo.bard.webservice.async.jobs.jobrows.JobField;
 import com.yahoo.bard.webservice.util.FilterTokenizer;
 import com.yahoo.bard.webservice.web.BadFilterException;
+import com.yahoo.bard.webservice.web.FilterOperation;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.util.Arrays;
 import java.util.LinkedHashSet;
 import java.util.Objects;
 import java.util.Set;
@@ -26,11 +28,11 @@
 /**
  * Class containing filter information to filter JobRows in ApiJobStore.
  */
-public class ApiJobStoreFilter {
-    private static final Logger LOG = LoggerFactory.getLogger(ApiJobStoreFilter.class);
+public class JobRowFilter {
+    private static final Logger LOG = LoggerFactory.getLogger(JobRowFilter.class);
 
     private final JobField jobField;
-    private final ApiJobStoreFilterOperation operation;
+    private final FilterOperation operation;
     private final Set<String> values;
 
     /*  url filter query pattern:  (JobField name)-(operation)[(value or comma separated numeric values)]?
@@ -44,7 +46,7 @@ public class ApiJobStoreFilter {
     private static final Pattern QUERY_PATTERN = Pattern.compile("([^\\|]+)-([^\\[]+)\\[([^\\]]+)\\]?");
 
     /**
-     * Parses the URL ApiJobStore filter query and generates the ApiJobStoreFilter object.
+     * Parses the URL ApiJobStore filter query and generates the JobRowFilter object.
      *
      * @param filterQuery  Expects a URL ApiJobStore filter query String in the format:
      * <p>
@@ -53,7 +55,7 @@ public class ApiJobStoreFilter {
      * @throws BadFilterException when filter pattern is not matched or when any of its properties are not
      * valid.
      */
-    public ApiJobStoreFilter(@NotNull String filterQuery) throws BadFilterException {
+    public JobRowFilter(@NotNull String filterQuery) throws BadFilterException {
         LOG.trace("filterQuery: {}", filterQuery);
 
         Matcher tokenizedQuery = QUERY_PATTERN.matcher(filterQuery);
@@ -70,13 +72,13 @@ public ApiJobStoreFilter(@NotNull String filterQuery) throws BadFilterException
     }
 
     /**
-     * Constructor for an ApiJobStoreFilter object whose data has already been parsed.
+     * Constructor for an JobRowFilter object whose data has already been parsed.
      *
      * @param jobField  The JobField to perform the filtering on
      * @param operation  The operation to perform (eg: eq)
      * @param values  A Set of Strings to compare the JobField's value to.
      */
-    private ApiJobStoreFilter(JobField jobField, ApiJobStoreFilterOperation operation, Set<String> values) {
+    private JobRowFilter(JobField jobField, FilterOperation operation, Set<String> values) {
         this.jobField = jobField;
         this.operation = operation;
         this.values = values;
@@ -86,7 +88,7 @@ public JobField getJobField() {
         return jobField;
     }
 
-    public ApiJobStoreFilterOperation getOperation() {
+    public FilterOperation getOperation() {
         return operation;
     }
 
@@ -95,85 +97,86 @@ public Set<String> getValues() {
     }
 
     /**
-     * Construct an ApiJobStoreFilter object using the same ApiJobStoreFilterOperation and values as the object on
+     * Construct an JobRowFilter object using the same FilterOperation and values as the object on
      * which this method is called and using the supplied JobField.
      *
      * @param jobField  The JobField to perform the filtering on
      *
-     * @return An instance of ApiJobStoreFilter created using the supplied JobField
+     * @return An instance of JobRowFilter created using the supplied JobField
      */
-    public ApiJobStoreFilter withJobField(JobField jobField) {
-        return new ApiJobStoreFilter(jobField, operation, values);
+    public JobRowFilter withJobField(JobField jobField) {
+        return new JobRowFilter(jobField, operation, values);
     }
 
     /**
-     * Construct an ApiJobStoreFilter object using the same JobField and values as the object on
-     * which this method is called and using the supplied ApiJobStoreFilterOperation.
+     * Construct an JobRowFilter object using the same JobField and values as the object on
+     * which this method is called and using the supplied FilterOperation.
      *
      * @param operation  The operation to perform (eg: eq)
      *
-     * @return An instance of ApiJobStoreFilter created using the supplied ApiJobStoreFilterOperation
+     * @return An instance of JobRowFilter created using the supplied FilterOperation
      */
-    public ApiJobStoreFilter withOperation(ApiJobStoreFilterOperation operation) {
-        return new ApiJobStoreFilter(jobField, operation, values);
+    public JobRowFilter withOperation(FilterOperation operation) {
+        return new JobRowFilter(jobField, operation, values);
     }
 
     /**
-     * Construct an ApiJobStoreFilter object using the same JobField and ApiJobStoreFilterOperation as the object on
+     * Construct an JobRowFilter object using the same JobField and FilterOperation as the object on
      * which this method is called and using the supplied values.
      *
      * @param values  A Set of Strings to compare the JobField's value to
      *
-     * @return  An instance of ApiJobStoreFilter created using the supplied values
+     * @return  An instance of JobRowFilter created using the supplied values
      */
-    public ApiJobStoreFilter withValues(Set<String> values) {
-        return new ApiJobStoreFilter(jobField, operation, values);
+    public JobRowFilter withValues(Set<String> values) {
+        return new JobRowFilter(jobField, operation, values);
     }
 
     /**
      * Extracts the JobField to be examined from the tokenizedQuery.
      *
-     * @param tokenizedQuery  The parsed ApiJobStore filter tokenizedQuery.
+     * @param tokenizedQuery  The tokenized filter expression.
      *
      * @return  The JobField to be examined
      * @throws BadFilterException is the JobField does not exist
      */
     private JobField extractJobField(Matcher tokenizedQuery) throws BadFilterException {
         String fieldName = tokenizedQuery.group(1);
-        for (JobField field : DefaultJobField.values()) {
-            if (field.getName().equals(fieldName)) {
-                return field;
-            }
-        }
-        LOG.debug(FILTER_JOBFIELD_UNDEFINED.logFormat(fieldName));
-        throw new BadFilterException(FILTER_JOBFIELD_UNDEFINED.format(fieldName));
+
+        return Arrays.stream(DefaultJobField.values())
+                .filter(field -> field.getName().equals(fieldName))
+                .findFirst()
+                .orElseThrow(() -> {
+                            LOG.debug(FILTER_JOBFIELD_UNDEFINED.logFormat(fieldName));
+                            return new BadFilterException(FILTER_JOBFIELD_UNDEFINED.format(fieldName));
+                });
     }
 
     /**
      * Extracts the operation to be performed by the ApiJobStore filter query.
      *
-     * @param tokenizedQuery  The parsed ApiJobStore filter tokenizedQuery.
+     * @param tokenizedQuery  The tokenized filter expression..
      *
      * @return The operation to be performed by the ApiJobStore filter query.
      * @throws BadFilterException if the operation does not exist
      */
-    private ApiJobStoreFilterOperation extractOperation(Matcher tokenizedQuery) throws BadFilterException {
+    private FilterOperation extractOperation(Matcher tokenizedQuery) throws BadFilterException {
         String operationName = tokenizedQuery.group(2);
         try {
-            return ApiJobStoreFilterOperation.valueOf(operationName);
+            return FilterOperation.valueOf(operationName);
         } catch (IllegalArgumentException ignored) {
             LOG.debug(FILTER_OPERATOR_INVALID.logFormat(operationName));
             throw new BadFilterException(FILTER_OPERATOR_INVALID.format(operationName));
         }
     }
 
     /**
-     * Extracts the values to be used in the ApiJobStoreFilter query from the query.
+     * Extracts the values to be used in the JobRowFilter query from the query.
      *
-     * @param tokenizedQuery  The parsed ApiJobStore filter tokenizedQuery.
+     * @param tokenizedQuery  The tokenized filter expression..
      * @param filterQuery  The raw query. Used for logging.
      *
-     * @return The set of values to be used in the ApiJobStoreFilter query.
+     * @return The set of values to be used in the JobRowFilter query.
      * @throws BadFilterException If the fragment of the query that specifies the values is malformed.
      */
     private Set<String> extractValues(Matcher tokenizedQuery, String filterQuery) throws BadFilterException {
@@ -196,14 +199,14 @@ private Set<String> extractValues(Matcher tokenizedQuery, String filterQuery) th
     @Override
     public boolean equals(final Object o) {
         if (this == o) { return true; }
-        if (!(o instanceof ApiJobStoreFilter)) { return false; }
+        if (!(o instanceof JobRowFilter)) { return false; }
 
-        ApiJobStoreFilter apiJobStoreFilter = (ApiJobStoreFilter) o;
+        JobRowFilter jobRowFilter = (JobRowFilter) o;
 
         return
-                Objects.equals(jobField, apiJobStoreFilter.jobField) &&
-                        Objects.equals(operation, apiJobStoreFilter.operation) &&
-                        Objects.equals(values, apiJobStoreFilter.values);
+                Objects.equals(jobField, jobRowFilter.jobField) &&
+                        Objects.equals(operation, jobRowFilter.operation) &&
+                        Objects.equals(values, jobRowFilter.values);
     }
 
     @Override

fili-core/src/main/java/com/yahoo/bard/webservice/async/jobs/stores/NoOpApiJobStore.java

@@ -34,7 +34,7 @@ public Observable<JobRow> getAllRows() {
     }
 
     @Override
-    public Observable<JobRow> getFilteredRows(Set<ApiJobStoreFilter> apiJobStoreFilter)
+    public Observable<JobRow> getFilteredRows(Set<JobRowFilter> jobRowFilter)
             throws IllegalArgumentException {
                 return Observable.empty();
     }