Inconsistent documentation and implementation for ServletModule

[GoogleCodeExporter]

This is a follow-up of
http://groups.google.com/group/google-guice/browse_thread/thread/bcb8f9c4d35cfe6
b

ServletModule.configureServlets()'s Javadoc reads "Every servlet is
required to be a singleton and will implicitly be bound as one if it isn't
already."

1) This conflicts with the actual implementation. Servlets aren't being
bound to singleton scope by default.

2) This should be consist across both servlets and filters. That is, the
documentation should state the same thing for filters.

3) This conflicts with what
http://code.google.com/p/google-guice/wiki/ServletModule says "Note: Every
servlet (or filter) is required to be a @Singleton. If you cannot annotate
the class directly, you must bind it separtely using
bind(..).in(Singleton.class), separate to the filter() or servlet() rules."

Personally, I prefer the approach of #1. Servlets and filters should be
bound as singletons unless the user specifies otherwise (which will trigger
an error since this is illegal).

Original issue reported on code.google.com by gili.tza...@gmail.com on 13 Feb 2009 at 6:19

[GoogleCodeExporter]

Since we don't have conditional bindings in Guice this is quite hard to 
accomplish without a major feature 
addition to Guice core.

I think annotating with @Singleton or a @provides method is an acceptable 
compromise.

The javadoc is out of date, I will fix it. Thanks for pointing it out.

Original comment by dha...@gmail.com on 14 Feb 2009 at 12:28

[GoogleCodeExporter]

Dhanji, keep in mind that a lot of the time we cannot touch the servlet class 
to add
@Singleton. There absolutely must be a way to get this working outside of the
servlet/filter class and ideally it should require the minimum amount of work 
possible.

If you insist on going down this road then I need to ask, why can't we define 
the
servlet scope directly in ServletModule? Why do we need to bind the servlet in
ServletModule and its scope in a separate Module?

Thirdly, what is the @Provides annotation you referred to? I don't see it in 
the Javadoc.

Original comment by gili.tza...@gmail.com on 14 Feb 2009 at 12:43

[GoogleCodeExporter]

Yea, it's for that case that I'm suggesting using a @Provides method. This is a 
very low overhead way of creating 
and binding sealed classes:
http://code.google.com/docreader/#p=google-guice&s=google-guice&t=Changes20

You certainly can define the scope in the ServletModule, if you like--you just 
need an additional bind() rule near 
the filter() rule, that's all. A separate module is not necessary.

Original comment by dha...@gmail.com on 14 Feb 2009 at 1:16

[GoogleCodeExporter]

I think both of you have a point. On one hand, the "common case" should be 
straightforward, on the other 
hand it would be very tricky to implement (conflicts with multiple 
ServletModules).

I wonder if this can't be solved using the SPI. Find all the web artifacts and 
generate a module that adds the 
missing bindings. Ideally this would ship with Guice Servlet because it would 
depend on its internals (the 
bindings it generates). You'd still have to install an extra module, but maybe 
it eases the pain?

Another option would be to ship different ServletModules; one for single module 
usage, and one for multi-
module usage. The single module one would bind automagically, the multimodule 
one would need the SPI 
utility. But, all in all this also complicates the API and could be confusing.

The real answer is that we have yet to discover a good pattern that keeps the 
"multiple modules" flexibility, 
but also retains central control over the bindings that get generated (avoiding 
duplicates). I also noticed this 
while working on Warp Persist 2.0. The trunk currently uses the SPI idea for 
this utility module (potentially 
needed for UnitOfWork.TRANSACTION in a non-web environment):
http://code.google.com/p/warp-persist/source/browse/trunk/warp-
persist/src/com/wideplay/warp/persist/PersistenceServiceExtrasModule.java

Original comment by robbie.v...@gmail.com on 14 Feb 2009 at 1:18

[GoogleCodeExporter]

I don't see any problem with explicitly binding the scope (or annotating the 
class directly) and with multiple 
ServletModules.

We are talking about 1 extra line of code for sealed classes and or an 
annotation for the common case. Do we 
really need to be doing all this to avoid that? And to lose the explicit 
documentation that these are singletons..

Why do you need an extra module? ServletModule is like any other Guice module 
and supports anything they do.

Original comment by dha...@gmail.com on 14 Feb 2009 at 1:34

[GoogleCodeExporter]

Dhanji,

I think the API would be clearer if you could bind the scope directly off the
servlet() or filter() method call. For example:
filter("/*").through(PersistenceFilter.class).in(Scopes.SINGLETON)

I only mention this because the rest of Guice works this way so I feel it would 
be
more consistent. Alternatively, discuss this explicitly in the Javadoc so users 
know
to use bind() -- I personally never thought of it.

Robbie,

I must be missing something. Doesn't Guice throw an error if the user binds the 
same
type multiple times (even across different modules)? I say we let the user bind 
the
servlet/filter once and if he tries binding them elsewhere we throw an error. I
recall Guice already throwing this sort of error for other classes which is why 
I
believe this is already implemented.

Original comment by gili.tza...@gmail.com on 14 Feb 2009 at 1:40

[GoogleCodeExporter]

Dhanji,

As you well know, good APIs "don't make the user do any work you can do on 
his/her
behalf". If we can do this then I really think we should. The complexity belongs
under the hood, not outside it :)

Original comment by gili.tza...@gmail.com on 14 Feb 2009 at 1:44

[GoogleCodeExporter]

The extra module idea was just that you could have one module type that could 
automatically install the 
missing bindings, and one module type that wouldn't. But like I said I also 
don't like this option, I'm just 
writing down some of my reasoning.

Other than that, I agree that this is not really a problem; it's just that it 
seems like something we'll be doing 
over and over. Be it the binding, or the @Singleton on the Servlet/Filter.

Now that I think about it, the best option would probably be to integrate it 
into the DSL somehow.

For example, the default behaviour would be to bind it on the spot:
// no Key overloads
serve("/my/*").with(MyServlet.class);
filter("/*").through(MyFilter.class);

// Key, class overloads
If people want to customize, they can use:
serve("/my/*").withBinding(MyServlet.class);
filter("/*").throughBinding(MyFilter.class);

Original comment by robbie.v...@gmail.com on 14 Feb 2009 at 1:51

[GoogleCodeExporter]

@gili

I have changed the javadoc accordingly, this morning. Note that our public doc 
has already had this info for 
some time now:
http://code.google.com/p/google-guice/wiki/Servlets

Jesse and I put a lot of thought into this and I believe our API is very 
self-consistent, consider the case when 
you bind an interface to a key:

bind(A.class).in(Singleton.class);
bind(Interface.class).to(A.class); //implicitly a singleton

This is how you should look at the filter()/bind() combination =)

I think good APIs are also consistent and concise. In the 80% case you will be 
able to annotate the class 
directly, which is a clean and self-documenting solution. In the 20% case, the 
explicit bind() is simple and 
effective. 

Original comment by dha...@gmail.com on 14 Feb 2009 at 1:59

[GoogleCodeExporter]

We have a hard requirement for singletons. We may want to investigate 
specifying servlet instances - I've opened 
issue 361 to track that.

Original comment by limpbizkit on 26 Apr 2009 at 9:11

• Changed state: Invalid
• Added labels: Extension-Servlet