{"id":21134,"date":"2014-01-31T19:00:13","date_gmt":"2014-01-31T17:00:13","guid":{"rendered":"http:\/\/www.javacodegeeks.com\/?p=21134"},"modified":"2014-01-31T08:20:17","modified_gmt":"2014-01-31T06:20:17","slug":"project-student-simplifying-code-with-aop","status":"publish","type":"post","link":"https:\/\/www.javacodegeeks.com\/2014\/01\/project-student-simplifying-code-with-aop.html","title":{"rendered":"Project Student: Simplifying Code With AOP"},"content":{"rendered":"

This is part of Project Student<\/a>.<\/p>\n

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\u2019s \u201cone thing\u201d is to coordinate the new methods.<\/p>\n

This does not mean breaking apart a method after an arbitrary number of lines. Sometimes methods are naturally larger. It\u2019s still always a good question to ask.<\/p>\n

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.<\/p>\n

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.<\/p>\n

That\u2019s the theory. In practice the code can be ugly and with modest gains. Fortunately there\u2019s 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.<\/p>\n

Design Decisions<\/h2>\n

AspectJ<\/strong> \u2013 I\u2019m using AspectJ via Spring injection.<\/p>\n

Limitations<\/h2>\n

The AspectJ pointcut expressions are relatively simple with the CRUD methods. That might not be true as more complex functionality is added.<\/p>\n

Unhandled Exceptions in Resource Methods<\/h2>\n

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\u2019t want an attacker to know. We can control what it includes if we send it ourselves. We could add a \u2018catch\u2019 block in all of our methods but it\u2019s boilerplate that we can move into an AOP method. That will leave all of our Resource methods a lot slimmer and easier to read.<\/p>\n

This class also checks for \u201cObject Not Found\u201d 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.<\/p>\n

This class has two optimizations. First, it explicitly checks for a UnitTestException<\/em> 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.<\/p>\n

Second, it uses the logger associated with the targeted class, e.g., CourseResource<\/em>, 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.<\/p>\n

Another trick is to call an ExceptionService<\/em> 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\u2019t been implemented so I just put in a comment to show where it goes.<\/p>\n

@Aspect\r\n@Component\r\npublic class UnexpectedResourceExceptionHandler {\r\n    @Around(\"target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource)\")\r\n    public Object checkForUnhandledException(ProceedingJoinPoint pjp) throws Throwable {\r\n        Object results = null;\r\n        Logger log = Logger.getLogger(pjp.getSignature().getClass());\r\n\r\n        try {\r\n            results = pjp.proceed(pjp.getArgs());\r\n        } catch (ObjectNotFoundException e) {\r\n            \/\/ this is safe to log since we know that we've passed filtering.\r\n            String args = Arrays.toString(pjp.getArgs());\r\n            results = Response.status(Status.NOT_FOUND).entity(\"object not found: \" + args).build();\r\n            if (log.isDebugEnabled()) {\r\n                log.debug(\"object not found: \" + args);\r\n            }\r\n        } catch (Exception e) {\r\n            \/\/ find the method we called. We can't cache this since the method\r\n            \/\/ may be overloaded\r\n            Method method = findMethod(pjp); \r\n            if ((method != null) && Response.class.isAssignableFrom(method.getReturnType())) {\r\n                \/\/ if the method returns a response we can return a 500 message.\r\n                if (!(e instanceof UnitTestException)) {\r\n                    if (log.isInfoEnabled()) {\r\n                        log.info(\r\n                                String.format(\"%s(): unhandled exception: %s\", pjp.getSignature().getName(),\r\n                                        e.getMessage()), e);\r\n                    }\r\n                } else if (log.isTraceEnabled()) {\r\n                    log.info(\"unit test exception: \" + e.getMessage());\r\n                }\r\n                results = Response.status(Status.INTERNAL_SERVER_ERROR).build();\r\n            } else {\r\n                \/\/ DO NOT LOG THE EXCEPTION. That just clutters the log - let\r\n                \/\/ the final handler log it.\r\n                throw e;\r\n            }\r\n        }\r\n\r\n        return results;\r\n    }\r\n\r\n    \/**\r\n     * Find method called via reflection.\r\n     *\/\r\n    Method findMethod(ProceedingJoinPoint pjp) {\r\n        Class[] argtypes = new Class[pjp.getArgs().length];\r\n        for (int i = 0; i < argtypes.length; i++) {\r\n            argtypes[i] = pjp.getArgs()[i].getClass();\r\n        }\r\n\r\n        Method method = null;\r\n\r\n        try {\r\n            \/\/ @SuppressWarnings(\"unchecked\")\r\n            method = pjp.getSignature().getDeclaringType().getMethod(pjp.getSignature().getName(), argtypes);\r\n        } catch (Exception e) {\r\n            Logger.getLogger(UnexpectedResourceExceptionHandler.class).info(\r\n                    String.format(\"could not find method for %s.%s\", pjp.getSignature().getDeclaringType().getName(),\r\n                            pjp.getSignature().getName()));\r\n        }\r\n\r\n        return method;\r\n    }\r\n}<\/pre>\n
REST Post Values Checking<\/h2>\n
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\u2019s easy to move much of this code into an AOP method and simplify the Resource method.<\/p>\n
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.
 
<\/div> <\/div><\/p>\n
public interface Validatable {\r\n\r\n    boolean validate();\r\n}<\/pre>\n
We now extend our earlier REST transfer objects to add a validation method.<\/p>\n
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.<\/p>\n
Second, I\u2019ve added a toString()<\/em> method but it\u2019s unsafe since it uses unsanitized values. I\u2019ll address sanitization later.<\/p>\n
@XmlRootElement\r\npublic class NameAndEmailAddressRTO implements Validatable {\r\n\r\n    \/\/ names must be alphabetic, an apostrophe, a dash or a space. (Anne-Marie,\r\n    \/\/ O'Brien). This pattern should accept non-Latin characters.\r\n    \/\/ digits and colon are added to aid testing. Unlikely but possible in real\r\n    \/\/ names.\r\n    private static final Pattern NAME_PATTERN = Pattern.compile(\"^[\\\\p{L}\\\\p{Digit}' :-]+$\");\r\n\r\n    \/\/ email address must be well-formed. This pattern should accept non-Latin\r\n    \/\/ characters.\r\n    private static final Pattern EMAIL_PATTERN = Pattern.compile(\"^[^@]+@([\\\\p{L}\\\\p{Digit}-]+\\\\.)?[\\\\p{L}]+\");\r\n\r\n    private String name;\r\n    private String emailAddress;\r\n    private String testUuid;\r\n\r\n    public String getName() {\r\n        return name;\r\n    }\r\n\r\n    public void setName(String name) {\r\n        this.name = name;\r\n    }\r\n\r\n    public String getEmailAddress() {\r\n        return emailAddress;\r\n    }\r\n\r\n    public void setEmailAddress(String emailAddress) {\r\n        this.emailAddress = emailAddress;\r\n    }\r\n\r\n    public String getTestUuid() {\r\n        return testUuid;\r\n    }\r\n\r\n    public void setTestUuid(String testUuid) {\r\n        this.testUuid = testUuid;\r\n    }\r\n\r\n    \/**\r\n     * Validate values.\r\n     *\/\r\n    @Override\r\n    public boolean validate() {\r\n        if ((name == null) || !NAME_PATTERN.matcher(name).matches()) {\r\n            return false;\r\n        }\r\n\r\n        if ((emailAddress == null) || !EMAIL_PATTERN.matcher(emailAddress).matches()) {\r\n            return false;\r\n        }\r\n\r\n        if ((testUuid != null) && !StudentUtil.isPossibleUuid(testUuid)) {\r\n            return false;\r\n        }\r\n\r\n        return true;\r\n    }\r\n\r\n    @Override\r\n    public String toString() {\r\n        \/\/ FIXME: this is unsafe!\r\n        return String.format(\"NameAndEmailAddress('%s', '%s', %s)\", name, emailAddress, testUuid);\r\n    }\r\n}<\/pre>\n
We make similar changes to the other REST transfer objects.<\/p>\n
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.<\/p>\n
These methods also log the entry of the Resource methods. Again it\u2019s boilerplate and doing it here simplifies the Resource methods. It would be trivial to also log the method\u2019s exit and elapsed time but we should use a stock logger AOP class in that case.<\/p>\n
@Aspect\r\n@Component\r\npublic class CheckPostValues {\r\n\r\n    \/**\r\n     * Check post values on create method.\r\n     * \r\n     * @param pjp\r\n     * @return\r\n     * @throws Throwable\r\n     *\/\r\n    @Around(\"target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource) && args(validatable,..)\")\r\n    public Object checkParametersCreate(ProceedingJoinPoint pjp, Validatable rto) throws Throwable {\r\n        final Logger log = Logger.getLogger(pjp.getSignature().getDeclaringType());\r\n        final String name = pjp.getSignature().getName();\r\n        Object results = null;\r\n\r\n        if (rto.validate()) {\r\n            \/\/ this should be safe since parameters have been validated.\r\n            if (log.isDebugEnabled()) {\r\n                log.debug(String.format(\"%s(%s): entry\", name, Arrays.toString(pjp.getArgs())));\r\n            }\r\n            results = pjp.proceed(pjp.getArgs());\r\n        } else {\r\n            \/\/ FIXME: this is unsafe\r\n            if (log.isInfoEnabled()) {\r\n                log.info(String.format(\"%s(%s): bad arguments\", name, Arrays.toString(pjp.getArgs())));\r\n            }\r\n            \/\/ TODO: tell caller what the problems were\r\n            results = Response.status(Status.BAD_REQUEST).build();\r\n        }\r\n\r\n        return results;\r\n    }\r\n\r\n    \/**\r\n     * Check post values on update method.\r\n     * \r\n     * @param pjp\r\n     * @return\r\n     * @throws Throwable\r\n     *\/\r\n    @Around(\"target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource) && args(uuid,validatable,..)\")\r\n    public Object checkParametersUpdate(ProceedingJoinPoint pjp, String uuid, Validatable rto) throws Throwable {\r\n        final Logger log = Logger.getLogger(pjp.getSignature().getDeclaringType());\r\n        final String name = pjp.getSignature().getName();\r\n        Object results = null;\r\n\r\n        if (!StudentUtil.isPossibleUuid(uuid)) {\r\n            \/\/ this is a possible attack.\r\n            if (log.isInfoEnabled()) {\r\n                log.info(String.format(\"%s(): uuid\", name));\r\n            }\r\n            results = Response.status(Status.BAD_REQUEST).build();\r\n        } else if (rto.validate()) {\r\n            \/\/ this should be safe since parameters have been validated.\r\n            if (log.isDebugEnabled()) {\r\n                log.debug(String.format(\"%s(%s): entry\", name, Arrays.toString(pjp.getArgs())));\r\n            }\r\n            results = pjp.proceed(pjp.getArgs());\r\n        } else {\r\n            \/\/ FIXME: this is unsafe\r\n            if (log.isInfoEnabled()) {\r\n                log.info(String.format(\"%s(%s): bad arguments\", name, Arrays.toString(pjp.getArgs())));\r\n            }\r\n            \/\/ TODO: tell caller what the problems were\r\n            results = Response.status(Status.BAD_REQUEST).build();\r\n        }\r\n\r\n        return results;\r\n    }\r\n\r\n    \/**\r\n     * Check post values on delete method. This is actually a no-op but it\r\n     * allows us to log method entry.\r\n     * \r\n     * @param pjp\r\n     * @return\r\n     * @throws Throwable\r\n     *\/\r\n    @Around(\"target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource) && args(uuid,version) && execution(* *.delete*(..))\")\r\n    public Object checkParametersDelete(ProceedingJoinPoint pjp, String uuid, Integer version) throws Throwable {\r\n        final Logger log = Logger.getLogger(pjp.getSignature().getDeclaringType());\r\n        final String name = pjp.getSignature().getName();\r\n        Object results = null;\r\n\r\n        if (!StudentUtil.isPossibleUuid(uuid)) {\r\n            \/\/ this is a possible attack.\r\n            if (log.isInfoEnabled()) {\r\n                log.info(String.format(\"%s(): uuid\", name));\r\n            }\r\n            results = Response.status(Status.BAD_REQUEST).build();\r\n        } else {\r\n            \/\/ this should be safe since parameters have been validated.\r\n            if (log.isDebugEnabled()) {\r\n                log.debug(String.format(\"%s(%s): entry\", name, Arrays.toString(pjp.getArgs())));\r\n            }\r\n            results = pjp.proceed(pjp.getArgs());\r\n        }\r\n\r\n        return results;\r\n    }\r\n\r\n    \/**\r\n     * Check post values on find methods. This is actually a no-op but it allows\r\n     * us to log method entry.\r\n     * \r\n     * @param pjp\r\n     * @return\r\n     * @throws Throwable\r\n     *\/\r\n    @Around(\"target(com.invariantproperties.sandbox.student.webservice.server.rest.AbstractResource) && execution(* *.find*(..))\")\r\n    public Object checkParametersFind(ProceedingJoinPoint pjp) throws Throwable {\r\n        final Logger log = Logger.getLogger(pjp.getSignature().getDeclaringType());\r\n\r\n        if (log.isDebugEnabled()) {\r\n            log.debug(String.format(\"%s(%s): entry\", pjp.getSignature().getName(), Arrays.toString(pjp.getArgs())));\r\n        }\r\n        final Object results = pjp.proceed(pjp.getArgs());\r\n\r\n        return results;\r\n    }\r\n}<\/pre>\n
Updated Spring Configuration<\/h2>\n
We must tell Spring to search for AOP classes. This is a one-line change to our configuration file.<\/p>\n
<beans xmlns=\"http:\/\/www.springframework.org\/schema\/beans\"\r\n\txmlns:xsi=\"http:\/\/www.w3.org\/2001\/XMLSchema-instance\" \r\n\txmlns:context=\"http:\/\/www.springframework.org\/schema\/context\"\r\n    xmlns:aop=\"http:\/\/www.springframework.org\/schema\/aop\"\r\n\txsi:schemaLocation=\"http:\/\/www.springframework.org\/schema\/beans\r\n\r\nhttp:\/\/www.springframework.org\/schema\/beans\/spring-beans-3.0.xsd\r\n\r\nhttp:\/\/www.springframework.org\/schema\/context\r\n\r\nhttp:\/\/www.springframework.org\/schema\/context\/spring-context-3.0.xsd\r\n\r\nhttp:\/\/www.springframework.org\/schema\/aop\r\n\r\n    http:\/\/www.springframework.org\/schema\/aop\/spring-aop-3.0.xsd\">\r\n\r\n    <aop:aspectj-autoproxy\/>\r\n<\/beans><\/pre>\n
Updated Resource<\/h2>\n
We can now simplify our Resource classes. Several methods have been reduced to the happy path alone.<\/p>\n
@Service\r\n@Path(\"\/course\")\r\npublic class CourseResource extends AbstractResource {\r\n    private static final Logger LOG = Logger.getLogger(CourseResource.class);\r\n    private static final Course[] EMPTY_COURSE_ARRAY = new Course[0];\r\n\r\n    @Resource\r\n    private CourseFinderService finder;\r\n\r\n    @Resource\r\n    private CourseManagerService manager;\r\n\r\n    @Resource\r\n    private TestRunService testRunService;\r\n\r\n    \/**\r\n     * Default constructor.\r\n     *\/\r\n    public CourseResource() {\r\n\r\n    }\r\n\r\n    \/**\r\n     * Set values used in unit tests. (Required due to AOP)\r\n     * \r\n     * @param finder\r\n     * @param manager\r\n     * @param testService\r\n     *\/\r\n    void setServices(CourseFinderService finder, CourseManagerService manager, TestRunService testRunService) {\r\n        this.finder = finder;\r\n        this.manager = manager;\r\n        this.testRunService = testRunService;\r\n    }\r\n\r\n    \/**\r\n     * Get all Courses.\r\n     * \r\n     * @return\r\n     *\/\r\n    @GET\r\n    @Produces({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })\r\n    public Response findAllCourses() {\r\n        final List courses = finder.findAllCourses();\r\n\r\n        final List results = new ArrayList(courses.size());\r\n        for (Course course : courses) {\r\n            results.add(scrubCourse(course));\r\n        }\r\n\r\n        final Response response = Response.ok(results.toArray(EMPTY_COURSE_ARRAY)).build();\r\n\r\n        return response;\r\n    }\r\n\r\n    \/**\r\n     * Create a Course.\r\n     * \r\n     * FIXME: what about uniqueness violations?\r\n     * \r\n     * @param req\r\n     * @return\r\n     *\/\r\n    @POST\r\n    @Consumes({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })\r\n    @Produces({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })\r\n    public Response createCourse(CourseInfo req) {\r\n        final String code = req.getCode();\r\n        final String name = req.getName();\r\n\r\n        Response response = null;\r\n        Course course = null;\r\n\r\n        if (req.getTestUuid() != null) {\r\n            TestRun testRun = testRunService.findTestRunByUuid(req.getTestUuid());\r\n            if (testRun != null) {\r\n                course = manager.createCourseForTesting(code, name, req.getSummary(), req.getDescription(),\r\n                        req.getCreditHours(), testRun);\r\n            } else {\r\n                response = Response.status(Status.BAD_REQUEST).entity(\"unknown test UUID\").build();\r\n            }\r\n        } else {\r\n            course = manager.createCourse(code, name, req.getSummary(), req.getDescription(), req.getCreditHours());\r\n        }\r\n        if (course == null) {\r\n            response = Response.status(Status.INTERNAL_SERVER_ERROR).build();\r\n        } else {\r\n            response = Response.created(URI.create(course.getUuid())).entity(scrubCourse(course)).build();\r\n        }\r\n\r\n        return response;\r\n    }\r\n\r\n    \/**\r\n     * Get a specific Course.\r\n     * \r\n     * @param uuid\r\n     * @return\r\n     *\/\r\n    @Path(\"\/{courseId}\")\r\n    @GET\r\n    @Produces({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })\r\n    public Response getCourse(@PathParam(\"courseId\") String id) {\r\n\r\n        \/\/ 'object not found' handled by AOP\r\n        Course course = finder.findCourseByUuid(id);\r\n        final Response response = Response.ok(scrubCourse(course)).build();\r\n\r\n        return response;\r\n    }\r\n\r\n    \/**\r\n     * Update a Course.\r\n     * \r\n     * FIXME: what about uniqueness violations?\r\n     * \r\n     * @param id\r\n     * @param req\r\n     * @return\r\n     *\/\r\n    @Path(\"\/{courseId}\")\r\n    @POST\r\n    @Consumes({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })\r\n    @Produces({ MediaType.APPLICATION_JSON, MediaType.TEXT_XML })\r\n    public Response updateCourse(@PathParam(\"courseId\") String id, CourseInfo req) {\r\n\r\n        final String name = req.getName();\r\n\r\n        \/\/ 'object not found' handled by AOP\r\n        final Course course = finder.findCourseByUuid(id);\r\n        final Course updatedCourse = manager.updateCourse(course, name, req.getSummary(), req.getDescription(),\r\n                req.getCreditHours());\r\n        final Response response = Response.ok(scrubCourse(updatedCourse)).build();\r\n\r\n        return response;\r\n    }\r\n\r\n    \/**\r\n     * Delete a Course.\r\n     * \r\n     * @param id\r\n     * @return\r\n     *\/\r\n    @Path(\"\/{courseId}\")\r\n    @DELETE\r\n    public Response deleteCourse(@PathParam(\"courseId\") String id, @PathParam(\"version\") Integer version) {\r\n\r\n        \/\/ we don't use AOP handler since it's okay for there to be no match\r\n        try {\r\n            manager.deleteCourse(id, version);\r\n        } catch (ObjectNotFoundException exception) {\r\n            LOG.debug(\"course not found: \" + id);\r\n        }\r\n\r\n        final Response response = Response.noContent().build();\r\n\r\n        return response;\r\n    }\r\n}<\/pre>\n
Unit Tests<\/h2>\n
The unit tests require a change to every test since we can\u2019t simply instantiate the objects being tested \u2013 we must use Spring so the AOP classes are properly weaved. Fortunately that\u2019s essentially the only change \u2013 we retrieve the Resource and set the services via a package-private method instead of via package-private constructors.<\/p>\n
We also need to create Spring values for our service beans. A Configurer class takes care of this.<\/p>\n
@Configuration\r\n@ComponentScan(basePackages = { \"com.invariantproperties.sandbox.student.webservice.server.rest\" })\r\n@ImportResource({ \"classpath:applicationContext-rest.xml\" })\r\n\/\/ @PropertySource(\"classpath:application.properties\")\r\npublic class TestRestApplicationContext1 {\r\n\r\n    @Bean\r\n    public CourseFinderService courseFinderService() {\r\n        return null;\r\n    }\r\n\r\n    @Bean\r\n    public CourseManagerService courseManagerService() {\r\n        return null;\r\n    }\r\n\r\n    ....<\/pre>\n
Integration Tests<\/h2>\n
The integration tests do not require any changes.<\/p>\n
Source Code<\/h2>\n