fix(adv search): two advanced search fixes #6252
Conversation
| Value of the field to filter by. Deprecated in favor of `values`, which should accept a single element array for a | ||
| value | ||
| """ | ||
| value: String @deprecated(reason: "Prefer `values` for single elements") |
| @@ -488,7 +490,7 @@ private void addMissingAggregationValueToAggregationMetadata(@Nonnull final Stri | |||
| || originalMetadata.getFilterValues().stream().noneMatch(entry -> entry.getValue().equals(value)) | |||
| ) { | |||
| // No aggregation found for filtered value -- inject one! | |||
| originalMetadata.getAggregations().put(value, 0L); | |||
| originalMetadata.getAggregations().put(value, MAX_VALUE_FOR_FILTER_VALUE); | |||
There was a problem hiding this comment.
What is the implication of this...
There was a problem hiding this comment.
This basically makes it so that if the user tries to filter on something that does not exist, we report it correctly to them on the product side
There was a problem hiding this comment.
This means that filters in the query will have priority over aggregations added just because they appear in search results
| @@ -22,5 +22,7 @@ record AggregationMetadata { | |||
| value: string | |||
| entity: optional Urn | |||
| facetCount: long | |||
| // indicates that the FilterValue is part of the search request | |||
There was a problem hiding this comment.
Broadly i very much dislike the entire FilterValue struct that has been just tossed in here (much earlier). But this feels like a useful way to capture that context, and even return it to the client if need be
| * @param aggregations the aggregations coming back from elasticsearch combined with the filters from the search request | ||
| * @param filteredValues the set of values provided by the search request | ||
| */ | ||
| public static List<FilterValue> convertToFilters(Map<String, Long> aggregations, Set<String> filteredValues) { |
There was a problem hiding this comment.
Nice. Do we have any unit tests for this SearchUtil test?
| @@ -136,10 +137,16 @@ public static AggregationMetadata merge(AggregationMetadata one, AggregationMeta | |||
| Map<String, Long> mergedMap = | |||
| Stream.concat(one.getAggregations().entrySet().stream(), two.getAggregations().entrySet().stream()) | |||
| .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, Long::sum)); | |||
|
|
|||
There was a problem hiding this comment.
Are there any unit tests for this static class? If not we should at least log some backlog ticket about the coverage here.
| @@ -135,15 +135,25 @@ public static Filter buildFilter(@Nullable List<FacetFilterInput> andFilters, @N | |||
| public static Criterion criterionFromFilter(final FacetFilterInput filter) { | |||
| Criterion result = new Criterion(); | |||
| result.setField(getFilterField(filter.getField())); | |||
There was a problem hiding this comment.
Do we have tests for this method elsewhere? It will be good to test the new changes
gabe-lyons
force-pushed
the
gabe--advSearchFixes
branch
from
ed02ba5 to
1878275
Compare
* two advanced search fixes * another approach * adding unit tests * fixing checkstyle issue
* two advanced search fixes * another approach * adding unit tests * fixing checkstyle issue
* two advanced search fixes * another approach * adding unit tests * fixing checkstyle issue
removes the backwards-incompatible API change introduced in the initial advanced search pr
Fixes a bug where advanced filters would not be hydrated in the case there were many facets in the result
Checklist