Skip to content

Commit

Permalink
Small bug fix to configuration validation
Browse files Browse the repository at this point in the history
Rawls changed the configuration payload field "methodStoreMethod" to "methodRepoMethod"
We check for the existence of this field during validation, and we were checking by the old name.

Fixed the field name in the configuration validation
Added better failure descriptions to our require statements
Added a validation exception so that we can avoid catching IllegalArgumentException at perRequest layer
Changed perRequest to respond with BadRequest on validation errors, not IllegalArgumentException
Changed methodStoreMethod -> methodRepoMethod in swagger, static test data
  • Loading branch information
bradtaylor committed Sep 14, 2015
1 parent 0d245f1 commit 1ac96d6
Show file tree
Hide file tree
Showing 7 changed files with 36 additions and 16 deletions.
4 changes: 2 additions & 2 deletions src/main/resources/swagger/agora.json
Expand Up @@ -1507,7 +1507,7 @@
"payload": {
"type": "string",
"description": "Payload of method -- must be in WDL format",
"default": "{ \"methodStoreMethod\": { \"methodNamespace\": \"YOUR_NAMESPACE\", \"methodName\": \"BWA\", \"methodVersion\": 1}}\n"
"default": "{ \"methodRepoMethod\": { \"methodNamespace\": \"YOUR_NAMESPACE\", \"methodName\": \"BWA\", \"methodVersion\": 1}}\n"
},
"entityType": {
"type": "string",
Expand Down Expand Up @@ -1556,7 +1556,7 @@
"payload": {
"type": "string",
"description": "Payload of method -- must be in WDL format. MUST BE REQUESTED EXPLICITLY.",
"default": "{ \"methodStoreMethod\": { \"methodNamespace\": \"YOUR_NAMESPACE\", \"methodName\": \"BWA\", \"methodVersion\": 1}}\n"
"default": "{ \"methodRepoMethod\": { \"methodNamespace\": \"YOUR_NAMESPACE\", \"methodName\": \"BWA\", \"methodVersion\": 1}}\n"
},
"entityType": {
"type": "string",
Expand Down
4 changes: 2 additions & 2 deletions src/main/resources/swagger/agora.yaml
Expand Up @@ -1055,7 +1055,7 @@ definitions:
type: string
description: Payload of method -- must be in WDL format
default: >
{ "methodStoreMethod": { "methodNamespace": "YOUR_NAMESPACE",
{ "methodRepoMethod": { "methodNamespace": "YOUR_NAMESPACE",
"methodName": "BWA", "methodVersion": 1}}
entityType:
type: string
Expand Down Expand Up @@ -1097,7 +1097,7 @@ definitions:
type: string
description: Payload of method -- must be in WDL format. MUST BE REQUESTED EXPLICITLY.
default: >
{ "methodStoreMethod": { "methodNamespace": "YOUR_NAMESPACE",
{ "methodRepoMethod": { "methodNamespace": "YOUR_NAMESPACE",
"methodName": "BWA", "methodVersion": 1}}
entityType:
type: string
Expand Down
Expand Up @@ -2,7 +2,7 @@ package org.broadinstitute.dsde.agora.server.business

import cromwell.parser.BackendType
import cromwell.parser.WdlParser.SyntaxError
import org.broadinstitute.dsde.agora.server.exceptions.{AgoraEntityNotFoundException, NamespaceAuthorizationException}
import org.broadinstitute.dsde.agora.server.exceptions.{ValidationException, AgoraException, AgoraEntityNotFoundException, NamespaceAuthorizationException}
import org.broadinstitute.dsde.agora.server.webservice.util.{DockerImageReference, DockerHubClient}

import cromwell.binding._
Expand Down Expand Up @@ -105,13 +105,18 @@ class AgoraBusiness {
// Passed basic validation. Now check if (any) docker images that are referenced exist
namespace.tasks.foreach { validateDockerImage }
case AgoraEntityType.Configuration =>
val json = agoraEntity.payload.get.parseJson
val fields = json.asJsObject.getFields("methodStoreMethod")
require(fields.size == 1)
val subFields = fields(0).asJsObject.getFields("methodNamespace", "methodName", "methodVersion")
require(subFields(0).isInstanceOf[JsString])
require(subFields(1).isInstanceOf[JsString])
require(subFields(2).isInstanceOf[JsNumber])
try {
val json = agoraEntity.payload.get.parseJson
val fields = json.asJsObject.getFields("methodRepoMethod")
require(fields.size == 1, "Configuration payload must define at least one field named 'methodRepoMethod'.")
val subFields = fields(0).asJsObject.getFields("methodNamespace", "methodName", "methodVersion")
require(subFields(0).isInstanceOf[JsString], "Configuration methodRepoMethod must include a 'methodNamespace' key with a string value")
require(subFields(1).isInstanceOf[JsString], "Configuration methodRepoMethod must include a 'methodName' key with a string value")
require(subFields(2).isInstanceOf[JsNumber], "Configuration methodRepoMethod must include a 'methodVersion' key with a JSNumber value")
} catch {
case e: IllegalArgumentException =>
throw new ValidationException(e)
}
}
}

Expand Down
@@ -0,0 +1,12 @@
package org.broadinstitute.dsde.agora.server.exceptions

import com.typesafe.scalalogging.slf4j.LazyLogging

/**
* Sugar over a generic underlying exception such that it can be handled properly higher in the system.
* @param ex - The underlying exception
*/
class ValidationException(ex: Throwable = null) extends Exception with LazyLogging {
override def getCause: Throwable = ex
override def getMessage: String = ex.getMessage
}
Expand Up @@ -144,7 +144,7 @@ object AgoraApiJsonSupport extends DefaultJsonProtocol {

def methodRef(payload: String): AgoraEntity = {
val json = payload.parseJson
val refJson = json.asJsObject.fields("methodStoreMethod").asJsObject
val refJson = json.asJsObject.fields("methodRepoMethod").asJsObject
val namespace = refJson.fields("methodNamespace").convertTo[String]
val name = refJson.fields("methodName").convertTo[String]
val snapshotId = refJson.fields("methodVersion").convertTo[Int]
Expand Down
Expand Up @@ -80,6 +80,9 @@ trait PerRequest extends Actor {
case e: SyntaxError =>
r.complete(BadRequest, AgoraException(e.getMessage, e.getCause, BadRequest))
Stop
case e: ValidationException =>
r.complete(BadRequest,AgoraException(e.getMessage, e.getCause, BadRequest))
Stop
case e: Throwable =>
r.complete(InternalServerError, AgoraException(e.getMessage, e.getCause, InternalServerError))
Stop
Expand Down
Expand Up @@ -171,7 +171,7 @@ object AgoraTestData {
| """.stripMargin)

val taskConfigPayload = Option( s"""{
| "methodStoreMethod": {
| "methodRepoMethod": {
| "methodNamespace": "${namespace1.get}",
| "methodName": "${name1.get}",
| "methodVersion": 1
Expand All @@ -195,7 +195,7 @@ object AgoraTestData {
|}""".stripMargin)

val taskConfigPayload2 = Option( s"""{
| "methodStoreMethod": {
| "methodRepoMethod": {
| "methodNamespace": "${namespace2.get}",
| "methodName": "${name1.get}",
| "methodVersion": 1
Expand All @@ -220,7 +220,7 @@ object AgoraTestData {


val taskConfigPayload3 = Option( s"""{
| "methodStoreMethod": {
| "methodRepoMethod": {
| "methodNamespace": "${namespace3.get}",
| "methodName": "${name3.get}",
| "methodVersion": 1
Expand Down

0 comments on commit 1ac96d6

Please sign in to comment.