Skip to content

Commit

Permalink
A bit better error handling
Browse files Browse the repository at this point in the history
  • Loading branch information
gunnarvelle committed Mar 5, 2025
1 parent 665fc27 commit f91c369
Show file tree
Hide file tree
Showing 12 changed files with 41 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ trait TapirErrorHandling extends StrictLogging {
"The search-context specified was not expected. Please create one by searching from page 1."
val INDEX_MISSING_DESCRIPTION: String =
s"Ooops. Our search index is not available at the moment, but we are trying to recreate it. Please try again in a few minutes. Feel free to contact ${props.ContactEmail} if the error persists."
val INDEX_CONFLICT_DESCRIPTION: String = "This document is already indexed in a newer version. Move along."

val ILLEGAL_STATUS_TRANSITION: String = "Illegal status transition"

Expand All @@ -119,6 +120,7 @@ trait TapirErrorHandling extends StrictLogging {
def invalidSearchContext: ErrorBody =
ErrorBody(INVALID_SEARCH_CONTEXT, INVALID_SEARCH_CONTEXT_DESCRIPTION, clock.now(), 400)
def methodNotAllowed: ErrorBody = ErrorBody(METHOD_NOT_ALLOWED, METHOD_NOT_ALLOWED_DESCRIPTION, clock.now(), 405)
def indexConflict: ErrorBody = ErrorBody(CONFLICT, INDEX_CONFLICT_DESCRIPTION, clock.now(), 409)
def validationError(ve: ValidationException): ValidationErrorBody =
ValidationErrorBody(VALIDATION, VALIDATION_DESCRIPTION, clock.now(), messages = ve.errors.some, 400)
def errorBody(code: String, description: String, statusCode: Int): ErrorBody =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ trait InternController {
oneOfVariant(jsonBody[Concept])
)
)
.errorOut(errorOutputsFor(400))
.errorOut(errorOutputsFor(400, 409))
.serverLogicPure { case (indexType, body) =>
indexType match {
case articleIndexService.documentType => indexRequestWithService(articleIndexService, body)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@ import cats.implicits.catsSyntaxOptionId
import no.ndla.common.Clock
import no.ndla.common.errors.{AccessDeniedException, ValidationException}
import no.ndla.network.tapir.{AllErrors, TapirErrorHandling, ValidationErrorBody}
import no.ndla.search.model.domain.DocumentConflictException
import no.ndla.search.{IndexNotFoundException, NdlaSearchException}
import no.ndla.searchapi.Props

trait ErrorHandling extends TapirErrorHandling {
this: Props with Clock =>
this: Props & Clock =>

import ErrorHelpers._
import SearchErrorHelpers._
import ErrorHelpers.*
import SearchErrorHelpers.*

override def handleErrors: PartialFunction[Throwable, AllErrors] = {
case rw: ResultWindowTooLargeException => errorBody(WINDOW_TOO_LARGE, rw.getMessage, 422)
Expand All @@ -28,7 +29,8 @@ trait ErrorHandling extends TapirErrorHandling {
case te: TaxonomyException => errorBody(TAXONOMY_FAILURE, te.getMessage, 500)
case v: ValidationException =>
ValidationErrorBody(VALIDATION, VALIDATION_DESCRIPTION, clock.now(), messages = v.errors.some, 400)
case ade: AccessDeniedException => forbiddenMsg(ade.getMessage)
case ade: AccessDeniedException => forbiddenMsg(ade.getMessage)
case _: DocumentConflictException => indexConflict
case NdlaSearchException(_, Some(rf), _, _)
if rf.error.rootCause
.exists(x => x.`type` == "search_context_missing_exception" || x.reason == "Cannot parse scroll id") =>
Expand Down
12 changes: 6 additions & 6 deletions search/src/main/scala/no/ndla/search/BaseIndexService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ trait BaseIndexService {
def createIndexWithGeneratedName: Try[String] =
createIndexWithName(getNewIndexName())

def reindexWithShards(numShards: Int): Try[_] = {
def reindexWithShards(numShards: Int): Try[?] = {
logger.info(s"Internal reindexing $searchIndex with $numShards shards...")
val maybeAliasTarget = getAliasTarget.?
val currentIndex = maybeAliasTarget match {
Expand All @@ -164,7 +164,7 @@ trait BaseIndexService {
} yield ()
}

def createIndexIfNotExists(): Try[_] = getAliasTarget.flatMap {
def createIndexIfNotExists(): Try[?] = getAliasTarget.flatMap {
case Some(index) => Success(index)
case None => createIndexAndAlias(indexShards.some)
}
Expand All @@ -187,13 +187,13 @@ trait BaseIndexService {
}
}

def updateReplicaNumber(overrideReplicaNumber: Int): Try[_] = getAliasTarget.flatMap {
def updateReplicaNumber(overrideReplicaNumber: Int): Try[?] = getAliasTarget.flatMap {
case None => Success(())
case Some(indexName) =>
updateReplicaNumber(indexName, overrideReplicaNumber.some)
}

private def updateReplicaNumber(indexName: String, overrideReplicaNumber: Option[Int]): Try[_] = {
private def updateReplicaNumber(indexName: String, overrideReplicaNumber: Option[Int]): Try[?] = {
if (props.Environment == "local") {
logger.info("Skipping replica change in local environment, since the cluster only has one node.")
Success(())
Expand Down Expand Up @@ -292,14 +292,14 @@ trait BaseIndexService {
}
}

def deleteIndexAndAlias(): Try[_] = {
def deleteIndexAndAlias(): Try[?] = {
for {
indexToDelete <- deleteAlias()
_ <- deleteIndexWithName(indexToDelete)
} yield ()
}

def deleteIndexWithName(optIndexName: Option[String]): Try[_] = {
def deleteIndexWithName(optIndexName: Option[String]): Try[?] = {
optIndexName match {
case None => Success(optIndexName)
case Some(indexName) =>
Expand Down
9 changes: 6 additions & 3 deletions search/src/main/scala/no/ndla/search/Elastic4sClient.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@

package no.ndla.search

import com.sksamuel.elastic4s._
import com.sksamuel.elastic4s.*
import com.sksamuel.elastic4s.http.JavaClient
import io.lemonlabs.uri.typesafe.dsl._
import io.lemonlabs.uri.typesafe.dsl.*
import no.ndla.common.configuration.HasBaseProps
import no.ndla.search.model.domain.DocumentConflictException
import org.apache.http.client.config.RequestConfig
import org.elasticsearch.client.RestClientBuilder.RequestConfigCallback

Expand All @@ -29,7 +30,7 @@ trait Elastic4sClient {
private def recreateClient(): Unit = client = Elastic4sClientFactory.getNonSigningClient(searchServer)
private val elasticTimeout = 10.minutes

def showQuery[T](t: T)(implicit handler: Handler[T, _]): String = client.show(t)
def showQuery[T](t: T)(implicit handler: Handler[T, ?]): String = client.show(t)

private val clientExecutionContext: ExecutionContextExecutor =
ExecutionContext.fromExecutor(Executors.newWorkStealingPool(props.MAX_SEARCH_THREADS))
Expand All @@ -38,6 +39,8 @@ trait Elastic4sClient {
request: T
)(implicit handler: Handler[T, U], mf: Manifest[U], ec: ExecutionContext): Future[Try[RequestSuccess[U]]] = {
val result = client.execute(request).map {
case failure: RequestFailure if failure.status == 409 =>
Failure(DocumentConflictException(failure.error.reason))
case failure: RequestFailure => Failure(NdlaSearchException(request, failure))
case result: RequestSuccess[U] => Success(result)
}
Expand Down
2 changes: 1 addition & 1 deletion search/src/main/scala/no/ndla/search/FakeAgg.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

package no.ndla.search

import com.sksamuel.elastic4s.ElasticDsl._
import com.sksamuel.elastic4s.ElasticDsl.*
import com.sksamuel.elastic4s.requests.searches.aggs.Aggregation

/** [[FakeAgg]] and inheriting classes are an abstraction to easier work with Elastic4s' Aggregations They are usually
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,11 @@ case class NdlaSearchException[T](
object NdlaSearchException {
def apply[T](request: T, rf: RequestFailure): NdlaSearchException[T] = {
val msg =
if (rf.status != 409)
s"""Got error from elasticsearch:
s"""Got error from elasticsearch:
| Status: ${rf.status}
| Error: ${rf.error}
| Caused by request: $request
|""".stripMargin
else "Document already exists"

new NdlaSearchException(msg, Some(rf))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ object SearchableLanguageValues {
case c: FailedCursor if !c.incorrectFocus => Right(SearchableLanguageValues(Seq.empty))
case c =>
c.as[Map[String, String]].map { map =>
SearchableLanguageValues.from(map.toSeq: _*)
SearchableLanguageValues.from(map.toSeq*)
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* Part of NDLA search
* Copyright (C) 2025 NDLA
*
* See LICENSE
*
*/

package no.ndla.search.model.domain

case class DocumentConflictException(message: String) extends RuntimeException(message)
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
package no.ndla.search

import com.sksamuel.elastic4s.ElasticApi.{nestedAggregation, termsAgg}
import com.sksamuel.elastic4s.ElasticDsl._
import com.sksamuel.elastic4s.ElasticDsl.*
import com.sksamuel.elastic4s.fields.ObjectField
import com.sksamuel.elastic4s.requests.mappings.MappingDefinition
import no.ndla.scalatestsuite.UnitTestSuite
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* See LICENSE
*
*/

package no.ndla.search

import no.ndla.scalatestsuite.UnitTestSuite
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import no.ndla.scalatestsuite.UnitTestSuite
class SearchableLanguageValuesTest extends UnitTestSuite {

test("That SearchableLanguageValues serialization and deserialization results in same object") {
import io.circe.syntax._
import io.circe.syntax.*
val searchableLanguageValues = SearchableLanguageValues(
Seq(
LanguageValue("nb", "Norsk"),
Expand All @@ -40,7 +40,7 @@ class SearchableLanguageValuesTest extends UnitTestSuite {
}

test("That SearchableLanguageValues serialization results in object with language as key") {
import io.circe.syntax._
import io.circe.syntax.*
val searchableLanguageValues = SearchableLanguageValues(
Seq(
LanguageValue("nb", "Norsk"),
Expand All @@ -55,7 +55,7 @@ class SearchableLanguageValuesTest extends UnitTestSuite {
}

test("That SearchableLanguageList serialization and deserialization results in same object") {
import io.circe.syntax._
import io.circe.syntax.*
val searchableLanguageList = SearchableLanguageList(
Seq(
LanguageValue("nb", List("Norsk", "Norskere", "Norskest")),
Expand All @@ -70,7 +70,7 @@ class SearchableLanguageValuesTest extends UnitTestSuite {
}

test("That SearchableLanguageList serialization results in object with language as key") {
import io.circe.syntax._
import io.circe.syntax.*
val searchableLanguageList = SearchableLanguageList(
Seq(
LanguageValue("nb", List("Norsk", "Norskere", "Norskest")),
Expand Down

0 comments on commit f91c369

Please sign in to comment.