Mind you, not a simple SAAJServlet, but an old fashioned legacy transaction script without the scent of a test. Anyway... let's pick up the usual books (
Working effectively with legacy code,
Refactoring,
Refactoring to patterns - just a starting point of course) to keep at hand as a reference and let's start refactoring.
First a word of caution: don't just copy from the books. Study them, try to apply the refactorings in a small and controlled environment, maybe in little katas, and get the basics beyond them, otherwise you'll do
more harm than good.
Back to business. We basically have to test the o
nMessage method, that gets a
SOAPMessage and returns another one. Without even looking at the code, one would expect an algorithm like the following one:
- parse the message into a parameter
- pass it to a service that executes whatever must be executed
- build a new message based on the input parameter and on the outcome of the previous step
Obviously one should also consider errors management, but let's keep it simple. Let's take a look at the actual code:
@Override
public SOAPMessage onMessage(SOAPMessage message) {
SOAPMessage result = null;
Dialogue dialogue = null;
try {
SOAPBody body = message.getSOAPBody();
SOAPFactory sFactory = SOAPFactory.newInstance();
Name bodyName = sFactory.createName("myLocalName", "myPrefix", "myUri");
Iterator it = body.getChildElements(bodyName);
if (it.hasNext()) {
SOAPBodyElement be = (SOAPBodyElement) it.next();
dialogue = parseBodyElement(be);
Operation operation = operationFactory.getOperation(dialogue);
if (operation != null) {
try {
operation.execute(dialogue);
} catch (Throwable th) {
getServletContext().log(th.getMessage(), th);
dialogue.setCode(Dialogue.KO);
dialogue.setMessage("Service unavailable.");
}
}
} else {
dialogue.setCode(Dialogue.KO);
dialogue.setMessage("Malformed message.");
}
} catch (Throwable th) {
getServletContext().log(th.getMessage(), th);
dialogue.setCode(Dialogue.KO);
dialogue.setMessage("Service unavailable.");
} finally {
try {
MessageResponseGenerator mrg = new MessageResponseGenerator();
result = mrg.generateResponseMessage(dialogue);
} catch (Throwable th) {
result = null;
th.printStackTrace();
}
}
return result;
}
Now, though
PMD does not complain too much about the structure of the method - just a few dataflow anomaly analysis, null assignments and catching throwables - I smell (more than) a rat.
Starting from the beginning, the first things that strucks me is the different level of abstractions of the code: you have both a low level implementation that extracts a
SOAPBodyElement to be parsed and a call to an
Operation that encapsulates the requested service (the
Command pattern), and this is not good. There are also too many nested ifs and try-catch blocks to my liking.
There is a separated object that creates the response message, and this is good. Why on earth are we missing an object or at least a method that parses the message in the first place?
Why do we ask a factory for an
Operation passing in a
Dialogue and then we have to pass the same parameter back to the operation we get back from the factory? couln't the operation hold a reference to the dialogue? Would it be better? we shall see later.
And, obviously, all the references are hard-coded...
So far, we have just conducted a small code review. But what's the point in the first place? We have to introduce a new
Operation object (maybe extending an existing one, and please note the alliteration). Nothing strange so far, but the resulting object for this operation should contain some more fields than the existing one. Still nothing terrible, but as you might have noticed currently the
Dialogue object works as a collecting parameter through all the method, while we would need two different objects (or modify the initial parsing method in such a way to have an instance of the right
Dialogue subclass, or introduce a composition in the
Dialogue class, or explore a thousand other possibilities).
By the way, it is mandatory that we do not change the interface for the service, which means that the incoming and outcoming messages for the existing operations cannot change.
The fact is that
before we start refactoring we have to guarantee that. Of course we could start refactoring right away, but, as you might have noticed if this is not the first post of mine you happen to read, I find it "just a little bit" risky.
So the answer to the first question, i.e. how to refactor a SAAJ servlet, is, at least in my mind, simple: write enough tests to describe the current behaviour before you do anything else. I know: easier said than done.
That's tricky: how can you do that? The class we have to test is a servlet, so we have four options:
- use manual testing: we already have this in place, but each single change would require at least a dozen tests, making the whole process too slow (and not reliably repeatable, as it is based on people's good will)
- mock all the objects provided by the servlet container needed by the servlet: quite a lot of work, and I'm not going down that way (even because I'm listening to Starway to Heaven and I don't wont to spoil the happy sensation)
- deploy the application to a servlet container and use in-container testing, possibly with a framework like Cactus: this introduces an unwanted complexity and I'll avoid it too even if it would be tempting, as the servlet is deployed in a proprietary framework in which you can only access the database if you actually start the webapp (don't ask, you don't want to know). Why? because I think it would not force me to separate responsibilities as much as the next approach.
- use a library such as HttpUnit, which contains ServletUnit. Being this a simulated container I am sure I would never have access to the database given the current situation, so I guess I'll have to work my way to a cleaner design.
Back to the servlet: there are cases in which the method returns a message without going all the way down to the database, so as a first step I'll exercise these and keep manual testing for end-to-end situations.
The very first test consists in producing a message with a wrong format: I should get a
SOAPMessage with the code property set as KO and an error message of "Malformed message". Or should I?
@Test
public void testOnMessageWithMalformedMessage() throws IOException, SAXException {
System.out.println("testOnMessageWithMalformedMessage");
InputStream resourceAsStream = this.getClass().
getClassLoader().
getResourceAsStream(
"path/to/my/message.xml");
ServletRunner sr = new ServletRunner();
sr.registerServlet("myServlet", MyServlet.class.getName());
ServletUnitClient sc = sr.newClient();
WebRequest request = new PostMethodWebRequest(
"http://localhost:8084/myServlet",
resourceAsStream,
"text/xml");
WebResponse response = sc.getResponse(request);
assertNotNull("No response received", response);
assertEquals("OK", response.getResponseMessage());
assertTrue(response.getText().contains("KO"));
assertTrue(response.getText().contains("Malformed message."));
}
These steps are detailed in the
ServletUnit documentation. Before we run the test, remember we're not going to change anything yet.
Too optimistic? actually I was. Running the tests gives me the following error:
Error on HTTP request: 500 javax.servlet.ServletException: SAAJ Post failed null.
Looks like the
MessageResponseGenerator cannot handle null parameters very well. I admit I knew I had been too optimistic about the error message (how could I trust the generator had the same message the servlet used and that I was expecting?), but at least I hoped in a valid
SOAPMessage... But after all isn't the
NullPointerException the most widespread error in Java code?
It's a long way to go... remeber that before we change the current behaviour we have to describe it, so the way is even longer. Yet, the first step has been made.
