Allow or filtering in source #12

nbelisle11 · 2018-10-04T03:38:00Z

Step 1 towards #4
Represents replaces the List on ParsedQuery with a CombinedFilterEntry. Which has an Operator (AND, OR) and itself has a List (which BoundFilterEntries now extend). This allows us to have nested complex where conditions as opposed to the previous simple ANDed list.

Other improvements

Updating to latest basepom as we were way behind
Sorted poms as I was getting errors in the sortpom-maven-plugin for not sorted poms
Fixed deprecation warnings on Jackson and Throwables.propagte

lcmartinez

This will be a great improvement to the library, thank you!

My comments/questions were mainly around testing/docs/readability. The approach seems sound to me, though.

lcmartinez · 2018-10-04T16:42:58Z

httpql/src/main/java/com/hubspot/httpql/ParsedQuery.java

+   * Similar to {@link #addFilter} but removes all existing filters for {@code fieldName} first.
+   *
+   * @param fieldName
+   *          Name as seen in the query; not multi-value proxies ("id", not "ids")


I see that our other addFilter (and similar) methods' docstrings don't include @param docs for the other params besides fieldName, but feels like it would be useful to briefly describe what filterEntryConditionCreator represents and does here, since it's a bit more complex.

lcmartinez · 2018-10-04T16:46:32Z

httpql/src/main/java/com/hubspot/httpql/ParsedQuery.java

+   * @param fieldName
+   *          Name as seen in the query; not multi-value proxies ("id", not "ids")
+   */
+  public void addFilterEntryConditionCreatorExclusively(String fieldName,


Seems like there should be test coverage for this in ParsedQueryTest.

lcmartinez · 2018-10-04T16:54:10Z

httpql/src/main/java/com/hubspot/httpql/ParsedQuery.java

-      }
-    }
-    return removed;
+    return combinedFilterEntry.removeAllFiltersFor(fieldName);


Looks like this cleans up a lot. Nice.

lcmartinez · 2018-10-04T16:59:38Z

httpql/src/main/java/com/hubspot/httpql/ann/Aggregated.java

