[ofbiz-framework] branch trunk updated: Improved: Headerize external script in multi-block html template (OFBIZ-11741)

[James Yong-2]

This is an automated email from the ASF dual-hosted git repository.

jamesyong pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git


The following commit(s) were added to refs/heads/trunk by this push:
     new f85040f  Improved: Headerize external script in multi-block html template (OFBIZ-11741)
f85040f is described below

commit f85040ff04a70ea17facfd312c329101c3fe817b
Author: James Yong <[hidden email]>
AuthorDate: Sun May 31 17:42:42 2020 +0800

    Improved: Headerize external script in multi-block html template (OFBIZ-11741)
   
    Allow expression in template location.
---
 build.gradle                                       |   2 +-
 framework/widget/dtd/widget-screen.xsd             |   2 +-
 .../widget/model/HtmlImportWidgetVisitor.java      | 236 ---------------------
 .../org/apache/ofbiz/widget/model/HtmlWidget.java  |  19 ++
 .../ofbiz/widget/model/ModelScreenWidget.java      |  19 --
 .../widget/model/MultiBlockHtmlTemplateUtil.java   | 132 +++++++++---
 6 files changed, 128 insertions(+), 282 deletions(-)

diff --git a/build.gradle b/build.gradle
index 7ad21a2..1e55676 100644
--- a/build.gradle
+++ b/build.gradle
@@ -286,7 +286,7 @@ checkstyle {
     // the sum of errors found last time it was changed after using the
     // ‘checkstyle’ tool present in the framework and in the official
     // plugins.
-    tasks.checkstyleMain.maxErrors = 26985
+    tasks.checkstyleMain.maxErrors = 26908
     // Currently there are a lot of errors so we need to temporarily
     // hide them to avoid polluting the terminal output.
     showViolations = false
diff --git a/framework/widget/dtd/widget-screen.xsd b/framework/widget/dtd/widget-screen.xsd
index 6e3fe97..3bf4f1b 100644
--- a/framework/widget/dtd/widget-screen.xsd
+++ b/framework/widget/dtd/widget-screen.xsd
@@ -524,7 +524,7 @@ under the License.
                 <xs:documentation>
                     Multi-block processing of template targeted for the html body.
                     Inline script will be rendered as external script after html body tag.
-                    External script tag with attribute data-import='head' will be rendered within html head tag if the template location is static.
+                    External script tag with attribute data-import='head' will be rendered within html head tag.
                 </xs:documentation>
             </xs:annotation>
         </xs:attribute>
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/HtmlImportWidgetVisitor.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/HtmlImportWidgetVisitor.java
deleted file mode 100644
index 9e6add0..0000000
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/HtmlImportWidgetVisitor.java
+++ /dev/null
@@ -1,236 +0,0 @@
-/*******************************************************************************
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- *******************************************************************************/
-package org.apache.ofbiz.widget.model;
-
-import java.util.LinkedHashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-
-import org.apache.ofbiz.base.util.FileUtil;
-import org.apache.ofbiz.base.util.UtilValidate;
-import org.jsoup.Jsoup;
-import org.jsoup.nodes.Document;
-import org.jsoup.select.Elements;
-
-public final class HtmlImportWidgetVisitor implements ModelWidgetVisitor {
-
-    private Set<String> jsImports = new LinkedHashSet<String>();
-
-    /**
-     * Get the script source locations collected by HtmlImportWidgetVisitor
-     * @return
-     */
-    public Set<String> getJsImports() {
-        return jsImports;
-    }
-
-    @Override
-    public void visit(HtmlWidget htmlWidget) throws Exception {
-        List<ModelScreenWidget> widgetList = htmlWidget.getSubWidgets();
-        for (ModelScreenWidget widget: widgetList) {
-            // HtmlTemplate
-            widget.accept(this);
-        }
-    }
-
-    @Override
-    public void visit(HtmlWidget.HtmlTemplate htmlTemplate) throws Exception {
-        String fileLocation = htmlTemplate.locationExdr.getOriginal();
-        boolean isStaticLocation = !fileLocation.contains("${");
-        if (isStaticLocation && htmlTemplate.isMultiBlock()) {
-            String template = FileUtil.readString("UTF-8", FileUtil.getFile(fileLocation));
-            Document doc = Jsoup.parseBodyFragment(template);
-            Elements scriptElements = doc.select("script");
-            if (scriptElements != null && scriptElements.size() > 0) {
-                for (org.jsoup.nodes.Element script : scriptElements) {
-                    String src = script.attr("src");
-                    if (UtilValidate.isNotEmpty(src)) {
-                        String dataImport = script.attr("data-import");
-                        if ("head".equals(dataImport)) {
-                            jsImports.add(src);
-                        }
-                    }
-                }
-            }
-        }
-    }
-
-    @Override
-    public void visit(HtmlWidget.HtmlTemplateDecorator htmlTemplateDecorator) throws Exception {
-
-    }
-
-    @Override
-    public void visit(HtmlWidget.HtmlTemplateDecoratorSection htmlTemplateDecoratorSection) throws Exception {
-
-    }
-
-    @Override
-    public void visit(IterateSectionWidget iterateSectionWidget) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelSingleForm modelForm) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelGrid modelGrid) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelMenu modelMenu) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelMenuItem modelMenuItem) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreen modelScreen) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.ColumnContainer columnContainer) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Container container) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Content content) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.DecoratorScreen decoratorScreen) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.DecoratorSection decoratorSection) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.DecoratorSectionInclude decoratorSectionInclude) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Form form) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Grid grid) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.HorizontalSeparator horizontalSeparator) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.ScreenImage image) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.IncludeScreen includeScreen) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Label label) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.ScreenLink link) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Menu menu) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.PlatformSpecific platformSpecific) throws Exception {
-        Map<String, ModelScreenWidget> widgetMap = platformSpecific.getSubWidgets();
-        for (Map.Entry<String, ModelScreenWidget> entry : widgetMap.entrySet()) {
-            if (entry.getKey().equals("html")) {
-                // HtmlWidget
-                entry.getValue().accept(this);
-            }
-        }
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.PortalPage portalPage) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Screenlet screenlet) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Section section) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Tree tree) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelTree modelTree) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelTree.ModelNode modelNode) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelTree.ModelNode.ModelSubNode modelSubNode) throws Exception {
-
-    }
-
-    @Override
-    public void visit(ModelScreenWidget.Column column) throws Exception {
-
-    }
-}
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/HtmlWidget.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/HtmlWidget.java
index 4817a37..3e4f3cd 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/HtmlWidget.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/HtmlWidget.java
@@ -24,8 +24,10 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.apache.ofbiz.base.util.Debug;
 import org.apache.ofbiz.base.util.GeneralException;
