xiaodududu / google-guice

Automatically exported from code.google.com/p/google-guice
Apache License 2.0
0 stars 0 forks source link

GuiceServletContextListener.getInjector should have one param: servletContext #603

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
This way, configuration parameters can be passed as constructor parameter to 
object. For example, this is possible in normal Java application:

public static void main(String[] args) {
  String fooServerAddress = args[0];
  Injector injector = Guice.createInjector(new FooModule(fooServerAddress));
  FooClient client = injector.getInstance(FooClient.class);
  ...
}

In Servlet application this should became:

public class AppGuiceServletContextListener extends GuiceServletContextListener 
{
  @Override
  protected Injector getInjector(ServletContext servletContext) {
    String fooServerAddress = servletContext.getInitParameter("fooServerAddress");
    return Guice.createInjector(new FooModule(fooServerAddress));
  }
}

Check also comments on

http://code.google.com/p/google-guice/wiki/FrequentlyAskedQuestions

and thread on

http://groups.google.com/group/google-guice/browse_thread/thread/dad68e87fd94261
e

Original issue reported on code.google.com by gianmarco.gherardi on 16 Feb 2011 at 4:04

GoogleCodeExporter commented 9 years ago

Original comment by sberlin on 22 Feb 2011 at 1:46

GoogleCodeExporter commented 9 years ago
GuiceServletContextListener receives and then drops the ServletContextEvent, 
subclasses have to override ServletContextListener#contextInitialized to save a 
reference of the event or the ServletContext.

Original comment by rrrrutdk on 8 Mar 2011 at 7:24

GoogleCodeExporter commented 9 years ago
Yes, this is what i currently do, but this is just a workaround to me, not very 
elegant:

public class AppGuiceServletContextListener extends GuiceServletContextListener 
{
    private ServletContext servletContext;

    @Override
    public void contextInitialized(ServletContextEvent servletContextEvent) {
        servletContext = servletContextEvent.getServletContext();
        super.contextInitialized(servletContextEvent);
    }

    @Override
    protected Injector getInjector() {
        Integer applicationId = Integer.valueOf(servletContext.getInitParameter("applicationId"));
        return Guice.createInjector(new AppModule(applicationId));
    }
}

Original comment by gianmarco.gherardi on 8 Mar 2011 at 7:58

GoogleCodeExporter commented 9 years ago
Any updates on this?

Original comment by gianmarco.gherardi on 20 Feb 2012 at 9:34

GoogleCodeExporter commented 9 years ago
It would indeed be a much nicer API if getInjector took a ServletContext  
parameter. The existing workaround of overriding contextInitialized and caching 
the value is clumsy and unintuitive. It also places the burden on the subclass 
of ensuring that the methods are invoked in the expected order and behaving 
sensibly if they are not.

This is especially critical given that the without this, the obvious way of 
getting a ServletContext is to @Inject it into a module, which leads to a 
deprecation warning and the possibility of bugs due to it being injected into 
singleton contexts.

It would have been really easy to have included this parameter when the class 
was first written, now of course the problem is how to include it without 
breaking existing users. Perhaps the cleanest option would be:
1. make the existing getInjector method non-abstract and mark it as deprecated.
2. add a new getInjector(ServletContext)
3. if the old method returns null, call the new method.
4. after a couple of years, kill the old method.

E.g.

public abstract class GuiceServletContextListener
    implements ServletContextListener {

  static final String INJECTOR_NAME = Injector.class.getName();

  public void contextInitialized(ServletContextEvent servletContextEvent) {
    final ServletContext servletContext = servletContextEvent.getServletContext();

    // Set the Servletcontext early for those people who are using this class.
    // NOTE(dhanji): This use of the servletContext is deprecated.
    GuiceFilter.servletContext = new WeakReference<ServletContext>(servletContext);

    Injector injector = getInjector();
    if (injector == null) {
      injector = getInjector(servletContext);
    }
    injector.getInstance(InternalServletModule.BackwardsCompatibleServletContextProvider.class)
        .set(servletContext);
    servletContext.setAttribute(INJECTOR_NAME, injector);
  }

  public void contextDestroyed(ServletContextEvent servletContextEvent) {
    ServletContext servletContext = servletContextEvent.getServletContext();
    servletContext.removeAttribute(INJECTOR_NAME);
  }

  /**
   * This method will be removed soon, please use getInjector(ServletContext) instead.
   */
  @Deprecated
  protected Injector getInjector() {
    return null;
  }

  /**
   * Override this method to create (or otherwise obtain a reference to) your
   * injector.
   *
   * @param servletContext the ServletContext of the current servlet.
   */
  protected Injector getInjector(ServletContext context) {
    return null;
  }
}

This has the advantage that it exposes a cleaner API that makes it much easier 
to figure out where and how you should obtain the ServletContext whilst not 
breaking any existing users.

Original comment by r...@moozvine.com on 9 Jul 2014 at 9:28

GoogleCodeExporter commented 9 years ago
The code.google.com guice project has migrated to GitHub.  This issue site is 
no longer being used.  Please use https://github.com/google/guice/issues/603 
instead.

Original comment by sberlin on 9 Jul 2014 at 12:19