+@Target({
+    ElementType.FIELD, ElementType.METHOD
+})
+public @interface Aggregated {


It would be helpful to use this in one of the test model/entity classes to demonstrate how it's intended to be used.

I actually ended up removing this in favor of updating FilterBy

Ok cool, ok, so no need for this file anymore.

lcmartinez · 2018-10-04T17:15:08Z

httpql/src/main/java/com/hubspot/httpql/impl/SelectBuilder.java

@@ -277,7 +277,7 @@ public String getFieldName(String name) {
    select = selectFrom.from(table);
    for (JoinCondition joinCondition : joinConditions) {
      if (joinCondition.isLeftJoin()) {
-        ((SelectJoinStep<?>) select).leftOuterJoin(joinCondition.getTable()).on(joinCondition.getCondition());
+        ((SelectJoinStep<?>) select).leftJoin(joinCondition.getTable()).on(joinCondition.getCondition());


Just to verify my understanding.. leftJoin is equivalent to leftOuterJoin right?

leftOuterJoin is a convenience method for leftJoin. Seemed odd that we were inconsistent in some places so I cleaned this up.

lcmartinez · 2018-10-04T17:36:04Z

httpql/src/main/java/com/hubspot/httpql/internal/CombinedFilterEntry.java

+        .getQueryName()
+        .equals(fieldName));
+    boolean removedFromChildren = conditionCreators.stream()
+        .filter(cc -> cc instanceof CombinedFilterEntry)


It looks like you're doing this instanceof logic a lot to identify leaf nodes vs. combinations... would it be cleaner to add an isCombinedCondition helper (and/or isLeafCondition, or something like that) to FilterEntryConditionCreator that does the instanceof checks?

I've removed most of these. I'll think about this some more but my knee jerk is that it seems odd to have helpers defined in the interface / higher level classes for checking if the instances are of a specific subclass type.

Could be an actual interface method (i.e. not actually implemented in FilterEntryConditionCreator), where the implementation in CombinedFilterEntry simply returns false (not a leaf node) and the one in BoundFilterEntry returns true (is a leaf node).

Ahh sorry, I missed your follow up here. Yea that doesn't seem so bad. I guess I'm not sure what it would buy though in the implementation. Every instanceOf check I do is right before a cast and I wouldn't feel comfortable casting without that anyways.

lcmartinez · 2018-10-04T17:42:24Z

httpql/src/main/java/com/hubspot/httpql/internal/CombinedFilterEntry.java

+    return removedFromThis || removedFromChildren;
+  }
+
+  public Optional<BoundFilterEntry<T>> getFirstFilterForFieldName(String fieldName) {


I'm not sure I totally understand what a "first" filter means in this kind of tree structure, or what it's used for. It looks like it'll first look through all top-level "leaf" filters and then dive into the combined filters... Will that find the "first"?

What about using getFlattenedBoundFilterEntries for this?

Yea this is a part of the API that was a little odd to maintain in the new structure. I think getFlattenedBoundFilterEntries should be fine actually.

lcmartinez · 2018-10-04T17:43:13Z

httpql/src/main/java/com/hubspot/httpql/internal/CombinedFilterEntry.java

+    return maybeBfe;
+  }
+
+  public List<BoundFilterEntry<T>> getAllFiltersForFieldName(String fieldName) {


Again, what about using getFlattenedBoundFilterEntries() for this?

lcmartinez · 2018-10-04T17:44:49Z

httpql/src/main/java/com/hubspot/httpql/impl/QueryParser.java

-          .get(filterName))).findFirst();
+      Optional<BoundFilterEntry<T>> filterEntryOptional = filterTable.rowKeySet().stream().filter(f -> Objects.equals(f
+          .getFieldName(), finalFieldName) && Objects.equals(f.getFilter(), UriParamParser.BY_NAME
+              .get(filterName))).findFirst();


Weird wrapping here again.

lcmartinez · 2018-10-04T17:45:21Z

httpql/src/main/java/com/hubspot/httpql/impl/QueryParser.java


      // Use reserved words instead of simple look-up to throw exception on disallowed fields
-      if (!filterEntryOptional.isPresent() || (!RESERVED_WORDS.contains(filterEntryOptional.get().getQueryName()) && !filterTable.contains(filterEntryOptional.get(), filterName))) {
+      if (!filterEntryOptional.isPresent() || (!RESERVED_WORDS.contains(filterEntryOptional.get().getQueryName()) && !filterTable
+          .contains(filterEntryOptional.get(), filterName))) {


And again 😄

boulter

I'm not sure I grok all the details here as I haven't worked much in this repo, but I left a few comments. Is there an example of what the query string would look like that this would be able to parse?

boulter · 2018-10-04T18:11:35Z

httpql/src/main/java/com/hubspot/httpql/ParsedQuery.java

@@ -45,7 +43,7 @@
  public ParsedQuery(
                     T boundQuerySpec,
                     Class<T> queryType,
-                     List<BoundFilterEntry<T>> boundFilterEntries,


this is a breaking change, right?

Nice catch, I'll fix

boulter · 2018-10-04T18:16:00Z

httpql/src/main/java/com/hubspot/httpql/internal/CombinedFilterEntry.java

+          .filter(cc -> cc instanceof CombinedFilterEntry)
+          .map(cc -> ((CombinedFilterEntry<T>) cc))
+          .collect(Collectors.toList());
+      for (CombinedFilterEntry<T> combinedFilterEntry : combinedFilterEntries) {


why collect to a list just to iterate over that list? You could just use map and findAny.

boulter · 2018-10-04T18:25:46Z

httpql/src/main/java/com/hubspot/httpql/internal/CombinedFilterEntry.java

+import com.hubspot.httpql.FieldFactory;
+import com.hubspot.httpql.QuerySpec;
+
+public class CombinedFilterEntry<T extends QuerySpec> implements FilterEntryConditionCreator<T> {


Maybe this is just a FilterEntryClause or BooleanFilterEntryClause? Combined doesn't really give any more information about what it combines. Or maybe use "expression" or "term" if we're creating boolean syntax trees?

I like the symmetry of the combined term with the CombinedCondition in jooq, which is really the plumbing of httpQL. Agree that FilterEntry doesn't make much sense so I went with CombinedConditionCreator instead

nbelisle11 · 2018-10-04T19:15:31Z

@boulter this isn't a change to actual parsing yet, there's a few more steps required for that. This is a step in that direction that will allow for modifying or augmenting ParsedQuerys with more complex where conditions with nested OR clauses

lcmartinez · 2018-10-05T03:28:53Z

httpql/src/main/java/com/hubspot/httpql/ParsedQuery.java

@@ -30,7 +29,8 @@

  private final Class<T> queryType;

-  private final List<BoundFilterEntry<T>> boundFilterEntries;
+  private final CombinedConditionCreator<T> combinedFilterEntry;


should probably rename the variable here to match the new classname

lcmartinez · 2018-10-05T03:30:35Z

httpql/src/main/java/com/hubspot/httpql/ann/Aggregated.java

+@Target({
+    ElementType.FIELD, ElementType.METHOD
+})
+public @interface Aggregated {


Ok cool, ok, so no need for this file anymore.

lcmartinez · 2018-10-05T03:35:41Z

httpql/src/main/java/com/hubspot/httpql/ann/FilterBy.java

@@ -25,4 +25,6 @@
  Class<? extends Filter>[] value();

  String as() default "";
+
+  Class<?> typeOverride() default void.class;


I'm actually not sure I understand what this annotation param does. Could you add it to the docstring above and to the tests?

lcmartinez · 2018-10-05T03:38:21Z

httpql/src/main/java/com/hubspot/httpql/impl/QueryParser.java

@@ -181,18 +191,20 @@ protected void buildOrderableFields(final Map<String, BeanPropertyDefinition> fi
      boundFilterEntries.add(boundColumn);
    }

+    CombinedConditionCreator<T> combinedFilterEntry = new CombinedConditionCreator<>(Operator.AND, Lists.newArrayList(boundFilterEntries));


Again you might want to rename the variable here to match the new class name.

nbelisle11 · 2018-10-05T04:15:38Z

Updated, thanks for the review all! Looking to merge this in the morning.

nbelisle added 3 commits October 3, 2018 19:56

Initial commit

787ef5d

Backwards compatibility

e53ca73

added tests

ea6e265

nbelisle11 requested review from boulter and lcmartinez October 4, 2018 03:38

nbelisle added 2 commits October 3, 2018 23:46

Handle aggregated fields appropriately

1984790

Add a getter for all filters

b548b7f

lcmartinez reviewed Oct 4, 2018

View reviewed changes

boulter reviewed Oct 4, 2018

View reviewed changes

Responding to PR comments

75b41ce

lcmartinez reviewed Oct 5, 2018

View reviewed changes

updates from PR comments

0681d0e

nbelisle11 merged commit 0b0b65b into HubSpot:master Oct 5, 2018

nbelisle11 deleted the allowOrFilteringInSource branch October 5, 2018 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow or filtering in source #12

Allow or filtering in source #12

nbelisle11 commented Oct 4, 2018

lcmartinez left a comment

lcmartinez Oct 4, 2018

lcmartinez Oct 4, 2018

lcmartinez Oct 4, 2018

lcmartinez Oct 4, 2018

nbelisle11 Oct 5, 2018

lcmartinez Oct 5, 2018

lcmartinez Oct 4, 2018

nbelisle11 Oct 4, 2018

lcmartinez Oct 4, 2018

nbelisle11 Oct 5, 2018

lcmartinez Oct 5, 2018

nbelisle11 Oct 5, 2018

lcmartinez Oct 4, 2018

nbelisle11 Oct 4, 2018

lcmartinez Oct 4, 2018

lcmartinez Oct 4, 2018

lcmartinez Oct 4, 2018

boulter left a comment

boulter Oct 4, 2018

nbelisle11 Oct 4, 2018

boulter Oct 4, 2018

boulter Oct 4, 2018

nbelisle11 Oct 5, 2018

nbelisle11 commented Oct 4, 2018

lcmartinez Oct 5, 2018

lcmartinez Oct 5, 2018

lcmartinez Oct 5, 2018

lcmartinez Oct 5, 2018

nbelisle11 commented Oct 5, 2018

Allow or filtering in source #12

Allow or filtering in source #12

Conversation

nbelisle11 commented Oct 4, 2018

lcmartinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boulter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbelisle11 commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbelisle11 commented Oct 5, 2018