Skip to content

Commit 2460b3f

Browse files
brndnmtthwsbrndnmtthws
authored
Improve API error handling. (#753)
1 parent cdbcf40 commit 2460b3f

13 files changed

+251
-91
lines changed

‎pom.xml‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@
2525
<cassandra-driver-core.version>3.1.2</cassandra-driver-core.version>
2626
<chaos.version>0.8.7</chaos.version>
2727
<commons-math3.version>3.2</commons-math3.version>
28+
<commons-codec.version>1.10</commons-codec.version>
29+
<commons-email.version>1.3.3</commons-email.version>
30+
<commons-lang3.version>3.5</commons-lang3.version>
2831
<curator-framework.version>2.11.0</curator-framework.version>
2932
<guava.version>16.0.1</guava.version>
3033
<jgrapht.version>1.0.0</jgrapht.version>
@@ -198,7 +201,7 @@
198201
<dependency>
199202
<groupId>org.apache.commons</groupId>
200203
<artifactId>commons-email</artifactId>
201-
<version>1.3.2</version>
204+
<version>${commons-email.version}</version>
202205
</dependency>
203206
<dependency>
204207
<groupId>org.apache.commons</groupId>
@@ -208,7 +211,12 @@
208211
<dependency>
209212
<groupId>commons-codec</groupId>
210213
<artifactId>commons-codec</artifactId>
211-
<version>1.10</version>
214+
<version>${commons-codec.version}</version>
215+
</dependency>
216+
<dependency>
217+
<groupId>org.apache.commons</groupId>
218+
<artifactId>commons-lang3</artifactId>
219+
<version>${commons-lang3.version}</version>
212220
</dependency>
213221

214222
<!-- Test scope -->

‎src/main/resources/ui/components/JsonEditor.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,14 @@ export default class JsonEditor extends React.Component {
7070
setTimeout(function() {
7171
btn.button('reset')
7272
const jsonStore = _this.props.jsonStore
73-
jsonStore.submitError = resp.responseText
74-
jsonStore.submitStatus = resp.status + ': ' + resp.statusText
73+
let result = resp.responseJSON
74+
if (result instanceof Object) {
75+
jsonStore.submitError = result.message
76+
jsonStore.submitStatus = resp.status + ': ' + resp.statusText
77+
} else {
78+
jsonStore.submitError = resp.responseText
79+
jsonStore.submitStatus = resp.status + ': ' + resp.statusText
80+
}
7581
}, 500)
7682
})
7783
}

‎src/main/resources/ui/models/JobForm.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -384,8 +384,14 @@ export default class JobForm {
384384
}).fail(function(resp) {
385385
setTimeout(function() {
386386
btn.button('reset')
387-
_this.submitError = resp.responseText
388-
_this.submitStatus = resp.status + ': ' + resp.statusText
387+
let result = resp.responseJSON
388+
if (result instanceof Object) {
389+
_this.submitError = result.message
390+
_this.submitStatus = resp.status + ': ' + resp.statusText
391+
} else {
392+
_this.submitError = resp.responseText
393+
_this.submitStatus = resp.status + ': ' + resp.statusText
394+
}
389395
}, 500)
390396
})
391397
}

‎src/main/scala/org/apache/mesos/chronos/scheduler/api/ApiResult.scala‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
package org.apache.mesos.chronos.scheduler.api
2+
3+
import javax.ws.rs.core.Response
4+
5+
class ApiResult(
6+
val message: String,
7+
val status: String = Response.Status.BAD_REQUEST.toString
8+
) {
9+
}
10+
11+

‎src/main/scala/org/apache/mesos/chronos/scheduler/api/ApiResultSerializer.scala‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package org.apache.mesos.chronos.scheduler.api
2+
3+
import com.fasterxml.jackson.core.JsonGenerator
4+
import com.fasterxml.jackson.databind.{JsonSerializer, SerializerProvider}
5+
6+
class ApiResultSerializer extends JsonSerializer[ApiResult] {
7+
def serialize(apiResult: ApiResult, json: JsonGenerator, provider: SerializerProvider) {
8+
json.writeStartObject()
9+
10+
json.writeFieldName("status")
11+
json.writeString(apiResult.status)
12+
13+
json.writeFieldName("message")
14+
json.writeString(apiResult.message)
15+
16+
json.writeEndObject()
17+
}
18+
}

‎src/main/scala/org/apache/mesos/chronos/scheduler/api/DependentJobResource.scala‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@ package org.apache.mesos.chronos.scheduler.api
22

