From e2a45082b27cfc416e2d0ad3deec246cb9114931 Mon Sep 17 00:00:00 2001 From: scott Date: Tue, 9 Dec 2025 06:58:36 -0700 Subject: [PATCH 01/16] Phase 2 of API improvements --- .../api/v2/search/ConcurrencyLimiter.scala | 90 ++++ .../paramValidators/ParamValidator.scala | 373 ++++++++++------- .../search/queryBuilders/QueryBuilder.scala | 212 +++++----- .../scala/dpla/api/helpers/FileReader.scala | 5 +- .../api/v2/endToEnd/InvalidParamsTest.scala | 90 +++- .../v2/search/ConcurrencyLimiterTest.scala | 180 ++++++++ .../ItemParamValidatorTest.scala | 15 +- .../paramValidators/ParamValidatorTest.scala | 286 +++++++------ .../queryBuilders/QueryBuilderTest.scala | 384 ++++++++++++------ 9 files changed, 1125 insertions(+), 510 deletions(-) create mode 100644 src/main/scala/dpla/api/v2/search/ConcurrencyLimiter.scala create mode 100644 src/test/scala/dpla/api/v2/search/ConcurrencyLimiterTest.scala diff --git a/src/main/scala/dpla/api/v2/search/ConcurrencyLimiter.scala b/src/main/scala/dpla/api/v2/search/ConcurrencyLimiter.scala new file mode 100644 index 00000000..9b4c3f00 --- /dev/null +++ b/src/main/scala/dpla/api/v2/search/ConcurrencyLimiter.scala @@ -0,0 +1,90 @@ +package dpla.api.v2.search + +import java.util.concurrent.{Semaphore, TimeUnit} +import scala.concurrent.{ExecutionContext, Future} + +/** Limits concurrent execution of Futures using a semaphore. + * + * IMPORTANT: The `apply` method uses `tryAcquire` with a timeout, which BLOCKS + * the calling thread for up to `timeoutSeconds`. In Akka actor contexts, this + * means actor threads may be blocked. This is intentional to provide + * backpressure when the system is overloaded, but callers should be aware of + * this behavior. + * + * For high-throughput scenarios, consider: + * - Using a dedicated blocking dispatcher for operations that use this + * limiter + * - Tuning `maxConcurrent` and `timeoutSeconds` based on your workload + * - Monitoring permit acquisition times + * + * @param maxConcurrent + * Maximum number of concurrent operations allowed + * @param timeoutSeconds + * Maximum time to wait for a permit before failing + */ +class ConcurrencyLimiter( + val maxConcurrent: Int, + val timeoutSeconds: Long +) { + require( + maxConcurrent > 0, + s"maxConcurrent must be positive, got: $maxConcurrent" + ) + require( + timeoutSeconds > 0, + s"timeoutSeconds must be positive, got: $timeoutSeconds" + ) + + private val semaphore = new Semaphore(maxConcurrent) + + /** Wraps a Future with concurrency limiting. + * + * - Attempts to acquire a permit with timeout + * - If permit acquired, executes the Future and releases permit on + * completion + * - If timeout exceeded, returns a failed Future immediately + * - Ensures permit is released even if Future construction throws + * + * @param f + * The Future to execute (call-by-name, evaluated only if permit acquired) + * @param ec + * ExecutionContext for Future callbacks + * @return + * The wrapped Future, or a failed Future if permit couldn't be acquired + */ + def apply[T](f: => Future[T])(implicit ec: ExecutionContext): Future[T] = { + if (!semaphore.tryAcquire(timeoutSeconds, TimeUnit.SECONDS)) { + Future.failed( + ConcurrencyLimitExceeded( + maxConcurrent = maxConcurrent, + timeoutSeconds = timeoutSeconds + ) + ) + } else { + try { + val future = f + future.andThen { case _ => semaphore.release() }(ec) + } catch { + case e: Throwable => + semaphore.release() + Future.failed(e) + } + } + } + + /** Returns the number of permits currently available. Useful for monitoring + * and debugging. + */ + def availablePermits: Int = semaphore.availablePermits() +} + +/** Exception thrown when a concurrency limit is exceeded and the timeout + * expires. + */ +case class ConcurrencyLimitExceeded( + maxConcurrent: Int, + timeoutSeconds: Long +) extends RuntimeException( + s"Concurrency limit ($maxConcurrent) exceeded, " + + s"timed out after ${timeoutSeconds}s waiting for permit" + ) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index 41a00b3b..02f8f625 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -9,57 +9,54 @@ import java.net.URL import scala.util.matching.Regex import scala.util.{Failure, Success, Try} -/** - * Validates user-submitted search and fetch parameters. - * Provides default values when appropriate. - * Bad actors may use invalid search params to try and hack the system, so they - * are logged as warnings. - */ +/** Validates user-submitted search and fetch parameters. Provides default + * values when appropriate. Bad actors may use invalid search params to try and + * hack the system, so they are logged as warnings. + */ /** Case classes for representing valid search parameters */ private[search] case class SearchParams( - exactFieldMatch: Boolean, - facets: Option[Seq[String]], - facetSize: Int, - fields: Option[Seq[String]], - fieldQueries: Seq[FieldQuery], - filter: Option[Seq[Filter]], - op: String, - page: Int, - pageSize: Int, - q: Option[String], - sortBy: Option[String], - sortByPin: Option[String], - sortOrder: String - ) + exactFieldMatch: Boolean, + facets: Option[Seq[String]], + facetSize: Int, + fields: Option[Seq[String]], + fieldQueries: Seq[FieldQuery], + filter: Option[Seq[Filter]], + op: String, + page: Int, + pageSize: Int, + q: Option[String], + sortBy: Option[String], + sortByPin: Option[String], + sortOrder: String +) private[search] case class FetchParams( - fields: Option[Seq[String]] = None - ) + fields: Option[Seq[String]] = None +) private[search] case class RandomParams( - filter: Option[Seq[Filter]] = None - ) + filter: Option[Seq[Filter]] = None +) private[search] case class FieldQuery( - fieldName: String, - value: String - ) + fieldName: String, + value: String +) private[search] case class Filter( - fieldName: String, - value: String - ) + fieldName: String, + value: String +) trait ParamValidator extends FieldDefinitions { def apply( - nextPhase: ActorRef[IntermediateSearchResult] - ): Behavior[IntermediateSearchResult] = { + nextPhase: ActorRef[IntermediateSearchResult] + ): Behavior[IntermediateSearchResult] = { Behaviors.setup { context => - Behaviors.receiveMessage[IntermediateSearchResult] { case RawSearchParams(rawParams, replyTo) => @@ -71,7 +68,9 @@ trait ParamValidator extends FieldDefinitions { context.log.warn2( "Invalid search params: '{}' for params '{}'", e.getMessage, - rawParams.map { case(key, value) => s"$key: $value"}.mkString(", ") + rawParams + .map { case (key, value) => s"$key: $value" } + .mkString(", ") ) replyTo ! InvalidSearchParams(e.getMessage) } @@ -86,7 +85,7 @@ trait ParamValidator extends FieldDefinitions { "Invalid fetch params: '{}' for params '{}'", e.getMessage, rawParams - .map { case(key, value) => s"$key: $value"} + .map { case (key, value) => s"$key: $value" } .++(Map("id" -> id)) .mkString(", ") ) @@ -102,7 +101,9 @@ trait ParamValidator extends FieldDefinitions { context.log.warn2( "Invalid random params: '{}' for params '{}'", e.getMessage, - rawParams.map { case(key, value) => s"$key: $value"}.mkString(", ") + rawParams + .map { case (key, value) => s"$key: $value" } + .mkString(", ") ) replyTo ! InvalidSearchParams(e.getMessage) } @@ -121,14 +122,14 @@ trait ParamValidator extends FieldDefinitions { protected val defaultExactFieldMatch: Boolean = false protected val defaultFacetSize: Int = 50 protected val minFacetSize: Int = 0 - protected val maxFacetSize: Int = 2000 + protected val maxFacetSize: Int = 200 protected val defaultOp: String = "AND" protected val defaultPage: Int = 1 protected val minPage: Int = 1 - protected val maxPage: Int = 100 + protected val maxPage: Int = 10 protected val defaultPageSize: Int = 10 protected val minPageSize: Int = 0 - protected val maxPageSize: Int = 500 + protected val maxPageSize: Int = 100 protected val defaultSortOrder: String = "asc" // Abstract. @@ -145,17 +146,16 @@ trait ParamValidator extends FieldDefinitions { protected val ignoredFields: Seq[String] private case class ValidationException( - private val message: String = "" - ) extends Exception(message) + private val message: String = "" + ) extends Exception(message) - /** - * Get valid fetch params. - * Fails with ValidationException if id or any raw params are invalid. - */ + /** Get valid fetch params. Fails with ValidationException if id or any raw + * params are invalid. + */ private def getFetchIds( - id: String, - rawParams: Map[String, String] - ): Try[Seq[String]] = + id: String, + rawParams: Map[String, String] + ): Try[Seq[String]] = Try { // There are no recognized params for a fetch request if (rawParams.nonEmpty) @@ -164,18 +164,20 @@ trait ParamValidator extends FieldDefinitions { ) else { val ids = id.split(",") - if (ids.size > maxPageSize) throw ValidationException( - s"The number of ids cannot exceed $maxPageSize" - ) + if (ids.size > maxPageSize) + throw ValidationException( + s"The number of ids cannot exceed $maxPageSize" + ) ids.map(getValidId) } } - /** - * Get valid search params. - * Fails with ValidationException if any raw params are invalid. - */ - private def getSearchParams(rawParams: Map[String, String]): Try[SearchParams] = + /** Get valid search params. Fails with ValidationException if any raw params + * are invalid. + */ + private def getSearchParams( + rawParams: Map[String, String] + ): Try[SearchParams] = Try { // Check for unrecognized params val unrecognized = rawParams.keys.toSeq diff acceptedSearchParams @@ -191,45 +193,90 @@ trait ParamValidator extends FieldDefinitions { val fieldQueries: Seq[FieldQuery] = searchableDataFields.flatMap(getValidFieldQuery(rawParams, _)) + val exactFieldMatch = + getValid(rawParams, "exact_field_match", validBoolean) + .getOrElse(defaultExactFieldMatch) + + val facets = + getValid(rawParams, "facets", validFields) + + val facetSize = + getValid(rawParams, "facet_size", validIntWithRange) + .getOrElse(defaultFacetSize) + + val fields = + getValid(rawParams, "fields", validFields) + + val filter = + getValidFilter(rawParams) + + val op = + getValid(rawParams, "op", validAndOr) + .getOrElse(defaultOp) + + val page = + getValid(rawParams, "page", validIntWithRange) + .getOrElse(defaultPage) + + val pageSize = + getValid(rawParams, "page_size", validIntWithRange) + .getOrElse(defaultPageSize) + + val q = + getValid(rawParams, "q", validText) + + val sortBy = + getValidSortField(rawParams) + + val sortByPin = + getValidSortByPin(rawParams) + + val sortOrder = + getValid(rawParams, "sort_order", validSortOrder) + .getOrElse(defaultSortOrder) + + val requestedPage = + rawParams.get("page").flatMap(_.toIntOption).getOrElse(page) + val requestedPageSize = + rawParams.get("page_size").flatMap(_.toIntOption).getOrElse(pageSize) + val fromValue = (requestedPage - 1) * requestedPageSize + + if (fromValue + requestedPageSize > 1000) + throw ValidationException( + s"Pagination too deep: from ($fromValue) + size ($pageSize) exceeds 1000" + ) + + val hasFacets = facets.nonEmpty + val hasQuery = q.isDefined || fieldQueries.nonEmpty + val hasFilter = filter.isDefined + + if (hasFacets && !hasQuery && !hasFilter) + throw ValidationException( + "Facet requests require at least one query term (q) or filter" + ) + // Return valid search params. Provide defaults when appropriate. SearchParams( - exactFieldMatch = - getValid(rawParams, "exact_field_match", validBoolean) - .getOrElse(defaultExactFieldMatch), - facets = - getValid(rawParams, "facets", validFields), - facetSize = - getValid(rawParams, "facet_size", validIntWithRange) - .getOrElse(defaultFacetSize), - fields = - getValid(rawParams, "fields", validFields), - fieldQueries = - fieldQueries, - filter = - getValidFilter(rawParams), - op = - getValid(rawParams, "op", validAndOr) - .getOrElse(defaultOp), - page = - getValid(rawParams, "page", validIntWithRange) - .getOrElse(defaultPage), - pageSize = - getValid(rawParams, "page_size", validIntWithRange) - .getOrElse(defaultPageSize), - q = - getValid(rawParams, "q", validText), - sortBy = - getValidSortField(rawParams), - sortByPin = - getValidSortByPin(rawParams), - sortOrder = - getValid(rawParams, "sort_order", validSortOrder) - .getOrElse(defaultSortOrder) + exactFieldMatch = exactFieldMatch, + facets = facets, + facetSize = facetSize, + fields = fields, + fieldQueries = fieldQueries, + filter = filter, + op = op, + page = page, + pageSize = pageSize, + q = q, + sortBy = sortBy, + sortByPin = sortByPin, + sortOrder = sortOrder ) } } - private def getRandomParams(rawParams: Map[String, String]): Try[RandomParams] = + private def getRandomParams( + rawParams: Map[String, String] + ): Try[RandomParams] = Try { // Check for unrecognized params val unrecognized = rawParams.keys.toSeq diff Seq("filter") @@ -252,12 +299,12 @@ trait ParamValidator extends FieldDefinitions { getDataFieldType(paramName) match { case Some(fieldType) => fieldType match { - case TextField => validText - case IntField => validInt - case URLField => validUrl - case DateField => validDate + case TextField => validText + case IntField => validInt + case URLField => validUrl + case DateField => validDate case WildcardField => validText - case _ => validText + case _ => validText } case None => throw ValidationException(s"Unrecognized parameter: $paramName") @@ -277,11 +324,12 @@ trait ParamValidator extends FieldDefinitions { else throw ValidationException(rule) } - /** - * Get a valid value for a field query. - */ - private def getValidFieldQuery(rawParams: Map[String, String], - paramName: String): Option[FieldQuery] = { + /** Get a valid value for a field query. + */ + private def getValidFieldQuery( + rawParams: Map[String, String], + paramName: String + ): Option[FieldQuery] = { val validationMethod = getValidationMethod(paramName) @@ -289,18 +337,23 @@ trait ParamValidator extends FieldDefinitions { .map(FieldQuery(paramName, _)) } - /** - * Get a valid field name and value for a filter query. - */ - private def getValidFilter(rawParams: Map[String, String]): Option[Seq[Filter]] = - rawParams.get("filter").map{ filter => - - val fieldName = filter.split(":", 2).headOption + /** Get a valid field name and value for a filter query. + */ + private def getValidFilter( + rawParams: Map[String, String] + ): Option[Seq[Filter]] = + rawParams.get("filter").map { filter => + val fieldName = filter + .split(":", 2) + .headOption .getOrElse(throw ValidationException(s"$filter is not a valid filter")) - val values = filter.split(":", 2).lastOption + val values = filter + .split(":", 2) + .lastOption .getOrElse(throw ValidationException(s"$filter is not a valid filter")) - .split("AND").map(_.trim) + .split("AND") + .map(_.trim) if (searchableDataFields.contains(fieldName)) { val validationMethod = getValidationMethod(fieldName) @@ -315,13 +368,14 @@ trait ParamValidator extends FieldDefinitions { } } - /** - * Get a valid value for sort_by parameter. - * Must be in the list of sortable fields. - * If coordinates, query must also contain the "sort_by_pin" parameter. - */ - private def getValidSortField(rawParams: Map[String, String]): Option[String] = - rawParams.get("sort_by").map{ sortField => + /** Get a valid value for sort_by parameter. Must be in the list of sortable + * fields. If coordinates, query must also contain the "sort_by_pin" + * parameter. + */ + private def getValidSortField( + rawParams: Map[String, String] + ): Option[String] = + rawParams.get("sort_by").map { sortField => // Check if field is sortable according to the field definition if (sortableDataFields.contains(sortField)) { // Check if field represents coordinates @@ -331,7 +385,7 @@ trait ParamValidator extends FieldDefinitions { // Check if raw params also contains sort_by_pin rawParams.get("sort_by_pin") match { case Some(_) => sortField - case None => + case None => throw ValidationException( "The sort_by_pin parameter is required." ) @@ -347,11 +401,12 @@ trait ParamValidator extends FieldDefinitions { ) } - /** - * Get valid value for sort_by_pin. - * Query must also contain the "sort_by" parameter with the coordinates field. - */ - private def getValidSortByPin(rawParams: Map[String, String]): Option[String] = { + /** Get valid value for sort_by_pin. Query must also contain the "sort_by" + * parameter with the coordinates field. + */ + private def getValidSortByPin( + rawParams: Map[String, String] + ): Option[String] = { rawParams.get("sort_by_pin").map { coordinates => // Check if field is valid text (will throw exception if not) val validCoordinates: String = validText(coordinates, "sort_by_pin") @@ -366,20 +421,24 @@ trait ParamValidator extends FieldDefinitions { ) case None => throw ValidationException( - s"The sort_by parameter is required." + s"The sort_by parameter is required." ) } } } - /** - * Find the raw parameter with the given name. - * Then validate with the given method. - */ - private def getValid[T](rawParams: Map[String, String], - paramName: String, - validationMethod: (String, String) => T): Option[T] = - rawParams.find(_._1 == paramName).map{case (k,v) => validationMethod(v,k)} + /** Find the raw parameter with the given name. Then validate with the given + * method. + */ + private def getValid[T]( + rawParams: Map[String, String], + paramName: String, + validationMethod: (String, String) => T + ): Option[T] = + rawParams.find(_._1 == paramName).flatMap { + case (k, v) if v.trim.isEmpty => None + case (k, v) => Some(validationMethod(v, k)) + } // Must be a Boolean value. private def validBoolean(boolString: String, param: String): Boolean = @@ -394,25 +453,30 @@ trait ParamValidator extends FieldDefinitions { val acceptedFields = param match { case "facets" => facetableDataFields case "fields" => allDataFields - case _ => Seq[String]() + case _ => Seq[String]() } - fieldString.split(",").flatMap(candidate => { - // Need to check ignoredFields first b/c acceptedFields may contain - // fields that are also in ignoredFields - if (ignoredFields.contains(candidate)) - None - else if (acceptedFields.contains(candidate)) - Some(candidate) - else if (param == "facets" && coordinatesField.map(_.name) - .contains(candidate.split(":").head)) - - Some(candidate) - else - throw ValidationException( - s"'$candidate' is not an allowable value for '$param'" + fieldString + .split(",") + .flatMap(candidate => { + // Need to check ignoredFields first b/c acceptedFields may contain + // fields that are also in ignoredFields + if (ignoredFields.contains(candidate)) + None + else if (acceptedFields.contains(candidate)) + Some(candidate) + else if ( + param == "facets" && coordinatesField + .map(_.name) + .contains(candidate.split(":").head) ) - }) + + Some(candidate) + else + throw ValidationException( + s"'$candidate' is not an allowable value for '$param'" + ) + }) } // Must be an integer @@ -427,9 +491,9 @@ trait ParamValidator extends FieldDefinitions { def validIntWithRange(intString: String, param: String): Int = { val (min: Int, max: Int) = param match { case "facet_size" => (minFacetSize, maxFacetSize) - case "page" => (minPage, maxPage) - case "page_size" => (minPageSize, maxPageSize) - case _ => (0, 2147483647) + case "page" => (minPage, maxPage) + case "page_size" => (minPageSize, maxPageSize) + case _ => (0, 2147483647) } val rule = s"$param must be an integer between 0 and $max" @@ -448,9 +512,9 @@ trait ParamValidator extends FieldDefinitions { // Must be a string between 2 and 200 characters. private def validText(text: String, param: String): String = if (text.length < 2 || text.length > 200) - // In the DPLA API (cultural heritage), an exception is thrown if q is too - // long, but not if q is too short. - // For internal consistency, and exception is thrown here in both cases. + // In the DPLA API (cultural heritage), an exception is thrown if q is too + // long, but not if q is too short. + // For internal consistency, and exception is thrown here in both cases. throw ValidationException(s"$param must be between 2 and 200 characters") else text @@ -462,7 +526,10 @@ trait ParamValidator extends FieldDefinitions { val yearMonth: Regex = raw"""\d{4}-\d{2}""".r val yearMonthDay: Regex = raw"""\d{4}-\d{2}-\d{2}""".r - if (year.matches(text) || yearMonth.matches(text) || yearMonthDay.matches(text)) + if ( + year.matches(text) || yearMonth + .matches(text) || yearMonthDay.matches(text) + ) text else throw ValidationException(rule) diff --git a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala index 4e40e405..d2edab66 100644 --- a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala +++ b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala @@ -9,32 +9,38 @@ import spray.json._ import scala.collection.mutable.ArrayBuffer -/** - * Composes ElasticSearch queries from user-submitted parameters. - */ +/** Composes ElasticSearch queries from user-submitted parameters. + */ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { - def apply(nextPhase: ActorRef[IntermediateSearchResult]): Behavior[IntermediateSearchResult] = { + def apply( + nextPhase: ActorRef[IntermediateSearchResult] + ): Behavior[IntermediateSearchResult] = { Behaviors.receiveMessage[IntermediateSearchResult] { case ValidSearchParams(searchParams, replyTo) => - nextPhase ! SearchQuery(searchParams, - composeSearchQuery(searchParams), replyTo) + nextPhase ! SearchQuery( + searchParams, + composeSearchQuery(searchParams), + replyTo + ) Behaviors.same case ValidFetchParams(ids, params, replyTo) => if (ids.size == 1) { nextPhase ! FetchQuery(ids.head, params, None, replyTo) - } - else { + } else { nextPhase ! MultiFetchQuery(composeMultiFetchQuery(ids), replyTo) } Behaviors.same case ValidRandomParams(randomParams, replyTo) => - nextPhase ! RandomQuery(randomParams, composeRandomQuery(randomParams), - replyTo) + nextPhase ! RandomQuery( + randomParams, + composeRandomQuery(randomParams), + replyTo + ) Behaviors.same case _ => @@ -59,7 +65,7 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { ).toJson def composeRandomQuery(params: RandomParams): JsValue = { - val filterClause: Option[JsObject] = params.filter.map(filterQuery) + val filterClause: Option[JsArray] = params.filter.map(filterQuery) // Setting "boost_mode" to "sum" ensures that if a filter is used, the // random query will return a different doc every time (otherwise, it will @@ -83,7 +89,7 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { "query" -> JsObject( "function_score" -> functionScore ), - "size" -> 1.toJson, + "size" -> 1.toJson ).toJson } @@ -91,54 +97,45 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { JsObject( "from" -> from(params.page, params.pageSize).toJson, "size" -> params.pageSize.toJson, - "query" -> query(params.q, params.filter, params.fieldQueries, params.exactFieldMatch, params.op), + "query" -> query( + params.q, + params.filter, + params.fieldQueries, + params.exactFieldMatch, + params.op + ), "aggs" -> aggs(params.facets, params.facetSize), "sort" -> sort(params), "_source" -> fieldRetrieval(params.fields), - "track_total_hits" -> true.toJson + "track_total_hits" -> 10000.toJson ).toJson - // Fields to search in a keyword query and their boost values + // Reduced high-value fields for multi_match keyword queries private val keywordQueryFields = Seq( - "dataProvider.name^1", - "intermediateProvider^1", - "provider.name^1", - "sourceResource.collection.description^1", - "sourceResource.collection.title^1", - "sourceResource.contributor^1", - "sourceResource.creator^1", - "sourceResource.description^0.75", - "sourceResource.extent^1", - "sourceResource.format^1", - "sourceResource.language.name^1", - "sourceResource.publisher^1", - "sourceResource.relation^1", - "sourceResource.rights^1", - "sourceResource.spatial.country^0.75", - "sourceResource.spatial.county^1", - "sourceResource.spatial.name^1", - "sourceResource.spatial.region^1", - "sourceResource.spatial.state^0.75", - "sourceResource.specType^1", - "sourceResource.subject.name^1", - "sourceResource.subtitle^2", "sourceResource.title^2", - "sourceResource.type^1" + "sourceResource.subtitle^2", + "sourceResource.subject.name^1", + "sourceResource.description^0.75", + "sourceResource.creator^1", + "sourceResource.collection.title^1", + "dataProvider.name^1", + "provider.name^1" ) // ElasticSearch param that defines the number of hits to skip private def from(page: Int, pageSize: Int): Int = (page - 1) * pageSize - private def query(q: Option[String], - filter: Option[Seq[Filter]], - fieldQueries: Seq[FieldQuery], - exactFieldMatch: Boolean, - op: String - ) = { + private def query( + q: Option[String], + filter: Option[Seq[Filter]], + fieldQueries: Seq[FieldQuery], + exactFieldMatch: Boolean, + op: String + ) = { val keyword: Seq[JsObject] = q.map(keywordQuery(_, keywordQueryFields)).toSeq - val filterClause: Option[JsObject] = filter.map(filterQuery) + val filterClause: Option[JsArray] = filter.map(filterQuery) val fieldQuery: Seq[JsObject] = fieldQueries.flatMap(singleFieldQuery(_, exactFieldMatch)) val queryTerms: Seq[JsObject] = keyword ++ fieldQuery @@ -163,60 +160,60 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { } } - /** - * A general keyword query on the given fields. - * "query_string" does a keyword search within the given fields. - * It is case-insensitive and analyzes the search term. - */ - private def keywordQuery(q: String, fields: Seq[String]): JsObject = + /** A general keyword query on the given fields. Uses multi_match for + * best_fields with lenient parsing. + */ + private def keywordQuery(q: String, fields: Seq[String]): JsObject = { + val targetFields = if (fields.nonEmpty) fields else keywordQueryFields + JsObject( - "query_string" -> JsObject( - "fields" -> fields.toJson, + "multi_match" -> JsObject( "query" -> q.toJson, - "analyze_wildcard" -> true.toJson, - "default_operator" -> "AND".toJson, + "fields" -> targetFields.toJson, + "type" -> "best_fields".toJson, + "operator" -> "AND".toJson, "lenient" -> true.toJson ) ) - - /** - * A filter for a specific field. - * This will filter out fields that do not match the given value, but will - * not affect the score for matching documents. - */ - private def filterQuery(filters: Seq[Filter]): JsObject = { - val mustArray: JsValue = filters.map(filter => { - JsObject( - "term" -> JsObject( - filter.fieldName -> filter.value.toJson - ) - ) - }).toJson - - JsObject( - "bool" -> JsObject( - "must" -> mustArray - ) - ) } - /** - * For general field query, use a keyword (i.e. "query_string") query. - * For exact field match, use "term" query. - * - term" searches for an exact term (with no additional text before or after). - * - It is case-sensitive and does not analyze the search term. - * - You can optionally set a parameter to ignore case, - * - but this is NOT applied in the cultural heritage API. - * - It is only for fields that non-analyzed (i.e. indexed as "keyword") - */ - private def singleFieldQuery(fieldQuery: FieldQuery, - exactFieldMatch: Boolean): Seq[JsObject] = + /** A filter for a specific field. This will filter out fields that do not + * match the given value, but will not affect the score for matching + * documents. + */ + private def filterQuery(filters: Seq[Filter]): JsArray = + filters + .map(filter => { + JsObject( + "term" -> JsObject( + filter.fieldName -> filter.value.toJson + ) + ) + }) + .toJson + .asInstanceOf[JsArray] + + /** For general field query, use a keyword (i.e. "query_string") query. For + * exact field match, use "term" query. + * - term" searches for an exact term (with no additional text before or + * after). + * - It is case-sensitive and does not analyze the search term. + * - You can optionally set a parameter to ignore case, + * - but this is NOT applied in the cultural heritage API. + * - It is only for fields that non-analyzed (i.e. indexed as "keyword") + */ + private def singleFieldQuery( + fieldQuery: FieldQuery, + exactFieldMatch: Boolean + ): Seq[JsObject] = if (fieldQuery.fieldName.endsWith(".before")) { // Range query val field: String = getElasticSearchField(fieldQuery.fieldName) .getOrElse( - throw new RuntimeException("Unrecognized field name: " + fieldQuery.fieldName) + throw new RuntimeException( + "Unrecognized field name: " + fieldQuery.fieldName + ) ) val obj = JsObject( @@ -232,7 +229,9 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { // Range query val field: String = getElasticSearchField(fieldQuery.fieldName) .getOrElse( - throw new RuntimeException("Unrecognized field name: " + fieldQuery.fieldName) + throw new RuntimeException( + "Unrecognized field name: " + fieldQuery.fieldName + ) ) val obj = JsObject( @@ -248,7 +247,9 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { // Exact match query val field: String = getElasticSearchExactMatchField(fieldQuery.fieldName) .getOrElse( - throw new RuntimeException("Unrecognized field name: " + fieldQuery.fieldName) + throw new RuntimeException( + "Unrecognized field name: " + fieldQuery.fieldName + ) ) // This should not happen val values = stripLeadingAndTrainingQuotationMarks(fieldQuery.value) @@ -273,20 +274,18 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { Seq(obj) } - /** - * Strip leading and trailing quotation marks only if there are no - * internal quotation marks. - */ + /** Strip leading and trailing quotation marks only if there are no internal + * quotation marks. + */ private def stripLeadingAndTrainingQuotationMarks(str: String): String = if (str.matches("^\"[^\"]*\"$")) str.stripPrefix("\"").stripSuffix("\"") else str - /** - * Composes an aggregates (facets) query object. - * Fields must be non-analyzed (i.e. indexed as keyword) - */ + /** Composes an aggregates (facets) query object. Fields must be non-analyzed + * (i.e. indexed as keyword) + */ private def aggs(facets: Option[Seq[String]], facetSize: Int): JsObject = facets match { case Some(facetArray) => @@ -302,7 +301,10 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { val ranges = ArrayBuffer.empty[JsValue] for (i <- 0 to 2000 by 100) - ranges += JsObject("from" -> i.toJson, "to" -> (i + 99).toJson) + ranges += JsObject( + "from" -> i.toJson, + "to" -> (i + 99).toJson + ) ranges += JsObject("from" -> 2100.toJson) val geoDistance = JsObject( @@ -325,17 +327,17 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { val interval = facet.split("\\.").lastOption match { case Some("month") => "month" - case _ => "year" + case _ => "year" } val format = facet.split("\\.").lastOption match { case Some("month") => "yyyy-MM" - case _ => "yyyy" + case _ => "yyyy" } val gte = facet.split("\\.").lastOption match { case Some("month") => "now-416y" - case _ => "now-2000y" + case _ => "now-2000y" } val dateHistogram = JsObject( @@ -390,9 +392,9 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { // This is the fastest way to sort documents but is meaningless. // It is the order in which they are saved to disk. val diskSort: JsArray = - JsArray( - "_doc".toJson - ) + JsArray( + "_doc".toJson + ) params.sortBy match { case Some(field) => @@ -448,7 +450,7 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { private def fieldRetrieval(fields: Option[Seq[String]]): JsValue = { fields match { case Some(f) => f.map(getElasticSearchField).toJson - case None => Seq("*").toJson + case None => Seq("*").toJson } } } diff --git a/src/test/scala/dpla/api/helpers/FileReader.scala b/src/test/scala/dpla/api/helpers/FileReader.scala index 6d3917a4..31c2882c 100644 --- a/src/test/scala/dpla/api/helpers/FileReader.scala +++ b/src/test/scala/dpla/api/helpers/FileReader.scala @@ -5,7 +5,8 @@ import scala.io.{BufferedSource, Source} trait FileReader { def readFile(filePath: String): String = { val source: String = getClass.getResource(filePath).getPath - val buffered: BufferedSource = Source.fromFile(source) - buffered.getLines.mkString + val buffered: BufferedSource = Source.fromFile(source, "UTF-8") + try buffered.getLines().mkString + finally buffered.close() } } diff --git a/src/test/scala/dpla/api/v2/endToEnd/InvalidParamsTest.scala b/src/test/scala/dpla/api/v2/endToEnd/InvalidParamsTest.scala index 65f2e878..5cf9ea28 100644 --- a/src/test/scala/dpla/api/v2/endToEnd/InvalidParamsTest.scala +++ b/src/test/scala/dpla/api/v2/endToEnd/InvalidParamsTest.scala @@ -8,14 +8,26 @@ import akka.http.scaladsl.testkit.ScalatestRouteTest import dpla.api.Routes import dpla.api.helpers.ActorHelper import dpla.api.helpers.Utils.fakeApiKey -import dpla.api.v2.registry.{MockSmrRegistry, SmrRegistryCommand} +import dpla.api.v2.registry.{ + MockEbookRegistry, + MockSmrRegistry, + SearchRegistryCommand, + SmrRegistryCommand +} +import dpla.api.v2.search.MockEbookSearch +import dpla.api.v2.search.SearchProtocol.SearchCommand import dpla.api.v2.smr.MockSmrRequestHandler import dpla.api.v2.smr.SmrProtocol.SmrCommand import org.scalatest.matchers.should.Matchers import org.scalatest.wordspec.AnyWordSpec +import scala.concurrent.duration._ +import akka.http.scaladsl.testkit.RouteTestTimeout -class InvalidParamsTest extends AnyWordSpec with Matchers - with ScalatestRouteTest with ActorHelper { +class InvalidParamsTest + extends AnyWordSpec + with Matchers + with ScalatestRouteTest + with ActorHelper { lazy val testKit: ActorTestKit = ActorTestKit() override def afterAll(): Unit = testKit.shutdownTestKit @@ -25,6 +37,19 @@ class InvalidParamsTest extends AnyWordSpec with Matchers override def createActorSystem(): akka.actor.ActorSystem = testKit.system.classicSystem + implicit val routeTestTimeout: RouteTestTimeout = RouteTestTimeout(3.seconds) + + val ebookSearch: ActorRef[SearchCommand] = + MockEbookSearch(testKit) + + val ebookRegistry: ActorRef[SearchRegistryCommand] = + MockEbookRegistry( + testKit, + authenticator, + ebookAnalyticsClient, + Some(ebookSearch) + ) + val smrRequestHandler: ActorRef[SmrCommand] = MockSmrRequestHandler(testKit, Some(s3ClientSuccess)) @@ -32,8 +57,13 @@ class InvalidParamsTest extends AnyWordSpec with Matchers MockSmrRegistry(testKit, authenticator, Some(smrRequestHandler)) lazy val routes: Route = - new Routes(itemRegistry, pssRegistry, apiKeyRegistry, - smrRegistryS3Success).applicationRoutes + new Routes( + ebookRegistry, + itemRegistry, + pssRegistry, + apiKeyRegistry, + smrRegistryS3Success + ).applicationRoutes "malformed api_key" should { "return Forbidden for a key that is too short" in { @@ -61,10 +91,50 @@ class InvalidParamsTest extends AnyWordSpec with Matchers } "still pass a well-formed key through to the registry" in { - val request = Get(s"/v2/items?api_key=$fakeApiKey") + val request = Get(s"/v2/ebooks?api_key=$fakeApiKey") request ~> Route.seal(routes) ~> check { status should not be StatusCodes.Forbidden - status should not be StatusCodes.NotFound + } + } + } + + "/v2/ebooks route" should { + "return BadRequest if params are invalid" in { + val request = Get(s"/v2/ebooks?page=foo&api_key=$fakeApiKey") + + request ~> Route.seal(routes) ~> check { + status shouldEqual StatusCodes.BadRequest + contentType should ===(ContentTypes.`application/json`) + } + } + + "ignore empty params" in { + val request = Get(s"/v2/ebooks?page=&api_key=$fakeApiKey") + + request ~> Route.seal(routes) ~> check { + status should not be StatusCodes.BadRequest + contentType should ===(ContentTypes.`application/json`) + } + } + } + + "/v2/ebooks[id] route" should { + "return BadRequest if id is invalid" in { + val request = Get(s"/v2/ebooks/?api_key=$fakeApiKey") + + request ~> Route.seal(routes) ~> check { + status shouldEqual StatusCodes.BadRequest + contentType should ===(ContentTypes.`application/json`) + } + } + + "return BadRequest if params are invalid" in { + val request = + Get(s"/v2/ebooks/R0VfVX4BfY91SSpFGqxt?foo=bar&api_key=$fakeApiKey") + + request ~> Route.seal(routes) ~> check { + status shouldEqual StatusCodes.BadRequest + contentType should ===(ContentTypes.`application/json`) } } } @@ -75,7 +145,9 @@ class InvalidParamsTest extends AnyWordSpec with Matchers val validPost = "123" val validUser = "abc" - val request = Post(s"/v2/smr?api_key=$fakeApiKey&service=$validService&post=$validPost&user=$validUser") + val request = Post( + s"/v2/smr?api_key=$fakeApiKey&service=$validService&post=$validPost&user=$validUser" + ) request ~> Route.seal(routes) ~> check { status shouldEqual StatusCodes.BadRequest @@ -89,7 +161,7 @@ class InvalidParamsTest extends AnyWordSpec with Matchers request ~> Route.seal(routes) ~> check { status shouldEqual StatusCodes.BadRequest - contentType should === (ContentTypes.`application/json`) + contentType should ===(ContentTypes.`application/json`) } } } diff --git a/src/test/scala/dpla/api/v2/search/ConcurrencyLimiterTest.scala b/src/test/scala/dpla/api/v2/search/ConcurrencyLimiterTest.scala new file mode 100644 index 00000000..93ea2436 --- /dev/null +++ b/src/test/scala/dpla/api/v2/search/ConcurrencyLimiterTest.scala @@ -0,0 +1,180 @@ +package dpla.api.v2.search + +import org.scalatest.matchers.should.Matchers +import org.scalatest.wordspec.AnyWordSpec +import org.scalatest.concurrent.{Eventually, ScalaFutures} +import org.scalatest.time.{Milliseconds, Seconds, Span} + +import scala.concurrent.{Future, Promise} +import scala.concurrent.ExecutionContext.Implicits.global + +class ConcurrencyLimiterTest + extends AnyWordSpec + with Matchers + with ScalaFutures + with Eventually { + + // Configure patience for async assertions + implicit override val patienceConfig: PatienceConfig = + PatienceConfig( + timeout = Span(5, Seconds), + interval = Span(50, Milliseconds) + ) + + "ConcurrencyLimiter" should { + + "allow a successful Future to complete and release permit" in { + val limiter = + new ConcurrencyLimiter(maxConcurrent = 2, timeoutSeconds = 1) + + limiter.availablePermits shouldBe 2 + + val result = limiter(Future.successful("success")) + + whenReady(result) { value => + value shouldBe "success" + } + + // Permit should be released after Future completes + eventually { + limiter.availablePermits shouldBe 2 + } + } + + "allow a failed Future to complete and release permit" in { + val limiter = + new ConcurrencyLimiter(maxConcurrent = 2, timeoutSeconds = 1) + val error = new RuntimeException("test error") + + limiter.availablePermits shouldBe 2 + + val result = limiter(Future.failed(error)) + + whenReady(result.failed) { ex => + ex shouldBe error + } + + // Permit should be released even on failure + eventually { + limiter.availablePermits shouldBe 2 + } + } + + "release permit if Future construction throws" in { + val limiter = + new ConcurrencyLimiter(maxConcurrent = 2, timeoutSeconds = 1) + val error = new RuntimeException("construction error") + + limiter.availablePermits shouldBe 2 + + val result = limiter { + throw error + } + + whenReady(result.failed) { ex => + ex shouldBe error + } + + // Permit should be released even when Future construction throws + limiter.availablePermits shouldBe 2 + } + + "limit concurrent executions to maxConcurrent" in { + val limiter = + new ConcurrencyLimiter(maxConcurrent = 2, timeoutSeconds = 1) + val promises = (1 to 3).map(_ => Promise[String]()) + + limiter.availablePermits shouldBe 2 + + // Start two futures that won't complete until we say so + val future1 = limiter(promises(0).future) + val future2 = limiter(promises(1).future) + + // Both should have acquired permits + eventually { + limiter.availablePermits shouldBe 0 + } + + // Third request should fail because limit is reached and timeout expires + val future3 = limiter(promises(2).future) + + whenReady(future3.failed) { ex => + ex shouldBe a[ConcurrencyLimitExceeded] + val cle = ex.asInstanceOf[ConcurrencyLimitExceeded] + cle.maxConcurrent shouldBe 2 + cle.timeoutSeconds shouldBe 1 + } + + // Complete the first two futures + promises(0).success("result1") + promises(1).success("result2") + + whenReady(future1) { _ shouldBe "result1" } + whenReady(future2) { _ shouldBe "result2" } + + // Permits should be released + eventually { + limiter.availablePermits shouldBe 2 + } + } + + "allow new requests after permits are released" in { + val limiter = + new ConcurrencyLimiter(maxConcurrent = 1, timeoutSeconds = 1) + val promise1 = Promise[String]() + + // First request acquires the only permit + val future1 = limiter(promise1.future) + eventually { + limiter.availablePermits shouldBe 0 + } + + // Complete first request + promise1.success("first") + whenReady(future1) { _ shouldBe "first" } + + // Permit should be released + eventually { + limiter.availablePermits shouldBe 1 + } + + // Second request should succeed + val future2 = limiter(Future.successful("second")) + whenReady(future2) { _ shouldBe "second" } + } + + "reject construction with non-positive maxConcurrent" in { + an[IllegalArgumentException] should be thrownBy { + new ConcurrencyLimiter(maxConcurrent = 0, timeoutSeconds = 1) + } + an[IllegalArgumentException] should be thrownBy { + new ConcurrencyLimiter(maxConcurrent = -1, timeoutSeconds = 1) + } + } + + "reject construction with non-positive timeoutSeconds" in { + an[IllegalArgumentException] should be thrownBy { + new ConcurrencyLimiter(maxConcurrent = 1, timeoutSeconds = 0) + } + an[IllegalArgumentException] should be thrownBy { + new ConcurrencyLimiter(maxConcurrent = 1, timeoutSeconds = -1) + } + } + } + + "ConcurrencyLimitExceeded" should { + + "contain useful error message" in { + val ex = ConcurrencyLimitExceeded(maxConcurrent = 32, timeoutSeconds = 5) + ex.getMessage should include("32") + ex.getMessage should include("5") + ex.getMessage should include("Concurrency limit") + } + + "expose maxConcurrent and timeoutSeconds" in { + val ex = ConcurrencyLimitExceeded(maxConcurrent = 32, timeoutSeconds = 5) + ex.maxConcurrent shouldBe 32 + ex.timeoutSeconds shouldBe 5 + } + } +} diff --git a/src/test/scala/dpla/api/v2/search/paramValidators/ItemParamValidatorTest.scala b/src/test/scala/dpla/api/v2/search/paramValidators/ItemParamValidatorTest.scala index b8b9ac59..87c7d44f 100644 --- a/src/test/scala/dpla/api/v2/search/paramValidators/ItemParamValidatorTest.scala +++ b/src/test/scala/dpla/api/v2/search/paramValidators/ItemParamValidatorTest.scala @@ -2,13 +2,20 @@ package dpla.api.v2.search.paramValidators import akka.actor.testkit.typed.scaladsl.{ActorTestKit, TestProbe} import akka.actor.typed.ActorRef -import dpla.api.v2.search.SearchProtocol.{IntermediateSearchResult, RawSearchParams, SearchResponse, ValidSearchParams} +import dpla.api.v2.search.SearchProtocol.{ + IntermediateSearchResult, + RawSearchParams, + SearchResponse, + ValidSearchParams +} import org.scalatest.BeforeAndAfterAll import org.scalatest.matchers.should.Matchers import org.scalatest.wordspec.AnyWordSpec -class ItemParamValidatorTest extends AnyWordSpec with Matchers - with BeforeAndAfterAll { +class ItemParamValidatorTest + extends AnyWordSpec + with Matchers + with BeforeAndAfterAll { lazy val testKit: ActorTestKit = ActorTestKit() override def afterAll(): Unit = testKit.shutdownTestKit() @@ -26,7 +33,7 @@ class ItemParamValidatorTest extends AnyWordSpec with Matchers "ignore valid DPLA Map fields not applicable to items" in { val given = "sourceResource.subtitle" val expected = Some(Seq()) - val params = Map("facets" -> given) + val params = Map("facets" -> given, "q" -> "test") itemParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.facets shouldEqual expected diff --git a/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala b/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala index 76a46a87..b9666c20 100644 --- a/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala +++ b/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala @@ -10,9 +10,10 @@ import org.scalatest.wordspec.AnyWordSpec import scala.util.Random - -class ParamValidatorTest extends AnyWordSpec with Matchers - with BeforeAndAfterAll { +class ParamValidatorTest + extends AnyWordSpec + with Matchers + with BeforeAndAfterAll { lazy val testKit: ActorTestKit = ActorTestKit() override def afterAll(): Unit = testKit.shutdownTestKit() @@ -23,13 +24,16 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val interProbe: TestProbe[IntermediateSearchResult] = testKit.createTestProbe[IntermediateSearchResult] + val ebookParamValidator: ActorRef[IntermediateSearchResult] = + testKit.spawn(EbookParamValidator(interProbe.ref)) + val itemParamValidator: ActorRef[IntermediateSearchResult] = testKit.spawn(ItemParamValidator(interProbe.ref)) "search param validator" should { "reject unrecognized params" in { val params = Map("foo" -> "bar") - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -38,14 +42,15 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject unrecognized params" in { val id = "R0VfVX4BfY91SSpFGqxt" val params = Map("foo" -> "bar") - itemParamValidator ! RawFetchParams(id, params, replyProbe.ref) + ebookParamValidator ! RawFetchParams(id, params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } "random param validator" should { "accept valid filter" in { - val params = Map("filter" -> "provider.@id:http://dp.la/api/contributor/lc") + val params = + Map("filter" -> "provider.@id:http://dp.la/api/contributor/lc") itemParamValidator ! RawRandomParams(params, replyProbe.ref) interProbe.expectMessageType[ValidRandomParams] } @@ -60,28 +65,32 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "ID validator" should { "accept valid ID" in { val id = "ufwPJ34Bj-MaVWqX9KZL" - itemParamValidator ! RawFetchParams(id, Map(), replyProbe.ref) + ebookParamValidator ! RawFetchParams(id, Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidFetchParams] msg.ids should contain only id } "accept multiple valid IDs" in { - val ids = "b70107e4fe29fe4a247ae46e118ce192,17b0da7b05805d78daf8753a6641b3f5" - val expected = Seq("b70107e4fe29fe4a247ae46e118ce192", "17b0da7b05805d78daf8753a6641b3f5") - itemParamValidator ! RawFetchParams(ids, Map(), replyProbe.ref) + val ids = + "b70107e4fe29fe4a247ae46e118ce192,17b0da7b05805d78daf8753a6641b3f5" + val expected = Seq( + "b70107e4fe29fe4a247ae46e118ce192", + "17b0da7b05805d78daf8753a6641b3f5" + ) + ebookParamValidator ! RawFetchParams(ids, Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidFetchParams] msg.ids should contain allElementsOf expected } "reject ID with special characters" in { val id = "" - itemParamValidator ! RawFetchParams(id, Map(), replyProbe.ref) + ebookParamValidator ! RawFetchParams(id, Map(), replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject too-long ID" in { val id = "asdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdf" - itemParamValidator ! RawFetchParams(id, Map(), replyProbe.ref) + ebookParamValidator ! RawFetchParams(id, Map(), replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } @@ -89,7 +98,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers var idSeq = Seq[String]() 501.times { idSeq = idSeq :+ Random.alphanumeric.take(32).mkString } val ids = idSeq.mkString(",") - itemParamValidator ! RawFetchParams(ids, Map(), replyProbe.ref) + ebookParamValidator ! RawFetchParams(ids, Map(), replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -111,7 +120,8 @@ class ParamValidatorTest extends AnyWordSpec with Matchers itemParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.date.begin").map(_.value) + .find(_.fieldName == "sourceResource.date.begin") + .map(_.value) fieldValue shouldEqual expected } @@ -122,7 +132,8 @@ class ParamValidatorTest extends AnyWordSpec with Matchers itemParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.date.begin").map(_.value) + .find(_.fieldName == "sourceResource.date.begin") + .map(_.value) fieldValue shouldEqual expected } @@ -133,7 +144,8 @@ class ParamValidatorTest extends AnyWordSpec with Matchers itemParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.date.begin").map(_.value) + .find(_.fieldName == "sourceResource.date.begin") + .map(_.value) fieldValue shouldEqual expected } @@ -148,7 +160,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "creator validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "sourceResource.creator") @@ -159,24 +171,25 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "Jules Verne" val expected = Some("Jules Verne") val params = Map("sourceResource.creator" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.creator").map(_.value) + .find(_.fieldName == "sourceResource.creator") + .map(_.value) fieldValue shouldEqual expected } "reject too-short param" in { val given = "d" val params = Map("sourceResource.creator" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject too-long param" in { val given: String = Random.alphanumeric.take(201).mkString val params = Map("sourceResource.creator" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -184,7 +197,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "provider id validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "provider.@id") @@ -195,17 +208,18 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "https://standardebooks.org" val expected = Some("https://standardebooks.org") val params = Map("provider.@id" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "provider.@id").map(_.value) + .find(_.fieldName == "provider.@id") + .map(_.value) fieldValue shouldEqual expected } "reject invalid URL" in { val given = "standardebooks" val params = Map("provider.@id" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -213,7 +227,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "description validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "sourceResource.description") @@ -224,24 +238,25 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "dogs" val expected = Some("dogs") val params = Map("sourceResource.description" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.description").map(_.value) + .find(_.fieldName == "sourceResource.description") + .map(_.value) fieldValue shouldEqual expected } "reject too-short param" in { val given = "d" val params = Map("sourceResource.description" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject too-long param" in { val given: String = Random.alphanumeric.take(201).mkString val params = Map("sourceResource.description" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -249,7 +264,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "exact_field_match validator" should { "handle empty param" in { val expected = false - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.exactFieldMatch shouldEqual expected } @@ -258,7 +273,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "true" val expected = true val params = Map("exact_field_match" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.exactFieldMatch shouldEqual expected } @@ -266,7 +281,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject non-boolean param" in { val given = "yes" val params = Map("exact_field_match" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -274,7 +289,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "facet validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.facets shouldEqual expected } @@ -282,8 +297,8 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "accept valid param" in { val given = "provider.@id,sourceResource.subject.name" val expected = Some(Seq("provider.@id", "sourceResource.subject.name")) - val params = Map("facets" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + val params = Map("facets" -> given, "q" -> "dogs") + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.facets shouldEqual expected } @@ -291,24 +306,30 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject unfacetable field" in { val given = "sourceResource.description" val params = Map("facets" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "accept valid coordinates param" in { val given = "sourceResource.spatial.coordinates:42:-70" val expected = Some(Seq("sourceResource.spatial.coordinates:42:-70")) - val params = Map("facets" -> given) + val params = Map("facets" -> given, "q" -> "dogs") itemParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.facets shouldEqual expected } + + "reject facets with no query or filter" in { + val params = Map("facets" -> "provider.@id") + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) + replyProbe.expectMessageType[InvalidSearchParams] + } } "facet size validator" should { "handle empty param" in { val expected = 50 - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.facetSize shouldEqual expected } @@ -317,7 +338,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "30" val expected = 30 val params = Map("facet_size" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.facetSize shouldEqual expected } @@ -325,15 +346,15 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject non-int param" in { val given = "foo" val params = Map("facet_size" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "default to max if param is too large" in { val given = "9999" - val expected = 2000 + val expected = 200 val params = Map("facet_size" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.facetSize shouldEqual expected } @@ -342,7 +363,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "fields validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.fields shouldEqual expected } @@ -351,7 +372,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "provider.@id,sourceResource.creator" val expected = Some(Seq("provider.@id", "sourceResource.creator")) val params = Map("fields" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.fields shouldEqual expected } @@ -359,7 +380,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject unrecognized field" in { val given = "foo" val params = Map("fields" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -367,7 +388,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "format validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "sourceResource.format") @@ -378,24 +399,25 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "article" val expected = Some("article") val params = Map("sourceResource.format" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.format").map(_.value) + .find(_.fieldName == "sourceResource.format") + .map(_.value) fieldValue shouldEqual expected } "reject too-short param" in { val given = "d" val params = Map("sourceResource.format" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject too-long param" in { val given: String = Random.alphanumeric.take(201).mkString val params = Map("sourceResource.format" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -403,7 +425,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "isShownAt validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "isShownAt") @@ -411,20 +433,24 @@ class ParamValidatorTest extends AnyWordSpec with Matchers } "accept valid param" in { - val given = "\"https://standardebooks.org/ebooks/j-s-fletcher/the-charing-cross-mystery\"" - val expected = Some("\"https://standardebooks.org/ebooks/j-s-fletcher/the-charing-cross-mystery\"") + val given = + "\"https://standardebooks.org/ebooks/j-s-fletcher/the-charing-cross-mystery\"" + val expected = Some( + "\"https://standardebooks.org/ebooks/j-s-fletcher/the-charing-cross-mystery\"" + ) val params = Map("isShownAt" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "isShownAt").map(_.value) + .find(_.fieldName == "isShownAt") + .map(_.value) fieldValue shouldEqual expected } "reject invalid URL" in { val given = "the-charing-cross-mystery" val params = Map("isShownAt" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -432,7 +458,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "language validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "sourceResource.language.name") @@ -443,24 +469,25 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "fr" val expected = Some("fr") val params = Map("sourceResource.language.name" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.language.name").map(_.value) + .find(_.fieldName == "sourceResource.language.name") + .map(_.value) fieldValue shouldEqual expected } "reject too-short param" in { val given = "d" val params = Map("sourceResource.language.name" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject too-long param" in { val given: String = Random.alphanumeric.take(201).mkString val params = Map("sourceResource.language.name" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -468,7 +495,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "object validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "object") @@ -479,17 +506,18 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "http://payload-permanent-address.dp.la" val expected = Some("http://payload-permanent-address.dp.la") val params = Map("object" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "object").map(_.value) + .find(_.fieldName == "object") + .map(_.value) fieldValue shouldEqual expected } "reject invalid URL" in { val given = "http/payload-permanent-address.dp.la" val params = Map("object" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -497,7 +525,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "op validator" should { "handle empty param" in { val expected = "AND" - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.op shouldEqual expected } @@ -506,7 +534,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "OR" val expected = "OR" val params = Map("op" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.op shouldEqual expected } @@ -514,7 +542,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject invalid param" in { val given = "or" val params = Map("op" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -522,16 +550,16 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "page validator" should { "handle empty param" in { val expected = 1 - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.page shouldEqual expected } "accept valid param" in { - val given = "27" - val expected = 27 + val given = "5" + val expected = 5 val params = Map("page" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.page shouldEqual expected } @@ -539,31 +567,35 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject non-int param" in { val given = "foo" val params = Map("page" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject out-of-range param" in { val given = "0" val params = Map("page" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "default to max if param is too large" in { val given = "600" - val expected = 100 val params = Map("page" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) - val msg = interProbe.expectMessageType[ValidSearchParams] - msg.params.page shouldEqual expected + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) + replyProbe.expectMessageType[InvalidSearchParams] + } + + "reject deep pagination beyond 1000 results" in { + val params = Map("page" -> "50", "page_size" -> "50") + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) + replyProbe.expectMessageType[InvalidSearchParams] } } "page size validator" should { "handle empty param" in { val expected = 10 - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.pageSize shouldEqual expected } @@ -572,7 +604,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "50" val expected = 50 val params = Map("page_size" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.pageSize shouldEqual expected } @@ -580,24 +612,22 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject non-int param" in { val given = "foo" val params = Map("page_size" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "default to max if param is too large" in { val given = "999999" - val expected = 500 val params = Map("page_size" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) - val msg = interProbe.expectMessageType[ValidSearchParams] - msg.params.pageSize shouldEqual expected + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) + replyProbe.expectMessageType[InvalidSearchParams] } } "publisher validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "sourceResource.publisher") @@ -608,24 +638,25 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "Penguin" val expected = Some("Penguin") val params = Map("sourceResource.publisher" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.publisher").map(_.value) + .find(_.fieldName == "sourceResource.publisher") + .map(_.value) fieldValue shouldEqual expected } "reject too-short param" in { val given = "d" val params = Map("sourceResource.publisher" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject too-long param" in { val given: String = Random.alphanumeric.take(201).mkString val params = Map("sourceResource.publisher" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -633,7 +664,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "q validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.q shouldEqual expected } @@ -642,7 +673,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "dogs" val expected = Some("dogs") val params = Map("q" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.q shouldEqual expected } @@ -650,14 +681,14 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject too-short param" in { val given = "d" val params = Map("q" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject too-long param" in { val given: String = Random.alphanumeric.take(201).mkString val params = Map("q" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -665,7 +696,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "sortBy validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.sortBy shouldEqual expected } @@ -674,7 +705,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "sourceResource.title" val expected = Some("sourceResource.title") val params = Map("sort_by" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.sortBy shouldEqual expected } @@ -682,7 +713,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject invalid param" in { val given = "sourceResource.description" val params = Map("sort_by" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -690,7 +721,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "sortOrder validator" should { "handle empty param" in { val expected = "asc" - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.sortOrder shouldEqual expected } @@ -699,7 +730,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "desc" val expected = "desc" val params = Map("sort_order" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.sortOrder shouldEqual expected } @@ -707,7 +738,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject invalid param" in { val given = "descending" val params = Map("sort_order" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -715,7 +746,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "subject validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "sourceResource.subject.name") @@ -726,24 +757,25 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "dogs" val expected = Some("dogs") val params = Map("sourceResource.subject.name" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.subject.name").map(_.value) + .find(_.fieldName == "sourceResource.subject.name") + .map(_.value) fieldValue shouldEqual expected } "reject too-short param" in { val given = "d" - val params = Map("sourceResource.subject.name" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + val params = Map("sourceResource.subject.name" -> given) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject too-long param" in { val given: String = Random.alphanumeric.take(201).mkString val params = Map("sourceResource.subject.name" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -751,7 +783,7 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "title validator" should { "handle empty param" in { val expected = None - itemParamValidator ! RawSearchParams(Map(), replyProbe.ref) + ebookParamValidator ! RawSearchParams(Map(), replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries .find(_.fieldName == "sourceResource.title") @@ -762,24 +794,25 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val given = "The Scarlet Letter" val expected = Some("The Scarlet Letter") val params = Map("sourceResource.title" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] val fieldValue = msg.params.fieldQueries - .find(_.fieldName == "sourceResource.title").map(_.value) + .find(_.fieldName == "sourceResource.title") + .map(_.value) fieldValue shouldEqual expected } "reject too-short param" in { val given = "d" val params = Map("sourceResource.title" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } "reject too-long param" in { val given: String = Random.alphanumeric.take(201).mkString val params = Map("sourceResource.title" -> given) - itemParamValidator ! RawSearchParams(params, replyProbe.ref) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } } @@ -789,13 +822,23 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "accept valid combination of sort_by and sort_by_pin" in { val givenSortBy = "sourceResource.spatial.coordinates" val givenSortByPin = "40,-73" - val params = Map("sort_by" -> givenSortBy, "sort_by_pin" -> givenSortByPin) + val params = + Map("sort_by" -> givenSortBy, "sort_by_pin" -> givenSortByPin) itemParamValidator ! RawSearchParams(params, replyProbe.ref) val msg = interProbe.expectMessageType[ValidSearchParams] msg.params.sortBy shouldEqual Some(givenSortBy) msg.params.sortByPin shouldEqual Some(givenSortByPin) } + "reject sort_by coordinates if sort_by_pin is not and accepted param" in { + val givenSortBy = "sourceResource.spatial.coordinates" + val givenSortByPin = "40,-73" + val params = + Map("sort_by" -> givenSortBy, "sort_by_pin" -> givenSortByPin) + ebookParamValidator ! RawSearchParams(params, replyProbe.ref) + replyProbe.expectMessageType[InvalidSearchParams] + } + "reject sort_by coordinates if sort_by_pin is not present" in { val givenSortBy = "sourceResource.spatial.coordinates" val params = Map("sort_by" -> givenSortBy) @@ -806,7 +849,8 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject sort_by_pin if sort_by coordinates is not present" in { val givenSortBy = "dataProvider.name" val givenSortByPin = "40,-73" - val params = Map("sort_by" -> givenSortBy, "sort_by_pin" -> givenSortByPin) + val params = + Map("sort_by" -> givenSortBy, "sort_by_pin" -> givenSortByPin) itemParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } @@ -814,7 +858,8 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject too-long sort_by_pin param" in { val givenSortBy = "sourceResource.spatial.coordinates" val givenSortByPin = Random.alphanumeric.take(201).mkString - val params = Map("sort_by" -> givenSortBy, "sort_by_pin" -> givenSortByPin) + val params = + Map("sort_by" -> givenSortBy, "sort_by_pin" -> givenSortByPin) itemParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } @@ -822,7 +867,8 @@ class ParamValidatorTest extends AnyWordSpec with Matchers "reject too-short sort_by_pin param" in { val givenSortBy = "sourceResource.spatial.coordinates" val givenSortByPin = "4" - val params = Map("sort_by" -> givenSortBy, "sort_by_pin" -> givenSortByPin) + val params = + Map("sort_by" -> givenSortBy, "sort_by_pin" -> givenSortByPin) itemParamValidator ! RawSearchParams(params, replyProbe.ref) replyProbe.expectMessageType[InvalidSearchParams] } @@ -838,8 +884,8 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val msg = interProbe.expectMessageType[ValidSearchParams] val fieldName = msg.params.filter.flatMap(_.headOption.map(_.fieldName)) val value = msg.params.filter.flatMap(_.headOption.map(_.value)) - fieldName should contain (expectedFieldName) - value should contain (expectedValue) + fieldName should contain(expectedFieldName) + value should contain(expectedValue) } "accept valid value for text field" in { @@ -851,8 +897,8 @@ class ParamValidatorTest extends AnyWordSpec with Matchers val msg = interProbe.expectMessageType[ValidSearchParams] val fieldName = msg.params.filter.flatMap(_.headOption.map(_.fieldName)) val value = msg.params.filter.flatMap(_.headOption.map(_.value)) - fieldName should contain (expectedFieldName) - value should contain (expectedValue) + fieldName should contain(expectedFieldName) + value should contain(expectedValue) } "reject unsearchable field" in { diff --git a/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala b/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala index b8b29de5..2970f6ec 100644 --- a/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala +++ b/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala @@ -5,14 +5,23 @@ import akka.actor.typed.ActorRef import dpla.api.v2.search.SearchProtocol._ import dpla.api.v2.search.mappings.JsonFieldReader import dpla.api.v2.search.paramValidators -import dpla.api.v2.search.paramValidators.{FieldQuery, Filter, RandomParams, SearchParams} +import dpla.api.v2.search.paramValidators.{ + FieldQuery, + Filter, + RandomParams, + SearchParams +} import org.scalatest.matchers.should.Matchers import org.scalatest.wordspec.AnyWordSpec import org.scalatest.{BeforeAndAfterAll, PrivateMethodTester} import spray.json._ -class QueryBuilderTest extends AnyWordSpec with Matchers - with PrivateMethodTester with JsonFieldReader with BeforeAndAfterAll { +class QueryBuilderTest + extends AnyWordSpec + with Matchers + with PrivateMethodTester + with JsonFieldReader + with BeforeAndAfterAll { lazy val testKit: ActorTestKit = ActorTestKit() override def afterAll(): Unit = testKit.shutdownTestKit() @@ -146,7 +155,8 @@ class QueryBuilderTest extends AnyWordSpec with Matchers facetSize = 100, fields = None, fieldQueries = Seq[FieldQuery](), - filter = Some(Seq(Filter("provider.@id", "http://dp.la/api/contributor/lc"))), + filter = + Some(Seq(Filter("provider.@id", "http://dp.la/api/contributor/lc"))), op = "AND", page = 3, pageSize = 20, @@ -166,7 +176,8 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val multiFetchQuery: JsObject = getJsFetchQuery(multiFetchIds) val randomParams: RandomParams = RandomParams( - filter = Some(Seq(Filter("provider.@id", "http://dp.la/api/contributor/lc"))), + filter = + Some(Seq(Filter("provider.@id", "http://dp.la/api/contributor/lc"))) ) val randomQuery: JsObject = getJsRandomQuery(randomParams) @@ -213,15 +224,15 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "random query builder" should { "specify random score" in { - val traversed = readObject(randomQuery, "query", "function_score", - "random_score") + val traversed = + readObject(randomQuery, "query", "function_score", "random_score") traversed should not be None } "specify boost mode" in { val expected = Some("sum") - val traversed = readString(randomQuery, "query", "function_score", - "boost_mode") + val traversed = + readString(randomQuery, "query", "function_score", "boost_mode") assert(traversed == expected) } @@ -233,9 +244,15 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "specify filter" in { val expected = Some("http://dp.la/api/contributor/lc") - val traversed = readObjectArray(randomQuery, "query", "function_score", - "query", "bool", "filter", "bool", "must").headOption - .flatMap(must => readString(must, "term", "provider.@id")) + val traversed = readObjectArray( + randomQuery, + "query", + "function_score", + "query", + "bool", + "filter" + ).headOption + .flatMap(f => readString(f, "term", "provider.@id")) assert(traversed == expected) } } @@ -254,8 +271,8 @@ class QueryBuilderTest extends AnyWordSpec with Matchers } "specify track_total_hits" in { - val expected = Some(true) - val traversed = readBoolean(minQuery, "track_total_hits") + val expected = Some(10000) + val traversed = readInt(minQuery, "track_total_hits") assert(traversed == expected) } } @@ -271,7 +288,7 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val expected = Some("dogs") val boolMust = readObjectArray(detailQuery, "query", "bool", "must") val queryString = - boolMust.flatMap(obj => readObject(obj, "query_string")).head + boolMust.flatMap(obj => readObject(obj, "multi_match")).head val traversed = readString(queryString, "query") assert(traversed == expected) } @@ -279,26 +296,17 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "specify fields to search" in { val boolMust = readObjectArray(detailQuery, "query", "bool", "must") val queryString = - boolMust.flatMap(obj => readObject(obj, "query_string")).head + boolMust.flatMap(obj => readObject(obj, "multi_match")).head val traversed = readStringArray(queryString, "fields") assert(traversed.nonEmpty) } - "specify wildcard analyzer" in { - val expected = Some(true) - val boolMust = readObjectArray(detailQuery, "query", "bool", "must") - val queryString = - boolMust.flatMap(obj => readObject(obj, "query_string")).head - val traversed = readBoolean(queryString, "analyze_wildcard") - assert(traversed == expected) - } - "specify default operator" in { val expected = Some("AND") val boolMust = readObjectArray(detailQuery, "query", "bool", "must") val queryString = - boolMust.flatMap(obj => readObject(obj, "query_string")).head - val traversed = readString(queryString, "default_operator") + boolMust.flatMap(obj => readObject(obj, "multi_match")).head + val traversed = readString(queryString, "operator") assert(traversed == expected) } @@ -306,7 +314,7 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val expected = Some(true) val boolMust = readObjectArray(detailQuery, "query", "bool", "must") val queryString = - boolMust.flatMap(obj => readObject(obj, "query_string")).head + boolMust.flatMap(obj => readObject(obj, "multi_match")).head val traversed = readBoolean(queryString, "lenient") assert(traversed == expected) } @@ -314,22 +322,22 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "field query builder" should { "handle no field search with q" in { - val params = minSearchParams.copy(q=Some("dogs")) + val params = minSearchParams.copy(q = Some("dogs")) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryString = - boolMust.flatMap(obj => readObject(obj, "query_string")) + boolMust.flatMap(obj => readObject(obj, "multi_match")) assert(queryString.size == 1) } "handle field search with no q" in { - val fieldQueries = Seq(FieldQuery("sourceResource.subject.name", "london")) - val params = minSearchParams.copy(fieldQueries=fieldQueries) + val fieldQueries = + Seq(FieldQuery("sourceResource.subject.name", "london")) + val params = minSearchParams.copy(fieldQueries = fieldQueries) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryString = - - boolMust.flatMap(obj => readObject(obj, "query_string")) + boolMust.flatMap(obj => readObject(obj, "multi_match")) assert(queryString.size == 1) } @@ -338,34 +346,35 @@ class QueryBuilderTest extends AnyWordSpec with Matchers FieldQuery("sourceResource.subject.name", "london"), FieldQuery("provider.@id", "http://standardebooks.org") ) - val params = minSearchParams.copy(fieldQueries=fieldQueries) + val params = minSearchParams.copy(fieldQueries = fieldQueries) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryMatch = - boolMust.flatMap(obj => readObject(obj, "query_string")) + boolMust.flatMap(obj => readObject(obj, "multi_match")) assert(queryMatch.size == 2) } "specify field query term" in { val expected = Some("london") val fieldQuery = Seq(FieldQuery("sourceResource.subject.name", "london")) - val params = minSearchParams.copy(fieldQueries=fieldQuery) + val params = minSearchParams.copy(fieldQueries = fieldQuery) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryString = - boolMust.flatMap(obj => readObject(obj, "query_string")).head + boolMust.flatMap(obj => readObject(obj, "multi_match")).head val traversed = readString(queryString, "query") assert(traversed == expected) } "specify field to search" in { val expected = Seq("sourceResource.subject.name") - val fieldQueries = Seq(FieldQuery("sourceResource.subject.name", "london")) - val params = minSearchParams.copy(fieldQueries=fieldQueries) + val fieldQueries = + Seq(FieldQuery("sourceResource.subject.name", "london")) + val params = minSearchParams.copy(fieldQueries = fieldQueries) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryString = - boolMust.flatMap(obj => readObject(obj, "query_string")).head + boolMust.flatMap(obj => readObject(obj, "multi_match")).head val traversed = readStringArray(queryString, "fields") assert(traversed == expected) } @@ -387,7 +396,8 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val boolMust = readObjectArray(query, "query", "bool", "must") val queryTerm = boolMust.flatMap(obj => readObject(obj, "term")).head - val traversed = readString(queryTerm, "sourceResource.subject.name.not_analyzed") + val traversed = + readString(queryTerm, "sourceResource.subject.name.not_analyzed") assert(traversed == expected) } @@ -395,42 +405,60 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val expected = Some("Mystery fiction") val fieldQueries = Seq(FieldQuery("sourceResource.subject.name", "\"Mystery fiction\"")) - val params = minSearchParams.copy(fieldQueries=fieldQueries, exactFieldMatch=true) + val params = minSearchParams.copy( + fieldQueries = fieldQueries, + exactFieldMatch = true + ) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryTerm = boolMust.flatMap(obj => readObject(obj, "term")).head - val traversed = readString(queryTerm, "sourceResource.subject.name.not_analyzed") + val traversed = + readString(queryTerm, "sourceResource.subject.name.not_analyzed") assert(traversed == expected) } "handle multiple terms joined by +AND+" in { val expected = Seq(Some("Legislators"), Some("City Council")) - val fieldQueries = Seq(FieldQuery( - "sourceResource.subject.name", - "\"Legislators\" AND \"City Council\"" - )) - val params = minSearchParams.copy(fieldQueries=fieldQueries, exactFieldMatch=true) + val fieldQueries = Seq( + FieldQuery( + "sourceResource.subject.name", + "\"Legislators\" AND \"City Council\"" + ) + ) + val params = minSearchParams.copy( + fieldQueries = fieldQueries, + exactFieldMatch = true + ) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryTerms = boolMust.flatMap(obj => readObject(obj, "term")) val traversed = - queryTerms.map(readString(_, "sourceResource.subject.name.not_analyzed")) + queryTerms.map( + readString(_, "sourceResource.subject.name.not_analyzed") + ) assert(traversed == expected) } "handle multiple terms joined by +OR+" in { val expected = Seq(Some("Legislators"), Some("City Council")) - val fieldQueries = Seq(FieldQuery( - "sourceResource.subject.name", - "\"Legislators OR City Council\"" - )) - val params = minSearchParams.copy(fieldQueries=fieldQueries, exactFieldMatch=true) + val fieldQueries = Seq( + FieldQuery( + "sourceResource.subject.name", + "\"Legislators OR City Council\"" + ) + ) + val params = minSearchParams.copy( + fieldQueries = fieldQueries, + exactFieldMatch = true + ) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryTerms = boolMust.flatMap(obj => readObject(obj, "term")) val traversed = - queryTerms.map(readString(_, "sourceResource.subject.name.not_analyzed")) + queryTerms.map( + readString(_, "sourceResource.subject.name.not_analyzed") + ) assert(traversed == expected) } } @@ -439,7 +467,7 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "specify after value" in { val expected = Some("1980") val fieldQueries = Seq(FieldQuery("sourceResource.date.after", "1980")) - val params = minSearchParams.copy(fieldQueries=fieldQueries) + val params = minSearchParams.copy(fieldQueries = fieldQueries) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryRange = @@ -451,12 +479,13 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "specify before value" in { val expected = Some("1980") val fieldQueries = Seq(FieldQuery("sourceResource.date.before", "1980")) - val params = minSearchParams.copy(fieldQueries=fieldQueries) + val params = minSearchParams.copy(fieldQueries = fieldQueries) val query = getJsSearchQuery(params) val boolMust = readObjectArray(query, "query", "bool", "must") val queryRange = boolMust.flatMap(obj => readObject(obj, "range")).head - val traversed = readString(queryRange, "sourceResource.date.begin", "lte") + val traversed = + readString(queryRange, "sourceResource.date.begin", "lte") assert(traversed == expected) } } @@ -470,9 +499,9 @@ class QueryBuilderTest extends AnyWordSpec with Matchers fieldNames should contain only expected } - "set should for OR" in { + "set should for OR" in { val expected = "should" - val params = detailSearchParams.copy(op="OR") + val params = detailSearchParams.copy(op = "OR") val query = getJsSearchQuery(params) val parent = readObject(query, "query", "bool") val fieldNames = parent.get.fields.keys @@ -489,7 +518,11 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "include all facets" in { val expected = - Seq("provider.@id", "sourceResource.publisher", "sourceResource.subject.name") + Seq( + "provider.@id", + "sourceResource.publisher", + "sourceResource.subject.name" + ) val parent = readObject(detailQuery, "aggs") val fieldNames = parent.get.fields.keys fieldNames should contain allElementsOf expected @@ -498,14 +531,26 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "specify facet field" in { val expected = Some("sourceResource.subject.name.not_analyzed") val traversed = - readString(detailQuery, "aggs", "sourceResource.subject.name", "terms", "field") + readString( + detailQuery, + "aggs", + "sourceResource.subject.name", + "terms", + "field" + ) assert(traversed == expected) } "specify facet size" in { val expected = Some(100) val traversed = - readInt(detailQuery, "aggs", "sourceResource.subject.name", "terms", "size") + readInt( + detailQuery, + "aggs", + "sourceResource.subject.name", + "terms", + "size" + ) assert(traversed == expected) } } @@ -513,37 +558,62 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "geo agg query builder" should { "specify facet field" in { val expected = Some("sourceResource.spatial.coordinates") - val traversed = readString(geoFacetQuery, "aggs", - "sourceResource.spatial.coordinates", "geo_distance", "field") + val traversed = readString( + geoFacetQuery, + "aggs", + "sourceResource.spatial.coordinates", + "geo_distance", + "field" + ) assert(traversed == expected) } "specify origin coordinates" in { val expected = Some("42,-70") - val traversed = readString(geoFacetQuery, "aggs", - "sourceResource.spatial.coordinates", "geo_distance", "origin") + val traversed = readString( + geoFacetQuery, + "aggs", + "sourceResource.spatial.coordinates", + "geo_distance", + "origin" + ) assert(traversed == expected) } "specify unit" in { val expected = Some("mi") - val traversed = readString(geoFacetQuery, "aggs", - "sourceResource.spatial.coordinates", "geo_distance", "unit") + val traversed = readString( + geoFacetQuery, + "aggs", + "sourceResource.spatial.coordinates", + "geo_distance", + "unit" + ) assert(traversed == expected) } "specify range start" in { val expected = Some(0) - val range = readObjectArray(geoFacetQuery, "aggs", - "sourceResource.spatial.coordinates", "geo_distance", "ranges").head + val range = readObjectArray( + geoFacetQuery, + "aggs", + "sourceResource.spatial.coordinates", + "geo_distance", + "ranges" + ).head val traversed = readInt(range, "from") assert(traversed == expected) } "specify range end" in { val expected = Some(99) - val range = readObjectArray(geoFacetQuery, "aggs", - "sourceResource.spatial.coordinates", "geo_distance", "ranges").head + val range = readObjectArray( + geoFacetQuery, + "aggs", + "sourceResource.spatial.coordinates", + "geo_distance", + "ranges" + ).head val traversed = readInt(range, "to") assert(traversed == expected) } @@ -552,17 +622,29 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "date agg query builder" should { "specify facet field" in { val expected = Some("sourceResource.date.begin") - val traversed = readString(dateFacetQuery, "aggs", - "sourceResource.date.begin", "aggs", "sourceResource.date.begin", - "date_histogram", "field") + val traversed = readString( + dateFacetQuery, + "aggs", + "sourceResource.date.begin", + "aggs", + "sourceResource.date.begin", + "date_histogram", + "field" + ) assert(traversed == expected) } "specify default interval" in { val expected = Some("year") - val traversed = readString(dateFacetQuery, "aggs", - "sourceResource.date.begin", "aggs", "sourceResource.date.begin", - "date_histogram", "interval") + val traversed = readString( + dateFacetQuery, + "aggs", + "sourceResource.date.begin", + "aggs", + "sourceResource.date.begin", + "date_histogram", + "interval" + ) assert(traversed == expected) } @@ -571,9 +653,15 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val params = minSearchParams .copy(facets = Some(Seq("sourceResource.date.begin.year"))) val query = getJsSearchQuery(params) - val traversed = readString(query, "aggs", - "sourceResource.date.begin.year", "aggs", - "sourceResource.date.begin.year", "date_histogram", "interval") + val traversed = readString( + query, + "aggs", + "sourceResource.date.begin.year", + "aggs", + "sourceResource.date.begin.year", + "date_histogram", + "interval" + ) assert(traversed == expected) } @@ -582,17 +670,29 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val params = minSearchParams .copy(facets = Some(Seq("sourceResource.date.begin.month"))) val query = getJsSearchQuery(params) - val traversed = readString(query, "aggs", - "sourceResource.date.begin.month", "aggs", - "sourceResource.date.begin.month", "date_histogram", "interval") + val traversed = readString( + query, + "aggs", + "sourceResource.date.begin.month", + "aggs", + "sourceResource.date.begin.month", + "date_histogram", + "interval" + ) assert(traversed == expected) } "specify default format" in { val expected = Some("yyyy") - val traversed = readString(dateFacetQuery, "aggs", - "sourceResource.date.begin", "aggs", "sourceResource.date.begin", - "date_histogram", "format") + val traversed = readString( + dateFacetQuery, + "aggs", + "sourceResource.date.begin", + "aggs", + "sourceResource.date.begin", + "date_histogram", + "format" + ) assert(traversed == expected) } @@ -601,9 +701,15 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val params = minSearchParams .copy(facets = Some(Seq("sourceResource.date.end.year"))) val query = getJsSearchQuery(params) - val traversed = readString(query, "aggs", - "sourceResource.date.end.year", "aggs", - "sourceResource.date.end.year", "date_histogram", "format") + val traversed = readString( + query, + "aggs", + "sourceResource.date.end.year", + "aggs", + "sourceResource.date.end.year", + "date_histogram", + "format" + ) assert(traversed == expected) } @@ -612,33 +718,58 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val params = minSearchParams .copy(facets = Some(Seq("sourceResource.date.end.month"))) val query = getJsSearchQuery(params) - val traversed = readString(query, "aggs", - "sourceResource.date.end.month", "aggs", - "sourceResource.date.end.month", "date_histogram", "format") + val traversed = readString( + query, + "aggs", + "sourceResource.date.end.month", + "aggs", + "sourceResource.date.end.month", + "date_histogram", + "format" + ) assert(traversed == expected) } "specify min doc count" in { val expected = Some("1") - val traversed = readString(dateFacetQuery, "aggs", - "sourceResource.date.begin", "aggs", "sourceResource.date.begin", - "date_histogram", "min_doc_count") + val traversed = readString( + dateFacetQuery, + "aggs", + "sourceResource.date.begin", + "aggs", + "sourceResource.date.begin", + "date_histogram", + "min_doc_count" + ) assert(traversed == expected) } "specify order" in { val expected = Some("desc") - val traversed = readString(dateFacetQuery, "aggs", - "sourceResource.date.begin", "aggs", "sourceResource.date.begin", - "date_histogram", "order", "_key") + val traversed = readString( + dateFacetQuery, + "aggs", + "sourceResource.date.begin", + "aggs", + "sourceResource.date.begin", + "date_histogram", + "order", + "_key" + ) assert(traversed == expected) } "specify default filter gte" in { val expected = Some("now-2000y") - val traversed = readString(dateFacetQuery, "aggs", - "sourceResource.date.begin", "filter", "range", - "sourceResource.date.begin", "gte") + val traversed = readString( + dateFacetQuery, + "aggs", + "sourceResource.date.begin", + "filter", + "range", + "sourceResource.date.begin", + "gte" + ) assert(traversed == expected) } @@ -647,9 +778,15 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val params = minSearchParams .copy(facets = Some(Seq("sourceResource.date.end.year"))) val query = getJsSearchQuery(params) - val traversed = readString(query, "aggs", - "sourceResource.date.end.year", "filter", "range", - "sourceResource.date.end", "gte") + val traversed = readString( + query, + "aggs", + "sourceResource.date.end.year", + "filter", + "range", + "sourceResource.date.end", + "gte" + ) assert(traversed == expected) } @@ -658,17 +795,29 @@ class QueryBuilderTest extends AnyWordSpec with Matchers val params = minSearchParams .copy(facets = Some(Seq("sourceResource.date.end.month"))) val query = getJsSearchQuery(params) - val traversed = readString(query, "aggs", - "sourceResource.date.end.month", "filter", "range", - "sourceResource.date.end", "gte") + val traversed = readString( + query, + "aggs", + "sourceResource.date.end.month", + "filter", + "range", + "sourceResource.date.end", + "gte" + ) assert(traversed == expected) } "specify filter lte" in { val expected = Some("now") - val traversed = readString(dateFacetQuery, "aggs", - "sourceResource.date.begin", "filter", "range", - "sourceResource.date.begin", "lte") + val traversed = readString( + dateFacetQuery, + "aggs", + "sourceResource.date.begin", + "filter", + "range", + "sourceResource.date.begin", + "lte" + ) assert(traversed == expected) } } @@ -691,7 +840,9 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "default to sorting by score when fieldQuery is present" in { val expected = "_score" val params = minSearchParams - .copy(fieldQueries = Seq(FieldQuery("sourceResource.subject.name", "adventure"))) + .copy(fieldQueries = + Seq(FieldQuery("sourceResource.subject.name", "adventure")) + ) val query: JsObject = getJsSearchQuery(params) val traversed: String = readStringArray(query, "sort").head assert(traversed == expected) @@ -780,9 +931,8 @@ class QueryBuilderTest extends AnyWordSpec with Matchers "specify field and term" in { val expected = Some("http://dp.la/api/contributor/lc") val traversed = - readObjectArray(filterQuery, "query", "bool", "filter", "bool", "must") - .headOption - .flatMap(must => readString(must, "term", "provider.@id")) + readObjectArray(filterQuery, "query", "bool", "filter").headOption + .flatMap(f => readString(f, "term", "provider.@id")) assert(traversed == expected) } } From 31c58ea217d5f348cfc92c9505a44b9da9a8f986 Mon Sep 17 00:00:00 2001 From: scott Date: Tue, 9 Dec 2025 07:08:18 -0700 Subject: [PATCH 02/16] Update src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../dpla/api/v2/search/queryBuilders/QueryBuilder.scala | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala index d2edab66..0c355c6b 100644 --- a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala +++ b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala @@ -193,14 +193,13 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { .toJson .asInstanceOf[JsArray] - /** For general field query, use a keyword (i.e. "query_string") query. For - * exact field match, use "term" query. - * - term" searches for an exact term (with no additional text before or + /** For general field query, use a multi_match query. For exact field match, use "term" query. + * - "term" searches for an exact term (with no additional text before or * after). * - It is case-sensitive and does not analyze the search term. * - You can optionally set a parameter to ignore case, * - but this is NOT applied in the cultural heritage API. - * - It is only for fields that non-analyzed (i.e. indexed as "keyword") + * - It is only for fields that are non-analyzed (i.e. indexed as "keyword") */ private def singleFieldQuery( fieldQuery: FieldQuery, From 573389ab86386b3ec420c0bf9a4094189d57bb93 Mon Sep 17 00:00:00 2001 From: scott Date: Tue, 9 Dec 2025 07:08:35 -0700 Subject: [PATCH 03/16] Update src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../dpla/api/v2/search/paramValidators/ParamValidator.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index 02f8f625..94517b94 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -243,7 +243,7 @@ trait ParamValidator extends FieldDefinitions { if (fromValue + requestedPageSize > 1000) throw ValidationException( - s"Pagination too deep: from ($fromValue) + size ($pageSize) exceeds 1000" + s"Pagination too deep: from ($fromValue) + size ($requestedPageSize) exceeds 1000" ) val hasFacets = facets.nonEmpty From 490ef38e82de359a2490779226fe16f4dbc5a416 Mon Sep 17 00:00:00 2001 From: scott Date: Tue, 9 Dec 2025 07:44:59 -0700 Subject: [PATCH 04/16] Fix tests and misspellings --- .../search/queryBuilders/QueryBuilder.scala | 13 +-- .../v2/search/models/DPLAMAPFieldsTest.scala | 31 +++++++ .../queryBuilders/QueryBuilderTest.scala | 90 +++++++++++++++++++ 3 files changed, 128 insertions(+), 6 deletions(-) diff --git a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala index 0c355c6b..71e71c49 100644 --- a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala +++ b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala @@ -113,7 +113,6 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { // Reduced high-value fields for multi_match keyword queries private val keywordQueryFields = Seq( "sourceResource.title^2", - "sourceResource.subtitle^2", "sourceResource.subject.name^1", "sourceResource.description^0.75", "sourceResource.creator^1", @@ -193,13 +192,15 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { .toJson .asInstanceOf[JsArray] - /** For general field query, use a multi_match query. For exact field match, use "term" query. + /** For general field query, use a multi_match query. For exact field match, + * use "term" query. * - "term" searches for an exact term (with no additional text before or * after). * - It is case-sensitive and does not analyze the search term. * - You can optionally set a parameter to ignore case, * - but this is NOT applied in the cultural heritage API. - * - It is only for fields that are non-analyzed (i.e. indexed as "keyword") + * - It is only for fields that are non-analyzed (i.e. indexed as + * "keyword") */ private def singleFieldQuery( fieldQuery: FieldQuery, @@ -251,11 +252,11 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { ) ) // This should not happen - val values = stripLeadingAndTrainingQuotationMarks(fieldQuery.value) + val values = stripLeadingAndTrailingQuotationMarks(fieldQuery.value) .split("AND") .flatMap(_.split("OR")) .map(_.trim) - .map(stripLeadingAndTrainingQuotationMarks) + .map(stripLeadingAndTrailingQuotationMarks) values.map { value => JsObject( @@ -276,7 +277,7 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { /** Strip leading and trailing quotation marks only if there are no internal * quotation marks. */ - private def stripLeadingAndTrainingQuotationMarks(str: String): String = + private def stripLeadingAndTrailingQuotationMarks(str: String): String = if (str.matches("^\"[^\"]*\"$")) str.stripPrefix("\"").stripSuffix("\"") else diff --git a/src/test/scala/dpla/api/v2/search/models/DPLAMAPFieldsTest.scala b/src/test/scala/dpla/api/v2/search/models/DPLAMAPFieldsTest.scala index 9690c095..18186abe 100644 --- a/src/test/scala/dpla/api/v2/search/models/DPLAMAPFieldsTest.scala +++ b/src/test/scala/dpla/api/v2/search/models/DPLAMAPFieldsTest.scala @@ -58,5 +58,36 @@ class DPLAMAPFieldsTest extends AnyWordSpec with Matchers { val sortable = tester.fields.filter(_.sortable) exactMatch should contain allElementsOf sortable } + + "have searchable keywordQuery fields defined in ES mapping" in { + val keywordFields = Seq( + "sourceResource.title", + "sourceResource.subject.name", + "sourceResource.description", + "sourceResource.creator", + "sourceResource.collection.title", + "dataProvider.name", + "provider.name" + ) + + val mapped = keywordFields.flatMap(tester.getElasticSearchField) + mapped.size shouldBe keywordFields.size + } + + "use not_analyzed subfields for exact matches on keyword-backed fields" in { + val exactMatchFields = Seq( + "dataProvider.name", + "provider.name", + "sourceResource.collection.title", + "sourceResource.subject.name", + "sourceResource.title" + ) + + exactMatchFields.foreach { field => + val exact = tester.getElasticSearchExactMatchField(field) + exact should not be empty + exact.get should include("not_analyzed") + } + } } } diff --git a/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala b/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala index 2970f6ec..f6546f55 100644 --- a/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala +++ b/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala @@ -418,6 +418,96 @@ class QueryBuilderTest assert(traversed == expected) } + "not strip when internal quotation marks are present" in { + val original = "\"Mystery \"fiction\"\"" + val expected = Some(original) + val fieldQueries = + Seq(FieldQuery("sourceResource.subject.name", original)) + val params = minSearchParams.copy( + fieldQueries = fieldQueries, + exactFieldMatch = true + ) + val query = getJsSearchQuery(params) + val boolMust = readObjectArray(query, "query", "bool", "must") + val queryTerm = + boolMust.flatMap(obj => readObject(obj, "term")).head + val traversed = + readString(queryTerm, "sourceResource.subject.name.not_analyzed") + assert(traversed == expected) + } + + "not strip when only leading quotation mark is present" in { + val original = "\"Mystery fiction" + val expected = Some(original) + val fieldQueries = + Seq(FieldQuery("sourceResource.subject.name", original)) + val params = minSearchParams.copy( + fieldQueries = fieldQueries, + exactFieldMatch = true + ) + val query = getJsSearchQuery(params) + val boolMust = readObjectArray(query, "query", "bool", "must") + val queryTerm = + boolMust.flatMap(obj => readObject(obj, "term")).head + val traversed = + readString(queryTerm, "sourceResource.subject.name.not_analyzed") + assert(traversed == expected) + } + + "not strip when only trailing quotation mark is present" in { + val original = "Mystery fiction\"" + val expected = Some(original) + val fieldQueries = + Seq(FieldQuery("sourceResource.subject.name", original)) + val params = minSearchParams.copy( + fieldQueries = fieldQueries, + exactFieldMatch = true + ) + val query = getJsSearchQuery(params) + val boolMust = readObjectArray(query, "query", "bool", "must") + val queryTerm = + boolMust.flatMap(obj => readObject(obj, "term")).head + val traversed = + readString(queryTerm, "sourceResource.subject.name.not_analyzed") + assert(traversed == expected) + } + + "strip empty quoted string to empty value" in { + val original = "\"\"" + val expected = Some("") + val fieldQueries = + Seq(FieldQuery("sourceResource.subject.name", original)) + val params = minSearchParams.copy( + fieldQueries = fieldQueries, + exactFieldMatch = true + ) + val query = getJsSearchQuery(params) + val boolMust = readObjectArray(query, "query", "bool", "must") + val queryTerm = + boolMust.flatMap(obj => readObject(obj, "term")).head + val traversed = + readString(queryTerm, "sourceResource.subject.name.not_analyzed") + assert(traversed == expected) + } + + "leave unquoted string unchanged" in { + val original = "Mystery fiction" + val expected = Some(original) + val fieldQueries = + Seq(FieldQuery("sourceResource.subject.name", original)) + val params = minSearchParams.copy( + fieldQueries = fieldQueries, + exactFieldMatch = true + ) + val query = getJsSearchQuery(params) + val boolMust = readObjectArray(query, "query", "bool", "must") + val queryTerm = + boolMust.flatMap(obj => readObject(obj, "term")).head + val traversed = + readString(queryTerm, "sourceResource.subject.name.not_analyzed") + assert(traversed == expected) + } + "handle multiple terms joined by +AND+" in { val expected = Seq(Some("Legislators"), Some("City Council")) val fieldQueries = Seq( From ad58d6de32485e5feeec56a9f098762ee15ab80e Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 9 Dec 2025 08:51:19 -0700 Subject: [PATCH 05/16] Fix parameter validation to reject over-limit values instead of clamping (#94) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: moltude <1419766+moltude@users.noreply.github.com> --- .../dpla/api/v2/search/paramValidators/ParamValidator.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index 94517b94..5812d1d8 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -501,7 +501,7 @@ trait ParamValidator extends FieldDefinitions { Try(intString.toInt).toOption match { case Some(int) => if (int < min) throw ValidationException(rule) - else if (int > max) max + else if (int > max) throw ValidationException(rule) else int case None => // not an integer From a4c778a21998a2167be83be9cce75022fbfccfae Mon Sep 17 00:00:00 2001 From: scott Date: Tue, 9 Dec 2025 09:01:07 -0700 Subject: [PATCH 06/16] Fix broken test --- .../api/v2/search/paramValidators/ParamValidatorTest.scala | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala b/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala index b9666c20..9c1a5276 100644 --- a/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala +++ b/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala @@ -350,13 +350,11 @@ class ParamValidatorTest replyProbe.expectMessageType[InvalidSearchParams] } - "default to max if param is too large" in { + "reject param if too large" in { val given = "9999" - val expected = 200 val params = Map("facet_size" -> given) ebookParamValidator ! RawSearchParams(params, replyProbe.ref) - val msg = interProbe.expectMessageType[ValidSearchParams] - msg.params.facetSize shouldEqual expected + replyProbe.expectMessageType[InvalidSearchParams] } } From 3c4f02ee8a072d3d8c513a48d1a5e567ce56bc5e Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Mon, 13 Apr 2026 18:38:07 -0400 Subject: [PATCH 07/16] Fix facet guard bypass via empty filter values filter=type: (no value after colon) produced Some(Seq.empty) because getValid silently drops empty strings. hasFilter.isDefined was then true with no filters applied, allowing high-cardinality facet requests past the facet guard. Split ":" once into parts, and use .collect to trim+filter in one pass. Empty values now throw ValidationException before reaching the guard. Co-Authored-By: Claude Sonnet 4.6 --- .../v2/search/paramValidators/ParamValidator.scala | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index 5812d1d8..bebb3d0e 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -343,17 +343,17 @@ trait ParamValidator extends FieldDefinitions { rawParams: Map[String, String] ): Option[Seq[Filter]] = rawParams.get("filter").map { filter => - val fieldName = filter - .split(":", 2) - .headOption + val parts = filter.split(":", 2) + val fieldName = parts.headOption .getOrElse(throw ValidationException(s"$filter is not a valid filter")) - val values = filter - .split(":", 2) - .lastOption + val values = parts.lastOption .getOrElse(throw ValidationException(s"$filter is not a valid filter")) .split("AND") - .map(_.trim) + .collect { case s if s.trim.nonEmpty => s.trim } + + if (values.isEmpty) + throw ValidationException(s"$filter is not a valid filter") if (searchableDataFields.contains(fieldName)) { val validationMethod = getValidationMethod(fieldName) From 99732bc33865e5f1fb48c8f7b4ef8bfa28775e1d Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Mon, 13 Apr 2026 19:17:13 -0400 Subject: [PATCH 08/16] Address CodeRabbit: normalize empty facets/fields, fix error message bounds - facets and fields: add .filter(_.nonEmpty) so Some(Seq.empty) from validFields (which drops ignoredFields entries) normalizes to None, preventing hasFacets/fields.isDefined from being misleadingly true - validIntWithRange: make rule a lazy val so the error string is only allocated on failure paths, not on every successful validation - validIntWithRange: use $min instead of hardcoded 0 in error message Co-Authored-By: Claude Sonnet 4.6 --- .../dpla/api/v2/search/paramValidators/ParamValidator.scala | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index bebb3d0e..b61c3f11 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -199,6 +199,7 @@ trait ParamValidator extends FieldDefinitions { val facets = getValid(rawParams, "facets", validFields) + .filter(_.nonEmpty) val facetSize = getValid(rawParams, "facet_size", validIntWithRange) @@ -206,6 +207,7 @@ trait ParamValidator extends FieldDefinitions { val fields = getValid(rawParams, "fields", validFields) + .filter(_.nonEmpty) val filter = getValidFilter(rawParams) @@ -496,7 +498,7 @@ trait ParamValidator extends FieldDefinitions { case _ => (0, 2147483647) } - val rule = s"$param must be an integer between 0 and $max" + lazy val rule = s"$param must be an integer between $min and $max" Try(intString.toInt).toOption match { case Some(int) => From d7524b1cf15de7fec3eac7bb52561230c84de94a Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Mon, 13 Apr 2026 19:39:13 -0400 Subject: [PATCH 09/16] Fix test failure: use exists(_.nonEmpty) for facets guard, keep filter for fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - facets: revert .filter(_.nonEmpty); Some(Seq.empty) is the intended return when all requested facet fields are dropped (e.g. valid DPLA Map fields not facetable for items). Fix the guard instead: facets.nonEmpty checks the Option (always true for Some), but facets.exists(_.nonEmpty) correctly checks the inner seq. - fields: keep .filter(_.nonEmpty) — fieldRetrieval(Some(Nil)) sends "_source":[] to ES which suppresses all source fields; None returns ["*"] (all fields) which is the correct fallback. Co-Authored-By: Claude Sonnet 4.6 --- .../dpla/api/v2/search/paramValidators/ParamValidator.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index b61c3f11..358d3aa3 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -199,7 +199,6 @@ trait ParamValidator extends FieldDefinitions { val facets = getValid(rawParams, "facets", validFields) - .filter(_.nonEmpty) val facetSize = getValid(rawParams, "facet_size", validIntWithRange) @@ -248,7 +247,7 @@ trait ParamValidator extends FieldDefinitions { s"Pagination too deep: from ($fromValue) + size ($requestedPageSize) exceeds 1000" ) - val hasFacets = facets.nonEmpty + val hasFacets = facets.exists(_.nonEmpty) val hasQuery = q.isDefined || fieldQueries.nonEmpty val hasFilter = filter.isDefined From 9bc809196d58ed1acf1114f529f52f59655ee4bf Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Tue, 14 Apr 2026 13:27:54 -0400 Subject: [PATCH 10/16] =?UTF-8?q?fix:=20address=20CodeRabbit=20review=20?= =?UTF-8?q?=E2=80=94=20ConcurrencyLimiter,=20fetch=20limit,=20filter,=20op?= =?UTF-8?q?=20propagation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ConcurrencyLimiter: handle InterruptedException from tryAcquire (restore interrupt status, return Future.failed); narrow Throwable catch to NonFatal to avoid swallowing fatal JVM errors - ParamValidator: add maxFetchBatchSize=500 constant so fetch ID batch limit is decoupled from search page_size (which was reduced to 100 in this PR) - ParamValidator: reject filter values without a field:value separator — filter=provider.name now throws ValidationException instead of being silently parsed as Filter("provider.name", "provider.name") - QueryBuilder: propagate op parameter into multi_match operator in keywordQuery and singleFieldQuery — op="OR" was previously a no-op for plain keyword and basic field queries Co-Authored-By: Claude Sonnet 4.6 --- .../dpla/api/v2/search/ConcurrencyLimiter.scala | 13 +++++++++++-- .../v2/search/paramValidators/ParamValidator.scala | 11 +++++++---- .../api/v2/search/queryBuilders/QueryBuilder.scala | 13 +++++++------ 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/main/scala/dpla/api/v2/search/ConcurrencyLimiter.scala b/src/main/scala/dpla/api/v2/search/ConcurrencyLimiter.scala index 9b4c3f00..d3765475 100644 --- a/src/main/scala/dpla/api/v2/search/ConcurrencyLimiter.scala +++ b/src/main/scala/dpla/api/v2/search/ConcurrencyLimiter.scala @@ -2,6 +2,7 @@ package dpla.api.v2.search import java.util.concurrent.{Semaphore, TimeUnit} import scala.concurrent.{ExecutionContext, Future} +import scala.util.control.NonFatal /** Limits concurrent execution of Futures using a semaphore. * @@ -53,7 +54,15 @@ class ConcurrencyLimiter( * The wrapped Future, or a failed Future if permit couldn't be acquired */ def apply[T](f: => Future[T])(implicit ec: ExecutionContext): Future[T] = { - if (!semaphore.tryAcquire(timeoutSeconds, TimeUnit.SECONDS)) { + val acquired = try { + semaphore.tryAcquire(timeoutSeconds, TimeUnit.SECONDS) + } catch { + case e: InterruptedException => + Thread.currentThread().interrupt() + return Future.failed(e) + } + + if (!acquired) { Future.failed( ConcurrencyLimitExceeded( maxConcurrent = maxConcurrent, @@ -65,7 +74,7 @@ class ConcurrencyLimiter( val future = f future.andThen { case _ => semaphore.release() }(ec) } catch { - case e: Throwable => + case NonFatal(e) => semaphore.release() Future.failed(e) } diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index 358d3aa3..aad17928 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -130,6 +130,7 @@ trait ParamValidator extends FieldDefinitions { protected val defaultPageSize: Int = 10 protected val minPageSize: Int = 0 protected val maxPageSize: Int = 100 + protected val maxFetchBatchSize: Int = 500 protected val defaultSortOrder: String = "asc" // Abstract. @@ -164,9 +165,9 @@ trait ParamValidator extends FieldDefinitions { ) else { val ids = id.split(",") - if (ids.size > maxPageSize) + if (ids.size > maxFetchBatchSize) throw ValidationException( - s"The number of ids cannot exceed $maxPageSize" + s"The number of ids cannot exceed $maxFetchBatchSize" ) ids.map(getValidId) } @@ -345,8 +346,10 @@ trait ParamValidator extends FieldDefinitions { ): Option[Seq[Filter]] = rawParams.get("filter").map { filter => val parts = filter.split(":", 2) - val fieldName = parts.headOption - .getOrElse(throw ValidationException(s"$filter is not a valid filter")) + if (parts.length != 2 || parts.head.trim.isEmpty) + throw ValidationException(s"$filter is not a valid filter") + + val fieldName = parts.head.trim val values = parts.lastOption .getOrElse(throw ValidationException(s"$filter is not a valid filter")) diff --git a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala index 71e71c49..f820cdfa 100644 --- a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala +++ b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala @@ -133,10 +133,10 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { ) = { val keyword: Seq[JsObject] = - q.map(keywordQuery(_, keywordQueryFields)).toSeq + q.map(keywordQuery(_, keywordQueryFields, op)).toSeq val filterClause: Option[JsArray] = filter.map(filterQuery) val fieldQuery: Seq[JsObject] = - fieldQueries.flatMap(singleFieldQuery(_, exactFieldMatch)) + fieldQueries.flatMap(singleFieldQuery(_, exactFieldMatch, op)) val queryTerms: Seq[JsObject] = keyword ++ fieldQuery val boolTerm: String = if (op == "OR") "should" else "must" @@ -162,7 +162,7 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { /** A general keyword query on the given fields. Uses multi_match for * best_fields with lenient parsing. */ - private def keywordQuery(q: String, fields: Seq[String]): JsObject = { + private def keywordQuery(q: String, fields: Seq[String], op: String = "AND"): JsObject = { val targetFields = if (fields.nonEmpty) fields else keywordQueryFields JsObject( @@ -170,7 +170,7 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { "query" -> q.toJson, "fields" -> targetFields.toJson, "type" -> "best_fields".toJson, - "operator" -> "AND".toJson, + "operator" -> op.toJson, "lenient" -> true.toJson ) ) @@ -204,7 +204,8 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { */ private def singleFieldQuery( fieldQuery: FieldQuery, - exactFieldMatch: Boolean + exactFieldMatch: Boolean, + op: String = "AND" ): Seq[JsObject] = if (fieldQuery.fieldName.endsWith(".before")) { @@ -270,7 +271,7 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { // Basic field query val fields: Seq[String] = Seq(getElasticSearchField(fieldQuery.fieldName)).flatten - val obj: JsObject = keywordQuery(fieldQuery.value, fields) + val obj: JsObject = keywordQuery(fieldQuery.value, fields, op) Seq(obj) } From 5dc77df1989e3f8824095c97f5780547f20e65c8 Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Tue, 14 Apr 2026 19:57:25 -0400 Subject: [PATCH 11/16] fix: restore exact hit counts and clean up query builder - Revert track_total_hits from 10000 to true: the 10000 cap causes ES to return relation="gte" with value=10000 for large result sets, but the parsers in DPLAMAPJsonFormats and PssJsonFormats only read hits.total.value without checking relation, silently surfacing 10000 as an exact count - Replace asInstanceOf[JsArray] in filterQuery with JsArray(...toVector) to avoid the unsafe runtime cast - Differentiate sort_by_pin error messages: "sort_by_pin is only allowed when sort_by is the coordinates field" vs "The sort_by parameter is required." so callers know whether sort_by is missing or just wrong Co-Authored-By: Claude Sonnet 4.6 --- .../v2/search/paramValidators/ParamValidator.scala | 4 ++-- .../api/v2/search/queryBuilders/QueryBuilder.scala | 11 +++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index aad17928..4b4e07d3 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -421,11 +421,11 @@ trait ParamValidator extends FieldDefinitions { validCoordinates else throw ValidationException( - s"The sort_by parameter is required." + "sort_by_pin is only allowed when sort_by is the coordinates field" ) case None => throw ValidationException( - s"The sort_by parameter is required." + "The sort_by parameter is required." ) } } diff --git a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala index f820cdfa..79c6ec4a 100644 --- a/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala +++ b/src/main/scala/dpla/api/v2/search/queryBuilders/QueryBuilder.scala @@ -107,7 +107,7 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { "aggs" -> aggs(params.facets, params.facetSize), "sort" -> sort(params), "_source" -> fieldRetrieval(params.fields), - "track_total_hits" -> 10000.toJson + "track_total_hits" -> true.toJson ).toJson // Reduced high-value fields for multi_match keyword queries @@ -181,16 +181,15 @@ trait QueryBuilder extends FieldDefinitions with DefaultJsonProtocol { * documents. */ private def filterQuery(filters: Seq[Filter]): JsArray = - filters - .map(filter => { + JsArray( + filters.map { filter => JsObject( "term" -> JsObject( filter.fieldName -> filter.value.toJson ) ) - }) - .toJson - .asInstanceOf[JsArray] + }.toVector + ) /** For general field query, use a multi_match query. For exact field match, * use "term" query. From 3a2583db63a051a4d34ffe04024b5ab573f86c0d Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Tue, 14 Apr 2026 20:50:48 -0400 Subject: [PATCH 12/16] fix: update track_total_hits test to expect true instead of 10000 Co-Authored-By: Claude Sonnet 4.6 --- .../dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala b/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala index f6546f55..685f402a 100644 --- a/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala +++ b/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala @@ -271,8 +271,8 @@ class QueryBuilderTest } "specify track_total_hits" in { - val expected = Some(10000) - val traversed = readInt(minQuery, "track_total_hits") + val expected = Some(true) + val traversed = readBoolean(minQuery, "track_total_hits") assert(traversed == expected) } } From e98d96a6900350c6ceb3b960c170022b6179617c Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Tue, 14 Apr 2026 20:58:32 -0400 Subject: [PATCH 13/16] fix: remove dead pagination depth guard; add OR operator multi_match test - Remove the from+size>1000 guard in ParamValidator: with maxPage=10 and maxPageSize=100, the maximum reachable value is exactly 1000 and the strict >1000 check can never fire; it was a no-op safety net from when the limits were higher - Add test verifying that op="OR" propagates to multi_match.operator field, not just to the outer bool clause Co-Authored-By: Claude Sonnet 4.6 --- .../v2/search/paramValidators/ParamValidator.scala | 5 ----- .../v2/search/queryBuilders/QueryBuilderTest.scala | 11 +++++++++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index 4b4e07d3..e7d7c98f 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -243,11 +243,6 @@ trait ParamValidator extends FieldDefinitions { rawParams.get("page_size").flatMap(_.toIntOption).getOrElse(pageSize) val fromValue = (requestedPage - 1) * requestedPageSize - if (fromValue + requestedPageSize > 1000) - throw ValidationException( - s"Pagination too deep: from ($fromValue) + size ($requestedPageSize) exceeds 1000" - ) - val hasFacets = facets.exists(_.nonEmpty) val hasQuery = q.isDefined || fieldQueries.nonEmpty val hasFilter = filter.isDefined diff --git a/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala b/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala index 685f402a..b33710d6 100644 --- a/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala +++ b/src/test/scala/dpla/api/v2/search/queryBuilders/QueryBuilderTest.scala @@ -597,6 +597,17 @@ class QueryBuilderTest val fieldNames = parent.get.fields.keys fieldNames should contain only expected } + + "specify OR operator in multi_match" in { + val expected = Some("OR") + val params = detailSearchParams.copy(op = "OR") + val query = getJsSearchQuery(params) + val boolShould = readObjectArray(query, "query", "bool", "should") + val multiMatch = + boolShould.flatMap(obj => readObject(obj, "multi_match")).head + val traversed = readString(multiMatch, "operator") + assert(traversed == expected) + } } "agg query builder" should { From 214fce2b9d45d7ec2386511bce64fb943ceb05e0 Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Tue, 14 Apr 2026 21:57:13 -0400 Subject: [PATCH 14/16] fix: remove leftover unused pagination variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit requestedPage, requestedPageSize, and fromValue were left behind after removing the pagination depth guard — dead code with no references. Co-Authored-By: Claude Sonnet 4.6 --- .../dpla/api/v2/search/paramValidators/ParamValidator.scala | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index e7d7c98f..baf4f3a8 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -237,12 +237,6 @@ trait ParamValidator extends FieldDefinitions { getValid(rawParams, "sort_order", validSortOrder) .getOrElse(defaultSortOrder) - val requestedPage = - rawParams.get("page").flatMap(_.toIntOption).getOrElse(page) - val requestedPageSize = - rawParams.get("page_size").flatMap(_.toIntOption).getOrElse(pageSize) - val fromValue = (requestedPage - 1) * requestedPageSize - val hasFacets = facets.exists(_.nonEmpty) val hasQuery = q.isDefined || fieldQueries.nonEmpty val hasFilter = filter.isDefined From 9407a7130e083342ba3c6f5825751fe4f086f6c2 Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Wed, 15 Apr 2026 18:53:08 -0400 Subject: [PATCH 15/16] test: rename outdated pagination test descriptions to reflect rejection behavior Co-Authored-By: Claude Sonnet 4.6 --- .../api/v2/search/paramValidators/ParamValidatorTest.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala b/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala index 9c1a5276..26d7179c 100644 --- a/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala +++ b/src/test/scala/dpla/api/v2/search/paramValidators/ParamValidatorTest.scala @@ -576,7 +576,7 @@ class ParamValidatorTest replyProbe.expectMessageType[InvalidSearchParams] } - "default to max if param is too large" in { + "reject param if too large" in { val given = "600" val params = Map("page" -> given) ebookParamValidator ! RawSearchParams(params, replyProbe.ref) @@ -614,7 +614,7 @@ class ParamValidatorTest replyProbe.expectMessageType[InvalidSearchParams] } - "default to max if param is too large" in { + "reject param if too large" in { val given = "999999" val params = Map("page_size" -> given) ebookParamValidator ! RawSearchParams(params, replyProbe.ref) From 815dc555f0dc7ad4b803d704022130f3bf32b61c Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Thu, 16 Apr 2026 00:07:00 -0400 Subject: [PATCH 16/16] fix: scope facet-only guard to ebook validator, preserving item endpoint compatibility The guard rejecting facets without a query/filter was applied to all validators, breaking three integration tests that exercise facet-only requests on /v2/items (coordinates, dataProvider with size limit, basic dataProvider). Item facet-only requests are a documented and supported part of the public API contract. Introduces requiresQueryForFacets (default false) as an overridable protected val in ParamValidator. EbookParamValidator overrides it to true, keeping the restriction exactly where the unit test already specified it. Co-Authored-By: Claude Sonnet 4.6 --- .../dpla/api/v2/search/paramValidators/ParamValidator.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala index baf4f3a8..8f991681 100644 --- a/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala +++ b/src/main/scala/dpla/api/v2/search/paramValidators/ParamValidator.scala @@ -120,6 +120,7 @@ trait ParamValidator extends FieldDefinitions { protected def preProcess(value: Map[String, String]): Map[String, String] protected val defaultExactFieldMatch: Boolean = false + protected val requiresQueryForFacets: Boolean = false protected val defaultFacetSize: Int = 50 protected val minFacetSize: Int = 0 protected val maxFacetSize: Int = 200 @@ -241,7 +242,7 @@ trait ParamValidator extends FieldDefinitions { val hasQuery = q.isDefined || fieldQueries.nonEmpty val hasFilter = filter.isDefined - if (hasFacets && !hasQuery && !hasFilter) + if (requiresQueryForFacets && hasFacets && !hasQuery && !hasFilter) throw ValidationException( "Facet requests require at least one query term (q) or filter" )