From f2f1a5ad2db71d792b2a9bbe4389563163b610ab Mon Sep 17 00:00:00 2001 From: Dominic Byrd-McDevitt Date: Mon, 20 Apr 2026 16:42:26 -0500 Subject: [PATCH] fix: return 400 Bad Request for Elasticsearch query parse errors instead of 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Malformed Lucene syntax (e.g. MARC subject headings with trailing `)-`) causes ES to return HTTP 400. Previously any non-200 ES response in the search and random paths was blindly mapped to SearchFailure → HTTP 500. Added SearchQueryParseFailure response type, wired it through the ES client, registry, and added end-to-end test coverage. Co-Authored-By: Claude Sonnet 4.6 --- .../v2/registry/SearchRegistryBehavior.scala | 14 +++++++++ .../api/v2/search/ElasticSearchClient.scala | 14 ++++++--- .../dpla/api/v2/search/SearchProtocol.scala | 1 + .../api/v2/endToEnd/InvalidParamsTest.scala | 24 ++++++++++++++- .../search/MockEsClientQueryParseError.scala | 29 +++++++++++++++++++ 5 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 src/test/scala/dpla/api/v2/search/MockEsClientQueryParseError.scala diff --git a/src/main/scala/dpla/api/v2/registry/SearchRegistryBehavior.scala b/src/main/scala/dpla/api/v2/registry/SearchRegistryBehavior.scala index 52282e6..a2dc7fa 100644 --- a/src/main/scala/dpla/api/v2/registry/SearchRegistryBehavior.scala +++ b/src/main/scala/dpla/api/v2/registry/SearchRegistryBehavior.scala @@ -40,6 +40,8 @@ final case class RegisterRandom( trait SearchRegistryBehavior { + private val QueryParseErrorMessage = "The q parameter contains invalid search syntax." + def spawnSearchActor(context: ActorContext[SearchRegistryCommand]): ActorRef[SearchCommand] @@ -183,6 +185,10 @@ trait SearchRegistryBehavior { searchResponse = Some(NotFoundFailure) possibleSessionResolution + case SearchQueryParseFailure => + searchResponse = Some(ValidationFailure(QueryParseErrorMessage)) + possibleSessionResolution + case SearchFailure => searchResponse = Some(InternalFailure) possibleSessionResolution @@ -298,6 +304,10 @@ trait SearchRegistryBehavior { fetchResponse = Some(NotFoundFailure) possibleSessionResolution + case SearchQueryParseFailure => + fetchResponse = Some(InternalFailure) + possibleSessionResolution + case SearchFailure => fetchResponse = Some(InternalFailure) possibleSessionResolution @@ -383,6 +393,10 @@ trait SearchRegistryBehavior { randomResponse = Some(ValidationFailure(message)) possibleSessionResolution + case SearchQueryParseFailure => + randomResponse = Some(ValidationFailure(QueryParseErrorMessage)) + possibleSessionResolution + case SearchFailure => randomResponse = Some(InternalFailure) possibleSessionResolution diff --git a/src/main/scala/dpla/api/v2/search/ElasticSearchClient.scala b/src/main/scala/dpla/api/v2/search/ElasticSearchClient.scala index 17b22cc..07f9aca 100644 --- a/src/main/scala/dpla/api/v2/search/ElasticSearchClient.scala +++ b/src/main/scala/dpla/api/v2/search/ElasticSearchClient.scala @@ -228,8 +228,11 @@ object ElasticSearchClient { nextPhase ! SearchQueryResponse(params, body, replyTo) Behaviors.stopped - case ElasticSearchHttpError(_) => - replyTo ! SearchFailure + case ElasticSearchHttpError(statusCode) => + if (statusCode.intValue == 400) + replyTo ! SearchQueryParseFailure + else + replyTo ! SearchFailure Behaviors.stopped case ElasticSearchResponseFailure => @@ -385,8 +388,11 @@ object ElasticSearchClient { nextPhase ! RandomQueryResponse(params, body, replyTo) Behaviors.stopped - case ElasticSearchHttpError(_) => - replyTo ! SearchFailure + case ElasticSearchHttpError(statusCode) => + if (statusCode.intValue == 400) + replyTo ! SearchQueryParseFailure + else + replyTo ! SearchFailure Behaviors.stopped case ElasticSearchResponseFailure => diff --git a/src/main/scala/dpla/api/v2/search/SearchProtocol.scala b/src/main/scala/dpla/api/v2/search/SearchProtocol.scala index 6490460..0cad4fd 100644 --- a/src/main/scala/dpla/api/v2/search/SearchProtocol.scala +++ b/src/main/scala/dpla/api/v2/search/SearchProtocol.scala @@ -52,6 +52,7 @@ object SearchProtocol { final case object FetchNotFound extends SearchResponse final case object SearchNotFound extends SearchResponse final case object SearchFailure extends SearchResponse + final case object SearchQueryParseFailure extends SearchResponse /** * Internal command protocol. diff --git a/src/test/scala/dpla/api/v2/endToEnd/InvalidParamsTest.scala b/src/test/scala/dpla/api/v2/endToEnd/InvalidParamsTest.scala index 65f2e87..857a4a9 100644 --- a/src/test/scala/dpla/api/v2/endToEnd/InvalidParamsTest.scala +++ b/src/test/scala/dpla/api/v2/endToEnd/InvalidParamsTest.scala @@ -8,7 +8,8 @@ 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.{MockItemRegistry, MockSmrRegistry, SmrRegistryCommand} +import dpla.api.v2.search.{MockEsClientQueryParseError, MockItemSearch} import dpla.api.v2.smr.MockSmrRequestHandler import dpla.api.v2.smr.SmrProtocol.SmrCommand import org.scalatest.matchers.should.Matchers @@ -35,6 +36,17 @@ class InvalidParamsTest extends AnyWordSpec with Matchers new Routes(itemRegistry, pssRegistry, apiKeyRegistry, smrRegistryS3Success).applicationRoutes + val esQueryParseErrorClient = + testKit.spawn(MockEsClientQueryParseError()) + val itemSearchWithParseError = + MockItemSearch(testKit, Some(esQueryParseErrorClient)) + val itemRegistryWithParseError = + MockItemRegistry(testKit, authenticator, itemAnalyticsClient, + Some(itemSearchWithParseError)) + val routesWithParseError: Route = + new Routes(itemRegistryWithParseError, pssRegistry, apiKeyRegistry, + smrRegistryS3Success).applicationRoutes + "malformed api_key" should { "return Forbidden for a key that is too short" in { val request = Get("/v2/items?api_key=tooshort") @@ -93,4 +105,14 @@ class InvalidParamsTest extends AnyWordSpec with Matchers } } } + + "Elasticsearch query parse error" should { + "return BadRequest for /v2/items" in { + val request = Get(s"/v2/items?api_key=$fakeApiKey&q=invalid%29-") + request ~> Route.seal(routesWithParseError) ~> check { + status shouldEqual StatusCodes.BadRequest + contentType should === (ContentTypes.`application/json`) + } + } + } } diff --git a/src/test/scala/dpla/api/v2/search/MockEsClientQueryParseError.scala b/src/test/scala/dpla/api/v2/search/MockEsClientQueryParseError.scala new file mode 100644 index 0000000..abb8918 --- /dev/null +++ b/src/test/scala/dpla/api/v2/search/MockEsClientQueryParseError.scala @@ -0,0 +1,29 @@ +package dpla.api.v2.search + +import akka.actor.typed.Behavior +import akka.actor.typed.scaladsl.Behaviors +import dpla.api.v2.search.SearchProtocol.{ + IntermediateSearchResult, + RandomQuery, + SearchQuery, + SearchQueryParseFailure +} + +object MockEsClientQueryParseError { + + def apply(): Behavior[IntermediateSearchResult] = { + Behaviors.receiveMessage[IntermediateSearchResult] { + + case SearchQuery(_, _, replyTo) => + replyTo ! SearchQueryParseFailure + Behaviors.same + + case RandomQuery(_, _, replyTo) => + replyTo ! SearchQueryParseFailure + Behaviors.same + + case _ => + Behaviors.unhandled + } + } +}