@@ -261,6 +263,23 @@ public class HtmlWidget extends ModelScreenWidget {
             super(modelScreen, htmlTemplateElement);
             this.locationExdr = FlexibleStringExpander.getInstance(htmlTemplateElement.getAttribute("location"));
             this.multiBlock = !"false".equals(htmlTemplateElement.getAttribute("multi-block"));
+
+            if (this.isMultiBlock()) {
+                String origLoc = this.locationExdr.getOriginal();
+                Set<String> urls = null;
+                if (origLoc.contains("${")) {
+                    urls = new LinkedHashSet<>();
+                    urls.add(origLoc);
+                } else {
+                    try {
+                        urls = MultiBlockHtmlTemplateUtil.getHtmlImportsFromHtmlTemplate(origLoc);
+                    } catch (IOException e) {
+                        String errMsg = "Error getting html imports from template at location [" + origLoc + "]: " + e.toString();
+                        Debug.logError(e, errMsg, MODULE);
+                    }
+                }
+                MultiBlockHtmlTemplateUtil.addLinksToHtmlImportCache(modelScreen.getSourceLocation(), modelScreen.getName(), urls);
+            }
         }
 
         public String getLocation(Map<String, Object> context) {
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelScreenWidget.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelScreenWidget.java
index a460679..c3701f3 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelScreenWidget.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelScreenWidget.java
@@ -259,25 +259,6 @@ public abstract class ModelScreenWidget extends ModelWidget {
                 this.failWidgets = Collections.emptyList();
             }
             this.isMainSection = isMainSection;
-
-            String screenLocation = modelScreen.getSourceLocation();
-            String screenName = modelScreen.getName();
-
-            // check required html import from the widgets
-            HtmlImportWidgetVisitor visitor = new HtmlImportWidgetVisitor();
-            try {
-                for (ModelScreenWidget widget : this.subWidgets) {
-                    widget.accept(visitor);
-                }
-                for (ModelScreenWidget widget : this.failWidgets) {
-                    widget.accept(visitor);
-                }
-                MultiBlockHtmlTemplateUtil.addLinksToHtmlImportCache(screenLocation, screenName, visitor.getJsImports());
-            } catch (Exception e) {
-                String errMsg = "Error adding links to Html Import Cache";
-                Debug.logError(e, errMsg, MODULE);
-                throw new RuntimeException(errMsg);
-            }
         }
 
         @Override
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/MultiBlockHtmlTemplateUtil.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/MultiBlockHtmlTemplateUtil.java
index 7404963..a7bc33e 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/MultiBlockHtmlTemplateUtil.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/MultiBlockHtmlTemplateUtil.java
@@ -18,6 +18,7 @@
  *******************************************************************************/
 package org.apache.ofbiz.widget.model;
 
