From edfd05dd9c9be4af1c61b30924d10725f8a7ec55 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 19 Jan 2017 13:49:50 +1100 Subject: [PATCH] Issue #1268 Add unit tests, tweak log and error messages, fix bug. --- .../jetty/jsp/TestJspFileNameToClass.java | 58 ++++++++++++ .../org/eclipse/jetty/servlet/BaseHolder.java | 4 +- .../eclipse/jetty/servlet/ServletHolder.java | 40 ++++++-- .../jetty/servlet/ServletHolderTest.java | 91 ++++++++++++++++++- 4 files changed, 178 insertions(+), 15 deletions(-) create mode 100644 apache-jsp/src/test/java/org/eclipse/jetty/jsp/TestJspFileNameToClass.java diff --git a/apache-jsp/src/test/java/org/eclipse/jetty/jsp/TestJspFileNameToClass.java b/apache-jsp/src/test/java/org/eclipse/jetty/jsp/TestJspFileNameToClass.java new file mode 100644 index 00000000000..76c24a51d0e --- /dev/null +++ b/apache-jsp/src/test/java/org/eclipse/jetty/jsp/TestJspFileNameToClass.java @@ -0,0 +1,58 @@ +// +// ======================================================================== +// Copyright (c) 1995-2016 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.jsp; + +import org.eclipse.jetty.servlet.ServletHolder; +import org.junit.Assert; +import org.junit.Test; + +public class TestJspFileNameToClass +{ + + @Test + public void testJspFileNameToClassName() throws Exception + { + ServletHolder h = new ServletHolder(); + h.setName("test"); + + + Assert.assertEquals(null, h.getClassNameForJsp(null)); + + Assert.assertEquals(null, h.getClassNameForJsp("")); + + Assert.assertEquals(null, h.getClassNameForJsp("/blah/")); + + Assert.assertEquals(null, h.getClassNameForJsp("//blah///")); + + Assert.assertEquals(null, h.getClassNameForJsp("/a/b/c/blah/")); + + Assert.assertEquals("org.apache.jsp.a.b.c.blah", h.getClassNameForJsp("/a/b/c/blah")); + + Assert.assertEquals("org.apache.jsp.blah_jsp", h.getClassNameForJsp("/blah.jsp")); + + Assert.assertEquals("org.apache.jsp.blah_jsp", h.getClassNameForJsp("//blah.jsp")); + + Assert.assertEquals("org.apache.jsp.blah_jsp", h.getClassNameForJsp("blah.jsp")); + + Assert.assertEquals("org.apache.jsp.a.b.c.blah_jsp", h.getClassNameForJsp("/a/b/c/blah.jsp")); + + Assert.assertEquals("org.apache.jsp.a.b.c.blah_jsp", h.getClassNameForJsp("a/b/c/blah.jsp")); + } + +} diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/BaseHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/BaseHolder.java index 5fda9d14be0..d58cac8cf36 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/BaseHolder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/BaseHolder.java @@ -85,7 +85,7 @@ public abstract class BaseHolder extends AbstractLifeCycle implements Dumpabl { //if no class already loaded and no classname, make permanently unavailable if (_class==null && (_className==null || _className.equals(""))) - throw new UnavailableException("No class in holder"); + throw new UnavailableException("No class in holder "+toString()); //try to load class if (_class==null) @@ -99,7 +99,7 @@ public abstract class BaseHolder extends AbstractLifeCycle implements Dumpabl catch (Exception e) { LOG.warn(e); - throw new UnavailableException(e.getMessage()); + throw new UnavailableException("Class loading error for holder "+toString()); } } } diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java index f2baa0809ba..c985e0d7981 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java @@ -336,7 +336,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope if (jsp!=null) { if (LOG.isDebugEnabled()) - LOG.debug("JSP file {} for {} mapped to Servlet class {}",_forcedPath, getName(),jsp.getClassName()); + LOG.debug("JSP file {} for {} mapped to JspServlet class {}",_forcedPath, getName(),jsp.getClassName()); setClassName(jsp.getClassName()); //copy jsp init params that don't exist for this servlet for (Map.Entry entry:jsp.getInitParameters().entrySet()) @@ -352,6 +352,8 @@ public class ServletHolder extends Holder implements UserIdentity.Scope } } } + else + LOG.warn("Bad jsp-file {} conversion to classname in holder {}", _forcedPath, getName()); } @@ -922,7 +924,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope * @param jsp the jsp-file * @return the simple classname of the jsp */ - protected String getNameOfJspClass (String jsp) + public String getNameOfJspClass (String jsp) { if (StringUtil.isBlank(jsp)) return ""; //empty @@ -945,16 +947,18 @@ public class ServletHolder extends Holder implements UserIdentity.Scope catch (Exception e) { String tmp = jsp.replace('.','_'); - LOG.warn("Unable to make identifier for jsp "+jsp +" trying "+tmp+" instead"); if (LOG.isDebugEnabled()) + { + LOG.warn("JspUtil.makeJavaIdentifier failed for jsp "+jsp +" using "+tmp+" instead"); LOG.warn(e); + } return tmp; } } /* ------------------------------------------------------------ */ - protected String getPackageOfJspClass (String jsp) + public String getPackageOfJspClass (String jsp) { if (jsp == null) return ""; @@ -971,11 +975,23 @@ public class ServletHolder extends Holder implements UserIdentity.Scope } catch (Exception e) { - String tmp = jsp.substring(1,i).replace('/','.').trim(); + String tmp = jsp; + + //remove any leading slash + int s = 0; + if ('/' == (tmp.charAt(0))) + s = 1; + + //remove the element after last slash, which should be name of jsp + tmp = tmp.substring(s,i); + + tmp = tmp.replace('/','.').trim(); tmp = (".".equals(tmp)? "": tmp); - LOG.warn("Unable to make package for jsp "+jsp +" trying "+tmp+" instead"); if (LOG.isDebugEnabled()) + { + LOG.warn("JspUtil.makeJavaPackage failed for "+jsp +" using "+tmp+" instead"); LOG.warn(e); + } return tmp; } } @@ -985,9 +1001,13 @@ public class ServletHolder extends Holder implements UserIdentity.Scope /** * @return the package for all jsps */ - protected String getJspPackagePrefix () + public String getJspPackagePrefix () { - String jspPackageName = (String)getServletHandler().getServletContext().getInitParameter(JSP_GENERATED_PACKAGE_NAME ); + String jspPackageName = null; + + if (getServletHandler() != null && getServletHandler().getServletContext() != null) + jspPackageName = (String)getServletHandler().getServletContext().getInitParameter(JSP_GENERATED_PACKAGE_NAME ); + if (jspPackageName == null) jspPackageName = "org.apache.jsp"; @@ -1000,7 +1020,7 @@ public class ServletHolder extends Holder implements UserIdentity.Scope * @param jsp the jsp-file from web.xml * @return the fully qualified classname */ - protected String getClassNameForJsp (String jsp) + public String getClassNameForJsp (String jsp) { if (jsp == null) return null; @@ -1281,6 +1301,6 @@ public class ServletHolder extends Holder implements UserIdentity.Scope @Override public String toString() { - return String.format("%s@%x==%s,%d,%b",_name,hashCode(),_className,_initOrder,_servlet!=null); + return String.format("%s@%x==%s,jsp=%s,order=%d,inst=%b",_name,hashCode(),_className,_forcedPath,_initOrder,_servlet!=null); } } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java index eb24406d332..4cfb007b9e7 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHolderTest.java @@ -18,12 +18,14 @@ package org.eclipse.jetty.servlet; +import javax.servlet.UnavailableException; + +import org.eclipse.jetty.server.handler.ContextHandler; +import org.eclipse.jetty.util.MultiException; +import org.eclipse.jetty.util.log.StacklessLogging; import org.junit.Assert; import org.junit.Test; -import java.util.Arrays; -import java.util.Collections; - public class ServletHolderTest { @Test @@ -52,4 +54,87 @@ public class ServletHolderTest { Assert.assertTrue(two.compareTo(three) < 0); Assert.assertTrue(one.compareTo(three) < 0); } + + + @Test + public void testJspFileNameToClassName() throws Exception + { + ServletHolder h = new ServletHolder(); + h.setName("test"); + + + Assert.assertEquals(null, h.getClassNameForJsp(null)); + + Assert.assertEquals(null, h.getClassNameForJsp("")); + + Assert.assertEquals(null, h.getClassNameForJsp("/blah/")); + + Assert.assertEquals(null, h.getClassNameForJsp("//blah///")); + + Assert.assertEquals(null, h.getClassNameForJsp("/a/b/c/blah/")); + + Assert.assertEquals("org.apache.jsp.a.b.c.blah", h.getClassNameForJsp("/a/b/c/blah")); + + Assert.assertEquals("org.apache.jsp.blah_jsp", h.getClassNameForJsp("/blah.jsp")); + + Assert.assertEquals("org.apache.jsp.blah_jsp", h.getClassNameForJsp("//blah.jsp")); + + Assert.assertEquals("org.apache.jsp.blah_jsp", h.getClassNameForJsp("blah.jsp")); + + Assert.assertEquals("org.apache.jsp.a.b.c.blah_jsp", h.getClassNameForJsp("/a/b/c/blah.jsp")); + + Assert.assertEquals("org.apache.jsp.a.b.c.blah_jsp", h.getClassNameForJsp("a/b/c/blah.jsp")); + } + + + @Test + public void testNoClassName() throws Exception + { + try (StacklessLogging stackless = new StacklessLogging(ServletHandler.class, ContextHandler.class, ServletContextHandler.class)) + { + ServletContextHandler context = new ServletContextHandler(); + ServletHandler handler = context.getServletHandler(); + ServletHolder holder = new ServletHolder(); + holder.setName("foo"); + holder.setForcedPath("/blah/"); + handler.addServlet(holder); + handler.start(); + Assert.fail("No class in ServletHolder"); + } + catch (UnavailableException e) + { + Assert.assertTrue(e.getMessage().contains("foo")); + } + catch (MultiException e) + { + MultiException m = (MultiException)e; + Assert.assertTrue(m.getCause().getMessage().contains("foo")); + } + } + + @Test + public void testUnloadableClassName() throws Exception + { + try (StacklessLogging stackless = new StacklessLogging(BaseHolder.class, ServletHandler.class, ContextHandler.class, ServletContextHandler.class)) + { + ServletContextHandler context = new ServletContextHandler(); + ServletHandler handler = context.getServletHandler(); + ServletHolder holder = new ServletHolder(); + holder.setName("foo"); + holder.setClassName("a.b.c.class"); + handler.addServlet(holder); + handler.start(); + Assert.fail("Unloadable class"); + } + catch (UnavailableException e) + { + Assert.assertTrue(e.getMessage().contains("foo")); + } + catch (MultiException e) + { + MultiException m = (MultiException)e; + Assert.assertTrue(m.getCause().getMessage().contains("foo")); + } + } + }