33
import java.util.logging.{Level, Logger}
44
import javax.ws.rs.core.Response
5+
import javax.ws.rs.core.Response.Status
56
import javax.ws.rs.{POST, Path, Produces}
67

78
import com.codahale.metrics.annotation.Timed
89
import com.google.common.base.Charsets
910
import com.google.inject.Inject
11+
import org.apache.commons.lang3.exception.ExceptionUtils
1012
import org.apache.mesos.chronos.scheduler.graph.JobGraph
1113
import org.apache.mesos.chronos.scheduler.jobs._
1214

@@ -82,11 +84,17 @@ class DependentJobResource @Inject()(
8284
} catch {
8385
case ex: IllegalArgumentException =>
8486
log.log(Level.INFO, "Bad Request", ex)
85-
Response.status(Response.Status.BAD_REQUEST).entity(ex.getMessage)
86-
.build()
87+
Response
88+
.status(Status.BAD_REQUEST)
89+
.entity(new ApiResult(ExceptionUtils.getStackTrace(ex)))
90+
.build
8791
case ex: Exception =>
8892
log.log(Level.WARNING, "Exception while serving request", ex)
89-
Response.serverError().build()
93+
Response
94+
.serverError()
95+
.entity(new ApiResult(ExceptionUtils.getStackTrace(ex),
96+
status = Status.INTERNAL_SERVER_ERROR.toString))
97+
.build
9098
}
9199
}
92100
}

‎src/main/scala/org/apache/mesos/chronos/scheduler/api/DescriptiveStatisticsSerializer.scala‎

Lines changed: 0 additions & 42 deletions
This file was deleted.

‎src/main/scala/org/apache/mesos/chronos/scheduler/api/GraphManagementResource.scala‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@ package org.apache.mesos.chronos.scheduler.api
22

33
import java.io.StringWriter
44
import java.util.logging.{Level, Logger}
5+
import javax.ws.rs._
56
import javax.ws.rs.core.Response.Status
67
import javax.ws.rs.core.{MediaType, Response}
7-
import javax.ws.rs._
88

99
import com.codahale.metrics.annotation.Timed
1010
import com.google.inject.Inject
11+
import org.apache.commons.lang3.exception.ExceptionUtils
1112
import org.apache.mesos.chronos.scheduler.config.SchedulerConfiguration
1213
import org.apache.mesos.chronos.scheduler.graph.JobGraph
1314
import org.apache.mesos.chronos.scheduler.jobs.graph.Exporter
@@ -36,9 +37,19 @@ class GraphManagementResource @Inject()(
3637
try {
3738
Response.ok(jobGraph.makeDotFile()).build
3839
} catch {
40+
case ex: IllegalArgumentException =>
41+
log.log(Level.INFO, "Bad Request", ex)
42+
Response
43+
.status(Status.BAD_REQUEST)
44+
.entity(new ApiResult(ExceptionUtils.getStackTrace(ex)))
45+
.build
3946
case ex: Exception =>
4047
log.log(Level.WARNING, "Exception while serving request", ex)
41-
throw new WebApplicationException(Status.INTERNAL_SERVER_ERROR)
48+
Response
49+
.serverError()
50+
.entity(new ApiResult(ExceptionUtils.getStackTrace(ex),
51+
status = Status.INTERNAL_SERVER_ERROR.toString))
52+
.build
4253
}
4354
}
4455

@@ -52,9 +63,19 @@ class GraphManagementResource @Inject()(
5263
Exporter.export(buffer, jobGraph, jobStats)
5364
Response.ok(buffer.toString).build
5465
} catch {
66+
case ex: IllegalArgumentException =>
67+
log.log(Level.INFO, "Bad Request", ex)
68+
Response
69+
.status(Status.BAD_REQUEST)
70+
.entity(new ApiResult(ExceptionUtils.getStackTrace(ex)))
71+
.build
5572
case ex: Exception =>
5673
log.log(Level.WARNING, "Exception while serving request", ex)
57-
throw new WebApplicationException(Status.INTERNAL_SERVER_ERROR)
74+
Response
75+
.serverError()
76+
.entity(new ApiResult(ExceptionUtils.getStackTrace(ex),
77+
status = Status.INTERNAL_SERVER_ERROR.toString))
78+
.build
5879
}
5980
}
6081
}

‎src/main/scala/org/apache/mesos/chronos/scheduler/api/Iso8601JobResource.scala‎

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,13 @@ package org.apache.mesos.chronos.scheduler.api
33
import java.util.concurrent.atomic.AtomicLong
44
import java.util.logging.{Level, Logger}
55
import javax.ws.rs._
6+
import javax.ws.rs.core.Response.Status
67
import javax.ws.rs.core.{MediaType, Response}
78

