Merge branch 'maint-rel-1.6' of https://github.com/vivo-project/Vitro into maint-rel-1.6

webapp/src/edu/cornell/mannlib/vitro/webapp/freemarker/config/FreemarkerConfigurationImpl.java

@@ -3,6 +3,7 @@
 package edu.cornell.mannlib.vitro.webapp.freemarker.config;
 
 import java.io.IOException;
+import java.lang.ref.WeakReference;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -35,28 +36,52 @@ import freemarker.template.utility.DeepUnwrap;
  * Extend the Freemarker Configuration class to include some information that is
  * particular to the current request.
  * 
- * Takes advantage of the fact that each servlet request runs in a separate
- * thread. Stores the request-based information in a ThreadLocal. Override any
- * methods that should return that information instead of (or in addition to)
- * the common info.
+ * A reference to the current request is not always available to the Freemarker
+ * Configuration, so we take advantage of the fact that each request runs in a
+ * separate thread, and store a reference to that request in a ThreadLocal
+ * object.
+ * 
+ * Then, we override any methods that should return that request-based
+ * information instead of (or in addition to) the common info.
  * 
  * Only the getters are overridden, not the setters. So if you call
  * setAllSharedVariables(), for example, it will have no effect on the
  * request-based information.
+ * 
+ * Notice that the reference to the current request is actually stored through a
+ * WeakReference. This is because the ThreadLocal will not be cleared when the
+ * webapp is stopped, so none of the references from that ThreadLocal are
+ * eligible for garbage collection. If any of those references is an instance of
+ * a class that is loaded by the webapp, then the webapp ClassLoader is not
+ * eligible for garbage collection. This would be a huge memory leak.
+ * 
+ * Thanks to the WeakReference, the request is eligible for garbage collection
+ * if nothing else refers to it. In theory, this means that the WeakReference
+ * could return a null, but if the garbage collector has taken the request, then
+ * who is invoking this object?
  */
 public class FreemarkerConfigurationImpl extends Configuration {
 	private static final Log log = LogFactory
 			.getLog(FreemarkerConfigurationImpl.class);
 
-	private final ThreadLocal<RequestBasedInformation> rbiRef = new ThreadLocal<>();
+	private static final String ATTRIBUTE_NAME = RequestBasedInformation.class
+			.getName();
+
+	private final ThreadLocal<WeakReference<HttpServletRequest>> reqRef = new ThreadLocal<>();
 
 	void setRequestInfo(HttpServletRequest req) {
-		rbiRef.set(new RequestBasedInformation(req, this));
+		reqRef.set(new WeakReference<>(req));
+		req.setAttribute(ATTRIBUTE_NAME, new RequestBasedInformation(req, this));
+	}
+
+	private RequestBasedInformation getRequestInfo() {
+		HttpServletRequest req = reqRef.get().get();
+		return (RequestBasedInformation) req.getAttribute(ATTRIBUTE_NAME);
 	}
 
 	@Override
 	public Object getCustomAttribute(String name) {
-		Map<String, Object> attribs = rbiRef.get().getCustomAttributes();
+		Map<String, Object> attribs = getRequestInfo().getCustomAttributes();
 		if (attribs.containsKey(name)) {
 			return attribs.get(name);
 		} else {
@@ -66,13 +91,13 @@ public class FreemarkerConfigurationImpl extends Configuration {
 
 	@Override
 	public String[] getCustomAttributeNames() {
-		Set<String> rbiNames = rbiRef.get().getCustomAttributes().keySet();
+		Set<String> rbiNames = getRequestInfo().getCustomAttributes().keySet();
 		return joinNames(rbiNames, super.getCustomAttributeNames());
 	}
 
 	@Override
 	public TemplateModel getSharedVariable(String name) {
-		Map<String, TemplateModel> vars = rbiRef.get().getSharedVariables();
+		Map<String, TemplateModel> vars = getRequestInfo().getSharedVariables();
 		if (vars.containsKey(name)) {
 			return vars.get(name);
 		} else {
@@ -82,7 +107,7 @@ public class FreemarkerConfigurationImpl extends Configuration {
 
 	@Override
 	public Set<String> getSharedVariableNames() {
-		Set<String> rbiNames = rbiRef.get().getSharedVariables().keySet();
+		Set<String> rbiNames = getRequestInfo().getSharedVariables().keySet();
 
 		@SuppressWarnings("unchecked")
 		Set<String> superNames = super.getSharedVariableNames();
@@ -94,7 +119,7 @@ public class FreemarkerConfigurationImpl extends Configuration {
 
 	@Override
 	public Locale getLocale() {
-		return rbiRef.get().getReq().getLocale();
+		return getRequestInfo().getReq().getLocale();
 	}
 
 	private String[] joinNames(Set<String> nameSet, String[] nameArray) {

webapp/src/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettings.java

@@ -179,7 +179,7 @@ public class DeveloperSettings {
 	// The factory
 	// ----------------------------------------------------------------------
 
-	private static final String ATTRIBUTE_NAME = DeveloperSettings.class
+	protected static final String ATTRIBUTE_NAME = DeveloperSettings.class
 			.getName();
 
 	public static DeveloperSettings getBean(HttpServletRequest req) {
@@ -203,7 +203,7 @@ public class DeveloperSettings {
 
 	private final Map<Keys, Object> settings = new EnumMap<>(Keys.class);
 
-	private DeveloperSettings(ServletContext ctx) {
+	protected DeveloperSettings(ServletContext ctx) {
 		updateFromFile(ctx);
 	}
 

webapp/test/stubs/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettingsStub.java

@@ -0,0 +1,33 @@
+/* $This file is distributed under the terms of the license in /doc/license.txt$ */
+
+package stubs.edu.cornell.mannlib.vitro.webapp.utils.developer;
+
+import javax.servlet.ServletContext;
+
+import edu.cornell.mannlib.vitro.webapp.utils.developer.DeveloperSettings;
+
+/**
+ * Do everything that a standard DeveloperSettings would do, except loading from
+ * a properties file.
+ * 
+ * That way, we don't require ConfigurationProperties to find the Vitro home
+ * directory, so we don't throw errors if there is no ConfigurationProperties.
+ */
+public class DeveloperSettingsStub extends DeveloperSettings {
+	/**
+	 * Factory method. Create the stub and set it into the ServletContext.
+	 */
+	public static void set(ServletContext ctx) {
+		ctx.setAttribute(ATTRIBUTE_NAME, new DeveloperSettingsStub(ctx));
+	}
+
+	protected DeveloperSettingsStub(ServletContext ctx) {
+		super(ctx);
+	}
+
+	@Override
+	protected void updateFromFile(ServletContext ctx) {
+		// Don't bother.
+	}
+
+}

webapp/test/stubs/javax/servlet/ServletContextStub.java

@@ -21,6 +21,8 @@ import javax.servlet.ServletException;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
+import stubs.edu.cornell.mannlib.vitro.webapp.utils.developer.DeveloperSettingsStub;
+
 /**
  * A simple stand-in for the {@link ServletContext}, for use in unit tests.
  */
@@ -36,6 +38,11 @@ public class ServletContextStub implements ServletContext {
 	private final Map<String, String> mockResources = new HashMap<String, String>();
 	private final Map<String, String> realPaths = new HashMap<String, String>();
 
+	public ServletContextStub() {
+		// Assume that unit tests won't want to use Developer mode.
+		DeveloperSettingsStub.set(this);
+	}
+	
 	public void setContextPath(String contextPath) {
 		if (contextPath == null) {
 			throw new NullPointerException("contextPath may not be null.");

webapp/web/js/vitroUtils.js

@@ -13,20 +13,16 @@ $(document).ready(function(){
     jQuery('section#flash-message').css('display', 'none').fadeIn(1500);
     
     /////////////////////////////
-     // Home search fiter
+     // Home search filter
      // Toggle filter select list
      var $searchFilterList = $('#filter-search-nav');
      var $isFilterOpen = false;
 
-     console.log("Filter is open = " + $isFilterOpen);
-
      $('a.filter-search').click(function(e) {
          e.preventDefault();
 
          if (!$isFilterOpen) {
 
-            console.log("Filter is closed = " + $isFilterOpen);
-
              //Change button filter state to selected
              //$(this).css('background','url(../../themes/vivo-cornell/images/filteredSearchActive.gif) no-repeat right top');
              $(this).removeClass('filter-default');
@@ -37,7 +33,6 @@ $(document).ready(function(){
 
              $isFilterOpen = true;
 
-             console.log("open");
          } else {
              //Change button filter state to default
              //$('a.filter-search').css('background','url(../../themes/vivo-cornell/images/filteredSearch.gif) no-repeat right top');
@@ -49,7 +44,6 @@ $(document).ready(function(){
 
              $isFilterOpen = false;
 
-             console.log("closed");
          }
     });
 
@@ -63,7 +57,6 @@ $(document).ready(function(){
                   //Selected filter feedback
                   $('.search-filter-selected').text('');
                   $('input[name="classgroup"]').val('');
-                  console.log("ALL");
                } else {
 
                      $('.search-filter-selected').text($(this).text()).fadeIn('slow');
@@ -110,6 +103,5 @@ $(document).ready(function(){
 
              }
 
-           console.log("HIDE input value ") ;
        });
 });