Skip to content
Print

Wicket 1.3 Released…Broken Guice and All

3
Jan
2008

I usually try to avoid such negative topics, but this time I really couldn’t help myself.  Once in a while something in the “current events” section of the blogosphere will bug me enough to merit a slam post.  The “support” for Google Guice in the latest stable release of Wicket is one of those things…

To start with, the good news: Martijn Dashorst has announced the release of Apache Wicket 1.3!  This really is a great release all-around and the guys in the band deserve a round of applause.  This release fixes the number one “bug” with Wicket: it’s rather odd package namespace.  (wicket.*:-)   Welcome to the land of happy packages and tired fingers.

Wicket is really starting (or just proceeding at an accelerated rate) to feel like a rock solid, production-ready framework.  I’ve used it quite a bit over the last few years, and I’ll say flat-out that I don’t think any framework matches it for productivity and maintainability (that includes Rails, dynlang notwithstanding).

One of the new features in 1.3 (important enough to merit inclusion in Martijn’s 20-odd points) is support for Google Guice dependency injection.  This is a huge deal for those of us who have nominated Guice for the “cleverest framework of the decade” award.  Support for Guice in Wicket makes it possible to utilize dependency injection right in your page classes (where it’s most needed).  Wicket has had similar support for Spring for a while now, but it was only recently that Al Maw got the chance to refactor the guts out into the wicket-ioc project and thus enable support for alternative DI frameworks like Guice.

This all seems well and good, but unfortunately Wicket’s Guice support is not quite up to par with the rest of the framework.  I tried the support a while back in beta4 and ran headlong into a fairly serious problem.  The following code doesn’t work:

public class MyModule extends AbstractModule {
    // ...
 
    @Override
    public void configure() {
        EntityManager manager = new EntityManager(uri, username, password);
        bind(EntityManager.class).toInstance(manager);
    }
}

This is fairly standard Guice configuration code.  All that’s happening here is I’m binding all injected fields of type EntityManager to a given instance.  Of course the classic use of DI is to have it instantiate the injected values based on classname (or class literal in Guice’s case).  However, this panacea of IOC breaks down when working with classes which lack default constructors (like EntityManager).  This is why Guice enables developers to bind classes to instances (as I’m doing above).  The problem is this code will crash when executed using wicket-guice.

I opened an issue in the Wicket JIRA back in November when I first identified the bug (WICKET-1130).  I even included some simple example code I could use to repeat the problem!  Since then the issue has been reassigned and bumped back in fix version twice, all without any word on if the problem is being looked at or how soon I could expect a solution.  Now I know the Wicket devs are busy and all with tons of more sweeping issues and last-minute polish for the 1.3 release, but this is pretty absurd.

Honestly, if this were a trivial edge-case that only effected me and my neighbor’s cow, I wouldn’t put up much of a fuss.  But the fact is, this is something so broad and repeatable that it will touch just about anyone who seriously uses Guice with Wicket.  Even if there wasn’t time to fix the problem before the 1.3 release, I would hope there would be some sort of prominent notice (“KNOWNBUGS” anyone?) included with the distributable.  Unfortunately the only reference to the problem I was able to find on the Wicket site was an obscure wiki article (well written though) done by Uwe Schäfer.  All this entry serves to do is further aggravate me since it means someone else has run headlong into this problem and been annoyed by it enough to write an article (still without receiving response from the Wicket core devs).

Uwe does propose a workaround (add a protected no-arg constructor to the injected class), but that’s impractical for my use case (and if I ran into it, you can bet your boots half a dozen other people did too).  I’m certainly not going to randomly add broken constructors to the ActiveObjects API, and I wouldn’t even have the option to consider doing so except for the fact that I’m the developer on the class I was trying to bind.  If I was trying to bind an instance from a third-party library, I’d be out of luck completely.

I’m very disappointed in the Wicket project for dropping the ball on this issue.  I really have the utmost respect for those guys, which is why it’s so surprising to see something like this happen.  As it stands, WICKET-1130 is slated for 1.3.1, but given its track record of reassignment I’m not holding my breath.  Hopefully this posting will serve as a more prominent warning for those considering using Wicket and Guice together in their project.

