Setting Ant properties in Hudson

As an update to an old post of mine, now I use a different approach for setting properties for the jobs defined on our Hudson instance. Instead of relying on the private.properties file defined for our NetBeans projects (which means all our projects) I simply copy those properties into the relative section in the job configuration page:


This also makes easier copying these properties between projects, besides the fact that it clearly points out which "hidden" variables we're relying upon.

The FizzBuzz kata

The first time I came over this kata was thanks to a post in which Matteo was looking for design problems. I reckon everyone has played FizzBuzz at school, so I already knew what it was about; I had in mind to try it sooner or later, and it looks like somehow I finally did.

Once I wrote my own solution, I was a little curious to compare it with others, so while googling around I landed on this page which states that

Michael Feathers and Emily Bache performed it at agile2008 when competing in "Programming with the stars" in python, in 4 minutes.

That made me feel a little inadequate, since my solution took a little more to emerge, and I guess it's not because I used Java. But, after all, I'm not Michael Feathers nor Emily Bache, so I guess it's all right.

The solution proposed follows the classic filter paradigm: you register some filters in a list (after all you want the program to say "FizzBuzz" and not "BuzzFizz") each of which processes the output of the previous one. The nice thing is that you can inject filters as you like, which is good: the main class itself has the only responsibility to iterate through the filters, whatever they might do.

My solution is somewhat different, as it is based on the decorator pattern, yet it is similar because you can still inject the decorator, which is built wrapping each rule in the outer one, and the main class has the only responsibility to ask its decorator to decorate the input. If no decorators are provided, a no-op default one is used: this is a small complication compared to the previous solution. Another complication is that the decorator actually knows it's wrapping another decorator, no-op decorator excluded, while a filter has no knowledge of whatsoever other object might operate on the input.

Anyway, the most important thing is that my design satisfies Matteo's (evolving) requirements and supports the Anti-If Campaign :-)

Of course, TDD was used :-)

Don't touch my code

There is not such thing as "my code". If there is, there should not be. This is the basis of collective code ownership: everybody can touch anything. Of course, we MUST make sure that our changes do not alter the existing behaviour (unless we're fixing bugs, or altering behaviour is exactly what we're after, of course).

Sentences like "if you touch my code I'll have to spend a lot of my time to correct your errors" are based on the (hopefully wrong) assumption that your fellow developers commit carelessly modified software. Collective code ownership also means collective responsibility, that should go hand in hand with the "leave your campfire better than you found it" habit.

If I touch "your" code (with which I actually mean "the code of which you were the first author") I magically become responsible for it. But be warned: this is a responsability I share with everybody else in the team. Actually I don't even have to touch "your" code to be responsible for it, as I already am. That's it. Do I see something unclear? I'll try to clarify it. Do I see obsolete comments? I delete them. Do I see comments? I'll probably delete them too, provided that the code speaks enough. If it doesn't, I'll try to make it speak. And delete the comments :-)

You'll never excel if you stick with Excell

Please, pay attention when you write something. I am sincerely tired of seeing the use of the word "Excell" (note the double "l") instead of Excel. I can accept it perfectly when a customer uses it, as most customers are not (why should they be, after all?) IT experts; on the other hand, I really can't stand it when a (should-be) IT expert falls in the same trap.

• It is Excel, not Excell.
• It is Outlook, not Outlock, Outloock, Out Look or other combinations.
• Ajax is not a programming language.
• Tomcat is not an application server. And, by the way, Apache sounds like "a-patchy"
  

I could go on forever, but I'm sure you get the point. At best you're considered sloppy because you didn't review what you wrote (which is already bad enough), but maybe you don't know what you're talking about at all. So why should I trust you? How can I think that you can possibly be able to solve my problems? Are you just a waffler? Please get your homework done.

How to refactor and test a SAAJServlet

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 onMessage 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.

Quarreling with classloaders

I admit that most of the quarreling, if not all of it, came from my not too deep knowledge of the subject. Anyway, at least I've learned something more.

Now that I know how to change the endpoint defined in a WSDL I was trying to put this knowledge to good use. We have a project that encapsulates the access to various web services, and we release it as a jar file that is normally used in web applications, in particular the one I'm mostly working on in these days. To easily let the administrator of the web application decide whether to use a monitoring application like wsmonitor I prepared a sevices.properties file:

proxyEnabled=true;
proxyAddress=http://...

Being test-infected I wrote a simple explorative test:

@Test
public void testLoadProperties() throws IOException {
 System.out.println("testLoadProperties");
 Properties properties = new Properties();
 InputStream systemResourceAsStream = ClassLoader.getSystemResourceAsStream("path/to/my/services.properties");
 properties.load(systemResourceAsStream);
 assertNotNull(properties.getProperty("proxyEnabled"));
}

Green bar, so I put the code in the service class. Well actually before that I wrote another small (failing) test:

@Test
public void testPropertiesAreCorrectlyInitialized(){
 System.out.println("testPropertiesAreCorrectlyInitialized");
 assertNotNull(MyService.properties.getProperty("proxyEnabled"));
}

After the green bar I switched to the webapp and started it, but the isProxyEnabled method of the service, which simply performs a test on the value of the isProxyEnabled property always returned false. If you are wondering... yes, there are the corresponding tests, which are not reported for the sake of simplicity.

After some researches, and mostly thanks to Dave Lander's contribution to this discussion, I've found the problem and changed my loading instructions:

InputStream is = MyService.class.getClassLoader().getResourceAsStream("path/to/my/services.properties");
properties.load(is);

Funny how simple things are when you know them, aren't they?

You should be so lucky

Ahhhh se solo la maggior parte dei manager sapesse… il mondo girerebbe diversamente, ma la resa dei conti arriverà presto caro cravattato dall’ignoranza bovina… ma sto divagando

Quoted from another interesting post by Gabriele Lana (aren't they all?).