This is part of Project Student.
Many people strongly believe that methods should fit within your editor window (say, 20 lines), and some people believe that methods should be even smaller than that. The idea is that a method should do one thing and only one thing. If it does more than that it should be broken apart into multiple methods and the old method’s “one thing” is to coordinate the new methods.
This does not mean breaking apart a method after an arbitrary number of lines. Sometimes methods are naturally larger. It’s still always a good question to ask.
So how do you recognize code that does more than one thing? A good touchstone is if the code is duplicated in multiple methods. The canonical example is transaction management in persistence classes. Every persistence class needs it and the code always looks the same.
Another example is the unhandled exception handler in my Resource classes. Every REST-facing method needs to deal with this and the code always looks the same.
That’s the theory. In practice the code can be ugly and with modest gains. Fortunately there’s a solution: Aspect-Oriented Programming (AOP). This allows us to transparently weave code before or after our method calls. This often lets us simplify our methods dramatically.
Design Decisions
AspectJ – I’m using AspectJ via Spring injection.
Limitations
The AspectJ pointcut expressions are relatively simple with the CRUD methods. That might not be true as more complex functionality is added.
Unhandled Exceptions in Resource Methods
Our first concern is unhandled exceptions in resource methods. Jersey will return a SERVER INTERNAL ERROR (500) message anyway but it will probably include a stack trace and other content that we don’t want an attacker to know. We can control what it includes if we send it ourselves. We could add a ‘catch’ block in all of our methods but it’s boilerplate that we can move into an AOP method. That will leave all of our Resource methods a lot slimmer and easier to read.
This class also checks for “Object Not Found” exceptions. It would be easy to handle in the individual Resource classes but it muddies the code. Putting the exception handler here allows our methods to focus on the happy path and guarantees consistency in the response.
This class has two optimizations. First, it explicitly checks for a UnitTestException and skips detailed logging in that case. One of my biggest pet peeve is tests that that flood the logs with stack traces when everything is working like expected. That makes it impossible to skim the logs for obvious problems. This single change can make problems much easier to find.
Second, it uses the logger associated with the targeted class, e.g., CourseResource, instead of with the AOP class. Besides being clearer this allows us to selectively change the logging level for a single Resource instead of all of them.
Another trick is to call an ExceptionService in our handler. This is a service that can do something useful with exceptions, e.g., it might create or update a Jira ticket. This hasn’t been implemented so I just put in a comment to show where it goes.
@Aspect
@Component
public class UnexpectedResourceExceptionHandler {
@Around("target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource)")
public Object checkForUnhandledException(ProceedingJoinPoint pjp) throws Throwable {
Object results = null;
Logger log = Logger.getLogger(pjp.getSignature().getClass());
try {
results = pjp.proceed(pjp.getArgs());
} catch (ObjectNotFoundException e) {
// this is safe to log since we know that we've passed filtering.
String args = Arrays.toString(pjp.getArgs());
results = Response.status(Status.NOT_FOUND).entity("object not found: " + args).build();
if (log.isDebugEnabled()) {
log.debug("object not found: " + args);
}
} catch (Exception e) {
// find the method we called. We can't cache this since the method
// may be overloaded
Method method = findMethod(pjp);
if ((method != null) && Response.class.isAssignableFrom(method.getReturnType())) {
// if the method returns a response we can return a 500 message.
if (!(e instanceof UnitTestException)) {
if (log.isInfoEnabled()) {
log.info(
String.format("%s(): unhandled exception: %s", pjp.getSignature().getName(),
e.getMessage()), e);
}
} else if (log.isTraceEnabled()) {
log.info("unit test exception: " + e.getMessage());
}
results = Response.status(Status.INTERNAL_SERVER_ERROR).build();
} else {
// DO NOT LOG THE EXCEPTION. That just clutters the log - let
// the final handler log it.
throw e;
}
}
return results;
}
/**
* Find method called via reflection.
*/
Method findMethod(ProceedingJoinPoint pjp) {
Class[] argtypes = new Class[pjp.getArgs().length];
for (int i = 0; i < argtypes.length; i++) {
argtypes[i] = pjp.getArgs()[i].getClass();
}
Method method = null;
try {
// @SuppressWarnings("unchecked")
method = pjp.getSignature().getDeclaringType().getMethod(pjp.getSignature().getName(), argtypes);
} catch (Exception e) {
Logger.getLogger(UnexpectedResourceExceptionHandler.class).info(
String.format("could not find method for %s.%s", pjp.getSignature().getDeclaringType().getName(),
pjp.getSignature().getName()));
}
return method;
}
}REST Post Values Checking
Our Resource methods also have a lot of boilerplate code to check the REST parameters. Are they non-null, are email addresses well-formed, etc. Again it’s easy to move much of this code into an AOP method and simplify the Resource method.
We start by defining an interface that indicates a REST transfer object can be validated. This first version gives us a simple thumbs-up or thumbs-down, an improved version will give us a way to tell the client what the specific problems are.
public interface Validatable {
boolean validate();
}We now extend our earlier REST transfer objects to add a validation method.
Two notes. First, the name and email address accept Unicode letters, not just the standard ASCII letters. This is important as our world becomes internationalized.
Second, I’ve added a toString() method but it’s unsafe since it uses unsanitized values. I’ll address sanitization later.
@XmlRootElement
public class NameAndEmailAddressRTO implements Validatable {
// names must be alphabetic, an apostrophe, a dash or a space. (Anne-Marie,
// O'Brien). This pattern should accept non-Latin characters.
// digits and colon are added to aid testing. Unlikely but possible in real
// names.
private static final Pattern NAME_PATTERN = Pattern.compile("^[\\p{L}\\p{Digit}' :-]+$");
// email address must be well-formed. This pattern should accept non-Latin
// characters.
private static final Pattern EMAIL_PATTERN = Pattern.compile("^[^@]+@([\\p{L}\\p{Digit}-]+\\.)?[\\p{L}]+");
private String name;
private String emailAddress;
private String testUuid;
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
public String getEmailAddress() {
return emailAddress;
}
public void setEmailAddress(String emailAddress) {
this.emailAddress = emailAddress;
}
public String getTestUuid() {
return testUuid;
}
public void setTestUuid(String testUuid) {
this.testUuid = testUuid;
}
/**
* Validate values.
*/
@Override
public boolean validate() {
if ((name == null) || !NAME_PATTERN.matcher(name).matches()) {
return false;
}
if ((emailAddress == null) || !EMAIL_PATTERN.matcher(emailAddress).matches()) {
return false;
}
if ((testUuid != null) && !StudentUtil.isPossibleUuid(testUuid)) {
return false;
}
return true;
}
@Override
public String toString() {
// FIXME: this is unsafe!
return String.format("NameAndEmailAddress('%s', '%s', %s)", name, emailAddress, testUuid);
}
}We make similar changes to the other REST transfer objects.
We can now write our AOP methods to check the parameters to our CRUD operations. As before the logs are written using the logger associated with the Resource instead of the AOP class.
These methods also log the entry of the Resource methods. Again it’s boilerplate and doing it here simplifies the Resource methods. It would be trivial to also log the method’s exit and elapsed time but we should use a stock logger AOP class in that case.
@Aspect
@Component
public class CheckPostValues {
/**
* Check post values on create method.
*
* @param pjp
* @return
* @throws Throwable
*/
@Around("target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource) && args(validatable,..)")
public Object checkParametersCreate(ProceedingJoinPoint pjp, Validatable rto) throws Throwable {
final Logger log = Logger.getLogger(pjp.getSignature().getDeclaringType());
final String name = pjp.getSignature().getName();
Object results = null;
if (rto.validate()) {
// this should be safe since parameters have been validated.
if (log.isDebugEnabled()) {
log.debug(String.format("%s(%s): entry", name, Arrays.toString(pjp.getArgs())));
}
results = pjp.proceed(pjp.getArgs());
} else {
// FIXME: this is unsafe
if (log.isInfoEnabled()) {
log.info(String.format("%s(%s): bad arguments", name, Arrays.toString(pjp.getArgs())));
}
// TODO: tell caller what the problems were
results = Response.status(Status.BAD_REQUEST).build();
}
return results;
}
/**
* Check post values on update method.
*
* @param pjp
* @return
* @throws Throwable
*/
@Around("target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource) && args(uuid,validatable,..)")
public Object checkParametersUpdate(ProceedingJoinPoint pjp, String uuid, Validatable rto) throws Throwable {
final Logger log = Logger.getLogger(pjp.getSignature().getDeclaringType());
final String name = pjp.getSignature().getName();
Object results = null;
if (!StudentUtil.isPossibleUuid(uuid)) {
// this is a possible attack.
if (log.isInfoEnabled()) {
log.info(String.format("%s(): uuid", name));
}
results = Response.status(Status.BAD_REQUEST).build();
} else if (rto.validate()) {
// this should be safe since parameters have been validated.
if (log.isDebugEnabled()) {
log.debug(String.format("%s(%s): entry", name, Arrays.toString(pjp.getArgs())));
}
results = pjp.proceed(pjp.getArgs());
} else {
// FIXME: this is unsafe
if (log.isInfoEnabled()) {
log.info(String.format("%s(%s): bad arguments", name, Arrays.toString(pjp.getArgs())));
}
// TODO: tell caller what the problems were
results = Response.status(Status.BAD_REQUEST).build();
}
return results;
}
/**
* Check post values on delete method. This is actually a no-op but it
* allows us to log method entry.
*
* @param pjp
* @return
* @throws Throwable
*/
@Around("target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource) && args(uuid,version) && execution(* *.delete*(..))")
public Object checkParametersDelete(ProceedingJoinPoint pjp, String uuid, Integer version) throws Throwable {
final Logger log = Logger.getLogger(pjp.getSignature().getDeclaringType());
final String name = pjp.getSignature().getName();
Object results = null;
if (!StudentUtil.isPossibleUuid(uuid)) {
// this is a possible attack.
if (log.isInfoEnabled()) {
log.info(String.format("%s(): uuid", name));
}
results = Response.status(Status.BAD_REQUEST).build();
} else {
// this should be safe since parameters have been validated.
if (log.isDebugEnabled()) {
log.debug(String.format("%s(%s): entry", name, Arrays.toString(pjp.getArgs())));
}
results = pjp.proceed(pjp.getArgs());
}
return results;
}
/**
* Check post values on find methods. This is actually a no-op but it allows
* us to log method entry.
*
* @param pjp
* @return
* @throws Throwable
*/
@Around("target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource) && execution(* *.find*(..))")
public Object checkParametersFind(ProceedingJoinPoint pjp) throws Throwable {
final Logger log = Logger.getLogger(pjp.getSignature().getDeclaringType());
if (log.isDebugEnabled()) {
log.debug(String.format("%s(%s): entry", pjp.getSignature().getName(), Arrays.toString(pjp.getArgs())));
}
final Object results = pjp.proceed(pjp.getArgs());
return results;
}
}Updated Spring Configuration
We must tell Spring to search for AOP classes. This is a one-line change to our configuration file.
<beans xmlns="http://www.springframework.org/schema/beans"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:context="http://www.springframework.org/schema/context"
xmlns:aop="http://www.springframework.org/schema/aop"
xsi:schemaLocation="http://www.springframework.org/schema/beans
http://www.springframework.org/schema/beans/spring-beans-3.0.xsd
http://www.springframework.org/schema/context
http://www.springframework.org/schema/context/spring-context-3.0.xsd
http://www.springframework.org/schema/aop
http://www.springframework.org/schema/aop/spring-aop-3.0.xsd">
<aop:aspectj-autoproxy/>
</beans>Updated Resource
We can now simplify our Resource classes. Several methods have been reduced to the happy path alone.
@Service
@Path("/course")
public class CourseResource extends AbstractResource {
private static final Logger LOG = Logger.getLogger(CourseResource.class);
private static final Course[] EMPTY_COURSE_ARRAY = new Course[0];
@Resource
private CourseFinderService finder;
@Resource
private CourseManagerService manager;
@Resource
private TestRunService testRunService;
/**
* Default constructor.
*/
public CourseResource() {
}
/**
* Set values used in unit tests. (Required due to AOP)
*
* @param finder
* @param manager
* @param testService
*/
void setServices(CourseFinderService finder, CourseManagerService manager, TestRunService testRunService) {
this.finder = finder;
this.manager = manager;
this.testRunService = testRunService;
}
/**
* Get all Courses.
*
* @return
*/
@GET
@Produces({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })
public Response findAllCourses() {
final List courses = finder.findAllCourses();
final List results = new ArrayList(courses.size());
for (Course course : courses) {
results.add(scrubCourse(course));
}
final Response response = Response.ok(results.toArray(EMPTY_COURSE_ARRAY)).build();
return response;
}
/**
* Create a Course.
*
* FIXME: what about uniqueness violations?
*
* @param req
* @return
*/
@POST
@Consumes({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })
@Produces({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })
public Response createCourse(CourseInfo req) {
final String code = req.getCode();
final String name = req.getName();
Response response = null;
Course course = null;
if (req.getTestUuid() != null) {
TestRun testRun = testRunService.findTestRunByUuid(req.getTestUuid());
if (testRun != null) {
course = manager.createCourseForTesting(code, name, req.getSummary(), req.getDescription(),
req.getCreditHours(), testRun);
} else {
response = Response.status(Status.BAD_REQUEST).entity("unknown test UUID").build();
}
} else {
course = manager.createCourse(code, name, req.getSummary(), req.getDescription(), req.getCreditHours());
}
if (course == null) {
response = Response.status(Status.INTERNAL_SERVER_ERROR).build();
} else {
response = Response.created(URI.create(course.getUuid())).entity(scrubCourse(course)).build();
}
return response;
}
/**
* Get a specific Course.
*
* @param uuid
* @return
*/
@Path("/{courseId}")
@GET
@Produces({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })
public Response getCourse(@PathParam("courseId") String id) {
// 'object not found' handled by AOP
Course course = finder.findCourseByUuid(id);
final Response response = Response.ok(scrubCourse(course)).build();
return response;
}
/**
* Update a Course.
*
* FIXME: what about uniqueness violations?
*
* @param id
* @param req
* @return
*/
@Path("/{courseId}")
@POST
@Consumes({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })
@Produces({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })
public Response updateCourse(@PathParam("courseId") String id, CourseInfo req) {
final String name = req.getName();
// 'object not found' handled by AOP
final Course course = finder.findCourseByUuid(id);
final Course updatedCourse = manager.updateCourse(course, name, req.getSummary(), req.getDescription(),
req.getCreditHours());
final Response response = Response.ok(scrubCourse(updatedCourse)).build();
return response;
}
/**
* Delete a Course.
*
* @param id
* @return
*/
@Path("/{courseId}")
@DELETE
public Response deleteCourse(@PathParam("courseId") String id, @PathParam("version") Integer version) {
// we don't use AOP handler since it's okay for there to be no match
try {
manager.deleteCourse(id, version);
} catch (ObjectNotFoundException exception) {
LOG.debug("course not found: " + id);
}
final Response response = Response.noContent().build();
return response;
}
}Unit Tests
The unit tests require a change to every test since we can’t simply instantiate the objects being tested – we must use Spring so the AOP classes are properly weaved. Fortunately that’s essentially the only change – we retrieve the Resource and set the services via a package-private method instead of via package-private constructors.
We also need to create Spring values for our service beans. A Configurer class takes care of this.
@Configuration
@ComponentScan(basePackages = { "com.invariantproperties.sandbox.student.webservice.server.rest" })
@ImportResource({ "classpath:applicationContext-rest.xml" })
// @PropertySource("classpath:application.properties")
public class TestRestApplicationContext1 {
@Bean
public CourseFinderService courseFinderService() {
return null;
}
@Bean
public CourseManagerService courseManagerService() {
return null;
}
....Integration Tests
The integration tests do not require any changes.
Source Code
- The source code is at https://github.com/beargiles/project-student [github] and http://beargiles.github.io/project-student/ [github pages].