Comments

  1. Yes, it’s a serious issue. However, I’ve started a new job and have therefore had very little time to work on Wicket leading up to the 1.3 release. I’m sorry this issue isn’t fixed yet, but I don’t get paid to develop things for you, and I’ve been very busy.

    Saying that “there has been no response from the core devs” is nothing other than a nefarious pack of lies. You e-mailed me directly about this just last week, and I told you I’d look into it ASAP.

    Patches are more than welcome. I can only surmise that if you have the time to write at length about this on your blog, you also have time to create some. We’d all appreciate more help and community support and a little less fud. Please try to be more constructive, the world will thank you for it.

    Alastair Maw Thursday, January 3, 2008 at 5:05 am
  2. Actually, I didn’t email you. Must have been someone else. :-)

    Fair enough on the constructive bit. If I’m going to write something like this I certainly would expect some feedback in that vein. I wasn’t trying to spread FUD though, just trying to bring some more attention to the issue. In all honesty, I don’t *really* blame you (or anyone else on the Wicket team) too much for letting this slip through the cracks. It’s a large scale, widely used Open Source project. You can’t always satisfy everyone or even fix everything.

    I thought about working on a patch, but it would have taken quite a bit of time (much more than the 20 minutes it took me to whip out this rant) and I have almost no expertise in the area in question. I could have done it certainly, but it would have taken me quite a bit longer than someone who knows what they’re doing (such as yourself).

    Once again, I’d like to clarify that I wasn’t *trying* to discredit you or Wicket (like I said, it’s a fantastic project with a great team), just trying to bring some much needed attention to a fairly significant issue. I would have done the same for any other project.

    Daniel Spiewak Thursday, January 3, 2008 at 8:10 am
  3. Oh, well there’s me with egg on my face – it was someone else now I look, so apologies for that.

    A simple comment on the bug (“this is a really serious issue for me”) or a post to wicket-dev or my inbox would probably suffice, however.

    FWIW, the reason this is a bug in the first place and is such a complex tangle of thorns is that Wicket can’t actually use Guice’s native injector. We need to insert a proxy to the object, not the object itself, so that we can serialize the components. This means we effectively end up duplicating Guice’s injector, which I can’t help but feel is the wrong strategy. :-(

    I’m trying to work out how to intercept it instead, but that in itself is no easy task. So progress is being made, and when a final solution appears, it will fix all the bugs like this one. I’ll be having a look at it this weekend, will keep you posted in the bug.

    Alastair Maw Thursday, January 3, 2008 at 8:19 am
  4. By the way, in the meantime you can probably work around this issue by creating a Provider, which is probably the more Guice-like way of doing this anyway (as it can be lazy-initialized then).

    Alastair Maw Thursday, January 3, 2008 at 8:30 am
  5. I probably should have dropped a line to wicket-dev at some point highlighting this. I actually considered it at one point, but I assumed that the bug was in priority stream for 1.3 or something like that. My bad for not hitting up the right channels first. :-)

    Yeah, I read Uwe’s wiki article and it mentioned something about the injector proxy but didn’t go into too much detail. Good to hear it straight from the horse’s mouth (or the beastie in this case).

    Just a thought, but what if you provide an API to setup instantiation delegates for specified types? For example:

    // …
    GuiceComponentInjector injector = new GuiceComponentInjector(this);
    injector.addInstantiationStrategy(EntityManager.class, new InstantiationStrategy() {
    public EntityManager create() { return new EntityManager(…, …, …); }
    });
    addComponentInstantiationListener(injector);

    A bit of generic magic, and you should have a typesafe method for adding delegates. This will allow your dynamic proxy to just create an instance indirectly, rather than guessing on the default constructor. If you like, I can take a look and maybe submit a patch; I owe you that much. :-)

    Daniel Spiewak Thursday, January 3, 2008 at 8:31 am
  6. Crud, WP ate the generics. The second line was supposed to look like this:

    injector.addInstantiationStrategy(EntityManager.class, new InstantiationStrategy<EntityManager>() {

    Daniel Spiewak Thursday, January 3, 2008 at 8:33 am
  7. Ah, good point on the provider bit. My use of Guice has been limited to really the basic, high-level features (since that’s all you need to use the API for 90% of the cases). Using a provider really would be a good idea since it only makes sense to lazily recreate the EntityManager when absolutely necessary (but only if there is no pre-existing instance). I’m not sure if vanilla Guice providers satisfy this second condition, but they would certainly suffice for the “instantiation strategy” concept I outlined.

    Daniel Spiewak Thursday, January 3, 2008 at 8:36 am
  8. Good to see you guys calm down and sort things out in a constructive manner. (In JBoss-Seam-Land things like this end up in a month of nasty bashing).
    Yes, the provider is the obvious way to go – actually so obvious, i did not see that :) . Daniel, could you perhaps add a few lines to the wiki, please, so that everyone could use this workaround until the bug is buried?

    Uwe Schaefer Thursday, January 3, 2008 at 1:36 pm
  9. Why not annotate the EntityManager constructor with @Inject, and all its parameters with BindingAnnotation annotations (since they’re really configuration parameters)? In this case, instead of creating and binding an instance in the module (which is a Bad Thing), you can write:

    bindConstant().annotatedWith(EntityManagerUri.class).to(“jdbc:…”);
    bindConstant().annotatedWith(EntityManagerUsername.class).to(“username”);
    bindConstant().annotatedWith(EntityManagerPassword.class).to(“password”);
    bind(EntityManager.class).asEagerSingleton();

    Sikon Tuesday, September 23, 2008 at 3:34 am

Post a Comment

Comments are automatically formatted. Markup are either stripped or will cause large blocks of text to be eaten, depending on the phase of the moon. Code snippets should be wrapped in <pre>...</pre> tags. Indentation within pre tags will be preserved, and most instances of "<" and ">" will work without a problem.

Please note that first-time commenters are moderated, so don't panic if your comment doesn't appear immediately.

*
*