+import java.io.IOException;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
@@ -28,42 +29,69 @@ import java.util.Set;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpSession;
 
+import org.apache.ofbiz.base.util.Debug;
+import org.apache.ofbiz.base.util.FileUtil;
 import org.apache.ofbiz.base.util.UtilGenerics;
 import org.apache.ofbiz.base.util.UtilValidate;
+import org.apache.ofbiz.base.util.string.FlexibleStringExpander;
+import org.jsoup.Jsoup;
+import org.jsoup.nodes.Document;
+import org.jsoup.select.Elements;
 
 public final class MultiBlockHtmlTemplateUtil {
 
+    private static final String MODULE = MultiBlockHtmlTemplateUtil.class.getName();
     public static final String MULTI_BLOCK_WRITER = "multiBlockWriter";
     private static final String HTML_LINKS_FOR_HEAD = "htmlLinksForHead";
     private static final String SCRIPT_LINKS_FOR_FOOT = "ScriptLinksForFoot";
-    private static int maxScriptCacheSizePerSession = 10;
-    // store inline script from freemarker template by user session
+    private static int maxScriptCacheSizePerUserSession = 10;
+    private static int estimatedConcurrentUserSessions = 250;
+    private static int estimatedScreensWithMultiBlockHtmlTemplate = 200;
+    private static int estimatedScreensWithChildScreen = 200;
+    /**
+     * Store inline script extracted from freemarker template for a user session.
+     * Number of inline scripts for a user session will be constraint by {@link MultiBlockHtmlTemplateUtil#maxScriptCacheSizePerUserSession}
+     * {@link MultiBlockHtmlTemplateUtil#cleanupScriptCache(HttpSession)} will be called to remove entry when session ends.
+     */
     private static LinkedHashMap<String, Map<String, String>> scriptCache =
             new LinkedHashMap<String, Map<String, String>>() {
                 private static final long serialVersionUID = 1L;
                 protected boolean removeEldestEntry(Map.Entry<String, Map<String, String>> eldest) {
-                    return size() > 100; // TODO probably set to max number of concurrent user
+                    return size() > estimatedConcurrentUserSessions;
                 }
             };
-    // store the additional html import by screen location
+    /**
+     * For each screen containing html-template, store a set of html imports headerized from html-template.
+     * The set may contain entry of an expression of the the html-template's location.
+     * In this case, we need to expand the location expression, read the html-template for any html imports.
+     */
     private static LinkedHashMap<String, Set<String>> htmlImportCache =
             new LinkedHashMap<String, Set<String>>() {
                 private static final long serialVersionUID = 1L;
                 protected boolean removeEldestEntry(Map.Entry<String, Set<String>> eldest) {
-                    return size() > 300; // TODO probably set to max number of screens
+                    return size() > estimatedScreensWithMultiBlockHtmlTemplate;
                 }
             };
-    // store the child screen
+    /**
+     * Store set of dependent screens info, in form of screen location + hash + name, of a given parent screen.
+     * The set may contain entry of an expression of the dependent screen location and will need to be expanded before use.
+     */
     private static LinkedHashMap<String, Set<String>> dependentScreenCache =
             new LinkedHashMap<String, Set<String>>() {
                 private static final long serialVersionUID = 1L;
                 protected boolean removeEldestEntry(Map.Entry<String, Set<String>> eldest) {
-                    return size() > 100;
+                    return size() > estimatedScreensWithChildScreen;
                 }
             };
 
     private MultiBlockHtmlTemplateUtil() { }
 
+    /**
+     * Add child screen info to {@link MultiBlockHtmlTemplateUtil#dependentScreenCache}.
+     * @param parentModelScreen parent screen.
+     * @param location screen location. Expression is allowed.
+     * @param name screen name. Expression is allowed.
+     */
     public static void collectChildScreenInfo(ModelScreen parentModelScreen, String location, String name) {
         String key = parentModelScreen.getSourceLocation() + "#" + parentModelScreen.getName();
         Set<String> childList = dependentScreenCache.get(key);
@@ -78,7 +106,14 @@ public final class MultiBlockHtmlTemplateUtil {
         }
     }
 
-    public static void addLinksToHtmlImportCache(String location, String name, Set<String> urls) throws Exception {
+    /**
+     * Add Html Imports to {@link MultiBlockHtmlTemplateUtil#htmlImportCache}.
+     * @param location screen location. Expression is not allowed.
+     * @param name screen name. Expression is not allowed.
+     * @param urls Set of html links associated with the screen. May contain expression to html-template location.
+     * @throws Exception
+     */
+    public static void addLinksToHtmlImportCache(String location, String name, Set<String> urls) {
         if (UtilValidate.isEmpty(urls)) {
             return;
         }
@@ -91,6 +126,37 @@ public final class MultiBlockHtmlTemplateUtil {
         existingUrls.addAll(urls);
     }
 
+    /**
+     * Get html import scr location from html template
+     * @param fileLocation Location to html template. Expression is not allowed.
+     * @return
+     */
+    public static Set<String> getHtmlImportsFromHtmlTemplate(String fileLocation) throws IOException {
+        Set<String> imports = new LinkedHashSet<>();
+        String template = FileUtil.readString("UTF-8", FileUtil.getFile(fileLocation));
+        Document doc = Jsoup.parseBodyFragment(template);
+        Elements scriptElements = doc.select("script");
+        if (scriptElements != null && scriptElements.size() > 0) {
+            for (org.jsoup.nodes.Element script : scriptElements) {
+                String src = script.attr("src");
+                if (UtilValidate.isNotEmpty(src)) {
+                    String dataImport = script.attr("data-import");
+                    if ("head".equals(dataImport)) {
+                        imports.add(src);
+                    }
+                }
+            }
+        }
+        return imports;
+    }
+
+    /**
+     * Add html links to the header
+     * @param context
+     * @param location screen location. Expression is not allowed.
+     * @param name screen name. Expression is not allowed.
+     * @throws Exception
+     */
     public static void addLinksToLayoutSettings(final Map<String, Object> context, String location, String name) throws Exception {
         HttpServletRequest request = (HttpServletRequest) context.get("request");
         if (request.getAttribute(HTML_LINKS_FOR_HEAD) == null) {
@@ -107,18 +173,37 @@ public final class MultiBlockHtmlTemplateUtil {
         if (UtilValidate.isEmpty(layoutSettingsJsList)) {
             return;
         }
+        // ensure initTheme.groovy has run.
+        Map<String, String> commonScreenLocations = UtilGenerics.cast(context.get("commonScreenLocations"));
+        if (UtilValidate.isEmpty(commonScreenLocations)) {
+            return;
+        }
         Object objValue = request.getAttribute(HTML_LINKS_FOR_HEAD);
         if (objValue instanceof String) {
             String currentLocationHashName = (String) request.getAttribute(HTML_LINKS_FOR_HEAD);
             Set<String> htmlLinks = new LinkedHashSet<>();
-            Set<String> locHashNameList = getRelatedScreenLocationHashName(currentLocationHashName);
+            Set<String> locHashNameList = getRelatedScreenLocationHashName(currentLocationHashName, context);
             for (String locHashName:locHashNameList) {
-                Set<String> urls = htmlImportCache.get(locHashName);
+                String expandLocHashName = "";
+                if (locHashName.contains("${")) {
+                    expandLocHashName = FlexibleStringExpander.expandString(locHashName, context);
+                } else {
+                    expandLocHashName = locHashName;
+                }
+                Set<String> urls = htmlImportCache.get(expandLocHashName);
                 if (UtilValidate.isNotEmpty(urls)) {
-                    // check url is not already in layoutSettings.javaScripts
                     for (String url : urls) {
-                        if (!htmlLinks.contains(url)) {
-                            htmlLinks.add(url);
+                        if (url.contains("${")) {
+                            String expandUrl = FlexibleStringExpander.expandString(url, context);
+                            if (UtilValidate.isNotEmpty(expandUrl)) {
+                                htmlLinks.addAll(getHtmlImportsFromHtmlTemplate(expandUrl));
+                            } else {
+                                Debug.log("Unable to expand " + url, MODULE);
+                            }
+                        } else {
+                            if (!htmlLinks.contains(url)) {
+                                htmlLinks.add(url);
+                            }
                         }
                     }
                 }
@@ -141,28 +226,25 @@ public final class MultiBlockHtmlTemplateUtil {
      * @param locationHashName
      * @return
      */
-    private static Set<String> getRelatedScreenLocationHashName(String locationHashName) {
+    private static Set<String> getRelatedScreenLocationHashName(String locationHashName, final Map<String, Object> context) {
         Set<String> resultList = new HashSet<>();
         resultList.add(locationHashName);
         Set<String> locHashNameList = dependentScreenCache.get(locationHashName);
         if (locHashNameList != null) {
             for (String locHashName : locHashNameList) {
-                resultList.addAll(getRelatedScreenLocationHashName(locHashName));
+                String exLocHashName = "";
+                if (locHashName.contains("${")) {
+                    exLocHashName = FlexibleStringExpander.expandString(locHashName, context);
+                } else {
+                    exLocHashName = locHashName;
+                }
+                resultList.addAll(getRelatedScreenLocationHashName(exLocHashName, context));
             }
         }
         return resultList;
     }
 
     /**
-     * add the script links that should be in the head tag
-     * @param context
-     * @param urls
-     */
-    private static void addJsLinkToLayoutSettings(final Map<String, Object> context, final Set<String> urls) {
-
-    }
-
-    /**
      * add script link for page footer.
      * @param context
      * @param filePath
@@ -203,7 +285,7 @@ public final class MultiBlockHtmlTemplateUtil {
             scriptMap = new LinkedHashMap<String, String>() {
                 private static final long serialVersionUID = 1L;
                 protected boolean removeEldestEntry(Map.Entry<String, String> eldest) {
-                    return size() > maxScriptCacheSizePerSession;
+                    return size() > maxScriptCacheSizePerUserSession;
                 }
             };
             scriptCache.put(sessionId, scriptMap);