FREEMARKER-55: cleaning up TaglibFactory.Builder
Project: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/commit/14396c83 Tree: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/tree/14396c83 Diff: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/diff/14396c83 Branch: refs/heads/3 Commit: 14396c83635d2b34612a429312c223afa8836fd4 Parents: e12792e Author: Woonsan Ko <[email protected]> Authored: Wed Jul 5 20:19:21 2017 -0400 Committer: Woonsan Ko <[email protected]> Committed: Wed Jul 5 20:19:21 2017 -0400 ---------------------------------------------------------------------- .../apache/freemarker/core/Configuration.java | 2 +- .../freemarker/core/TemplateConfiguration.java | 2 +- .../freemarker/core/util/_CollectionUtil.java | 30 +++- .../freemarker/servlet/FreemarkerServlet.java | 30 ++-- .../freemarker/servlet/jsp/TaglibFactory.java | 169 +++++++++++-------- .../spring/web/view/FreemarkerViewResolver.java | 6 +- .../spring/web/view/FreemarkerViewTest.java | 5 +- 7 files changed, 143 insertions(+), 101 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/14396c83/freemarker-core/src/main/java/org/apache/freemarker/core/Configuration.java ---------------------------------------------------------------------- diff --git a/freemarker-core/src/main/java/org/apache/freemarker/core/Configuration.java b/freemarker-core/src/main/java/org/apache/freemarker/core/Configuration.java index 9f31556..01ba1b2 100644 --- a/freemarker-core/src/main/java/org/apache/freemarker/core/Configuration.java +++ b/freemarker-core/src/main/java/org/apache/freemarker/core/Configuration.java @@ -434,7 +434,7 @@ public final class Configuration implements TopLevelConfiguration, CustomStateSc builder.getImpliedCustomNumberFormats(), builder.getCustomNumberFormats(), false); autoImports = _CollectionUtil.mergeImmutableMaps( builder.getImpliedAutoImports(), builder.getAutoImports(), true); - autoIncludes = _CollectionUtil.mergeImmutableLists( + autoIncludes = _CollectionUtil.mergeTwoListsToImmutableList( builder.getImpliedAutoIncludes(), builder.getAutoIncludes(), true); lazyImports = builder.getLazyImports(); lazyAutoImports = builder.getLazyAutoImports(); http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/14396c83/freemarker-core/src/main/java/org/apache/freemarker/core/TemplateConfiguration.java ---------------------------------------------------------------------- diff --git a/freemarker-core/src/main/java/org/apache/freemarker/core/TemplateConfiguration.java b/freemarker-core/src/main/java/org/apache/freemarker/core/TemplateConfiguration.java index 17583e5..019c3f6 100644 --- a/freemarker-core/src/main/java/org/apache/freemarker/core/TemplateConfiguration.java +++ b/freemarker-core/src/main/java/org/apache/freemarker/core/TemplateConfiguration.java @@ -876,7 +876,7 @@ public final class TemplateConfiguration implements ParsingAndProcessingConfigur true); } if (tc.isAutoIncludesSet()) { - setAutoIncludes(_CollectionUtil.mergeImmutableLists( + setAutoIncludes(_CollectionUtil.mergeTwoListsToImmutableList( isAutoIncludesSet() ? getAutoIncludes() : null, tc.isAutoIncludesSet() ? tc.getAutoIncludes() : null, true), http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/14396c83/freemarker-core/src/main/java/org/apache/freemarker/core/util/_CollectionUtil.java ---------------------------------------------------------------------- diff --git a/freemarker-core/src/main/java/org/apache/freemarker/core/util/_CollectionUtil.java b/freemarker-core/src/main/java/org/apache/freemarker/core/util/_CollectionUtil.java index d5a6574..7cafed3 100644 --- a/freemarker-core/src/main/java/org/apache/freemarker/core/util/_CollectionUtil.java +++ b/freemarker-core/src/main/java/org/apache/freemarker/core/util/_CollectionUtil.java @@ -171,20 +171,41 @@ public class _CollectionUtil { } /** + * Adds multiple {@link List}-s; assuming the inputs are already unmodifiable and unchanging, it returns an + * unmodifiable and unchanging {@link List} itself. + */ + public static <T> List<T> mergeListsToImmutableList(boolean skipDuplicatesInList1, List<T> ... lists) { + if (lists == null || lists.length == 0) { + return null; + } + + if (lists.length == 1) { + return mergeTwoListsToImmutableList(lists[0], null, skipDuplicatesInList1); + } else if (lists.length == 2) { + return mergeTwoListsToImmutableList(lists[0], lists[1], skipDuplicatesInList1); + } else { + List<T> [] reducedLists = new List[lists.length - 1]; + reducedLists[0] = mergeTwoListsToImmutableList(lists[0], lists[1], skipDuplicatesInList1); + System.arraycopy(lists, 2, reducedLists, 1, lists.length - 2); + return mergeListsToImmutableList(skipDuplicatesInList1, reducedLists); + } + } + + /** * Adds two {@link List}-s; assuming the inputs are already unmodifiable and unchanging, it returns an * unmodifiable and unchanging {@link List} itself. */ - public static List<String> mergeImmutableLists(List<String> list1, List<String> list2, + public static <T> List<T> mergeTwoListsToImmutableList(List<T> list1, List<T> list2, boolean skipDuplicatesInList1) { if (list1 == null) return list2; if (list2 == null) return list1; if (list1.isEmpty()) return list2; if (list2.isEmpty()) return list1; - ArrayList<String> mergedList = new ArrayList<>(list1.size() + list2.size()); + ArrayList<T> mergedList = new ArrayList<>(list1.size() + list2.size()); if (skipDuplicatesInList1) { - Set<String> list2Set = new HashSet<>(list2); - for (String it : list1) { + Set<T> list2Set = new HashSet<>(list2); + for (T it : list1) { if (!list2Set.contains(it)) { mergedList.add(it); } @@ -195,4 +216,5 @@ public class _CollectionUtil { mergedList.addAll(list2); return Collections.unmodifiableList(mergedList); } + } http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/14396c83/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/FreemarkerServlet.java ---------------------------------------------------------------------- diff --git a/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/FreemarkerServlet.java b/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/FreemarkerServlet.java index da0435d..bcd4d04 100644 --- a/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/FreemarkerServlet.java +++ b/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/FreemarkerServlet.java @@ -61,9 +61,11 @@ import org.apache.freemarker.core.templateresolver.TemplateLoader; import org.apache.freemarker.core.templateresolver.impl.ClassTemplateLoader; import org.apache.freemarker.core.templateresolver.impl.FileTemplateLoader; import org.apache.freemarker.core.templateresolver.impl.MultiTemplateLoader; +import org.apache.freemarker.core.util._CollectionUtil; import org.apache.freemarker.core.util._SecurityUtil; import org.apache.freemarker.core.util._StringUtil; import org.apache.freemarker.servlet.jsp.TaglibFactory; +import org.apache.freemarker.servlet.jsp.TaglibFactory.ClasspathMetaInfTldSource; import org.apache.freemarker.servlet.jsp.TaglibFactory.MetaInfTldSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -490,8 +492,8 @@ public class FreemarkerServlet extends HttpServlet { private ResponseCharacterEncoding responseCharacterEncoding = ResponseCharacterEncoding.LEGACY; private Charset forcedResponseCharacterEncoding; private OverrideResponseLocale overrideResponseLocale = OverrideResponseLocale.ALWAYS; - private List/*<MetaInfTldSource>*/ metaInfTldSources; - private List/*<String>*/ classpathTlds; + private List<MetaInfTldSource> metaInfTldSources; + private List<String> classpathTlds; private Object lazyInitFieldsLock = new Object(); @SuppressFBWarnings(value="SE_BAD_FIELD", justification="Not investing into making this Servlet serializable") @@ -1007,11 +1009,15 @@ public class FreemarkerServlet extends HttpServlet { "Failed to parse system property \"" + SYSTEM_PROPERTY_META_INF_TLD_SOURCES + "\"", e); } - List<Pattern> jettyTaglibJarPatterns = null; + List<MetaInfTldSource> jettyMetaInfTldSources = null; try { final String attrVal = (String) servletContext.getAttribute(ATTR_JETTY_CP_TAGLIB_JAR_PATTERNS); - jettyTaglibJarPatterns = (attrVal != null) ? InitParamParser.parseCommaSeparatedPatterns(attrVal) - : Collections.emptyList(); + List<Pattern> jettyTaglibJarPatterns = (attrVal != null) + ? InitParamParser.parseCommaSeparatedPatterns(attrVal) : Collections.emptyList(); + jettyMetaInfTldSources = new ArrayList<>(jettyTaglibJarPatterns.size()); + for (Pattern pattern : jettyTaglibJarPatterns) { + jettyMetaInfTldSources.add(new ClasspathMetaInfTldSource(pattern)); + } } catch (Exception e) { LOG.error("Failed to parse application context attribute \"" + ATTR_JETTY_CP_TAGLIB_JAR_PATTERNS + "\" - it will be ignored", e); @@ -1027,14 +1033,12 @@ public class FreemarkerServlet extends HttpServlet { "Failed to parse system property \"" + SYSTEM_PROPERTY_CLASSPATH_TLDS + "\"", e); } - return new TaglibFactory.Builder() - .servletContext(servletContext) - .objectWrapper(objectWrapper) - .addAllMetaInfTldSources(metaInfTldSources) - .addAllMetaInfTldSources(metaInfTldSourcesFromSysProp) - .addAllJettyMetaInfTldJarPatterns(jettyTaglibJarPatterns) - .addAllClasspathTlds(classpathTlds) - .addAllClasspathTlds(classpathTldsFromSysProp).build(); + return new TaglibFactory.Builder(servletContext, objectWrapper) + .metaInfTldSources(_CollectionUtil.mergeListsToImmutableList(true, metaInfTldSources, + metaInfTldSourcesFromSysProp, jettyMetaInfTldSources)) + .classpathTlds( + _CollectionUtil.mergeTwoListsToImmutableList(classpathTlds, classpathTldsFromSysProp, true)) + .build(); } /** http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/14396c83/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/jsp/TaglibFactory.java ---------------------------------------------------------------------- diff --git a/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/jsp/TaglibFactory.java b/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/jsp/TaglibFactory.java index f4fea3b..cb45aa4 100644 --- a/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/jsp/TaglibFactory.java +++ b/freemarker-servlet/src/main/java/org/apache/freemarker/servlet/jsp/TaglibFactory.java @@ -136,6 +136,12 @@ public class TaglibFactory implements TemplateHashModel { private List<String> failedTldLocations = new ArrayList<>(); private int nextTldLocationLookupPhase = 0; + /** + * Parse TLD location string {@code value}. e.g, "webInfPerLibJars", "classpath:*", etc. + * @param value TLD location string + * @return {@link MetaInfTldSource} instance for the value + * @throws ParseException if invalid value syntax found + */ public static MetaInfTldSource parseMetaInfTldLocation(String value) throws ParseException { MetaInfTldSource metaInfTldSource; @@ -167,6 +173,13 @@ public class TaglibFactory implements TemplateHashModel { return metaInfTldSource; } + /** + * Parse each TLD location string in the {@code list} and return a list of type {@link MetaInfTldSource}. + * @param values TLD location string value list + * @return a list of type {@link MetaInfTldSource} by parsing each TLD location string item value + * @throws ParseException if invalid value syntax found + * @see {@link #parseMetaInfTldLocation(String)} + */ public static List<MetaInfTldSource> parseMetaInfTldLocations(List<String> values) throws ParseException { List<MetaInfTldSource> metaInfTldSources = null; @@ -205,7 +218,7 @@ public class TaglibFactory implements TemplateHashModel { servletContext = builder.getServletContext(); objectWrapper = builder.getObjectWrapper(); metaInfTldSources = builder.getMetaInfTldSources(); - classpathTlds = builder.getClassPathTlds(); + classpathTlds = builder.getClasspathTlds(); } /** @@ -496,7 +509,7 @@ public class TaglibFactory implements TemplateHashModel { } } } - + private void addTldLocationsFromWebInfPerLibJarMetaInfTlds() throws IOException, SAXException { LOG.debug("Looking for TLD locations in servletContext:/WEB-INF/lib/*.{jar,zip}{}*.tld", META_INF_ABS_PATH); @@ -583,7 +596,7 @@ public class TaglibFactory implements TemplateHashModel { LOG.debug("Scanning for " + META_INF_ABS_PATH + "*.tld-s in ZipInputStream (slow): servletContext:" + jarResourcePath); } - + final InputStream in = servletContext.getResourceAsStream(jarResourcePath); if (in == null) { throw new IOException("ServletContext resource not found: " + jarResourcePath); @@ -1060,6 +1073,7 @@ public class TaglibFactory implements TemplateHashModel { if (jarFileUrl == null) { throw new IOException("Servlet context resource not found: " + servletContextJarFilePath); } + return new URL( "jar:" + jarFileUrl.toURI() @@ -1329,17 +1343,20 @@ public class TaglibFactory implements TemplateHashModel { } final String entryPath; + if (this.entryPath != null) { entryPath = this.entryPath; } else { if (entryUrl == null) { throw new IOException("Nothing to deduce jar entry path from."); } + String urlEF = entryUrl.toExternalForm(); int sepIdx = urlEF.indexOf(JAR_URL_ENTRY_PATH_START); if (sepIdx == -1) { throw new IOException("Couldn't extract jar entry path from: " + urlEF); } + entryPath = normalizeJarEntryPath( URLDecoder.decode( urlEF.substring(sepIdx + JAR_URL_ENTRY_PATH_START.length()), @@ -1357,6 +1374,7 @@ public class TaglibFactory implements TemplateHashModel { throw new IOException("Jar's InputStreamFactory (" + fallbackRawJarContentInputStreamFactory + ") says the resource doesn't exist."); } + zipIn = new ZipInputStream(rawIn); while (true) { @@ -2014,69 +2032,75 @@ public class TaglibFactory implements TemplateHashModel { public TaglibGettingException(String message) { super(message); } - + } + /** + * Creates a new {@link TaglibFactory}. + */ public static class Builder implements CommonBuilder<TaglibFactory> { /** * Servlet context. */ - private ServletContext servletContext; + private final ServletContext servletContext; /** - * Object wrapper to be used in model building. + * ObjectWrapper to be used in model building. */ - private ObjectWrapper objectWrapper; + private final ObjectWrapper objectWrapper; /** * TLD locations to look for when finding available JSP tag libraries. */ - private List<MetaInfTldSource> metaInfTldSources = new ArrayList<>(); + private List<MetaInfTldSource> metaInfTldSources; /** * TLD classpath locations to look for when finding available JSP tag libraries. */ - private List<String> classPathTlds = new ArrayList<>(); + private List<String> classpathTlds; private boolean alreadyBuilt; - public Builder() { + /** + * Constructs a Builder with ServletContext and ObjectWrapper. + * @param servletContext ServletContext instance + * @param objectWrapper the {@link ObjectWrapper} used when building the JSP tag library {@link TemplateHashModel}-s from the TLD-s. + * Usually, it should be the same {@link ObjectWrapper} that will be used inside the templates. {@code null} value + * is only supported for backward compatibility. For custom EL functions to be exposed, it must be non-{@code null} + * and an {@code intanceof} {@link DefaultObjectWrapper} (like typically, a {@link DefaultObjectWrapper}). + */ + public Builder(ServletContext servletContext, ObjectWrapper objectWrapper) { + _NullArgumentException.check("servletContext", servletContext); + _NullArgumentException.check("objectWrapper", objectWrapper); + this.servletContext = servletContext; + this.objectWrapper = objectWrapper; } + /** + * Get ServletContext instance. + * @return ServletContext instance + */ public ServletContext getServletContext() { return servletContext; } - public void setServletContext(ServletContext servletContext) { - this.servletContext = servletContext; - } - - public Builder servletContext(ServletContext servletContext) { - setServletContext(servletContext); - return this; - } - + /** + * Get ObjectWrapper to be used in model building. + * @return ObjectWrapper to be used in model building + */ public ObjectWrapper getObjectWrapper() { return objectWrapper; } /** - * Sets the {@link ObjectWrapper} used when building the JSP tag library {@link TemplateHashModel}-s from the TLD-s. - * Usually, it should be the same {@link ObjectWrapper} that will be used inside the templates. {@code null} value - * is only supported for backward compatibility. For custom EL functions to be exposed, it must be non-{@code null} - * and an {@code intanceof} {@link DefaultObjectWrapper} (like typically, a {@link DefaultObjectWrapper}). + * Get the list of places where to look for {@code META-INF/**}{@code /*.tld} files. + * @return the list of places where to look for {@code META-INF/**}{@code /*.tld} files */ - public void setObjectWrapper(ObjectWrapper objectWrapper) { - this.objectWrapper = objectWrapper; - } - - public Builder objectWrapper(ObjectWrapper objectWrapper) { - setObjectWrapper(objectWrapper); - return this; - } - public List<MetaInfTldSource> getMetaInfTldSources() { + if (metaInfTldSources == null) { + return Collections.emptyList(); + } return metaInfTldSources; } @@ -2097,11 +2121,34 @@ public class TaglibFactory implements TemplateHashModel { * @see #setClasspathTlds(List) */ public void setMetaInfTldSources(List<MetaInfTldSource> metaInfTldSources) { - this.metaInfTldSources = metaInfTldSources; + _NullArgumentException.check("metaInfTldSources", metaInfTldSources); + this.metaInfTldSources = Collections.unmodifiableList(new ArrayList<>(metaInfTldSources)); + } + + /** + * Fluent API equivalent of {@link #setMetaInfTldSources(List)}. + * @param metaInfTldSources + * The list of {@link MetaInfTldSource} subclass instances. Their order matters if multiple TLD-s define + * a taglib with the same {@code taglib-uri}. In that case, the one found by the earlier + * {@link MetaInfTldSource} wins. + * @return this builder + */ + public Builder metaInfTldSources(List<MetaInfTldSource> metaInfTldSources) { + setMetaInfTldSources(metaInfTldSources); + return this; } - public List<String> getClassPathTlds() { - return classPathTlds; + /** + * Get the class-loader resource paths of the TLD-s that aren't inside the locations covered by + * {@link #setMetaInfTldSources(List)}, yet you want them to be discovered. + * @return the class-loader resource paths of the TLD-s that aren't inside the locations covered by + * {@link #setMetaInfTldSources(List)}, yet you want them to be discovered + */ + public List<String> getClasspathTlds() { + if (classpathTlds == null) { + return Collections.emptyList(); + } + return classpathTlds; } /** @@ -2118,50 +2165,24 @@ public class TaglibFactory implements TemplateHashModel { * * @see #setMetaInfTldSources(List) */ - public void setClassPathTlds(List<String> classPathTlds) { - this.classPathTlds = classPathTlds; - } - - public Builder addMetaInfTldSource(MetaInfTldSource metaInfTldSource) { - metaInfTldSources.add(metaInfTldSource); - return this; - } - - public Builder addAllMetaInfTldSources(List<MetaInfTldSource> metaInfTldSources) { - this.metaInfTldSources.addAll(metaInfTldSources); - return this; + public void setClasspathTlds(List<String> classpathTlds) { + _NullArgumentException.check("objectWrapper", objectWrapper); + this.classpathTlds = Collections.unmodifiableList(new ArrayList<>(classpathTlds)); } - public Builder addMetaInfTldLocation(String metaInfTldLocation) throws ParseException { - return addMetaInfTldSource(parseMetaInfTldLocation(metaInfTldLocation)); - } - - public Builder addMetaInfTldLocations(List<String> metaInfTldLocations) throws ParseException { - return addAllMetaInfTldSources(parseMetaInfTldLocations(metaInfTldLocations)); - } - - public Builder addJettyMetaInfTldJarPattern(Pattern pattern) { - return addMetaInfTldSource(new ClasspathMetaInfTldSource(pattern)); - } - - public Builder addAllJettyMetaInfTldJarPatterns(List<Pattern> patterns) { - for (Pattern pattern : patterns) { - addJettyMetaInfTldJarPattern(pattern); - } - - return this; - } - - public Builder addClasspathTld(String classpathTld) { - classPathTlds.add(classpathTld); - return this; - } - - public Builder addAllClasspathTlds(List<String> classpathTlds) { - classPathTlds.addAll(classpathTlds); + /** + * Fluent API equivalent of {@link #setClasspathTlds(List)}. + * @param classpathTlds + * List of {@code String}-s, maybe {@code null}. Each item is a resource path, like + * {@code "/META-INF/my.tld"}. (Relative resource paths will be interpreted as root-relative.) + * @return this builder + */ + public Builder classpathTlds(List<String> classpathTlds) { + setClasspathTlds(classpathTlds); return this; } + @Override public TaglibFactory build() throws ConfigurationException { if (alreadyBuilt) { throw new IllegalStateException("build() can only be executed once."); http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/14396c83/freemarker-spring/src/main/java/org/apache/freemarker/spring/web/view/FreemarkerViewResolver.java ---------------------------------------------------------------------- diff --git a/freemarker-spring/src/main/java/org/apache/freemarker/spring/web/view/FreemarkerViewResolver.java b/freemarker-spring/src/main/java/org/apache/freemarker/spring/web/view/FreemarkerViewResolver.java index fa4b226..c58e7db 100644 --- a/freemarker-spring/src/main/java/org/apache/freemarker/spring/web/view/FreemarkerViewResolver.java +++ b/freemarker-spring/src/main/java/org/apache/freemarker/spring/web/view/FreemarkerViewResolver.java @@ -127,10 +127,8 @@ public class FreemarkerViewResolver extends AbstractTemplateViewResolver impleme servletContextModel = new ServletContextHashModel(pageContextServlet, objectWrapper); - taglibFactory = new TaglibFactory.Builder() - .servletContext(getServletContext()) - .objectWrapper(objectWrapper) - .build(); + // TODO: additional meta-inf / classpath tld resources? + taglibFactory = new TaglibFactory.Builder(getServletContext(), objectWrapper).build(); } @Override http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/14396c83/freemarker-spring/src/test/java/org/apache/freemarker/spring/web/view/FreemarkerViewTest.java ---------------------------------------------------------------------- diff --git a/freemarker-spring/src/test/java/org/apache/freemarker/spring/web/view/FreemarkerViewTest.java b/freemarker-spring/src/test/java/org/apache/freemarker/spring/web/view/FreemarkerViewTest.java index 8036f30..1ed2831 100644 --- a/freemarker-spring/src/test/java/org/apache/freemarker/spring/web/view/FreemarkerViewTest.java +++ b/freemarker-spring/src/test/java/org/apache/freemarker/spring/web/view/FreemarkerViewTest.java @@ -72,10 +72,7 @@ public class FreemarkerViewTest { pageContextServlet = new PageContextServlet(); pageContextServlet.init(new PageContextServletConfig(servletContext, PageContextServlet.class.getSimpleName())); - taglibFactory = new TaglibFactory.Builder() - .servletContext(servletContext) - .objectWrapper(objectWrapper) - .build(); + taglibFactory = new TaglibFactory.Builder(servletContext, objectWrapper).build(); viewResolver = new FreemarkerViewResolver(); viewResolver.setServletContext(servletContext);
