From a8a9dbbc0892c807e5893127972089316674801d Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Tue, 17 Mar 2020 11:47:15 +0100 Subject: [PATCH] Issue #4644 Allow jetty-env EnvEntry to override empty env-entry (#4660) * Issue #4644 Allow jetty-env EnvEntry to override empty env-entry Signed-off-by: Jan Bartel --- .../plus/webapp/PlusDescriptorProcessor.java | 92 ++++++---- .../webapp/PlusDescriptorProcessorTest.java | 158 +++++++++++++++++- .../jetty/plus/webapp/TestConfiguration.java | 136 --------------- jetty-plus/src/test/resources/web.xml | 66 +++++++- 4 files changed, 269 insertions(+), 183 deletions(-) delete mode 100644 jetty-plus/src/test/java/org/eclipse/jetty/plus/webapp/TestConfiguration.java diff --git a/jetty-plus/src/main/java/org/eclipse/jetty/plus/webapp/PlusDescriptorProcessor.java b/jetty-plus/src/main/java/org/eclipse/jetty/plus/webapp/PlusDescriptorProcessor.java index 6489dbaa24b..410a61cba73 100644 --- a/jetty-plus/src/main/java/org/eclipse/jetty/plus/webapp/PlusDescriptorProcessor.java +++ b/jetty-plus/src/main/java/org/eclipse/jetty/plus/webapp/PlusDescriptorProcessor.java @@ -19,9 +19,12 @@ package org.eclipse.jetty.plus.webapp; import java.util.Iterator; +import java.util.Objects; + import javax.naming.Context; import javax.naming.InitialContext; import javax.naming.NameNotFoundException; +import javax.naming.NamingException; import org.eclipse.jetty.jndi.NamingUtil; import org.eclipse.jetty.plus.annotation.Injection; @@ -35,6 +38,7 @@ import org.eclipse.jetty.plus.jndi.EnvEntry; import org.eclipse.jetty.plus.jndi.Link; import org.eclipse.jetty.plus.jndi.NamingEntry; import org.eclipse.jetty.plus.jndi.NamingEntryUtil; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -121,14 +125,6 @@ public class PlusDescriptorProcessor extends IterativeDescriptorProcessor String type = node.getString("env-entry-type", false, true); String valueStr = node.getString("env-entry-value", false, true); - //if there's no value there's no point in making a jndi entry - //nor processing injection entries - if (valueStr == null || valueStr.equals("")) - { - LOG.warn("No value for env-entry-name " + name); - return; - } - Origin o = context.getMetaData().getOrigin("env-entry." + name); switch (o) { @@ -136,14 +132,7 @@ public class PlusDescriptorProcessor extends IterativeDescriptorProcessor { //no descriptor has configured an env-entry of this name previously context.getMetaData().setOrigin("env-entry." + name, descriptor); - //the javaee_5.xsd says that the env-entry-type is optional - //if there is an element, because you can get - //type from the element, but what to do if there is more - //than one element, do you just pick the type - //of the first one? - addInjections(context, descriptor, node, name, TypeUtil.fromName(type)); - Object value = TypeUtil.valueOf(type, valueStr); - bindEnvEntry(name, value); + makeEnvEntryInjectionsAndBindings(context, descriptor, node, name, type, valueStr); break; } case WebXml: @@ -158,9 +147,7 @@ public class PlusDescriptorProcessor extends IterativeDescriptorProcessor //We're processing web-defaults, web.xml or web-override. Any of them can //set or change the env-entry. context.getMetaData().setOrigin("env-entry." + name, descriptor); - addInjections(context, descriptor, node, name, TypeUtil.fromName(type)); - Object value = TypeUtil.valueOf(type, valueStr); - bindEnvEntry(name, value); + makeEnvEntryInjectionsAndBindings(context, descriptor, node, name, type, valueStr); } else { @@ -705,6 +692,9 @@ public class PlusDescriptorProcessor extends IterativeDescriptorProcessor */ public void addInjections(WebAppContext context, Descriptor descriptor, XmlParser.Node node, String jndiName, Class valueClass) { + Objects.requireNonNull(context); + Objects.requireNonNull(node); + Iterator itor = node.iterator("injection-target"); while (itor.hasNext()) @@ -755,31 +745,59 @@ public class PlusDescriptorProcessor extends IterativeDescriptorProcessor */ public void bindEnvEntry(String name, Object value) throws Exception { - InitialContext ic = null; - boolean bound = false; - //check to see if we bound a value and an EnvEntry with this name already - //when we processed the server and the webapp's naming environment - //@see EnvConfiguration.bindEnvEntries() - ic = new InitialContext(); + InitialContext ic = new InitialContext(); + Context envCtx = (Context)ic.lookup("java:comp/env"); + NamingUtil.bind(envCtx, name, value); + } + + /** + * Make injections and any java:comp/env bindings necessary given an env-entry declaration. + * The handling of env-entries is different to other resource declarations like resource-ref, resource-env-ref etc + * because we allow the EnvEntry (@see org.eclipse.jetty.plus.jndi.EnvEntry) class that is configured externally to the webapp + * to specify a value that can override a value present in a web.xml descriptor. + * + * @param context the WebAppContext of the env-entry + * @param descriptor the web.xml, web-default.xml, web-override.xml or web-fragment.xml + * @param node the parsed xml representation of the env-entry declaration + * @param name the name field of the env-entry + * @param type the type field of the env-entry + * @param value the value field of the env-entry + * @throws Exception + */ + public void makeEnvEntryInjectionsAndBindings(WebAppContext context, Descriptor descriptor, XmlParser.Node node, String name, String type, String value) throws Exception + { + InitialContext ic = new InitialContext(); try { - NamingEntry ne = (NamingEntry)ic.lookup("java:comp/env/" + NamingEntryUtil.makeNamingEntryName(ic.getNameParser(""), name)); - if (ne != null && ne instanceof EnvEntry) + EnvEntry envEntry = (EnvEntry)ic.lookup("java:comp/env/" + NamingEntryUtil.makeNamingEntryName(ic.getNameParser(""), name)); + + if (StringUtil.isEmpty(value)) { - EnvEntry ee = (EnvEntry)ne; - bound = ee.isOverrideWebXml(); + //There is an empty or missing value in the env-entry: + //If there is an existing EnvEntry (eg from a declaration in jetty-env.xml) that is override=true, then + //we make the injection, but we can skip the rebinding becase we want to use the value already bound. + //If there isn't an existing EnvEntry then there is nothing to do: according to the spec we do not make + //an injection if the env-entry value is missing, and of course there is no value to rebind. + if (envEntry != null && envEntry.isOverrideWebXml()) + addInjections(context, descriptor, node, name, TypeUtil.fromName(type)); + } + else + { + //There is a value for the env-entry: + //According to the spec, we need to make an injection (if one is present). + //If there is an existing EnvEntry(eg from a declaration in jetty-env.xml) that is override=false, then + //we need to rebind name with the value from web.xml. + addInjections(context, descriptor, node, name, TypeUtil.fromName(type)); + + if (envEntry == null || !envEntry.isOverrideWebXml()) + bindEnvEntry(name, TypeUtil.valueOf(type, value)); } } catch (NameNotFoundException e) { - bound = false; - } - - if (!bound) - { - //either nothing was bound or the value from web.xml should override - Context envCtx = (Context)ic.lookup("java:comp/env"); - NamingUtil.bind(envCtx, name, value); + //No matching EnvEntry has previously been bound so make the injection and do the binding with the value from web.xml + addInjections(context, descriptor, node, name, TypeUtil.fromName(type)); + bindEnvEntry(name, TypeUtil.valueOf(type, value)); } } diff --git a/jetty-plus/src/test/java/org/eclipse/jetty/plus/webapp/PlusDescriptorProcessorTest.java b/jetty-plus/src/test/java/org/eclipse/jetty/plus/webapp/PlusDescriptorProcessorTest.java index 550b50ce947..edc2bd41d9d 100644 --- a/jetty-plus/src/test/java/org/eclipse/jetty/plus/webapp/PlusDescriptorProcessorTest.java +++ b/jetty-plus/src/test/java/org/eclipse/jetty/plus/webapp/PlusDescriptorProcessorTest.java @@ -22,7 +22,14 @@ import java.lang.reflect.InvocationTargetException; import java.net.URL; import javax.naming.Context; import javax.naming.InitialContext; +import javax.naming.Name; +import org.eclipse.jetty.jndi.NamingUtil; +import org.eclipse.jetty.plus.annotation.Injection; +import org.eclipse.jetty.plus.annotation.InjectionCollection; +import org.eclipse.jetty.plus.jndi.EnvEntry; +import org.eclipse.jetty.plus.jndi.NamingEntryUtil; +import org.eclipse.jetty.util.IntrospectionUtil; import org.eclipse.jetty.webapp.Descriptor; import org.eclipse.jetty.webapp.FragmentDescriptor; import org.eclipse.jetty.webapp.Origin; @@ -39,6 +46,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -48,6 +56,7 @@ import static org.junit.jupiter.api.Assertions.fail; */ public class PlusDescriptorProcessorTest { + protected static final Class[] STRING_ARG = new Class[]{String.class}; protected WebDescriptor webDescriptor; protected FragmentDescriptor fragDescriptor1; protected FragmentDescriptor fragDescriptor2; @@ -55,19 +64,95 @@ public class PlusDescriptorProcessorTest protected FragmentDescriptor fragDescriptor4; protected WebAppContext context; + public static class TestInjections + { + private String foo; + private String bah; + private String empty; + private String vacuum; + private String webXmlOnly; + + public String getWebXmlOnly() + { + return webXmlOnly; + } + + public void setWebXmlOnly(String webXmlOnly) + { + this.webXmlOnly = webXmlOnly; + } + + public String getVacuum() + { + return vacuum; + } + + public void setVacuum(String val) + { + vacuum = val; + } + + public String getEmpty() + { + return empty; + } + + public void setEmpty(String val) + { + empty = val; + } + + public void setFoo(String val) + { + foo = val; + } + + public String getFoo() + { + return foo; + } + + public String getBah() + { + return bah; + } + + public void setBah(String val) + { + bah = val; + } + } + @BeforeEach public void setUp() throws Exception { context = new WebAppContext(); context.setClassLoader(new WebAppClassLoader(Thread.currentThread().getContextClassLoader(), context)); + context.getServerClasspathPattern().add("-org.eclipse.jetty.plus.webapp."); //need visbility of the TestInjections class ClassLoader oldLoader = Thread.currentThread().getContextClassLoader(); Thread.currentThread().setContextClassLoader(context.getClassLoader()); Context icontext = new InitialContext(); Context compCtx = (Context)icontext.lookup("java:comp"); - compCtx.createSubcontext("env"); - Thread.currentThread().setContextClassLoader(oldLoader); + Context envCtx = compCtx.createSubcontext("env"); + @SuppressWarnings("unused") org.eclipse.jetty.plus.jndi.Resource ds = new org.eclipse.jetty.plus.jndi.Resource(context, "jdbc/mydatasource", new Object()); + + //An EnvEntry that should override any value supplied in a web.xml file + org.eclipse.jetty.plus.jndi.EnvEntry fooStringEnvEntry = new org.eclipse.jetty.plus.jndi.EnvEntry("foo", "FOO", true); + doEnvConfiguration(envCtx, fooStringEnvEntry); + + //An EnvEntry that should NOT override any value supplied in a web.xml file + org.eclipse.jetty.plus.jndi.EnvEntry bahStringEnvEntry = new org.eclipse.jetty.plus.jndi.EnvEntry("bah", "BAH", false); + doEnvConfiguration(envCtx, bahStringEnvEntry); + + //An EnvEntry that will override an empty value in web.xml + org.eclipse.jetty.plus.jndi.EnvEntry emptyStringEnvEntry = new org.eclipse.jetty.plus.jndi.EnvEntry("empty", "EMPTY", true); + doEnvConfiguration(envCtx, emptyStringEnvEntry); + + //An EnvEntry that will NOT override an empty value in web.xml + org.eclipse.jetty.plus.jndi.EnvEntry vacuumStringEnvEntry = new org.eclipse.jetty.plus.jndi.EnvEntry("vacuum", "VACUUM", false); + doEnvConfiguration(envCtx, vacuumStringEnvEntry); URL webXml = Thread.currentThread().getContextClassLoader().getResource("web.xml"); webDescriptor = new WebDescriptor(org.eclipse.jetty.util.resource.Resource.newResource(webXml)); @@ -85,6 +170,22 @@ public class PlusDescriptorProcessorTest URL frag4Xml = Thread.currentThread().getContextClassLoader().getResource("web-fragment-4.xml"); fragDescriptor4 = new FragmentDescriptor(org.eclipse.jetty.util.resource.Resource.newResource(frag4Xml)); fragDescriptor4.parse(); + + Thread.currentThread().setContextClassLoader(oldLoader); + } + + /** + * Do the kind of processing that EnvConfiguration would do. + * + * @param envCtx the java:comp/env context + * @param envEntry the EnvEntry + * @throws Exception + */ + private void doEnvConfiguration(Context envCtx, EnvEntry envEntry) throws Exception + { + envEntry.bindToENC(envEntry.getJndiName()); + Name namingEntryName = NamingEntryUtil.makeNamingEntryName(null, envEntry); + NamingUtil.bind(envCtx, namingEntryName.toString(), envEntry); } @AfterEach @@ -140,6 +241,59 @@ public class PlusDescriptorProcessorTest Thread.currentThread().setContextClassLoader(oldLoader); } } + + @Test + public void testEnvEntries() throws Exception + { + ClassLoader oldLoader = Thread.currentThread().getContextClassLoader(); + Thread.currentThread().setContextClassLoader(context.getClassLoader()); + try + { + PlusDescriptorProcessor pdp = new PlusDescriptorProcessor(); + //process web.xml + pdp.process(context, webDescriptor); + InjectionCollection injections = (InjectionCollection)context.getAttribute(InjectionCollection.INJECTION_COLLECTION); + assertNotNull(injections); + + //check that there is an injection for "foo" with the value from the overriding EnvEntry of "FOO" + Injection foo = injections.getInjection("foo", TestInjections.class, + IntrospectionUtil.findMethod(TestInjections.class, "setFoo", STRING_ARG, false, true), + String.class); + assertNotNull(foo); + assertEquals("FOO", foo.lookupInjectedValue()); + + //check that there is an injection for "bah" with the value from web.xml of "beer" + Injection bah = injections.getInjection("bah", TestInjections.class, + IntrospectionUtil.findMethod(TestInjections.class, "setBah", STRING_ARG, false, true), + String.class); + assertNotNull(bah); + assertEquals("beer", bah.lookupInjectedValue()); + + //check that there is an injection for "empty" with the value from the overriding EnvEntry of "EMPTY" + Injection empty = injections.getInjection("empty", TestInjections.class, + IntrospectionUtil.findMethod(TestInjections.class, "setEmpty", STRING_ARG, false, true), + String.class); + assertNotNull(empty); + assertEquals("EMPTY", empty.lookupInjectedValue()); + + //check that there is NOT an injection for "vacuum" + Injection vacuum = injections.getInjection("vacuum", TestInjections.class, + IntrospectionUtil.findMethod(TestInjections.class, "setVacuum", STRING_ARG, false, true), + String.class); + assertNull(vacuum); + + //check that there is an injection for "webxmlonly" with the value from web.xml of "WEBXMLONLY" + Injection webXmlOnly = injections.getInjection("webxmlonly", TestInjections.class, + IntrospectionUtil.findMethod(TestInjections.class, "setWebXmlOnly", STRING_ARG, false, true), + String.class); + assertNotNull(webXmlOnly); + assertEquals("WEBXMLONLY", webXmlOnly.lookupInjectedValue()); + } + finally + { + Thread.currentThread().setContextClassLoader(oldLoader); + } + } @Test public void testMismatchedFragmentResourceDeclarations() diff --git a/jetty-plus/src/test/java/org/eclipse/jetty/plus/webapp/TestConfiguration.java b/jetty-plus/src/test/java/org/eclipse/jetty/plus/webapp/TestConfiguration.java deleted file mode 100644 index e4a09a62f4d..00000000000 --- a/jetty-plus/src/test/java/org/eclipse/jetty/plus/webapp/TestConfiguration.java +++ /dev/null @@ -1,136 +0,0 @@ -// -// ======================================================================== -// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. -// ------------------------------------------------------------------------ -// 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.plus.webapp; - -import javax.naming.Context; -import javax.naming.InitialContext; - -import org.eclipse.jetty.plus.jndi.EnvEntry; -import org.eclipse.jetty.plus.jndi.NamingEntry; -import org.eclipse.jetty.plus.jndi.NamingEntryUtil; -import org.eclipse.jetty.server.Server; -import org.eclipse.jetty.webapp.MetaData; -import org.eclipse.jetty.webapp.WebAppClassLoader; -import org.eclipse.jetty.webapp.WebAppContext; -import org.junit.jupiter.api.Test; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; - -public class TestConfiguration -{ - @Test - public void testIt() throws Exception - { - ClassLoader oldLoader = Thread.currentThread().getContextClassLoader(); - - try - { - InitialContext ic = new InitialContext(); - - Server server = new Server(); - - WebAppContext wac = new WebAppContext(); - wac.setServer(server); - wac.setClassLoader(new WebAppClassLoader(Thread.currentThread().getContextClassLoader(), wac)); - - MetaData metaData = new MetaData(); - - PlusDescriptorProcessor plusProcessor = new PlusDescriptorProcessor(); - - //bind some EnvEntrys at the server level - EnvEntry ee1 = new EnvEntry(server, "xxx/a", "100", true); - EnvEntry ee2 = new EnvEntry(server, "yyy/b", "200", false); - EnvEntry ee3 = new EnvEntry(server, "zzz/c", "300", false); - EnvEntry ee4 = new EnvEntry(server, "zzz/d", "400", false); - EnvEntry ee5 = new EnvEntry(server, "zzz/f", "500", true); - - //bind some EnvEntrys at the webapp level - EnvEntry ee6 = new EnvEntry(wac, "xxx/a", "900", true); - EnvEntry ee7 = new EnvEntry(wac, "yyy/b", "910", true); - EnvEntry ee8 = new EnvEntry(wac, "zzz/c", "920", false); - EnvEntry ee9 = new EnvEntry(wac, "zzz/e", "930", false); - - assertNotNull(NamingEntryUtil.lookupNamingEntry(server, "xxx/a")); - assertNotNull(NamingEntryUtil.lookupNamingEntry(server, "yyy/b")); - assertNotNull(NamingEntryUtil.lookupNamingEntry(server, "zzz/c")); - assertNotNull(NamingEntryUtil.lookupNamingEntry(server, "zzz/d")); - assertNotNull(NamingEntryUtil.lookupNamingEntry(wac, "xxx/a")); - assertNotNull(NamingEntryUtil.lookupNamingEntry(wac, "yyy/b")); - assertNotNull(NamingEntryUtil.lookupNamingEntry(wac, "zzz/c")); - assertNotNull(NamingEntryUtil.lookupNamingEntry(wac, "zzz/e")); - - //make a new env configuration - EnvConfiguration envConfig = new EnvConfiguration(); - - Thread.currentThread().setContextClassLoader(wac.getClassLoader()); - MetaData metadata = new MetaData(); - envConfig.preConfigure(wac); - envConfig.configure(wac); - envConfig.bindEnvEntries(wac); - - String val = (String)ic.lookup("java:comp/env/xxx/a"); - assertEquals("900", val); //webapp naming overrides server - val = (String)ic.lookup("java:comp/env/yyy/b"); - assertEquals("910", val);//webapp overrides server - val = (String)ic.lookup("java:comp/env/zzz/c"); - assertEquals("920", val);//webapp overrides server - val = (String)ic.lookup("java:comp/env/zzz/d"); - assertEquals("400", val);//from server naming - val = (String)ic.lookup("java:comp/env/zzz/e"); - assertEquals("930", val);//from webapp naming - - NamingEntry ne = (NamingEntry)ic.lookup("java:comp/env/" + NamingEntry.__contextName + "/xxx/a"); - assertNotNull(ne); - ne = (NamingEntry)ic.lookup("java:comp/env/" + NamingEntry.__contextName + "/yyy/b"); - assertNotNull(ne); - ne = (NamingEntry)ic.lookup("java:comp/env/" + NamingEntry.__contextName + "/zzz/c"); - assertNotNull(ne); - ne = (NamingEntry)ic.lookup("java:comp/env/" + NamingEntry.__contextName + "/zzz/d"); - assertNotNull(ne); - ne = (NamingEntry)ic.lookup("java:comp/env/" + NamingEntry.__contextName + "/zzz/e"); - assertNotNull(ne); - - plusProcessor.bindEnvEntry("foo", "99"); - assertEquals("99", ic.lookup("java:comp/env/foo")); - - plusProcessor.bindEnvEntry("xxx/a", "7"); - assertEquals("900", ic.lookup("java:comp/env/xxx/a")); //webapp overrides web.xml - plusProcessor.bindEnvEntry("yyy/b", "7"); - assertEquals("910", ic.lookup("java:comp/env/yyy/b"));//webapp overrides web.xml - plusProcessor.bindEnvEntry("zzz/c", "7"); - assertEquals("7", ic.lookup("java:comp/env/zzz/c"));//webapp does NOT override web.xml - plusProcessor.bindEnvEntry("zzz/d", "7"); - assertEquals("7", ic.lookup("java:comp/env/zzz/d"));//server does NOT override web.xml - plusProcessor.bindEnvEntry("zzz/e", "7"); - assertEquals("7", ic.lookup("java:comp/env/zzz/e"));//webapp does NOT override web.xml - plusProcessor.bindEnvEntry("zzz/f", "7"); - assertEquals("500", ic.lookup("java:comp/env/zzz/f"));//server overrides web.xml - - ((Context)ic.lookup("java:comp")).destroySubcontext("env"); - ic.destroySubcontext("xxx"); - ic.destroySubcontext("yyy"); - ic.destroySubcontext("zzz"); - } - finally - { - Thread.currentThread().setContextClassLoader(oldLoader); - } - } -} diff --git a/jetty-plus/src/test/resources/web.xml b/jetty-plus/src/test/resources/web.xml index f64129952ff..f058465baf4 100644 --- a/jetty-plus/src/test/resources/web.xml +++ b/jetty-plus/src/test/resources/web.xml @@ -4,20 +4,70 @@ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://java.sun.com/xml/ns/javaee http://java.sun.com/xml/ns/javaee/web-app_3_0.xsd" metadata-complete="false" - version="3.0"> + version="3.0"> - Test WebApp + Test WebApp jdbc/mydatasource javax.sql.DataSource Container - + + + + foo + java.lang.String + beer + + org.eclipse.jetty.plus.webapp.PlusDescriptorProcessorTest$TestInjections + foo + + + + + + bah + java.lang.String + beer + + org.eclipse.jetty.plus.webapp.PlusDescriptorProcessorTest$TestInjections + bah + + + + + + empty + java.lang.String + + + org.eclipse.jetty.plus.webapp.PlusDescriptorProcessorTest$TestInjections + empty + + + + + + vacuum + java.lang.String + + + org.eclipse.jetty.plus.webapp.PlusDescriptorProcessorTest$TestInjections + vacuum + + + + + + webxmlonly + java.lang.String + WEBXMLONLY + + org.eclipse.jetty.plus.webapp.PlusDescriptorProcessorTest$TestInjections + webXmlOnly + +