Skip to content

Commit

Permalink
CORE-247: try to fix 'Non-standard status codes cannot be created' er…
Browse files Browse the repository at this point in the history
…ror (#1528)
  • Loading branch information
davidangb authored Jan 6, 2025
1 parent caa40c9 commit db3ffdd
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import org.broadinstitute.dsde.firecloud.model.{ModelSchema, _}
import org.broadinstitute.dsde.firecloud.service.PerRequest.{PerRequestMessage, RequestComplete}
import org.broadinstitute.dsde.firecloud.service.TsvTypes.TsvType
import org.broadinstitute.dsde.firecloud.service.{TSVFileSupport, TsvTypes}
import org.broadinstitute.dsde.firecloud.utils.TSVLoadFile
import org.broadinstitute.dsde.firecloud.utils.{StatusCodeUtils, TSVLoadFile}
import org.broadinstitute.dsde.rawls.model._
import org.broadinstitute.dsde.workbench.model.google.{GcsBucketName, GcsObjectName}
import org.databiosphere.workspacedata.client.ApiException
Expand Down Expand Up @@ -59,6 +59,7 @@ class EntityService(rawlsDAO: RawlsDAO,
modelSchema: ModelSchema
)(implicit val executionContext: ExecutionContext)
extends TSVFileSupport
with StatusCodeUtils
with LazyLogging {

val format = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZZ")
Expand Down Expand Up @@ -379,7 +380,7 @@ class EntityService(rawlsDAO: RawlsDAO,
}
// if human-readable message extraction fails, just default to the ApiException message
val errMsg = Try(extractMessage(apiEx.getResponseBody)).toOption.getOrElse(apiEx.getMessage)
new FireCloudExceptionWithErrorReport(ErrorReport(apiEx.getCode, errMsg))
new FireCloudExceptionWithErrorReport(ErrorReport(statusCodeFrom(apiEx.getCode), errMsg))
}

def getEntitiesWithType(workspaceNamespace: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import org.broadinstitute.dsde.firecloud.model.Project.ProjectRoles.ProjectRole
import org.broadinstitute.dsde.firecloud.model.Project._
import org.broadinstitute.dsde.firecloud.model.SamResource.{AccessPolicyName, ResourceId, UserPolicy}
import org.broadinstitute.dsde.firecloud.model.ShareLog.{Share, ShareType}
import org.broadinstitute.dsde.firecloud.utils.StatusCodeUtils
import org.broadinstitute.dsde.rawls.model.UserModelJsonSupport._
import org.broadinstitute.dsde.rawls.model.WorkspaceACLJsonSupport.WorkspaceAccessLevelFormat
import org.broadinstitute.dsde.rawls.model._
Expand All @@ -25,7 +26,7 @@ import spray.json._
import scala.util.{Failure, Success, Try}

//noinspection TypeAnnotation,RedundantNewCaseClass
object ModelJsonProtocol extends WorkspaceJsonSupport with SprayJsonSupport {
object ModelJsonProtocol extends WorkspaceJsonSupport with SprayJsonSupport with StatusCodeUtils {
import spray.json.DefaultJsonProtocol._

def optionalEntryIntReader(fieldName: String, data: Map[String, JsValue]): Option[Int] =
Expand Down Expand Up @@ -55,7 +56,7 @@ object ModelJsonProtocol extends WorkspaceJsonSupport with SprayJsonSupport {
override def write(code: StatusCode): JsValue = JsNumber(code.intValue)

override def read(json: JsValue): StatusCode = json match {
case JsNumber(n) => n.intValue
case JsNumber(n) => statusCodeFrom(n.intValue)
case _ => throw DeserializationException("unexpected json type")
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import org.broadinstitute.dsde.workbench.util.FutureSupport.toFutureTry
import scala.concurrent.{ExecutionContext, Future, Promise}
import scala.util.{Failure, Success}

trait EnabledUserDirectives extends LazyLogging with SprayJsonSupport {
trait EnabledUserDirectives extends LazyLogging with SprayJsonSupport with StatusCodeUtils {

// Hardcode an ErrorReportSource to allow differentiating between enabled-user errors and other errors.
implicit val errorReportSource: ErrorReportSource = ErrorReportSource("Orchestration-enabled-check")
Expand Down Expand Up @@ -58,7 +58,7 @@ trait EnabledUserDirectives extends LazyLogging with SprayJsonSupport {
s"ApiException exception checking enabled status for user ${userInfo.userEmail}: (${apiex.getMessage}) while calling $uri",
apiex
)
val code = StatusCode.int2StatusCode(apiex.getCode)
val code = statusCodeFrom(apiex.getCode, Option(StatusCodes.InternalServerError))
if (code == StatusCodes.NotFound) {
throwErrorReport(StatusCodes.Unauthorized, "User is not registered.")
} else {
Expand Down Expand Up @@ -99,7 +99,7 @@ trait EnabledUserDirectives extends LazyLogging with SprayJsonSupport {
case Failure(_) => response
}
throw new FireCloudExceptionWithErrorReport(
ErrorReport(StatusCode.int2StatusCode(statusCode),
ErrorReport(statusCodeFrom(statusCode, Option(StatusCodes.InternalServerError)),
s"Sam call to $functionName failed with error '$stringErrMsg'"
)
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package org.broadinstitute.dsde.firecloud.utils

import akka.http.scaladsl.model.{StatusCode, StatusCodes}

import scala.util.Try

trait StatusCodeUtils {

/**
* Safely translates an integer to a StatusCode. This method avoids the RuntimeException
* thrown by StatusCode.int2StatusCode when supplied with an unknown code and returns
* a default status code instead.
*
* @param intCode the integer value to translate
* @param default the code to return if the integer value is unknown;
* defaults to Internal Server Error
* @return the final status code
*/
def statusCodeFrom(intCode: Int, default: Option[StatusCode] = None): StatusCode =
Try(StatusCode.int2StatusCode(intCode)).getOrElse(
default.getOrElse(
StatusCodes.custom(intCode,
reason = s"unknown status $intCode",
defaultMessage = s"unknown status $intCode",
isSuccess = false,
allowsEntity = true
)
)
)

}
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,10 @@ trait UserApiService
}
case x =>
// if we get any other error from Sam, pass that error on
respondWithErrorReport(x.intValue, "Unexpected response validating registration: " + x.toString, requestContext)
respondWithErrorReport(statusCodeFrom(x.intValue),
"Unexpected response validating registration: " + x.toString,
requestContext
)
}

private def handleOkResponse(regInfo: RegistrationInfoV2,
Expand Down Expand Up @@ -232,7 +235,7 @@ trait UserApiService
}
}
case x =>
respondWithErrorReport(x.intValue,
respondWithErrorReport(statusCodeFrom(x.intValue),
"Unexpected response validating registration: " + x.toString,
requestContext
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package org.broadinstitute.dsde.firecloud.utils

import akka.http.scaladsl.model.StatusCode
import akka.http.scaladsl.model.StatusCodes._
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers.convertToAnyShouldWrapper

class StatusCodeUtilsSpec extends AnyFlatSpec with StatusCodeUtils {

behavior of "statusCodeFrom"

val expectedCases: Map[Int, StatusCode] = Map(
200 -> OK,
404 -> NotFound,
503 -> ServiceUnavailable
)

expectedCases.foreach { case (intCode, statusCode) =>
it should s"handle known code $intCode" in {
statusCodeFrom(intCode) shouldBe statusCode
}
}

val unknownCodes: List[Int] = List(-1, 0, 42, 222, 555)

unknownCodes.foreach { intCode =>
it should s"create a custom status code $intCode for unknown values" in {
val actual = statusCodeFrom(intCode)
actual.intValue() shouldBe intCode
actual.isSuccess() shouldBe false
actual.defaultMessage() shouldBe s"unknown status $intCode"
}
}

unknownCodes.foreach { intCode =>
it should s"default unknown code $intCode to the caller-supplied default" in {
statusCodeFrom(intCode, Option(ImATeapot)) shouldBe ImATeapot
}
}

}

0 comments on commit db3ffdd

Please sign in to comment.