refactor(disjunctive faceting): Fix handling of commas via JSONArray syntax (#545)

refactor(disjunctive faceting): Fix handling of commas via JSONArray syntax

Author: PLNech

algoliasearch/src/main/java/com/algolia/search/saas/helpers/DisjunctiveFaceting.java

@@ -91,7 +91,7 @@ public void requestCompleted(JSONObject content, AlgoliaException error) {
      * @param refinements       Map representing the current refinements
      * @return The disjunctive refinements
      */
-    static private @NonNull <T extends Collection<String>> Map<String, T> computeDisjunctiveRefinements(@NonNull Collection<String> disjunctiveFacets, @NonNull Map<String, T> refinements)
+    static private @NonNull <T extends Collection<String>> Map<String, T> filterDisjunctiveRefinements(@NonNull Collection<String> disjunctiveFacets, @NonNull Map<String, T> refinements)
     {
         Map<String, T> disjunctiveRefinements = new HashMap<>();
         for (Map.Entry<String, T> elt : refinements.entrySet()) {
@@ -112,91 +112,63 @@ public void requestCompleted(JSONObject content, AlgoliaException error) {
      */
     static private @NonNull <T extends Collection<String>> List<Query> computeDisjunctiveFacetingQueries(@NonNull Query query, @NonNull Collection<String> disjunctiveFacets, @NonNull Map<String, T> refinements) {
         // Retain only refinements corresponding to the disjunctive facets.
-        Map<String, ? extends Collection<String>> disjunctiveRefinements = computeDisjunctiveRefinements(disjunctiveFacets, refinements);
+        Map<String, ? extends Collection<String>> disjunctiveRefinements = filterDisjunctiveRefinements(disjunctiveFacets, refinements);
 
         // build queries
-        // TODO: Refactor using JSON array notation: safer and clearer.
         List<Query> queries = new ArrayList<>();
-        // hits + regular facets query
-        StringBuilder filters = new StringBuilder();
-        boolean first_global = true;
+
+        // first query: hits + regular facets
+        JSONArray facetFilters = new JSONArray();
         for (Map.Entry<String, T> elt : refinements.entrySet()) {
-            StringBuilder or = new StringBuilder();
-            or.append("(");
-            boolean first = true;
+            JSONArray orFilters = new JSONArray();
+
             for (String val : elt.getValue()) {
+                // When already refined facet, or with existing refinements
                 if (disjunctiveRefinements.containsKey(elt.getKey())) {
-                    // disjunctive refinements are ORed
-                    if (!first) {
-                        or.append(',');
-                    }
-                    first = false;
-                    or.append(String.format("%s:%s", elt.getKey(), val));
+                    orFilters.put(formatFilter(elt, val));
                 } else {
-                    if (!first_global) {
-                        filters.append(',');
-                    }
-                    first_global = false;
-                    filters.append(String.format("%s:%s", elt.getKey(), val));
+                    facetFilters.put(formatFilter(elt, val));
                 }
             }
             // Add or
             if (disjunctiveRefinements.containsKey(elt.getKey())) {
-                or.append(')');
-                if (!first_global) {
-                    filters.append(',');
-                }
-                first_global = false;
-                filters.append(or.toString());
+                facetFilters.put(orFilters);
             }
         }
 
-        queries.add(new Query(query).set("facetFilters", filters.toString()));
+        queries.add(new Query(query).setFacetFilters(facetFilters));
         // one query per disjunctive facet (use all refinements but the current one + hitsPerPage=1 + single facet
         for (String disjunctiveFacet : disjunctiveFacets) {
-            filters = new StringBuilder();
-            first_global = true;
+            facetFilters = new JSONArray();
             for (Map.Entry<String, T> elt : refinements.entrySet()) {
                 if (disjunctiveFacet.equals(elt.getKey())) {
                     continue;
                 }
-                StringBuilder or = new StringBuilder();
-                or.append("(");
-                boolean first = true;
+                JSONArray orFilters = new JSONArray();
                 for (String val : elt.getValue()) {
                     if (disjunctiveRefinements.containsKey(elt.getKey())) {
-                        // disjunctive refinements are ORed
-                        if (!first) {
-                            or.append(',');
-                        }
-                        first = false;
-                        or.append(String.format("%s:%s", elt.getKey(), val));
+                        orFilters.put(formatFilter(elt, val));
                     } else {
-                        if (!first_global) {
-                            filters.append(',');
-                        }
-                        first_global = false;
-                        filters.append(String.format("%s:%s", elt.getKey(), val));
+                        facetFilters.put(formatFilter(elt, val));
                     }
                 }
                 // Add or
                 if (disjunctiveRefinements.containsKey(elt.getKey())) {
-                    or.append(')');
-                    if (!first_global) {
-                        filters.append(',');
-                    }
-                    first_global = false;
-                    filters.append(or.toString());
+                    facetFilters.put(orFilters);
                 }
             }
             String[] facets = new String[]{disjunctiveFacet};
             queries.add(new Query(query).setHitsPerPage(0).setAnalytics(false)
                     .setAttributesToRetrieve().setAttributesToHighlight().setAttributesToSnippet()
-                    .setFacets(facets).set("facetFilters", filters.toString()));
+                    .setFacets(facets).setFacetFilters(facetFilters));
         }
         return queries;
     }
 
+    private static <T extends Collection<String>> String formatFilter(Map.Entry<String, T> refinement, String value) {
+        return String.format("%s:%s", refinement.getKey(), value);
+    }
+
     /**
      * Aggregate results from multiple queries into disjunctive faceting results.
      *
@@ -208,7 +180,7 @@ public void requestCompleted(JSONObject content, AlgoliaException error) {
      */
     static private <T extends Collection<String>> JSONObject aggregateDisjunctiveFacetingResults(@NonNull JSONObject answers, @NonNull Collection<String> disjunctiveFacets, @NonNull Map<String, T> refinements) throws AlgoliaException
     {
-        Map<String, T> disjunctiveRefinements = computeDisjunctiveRefinements(disjunctiveFacets, refinements);
+        Map<String, T> disjunctiveRefinements = filterDisjunctiveRefinements(disjunctiveFacets, refinements);
 
         // aggregate answers
         // first answer stores the hits + regular facets

algoliasearch/src/test/java/com/algolia/search/saas/IndexTest.java

@@ -210,28 +210,31 @@ public void searchDisjunctiveFacetingAsync() throws Exception {
         objects.add(new JSONObject("{\"name\": \"Platinum Phone Cover\", \"brand\": \"Samsung\", \"category\": \"accessory\",\"stars\":2}"));
         objects.add(new JSONObject("{\"name\": \"Lame Phone\", \"brand\": \"Whatever\", \"category\": \"device\",\"stars\":1}"));
         objects.add(new JSONObject("{\"name\": \"Lame Phone cover\", \"brand\": \"Whatever\", \"category\": \"accessory\",\"stars\":1}"));
+        // Testing commas and quotes in facet values
+        objects.add(new JSONObject("{\"name\": \"Symbols Phone\", \"brand\": \"Commas' voice, Ltd\", \"category\": \"device\",\"stars\":2}"));
         JSONObject task = index.addObjects(new JSONArray(objects), /* requestOptions: */ null);
         index.waitTask(task.getString("taskID"));
 
         final Query query = new Query("phone").setFacets("brand", "category", "stars");
         final List<String> disjunctiveFacets = Collections.singletonList("brand");
         final Map<String, List<String>> refinements = new HashMap<>();
-        refinements.put("brand", Arrays.asList("Apple", "Samsung")); // disjunctive facet
+        refinements.put("brand", Arrays.asList("Apple", "Samsung", "Commas' voice, Ltd")); // disjunctive facet
         refinements.put("category", Collections.singletonList("device")); // conjunctive facet
         index.searchDisjunctiveFacetingAsync(query, disjunctiveFacets, refinements, new AssertCompletionHandler() {
             @Override
             public void doRequestCompleted(JSONObject content, AlgoliaException error) {
                 if (error != null) {
                     fail(error.getMessage());
                 } else {
-                    assertEquals(3, content.optInt("nbHits"));
+                    assertEquals(4, content.optInt("nbHits"));
                     JSONObject disjunctiveFacetsResult = content.optJSONObject("disjunctiveFacets");
                     assertNotNull(disjunctiveFacetsResult);
                     JSONObject brandFacetCounts = disjunctiveFacetsResult.optJSONObject("brand");
                     assertNotNull(brandFacetCounts);
                     assertEquals(2, brandFacetCounts.optInt("Apple"));
                     assertEquals(1, brandFacetCounts.optInt("Samsung"));
                     assertEquals(1, brandFacetCounts.optInt("Whatever"));
+                    assertEquals(1, brandFacetCounts.optInt("Commas' voice, Ltd"));
                 }
             }
         });