From d3519ac0a3816ce6b7669287078285f49ca95af2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Le=20Provost?= Date: Tue, 13 Dec 2016 10:53:27 +0100 Subject: [PATCH] Fix handling of numeric refinements when disjunctive faceting is used They were ignored. The fix involves introducing specific methods to generate `Query.filters` from numeric refinements or facet refinements alone. --- Sources/SearchParameters.swift | 116 ++++++++++++++++++++++++------- Sources/Searcher.swift | 7 +- Tests/SearchParametersTest.swift | 71 +++++++++++++++++++ 3 files changed, 166 insertions(+), 28 deletions(-) diff --git a/Sources/SearchParameters.swift b/Sources/SearchParameters.swift index cd3a2c9d..2a597e8f 100644 --- a/Sources/SearchParameters.swift +++ b/Sources/SearchParameters.swift @@ -376,38 +376,53 @@ import Foundation // MARK: - Generate filters + /// Build a URL query string from the current parameters, including numeric and facet refinements. + /// + /// - returns: A URL query string. + /// @objc override open func build() -> String { - // Override the `filters` parameter with the current refinements. var parameters = self.parameters parameters["filters"] = buildFilters() return AbstractQuery.build(parameters: parameters) } - /// Generate a filter expression from the current filters. + /// Generate a filter expression from the current refinements. + /// This will combine numeric refinements and facet refinements into the same expression. /// - /// - returns: An expression suitable for use with `Query.filters`. The expression may be nil if no refinements - /// are being used. + /// - returns: An expression suitable for use with `Query.filters`. The expression may be `nil` if no refinements + /// are currently set. /// @objc public func buildFilters() -> String? { if !hasRefinements() { return nil } - // NOTE: We sort attribute names to get predictable output. - // Facet filters. - let facetExpression = facetRefinements.keys.sorted().flatMap({ (facetName: String) -> String? in - let refinements = self.facetRefinements[facetName]! - if refinements.isEmpty { - return nil - } - if self.isDisjunctiveFacet(name: facetName) { - let innerFilters = refinements.map({ return $0.buildFilter() }).joined(separator: " OR ") - return "(\(innerFilters))" + let facetExpression = buildFiltersFromFacets() + let numericExpression = buildFiltersFromNumerics() + if let facetExpression = facetExpression { + if let numericExpression = numericExpression { + return facetExpression + " AND " + numericExpression } else { - return refinements.map({ return $0.buildFilter() }).joined(separator: " AND ") + return facetExpression } - }).joined(separator: " AND ") - // Numeric filters. - let numericExpression = numericRefinements.keys.sorted().flatMap({ (attributeName: String) -> String? in + } else if let numericExpression = numericExpression { + return numericExpression + } else { + assert(false) // should not happen, as per above + return nil + } + } + + /// Generate a filter expression from the current numeric refinements. + /// + /// - returns: An expression suitable for use with `Query.filters`. The expression may be `nil` if no numeric + /// refinements are currently set. + /// + @objc public func buildFiltersFromNumerics() -> String? { + if (!hasNumericRefinements()) { + return nil + } + // NOTE: We sort attribute names to get predictable output. + let expression = numericRefinements.keys.sorted().flatMap({ (attributeName: String) -> String? in let filters = self.numericRefinements[attributeName]! if filters.isEmpty { return nil @@ -419,19 +434,41 @@ import Foundation return filters.map({ return $0.buildFilter() }).joined(separator: " AND ") } }).joined(separator: " AND ") - // Combine everything together. - var expression = facetExpression - if !numericExpression.isEmpty { - if !facetExpression.isEmpty { - expression += " AND " - } - expression += numericExpression + assert(!expression.isEmpty) + return expression + } + + /// Generate a filter expression from the current facet refinements. + /// + /// - returns: An expression suitable for use with `Query.filters`. The expression may be `nil` if no facet + /// refinements are currently set. + /// + @objc public func buildFiltersFromFacets() -> String? { + if (!hasFacetRefinements()) { + return nil } + // NOTE: We sort attribute names to get predictable output. + let expression = facetRefinements.keys.sorted().flatMap({ (facetName: String) -> String? in + let refinements = self.facetRefinements[facetName]! + if refinements.isEmpty { + return nil + } + if self.isDisjunctiveFacet(name: facetName) { + let innerFilters = refinements.map({ return $0.buildFilter() }).joined(separator: " OR ") + return "(\(innerFilters))" + } else { + return refinements.map({ return $0.buildFilter() }).joined(separator: " AND ") + } + }).joined(separator: " AND ") + assert(!expression.isEmpty) return expression } /// Generate facet refinements from the current filters. /// + /// + Note: Those are intended for use with `Index.searchDisjunctiveFaceting(...)`. In the general case, please + /// use `buildFilters()` instead. + /// /// - returns: Facet refinements suitable for use with `Index.searchDisjunctiveFaceting(...)`. /// @objc public func buildFacetRefinements() -> [String: [String]] { @@ -444,6 +481,9 @@ import Foundation /// Generate facet filters from the current filters. /// + /// + Note: Those are intended for use with `Query.facetFilters`. In the general case, please use + /// `buildFiltersFromNumerics()` instead. + /// /// - returns: An expression suitable for use with `Query.facetFilters`. /// @objc public func buildFacetFilters() -> [Any] { @@ -463,7 +503,33 @@ import Foundation } return facetFilters } + + // MARK: - Update filters in place + /// Update the parameters from the current refinements. + /// + /// + Warning: This will override the `filters` parameter. + /// + @objc public func update() { + self["filters"] = buildFilters() + } + + /// Update the parameters from the current *facet* refinements. Numeric refinements are ignored. + /// + /// + Warning: This will override the `filters` parameter. + /// + @objc public func updateFromFacets() { + self["filters"] = buildFiltersFromFacets() + } + + /// Update the parameters from the current *numeric* refinements. Facet refinements are ignored. + /// + /// + Warning: This will override the `filters` parameter. + /// + @objc public func updateFromNumerics() { + self["filters"] = buildFiltersFromNumerics() + } + // MARK: - Facets /// Set a given facet as disjunctive or conjunctive. diff --git a/Sources/Searcher.swift b/Sources/Searcher.swift index 9176b858..e971c303 100644 --- a/Sources/Searcher.swift +++ b/Sources/Searcher.swift @@ -367,12 +367,13 @@ import Foundation this.handleResults(content: content, error: error, userInfo: userInfo) } if state.disjunctiveFacets.isEmpty { - // All facets are conjunctive; build facet filters accordingly. - // TODO: Automate this? - params.facetFilters = params.buildFacetFilters() + // All facets are conjunctive; build regular filters combining numeric and facet refinements. + // NOTE: Not strictly necessary since `Index.search(...)` calls `Query.build()`, but let's not rely on that. + params.update() operation = index.search(params, completionHandler: completionHandler) } else { // Facet filters are built directly by the disjunctive faceting search helper method. + params.updateFromNumerics() // this is really necessary (in contrast to the above) let refinements = params.buildFacetRefinements() operation = index.searchDisjunctiveFaceting(params, disjunctiveFacets: state.disjunctiveFacets, refinements: refinements, completionHandler: completionHandler) } diff --git a/Tests/SearchParametersTest.swift b/Tests/SearchParametersTest.swift index 48a669e2..4bdc85e0 100644 --- a/Tests/SearchParametersTest.swift +++ b/Tests/SearchParametersTest.swift @@ -50,42 +50,52 @@ class SearchParametersTest: XCTestCase { // Empty params should produce empty string. XCTAssertNil(params.buildFilters()) + XCTAssertNil(params.buildFiltersFromFacets()) // One conjunctive facet with one refinement. params.addFacetRefinement(name: "foo", value: "bar1") XCTAssertEqual(params.buildFilters(), "\"foo\":\"bar1\"") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromFacets()) // One conjunctive facet with two refinements. params.addFacetRefinement(name: "foo", value: "bar2") XCTAssertEqual(params.buildFilters(), "\"foo\":\"bar1\" AND \"foo\":\"bar2\"") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromFacets()) // Two conjunctive facets with one refinement. params.removeFacetRefinement(name: "foo", value: "bar1") params.addFacetRefinement(name: "abc", value: "xyz") XCTAssertEqual(params.buildFilters(), "\"abc\":\"xyz\" AND \"foo\":\"bar2\"") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromFacets()) // Two conjunctive facets with two refinements (one negated). params.addFacetRefinement(name: "foo", value: "bar3") params.addFacetRefinement(name: "abc", value: "tuv", inclusive: false) XCTAssertEqual(params.buildFilters(), "\"abc\":\"xyz\" AND NOT \"abc\":\"tuv\" AND \"foo\":\"bar2\" AND \"foo\":\"bar3\"") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromFacets()) // One conjunctive facet and one disjunctive facet. params.setFacet(withName: "abc", disjunctive: true) XCTAssertEqual(params.buildFilters(), "(\"abc\":\"xyz\" OR NOT \"abc\":\"tuv\") AND \"foo\":\"bar2\" AND \"foo\":\"bar3\"") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromFacets()) // Two disjunctive facets. params.setFacet(withName: "foo", disjunctive: true) XCTAssertEqual(params.buildFilters(), "(\"abc\":\"xyz\" OR NOT \"abc\":\"tuv\") AND (\"foo\":\"bar2\" OR \"foo\":\"bar3\")") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromFacets()) // Disjunctive facet with only one refinement. params.removeFacetRefinement(name: "abc", value: "tuv") XCTAssertEqual(params.buildFilters(), "(\"abc\":\"xyz\") AND (\"foo\":\"bar2\" OR \"foo\":\"bar3\")") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromFacets()) // Remove all refinements: facet should disappear from params. params.removeFacetRefinement(name: "abc", value: "xyz") XCTAssertEqual(params.buildFilters(), "(\"foo\":\"bar2\" OR \"foo\":\"bar3\")") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromFacets()) params.clearFacetRefinements(name: "foo") XCTAssertNil(params.buildFilters()) + XCTAssertNil(params.buildFiltersFromFacets()) } func testFacetExistence() { @@ -121,42 +131,52 @@ class SearchParametersTest: XCTestCase { // Empty params should produce empty string. XCTAssertNil(params.buildFilters()) + XCTAssertNil(params.buildFiltersFromNumerics()) // One conjunctive numeric with one refinement. params.addNumericRefinement("foo", .greaterThanOrEqual, 3) XCTAssertEqual(params.buildFilters(), "\"foo\" >= 3") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromNumerics()) // One conjunctive numeric with two refinements. params.addNumericRefinement("foo", .lessThan, 4.0) XCTAssertEqual(params.buildFilters(), "\"foo\" >= 3 AND \"foo\" < 4") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromNumerics()) // Two conjunctive numeric with one refinement. params.removeNumericRefinement(NumericRefinement("foo", .greaterThanOrEqual, 3.0)) params.addNumericRefinement(NumericRefinement("bar", .greaterThan, 456.789)) XCTAssertEqual(params.buildFilters(), "\"bar\" > 456.789 AND \"foo\" < 4") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromNumerics()) // Two conjunctive numerics with two refinements (one negated). params.addNumericRefinement("foo", .notEqual, 0) params.addNumericRefinement("bar", .equal, 0, inclusive: false) XCTAssertEqual(params.buildFilters(), "\"bar\" > 456.789 AND NOT \"bar\" = 0 AND \"foo\" < 4 AND \"foo\" != 0") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromNumerics()) // One conjunctive numeric and one disjunctive. params.setNumeric(withName: "foo", disjunctive: true) XCTAssertEqual(params.buildFilters(), "\"bar\" > 456.789 AND NOT \"bar\" = 0 AND (\"foo\" < 4 OR \"foo\" != 0)") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromNumerics()) // Two disjunctive numeric. params.setNumeric(withName: "bar", disjunctive: true) XCTAssertEqual(params.buildFilters(), "(\"bar\" > 456.789 OR NOT \"bar\" = 0) AND (\"foo\" < 4 OR \"foo\" != 0)") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromNumerics()) // Disjunctive numeric with only one refinement. params.removeNumericRefinement("foo", .lessThan, 4) XCTAssertEqual(params.buildFilters(), "(\"bar\" > 456.789 OR NOT \"bar\" = 0) AND (\"foo\" != 0)") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromNumerics()) // Remove all refinements: numerics should disappear from params. params.removeNumericRefinement("foo", .notEqual, 0.0) XCTAssertEqual(params.buildFilters(), "(\"bar\" > 456.789 OR NOT \"bar\" = 0)") + XCTAssertEqual(params.buildFilters(), params.buildFiltersFromNumerics()) params.clearNumericRefinements(name: "bar") XCTAssertNil(params.buildFilters()) + XCTAssertNil(params.buildFiltersFromNumerics()) } func testBooleanNumeric() { @@ -188,6 +208,8 @@ class SearchParametersTest: XCTestCase { XCTAssertFalse(params.hasNumericRefinements(name: "foo")) } + /// Test combining facet refinements and numeric refinements. + /// func testFacetAndNumeric() { let params = SearchParameters() params.addNumericRefinement("foo", .greaterThanOrEqual, 123) @@ -195,9 +217,58 @@ class SearchParametersTest: XCTestCase { params.addNumericRefinement("bar", .lessThan, 456.789) params.addFacetRefinement(name: "xyz", value: "other") XCTAssertEqual(params.buildFilters(), "\"abc\":\"something\" AND \"xyz\":\"other\" AND \"bar\" < 456.789 AND \"foo\" >= 123") + XCTAssertEqual(params.buildFiltersFromFacets(), "\"abc\":\"something\" AND \"xyz\":\"other\"") + XCTAssertEqual(params.buildFiltersFromNumerics(), "\"bar\" < 456.789 AND \"foo\" >= 123") + + let params2 = SearchParameters(from: params) + params2.clearNumericRefinements() + XCTAssertEqual(params2.buildFilters(), "\"abc\":\"something\" AND \"xyz\":\"other\"") + XCTAssertEqual(params2.buildFiltersFromFacets(), "\"abc\":\"something\" AND \"xyz\":\"other\"") + XCTAssertNil(params2.buildFiltersFromNumerics()) + + let params3 = SearchParameters(from: params) + params3.clearFacetRefinements() + XCTAssertEqual(params3.buildFilters(), "\"bar\" < 456.789 AND \"foo\" >= 123") + XCTAssertEqual(params3.buildFiltersFromNumerics(), "\"bar\" < 456.789 AND \"foo\" >= 123") + XCTAssertNil(params3.buildFiltersFromFacets()) } + /// Test updating parameters in place. + /// + func testUpdate() { + let params = SearchParameters() + params.addNumericRefinement("foo", .greaterThanOrEqual, 123) + params.addFacetRefinement(name: "abc", value: "something") + params.addNumericRefinement("bar", .lessThan, 456.789) + params.addFacetRefinement(name: "xyz", value: "other") + params.update() + XCTAssertEqual(params["filters"], "\"abc\":\"something\" AND \"xyz\":\"other\" AND \"bar\" < 456.789 AND \"foo\" >= 123") + params.updateFromFacets() + XCTAssertEqual(params["filters"], "\"abc\":\"something\" AND \"xyz\":\"other\"") + params.updateFromNumerics() + XCTAssertEqual(params["filters"], "\"bar\" < 456.789 AND \"foo\" >= 123") + + let params2 = SearchParameters(from: params) + params2.clearNumericRefinements() + params2.update() + XCTAssertEqual(params2["filters"], "\"abc\":\"something\" AND \"xyz\":\"other\"") + params2.updateFromFacets() + XCTAssertEqual(params2["filters"], "\"abc\":\"something\" AND \"xyz\":\"other\"") + params2.updateFromNumerics() + XCTAssertNil(params2["filters"]) + + let params3 = SearchParameters(from: params) + params3.clearFacetRefinements() + params3.update() + XCTAssertEqual(params3["filters"], "\"bar\" < 456.789 AND \"foo\" >= 123") + params3.updateFromFacets() + XCTAssertNil(params3["filters"]) + params3.updateFromNumerics() + XCTAssertEqual(params3["filters"], "\"bar\" < 456.789 AND \"foo\" >= 123") + } + /// Test interaction of params with other search parameters. + /// func testOtherParams() { let params = SearchParameters()