89
import com.codahale.metrics.annotation.Timed
910
import com.google.common.base.Charsets
1011
import com.google.inject.Inject
12+
import org.apache.commons.lang3.exception.ExceptionUtils
1113
import org.apache.mesos.chronos.scheduler.graph.JobGraph
1214
import org.apache.mesos.chronos.scheduler.jobs.{ScheduleBasedJob, _}
1315
import org.joda.time.{DateTime, DateTimeZone}
@@ -37,21 +39,39 @@ class Iso8601JobResource @Inject()(
3739
require(JobUtils.isValidJobName(newJob.name),
3840
"the job's name is invalid. Allowed names: '%s'".format(JobUtils.jobNamePattern.toString()))
3941
if (!Iso8601Expressions.canParse(newJob.schedule, newJob.scheduleTimeZone)) {
40-
return Response.status(Response.Status.BAD_REQUEST).entity("Could not parse schedule").build()
42+
val message = s"Cannot parse schedule '${newJob.schedule}' (wtf bro)"
43+
return Response
44+
.status(Status.BAD_REQUEST)
45+
.entity(new ApiResult(message))
46+
.build()
4147
}
4248

4349
Iso8601Expressions.parse(newJob.schedule, newJob.scheduleTimeZone) match {
4450
case Some((_, startDate, _)) =>
4551
if (startDate.isBefore(new DateTime(DateTimeZone.UTC).minusYears(1))) {
46-
return Response.status(Response.Status.BAD_REQUEST).entity("Start date is more than 1 year in the past").build()
52+
val message = s"Scheduled start date '${startDate.toString}' is more than 1 year in the past!"
53+
log.warning(message)
54+
return Response
55+
.status(Status.BAD_REQUEST)
56+
.entity(new ApiResult(message))
57+
.build()
4758
}
4859
case _ =>
49-
return Response.status(Response.Status.BAD_REQUEST).entity("Schedule didn't parse (wtf bro)").build()
60+
val message = s"Cannot parse schedule '${newJob.schedule}' (wtf bro)"
61+
log.warning(message)
62+
return Response
63+
.status(Status.BAD_REQUEST)
64+
.entity(new ApiResult(message))
65+
.build()
5066
}
5167

5268
if (!JobUtils.isValidURIDefinition(newJob)) {
53-
log.warning(s"Tried to add both uri (deprecated) and fetch parameters on ${newJob.name}")
54-
return Response.status(Response.Status.BAD_REQUEST).build()
69+
val message = s"Tried to add both uri (deprecated) and fetch parameters on ${newJob.name}"
70+
log.warning(message)
71+
return Response
72+
.status(Status.BAD_REQUEST)
73+
.entity(new ApiResult(message))
74+
.build()
5575
}
5676

5777
//TODO(FL): Create a wrapper class that handles adding & removing jobs!
@@ -63,7 +83,12 @@ class Iso8601JobResource @Inject()(
6383
val oldJob = oldJobOpt.get
6484

6585
if (!Iso8601Expressions.canParse(newJob.schedule, newJob.scheduleTimeZone)) {
66-
return Response.status(Response.Status.BAD_REQUEST).build()
86+
val message = s"Cannot parse schedule '${newJob.schedule}' (wtf bro)"
87+
log.warning(message)
88+
return Response
89+
.status(Status.BAD_REQUEST)
90+
.entity(new ApiResult(message))
91+
.build()
6792
}
6893

6994
oldJob match {
@@ -85,11 +110,17 @@ class Iso8601JobResource @Inject()(
85110
} catch {
86111
case ex: IllegalArgumentException =>
87112
log.log(Level.INFO, "Bad Request", ex)
88-
Response.status(Response.Status.BAD_REQUEST).entity(ex.getMessage)
113+
Response
114+
.status(Status.BAD_REQUEST)
115+
.entity(new ApiResult(ExceptionUtils.getStackTrace(ex)))
89116
.build
90117
case ex: Exception =>
91118
log.log(Level.WARNING, "Exception while serving request", ex)
92-
Response.serverError().build
119+
Response
120+
.serverError()
121+
.entity(new ApiResult(ExceptionUtils.getStackTrace(ex),
122+
status = Status.INTERNAL_SERVER_ERROR.toString))
123+
.build
93124
}
94125
}
95126

0 commit comments

Comments
 (0)