dev@jsftemplating.java.net

Re: JSFTemplating: Code Review for Issue #34

From: Karam Singh Badesha <Karam.Badesha_at_Sun.COM>
Date: Sun, 03 Feb 2008 11:16:57 -0800
Ken,
Can I see the test case or example code which shows the problem? I do have multiple includes in my code and haven't seen any problems.

thanks
Karam

Ken Paulsen wrote:

Issue #34 reports problems using the <!include> multiple times.  I suspect this is w/i the same naming container as JSFT currently does nothing to ensure there are no namespace collisions.

I wrote a simple test case which confirms the problem when multiple inserts or multiple includes are used w/i the same NamingContainer.

There are two approaches that come to mind in order to solve this problem:

1) Make sure the ids don't conflict by dynamically generating every id.  This is not ideal as it is very difficult to know when a dynamic id is needed, which would likely result in all ids being dynamic.  This means ids become unpredictable in most or all cases.

2) Insert a NamingContainer during inserts or includes.  This modifies the clientId (vs. the id).  This may also confuse some people who don't expect a insert or include to change their clientId... however, the "id" itself stays the same.  I suspect in most cases, nobody will have a problem with this approach.

Question for those still reading this: do you think #2 is a good approach to do automatically?

Today, you can solve this problem by adding your own NamingContainer around each duplicate insert or include.  But this is a pain and not obvious to new users.  But if option #2 above is a bad idea... this may be all we can do.  I have implemented option #2 and the diffs below show this.  The solution involves adding a new "NamingContainer" component which is automatically added while building the UIComponent tree around each insert/include.  Please review and let me know what you think.

Thanks!

Ken


Index: build.xml
===================================================================
RCS file: /cvs/jsftemplating/build.xml,v
retrieving revision 1.36
diff -u -r1.36 build.xml
--- build.xml   1 Feb 2008 01:07:58 -0000       1.36
+++ build.xml   3 Feb 2008 09:25:57 -0000
@@ -128,13 +128,12 @@
            <fileset dir="${build}" includes="**/*.class, **/*.map" />
            <zipfileset dir="${src}" prefix="" includes="**/*.properties" />
            <zipfileset dir="${jsftemplating.home}/layout/pages" prefix="" includes="*.jsf" />
-            <zipfileset dir="${jsftemplating.home}/component/layout" prefix="META-INF/jsftemplating" includes="*.xml" />
+            <zipfileset dir="${jsftemplating.home}/component/layout" prefix="META-INF/jsftemplating" includes="*.xml,*.xhtml,*.jsf" />
            <zipfileset dir="${jsftemplating.home}/layout/xml" prefix="META-INF/jsftemplating" includes="*.dtd" />
-            <zipfileset dir="${jsftemplating.home}/layout/facelets" prefix="META-INF/" includes="*.dtd" />
-            <zipfileset dir="${jsftemplating.home}/layout/facelets" prefix="META-INF/" includes="*.ent" />
+            <zipfileset dir="${jsftemplating.home}/layout/facelets" prefix="META-INF/" includes="*.dtd,*.ent" />
            <zipfileset dir="${jsftemplating.home}" prefix="META-INF" includes="faces-config.xml" />
        </jar>
-       <jar jarfile="${jsftemplating-src.jar}" basedir="${src}" excludes="**/CVS/*" update="true"/>
+       <!-- <jar jarfile="${jsftemplating-src.jar}" basedir="${src}" excludes="**/CVS/*" update="true"/> -->
    </target>

    <target name="javadoc" depends="build" description="Generate javadocs">
Index: src/java/com/sun/jsftemplating/faces-config.xml
===================================================================
RCS file: /cvs/jsftemplating/src/java/com/sun/jsftemplating/faces-config.xml,v
retrieving revision 1.7
diff -u -r1.7 faces-config.xml
--- src/java/com/sun/jsftemplating/faces-config.xml     9 Apr 2007 15:28:30 -0000       1.7
+++ src/java/com/sun/jsftemplating/faces-config.xml     3 Feb 2008 09:25:57 -0000
@@ -67,6 +67,10 @@
       <component-type>com.sun.jsftemplating.StaticText</component-type>
       <component-class>com.sun.jsftemplating.component.StaticText</component-class>
    </component>
+    <component>
+       <component-type>com.sun.jsftemplating.NamingContainer</component-type>
+       <component-class>com.sun.jsftemplating.component.NamingContainer</component-class>
+    </component>

    <render-kit>
       <renderer>
@@ -99,6 +103,11 @@
           <renderer-type>com.sun.jsftemplating.StaticText</renderer-type>
           <renderer-class>com.sun.jsftemplating.renderer.TemplateRenderer</renderer-class>
       </renderer>
+       <renderer>
+           <component-family>com.sun.jsftemplating.NamingContainer</component-family>
+           <renderer-type>com.sun.jsftemplating.NamingContainer</renderer-type>
+           <renderer-class>com.sun.jsftemplating.renderer.TemplateRenderer</renderer-class>
+       </renderer>
    </render-kit>

    <managed-bean>
Index: src/java/com/sun/jsftemplating/layout/LayoutViewHandler.java
===================================================================
RCS file: /cvs/jsftemplating/src/java/com/sun/jsftemplating/layout/LayoutViewHandler.java,v
retrieving revision 1.35
diff -u -r1.35 LayoutViewHandler.java
--- src/java/com/sun/jsftemplating/layout/LayoutViewHandler.java        2 Feb 2008 05:54:06 -0000       1.35
+++ src/java/com/sun/jsftemplating/layout/LayoutViewHandler.java        3 Feb 2008 09:25:58 -0000
@@ -50,6 +50,7 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

+import com.sun.jsftemplating.component.NamingContainer;
import com.sun.jsftemplating.el.PageSessionResolver;
import com.sun.jsftemplating.layout.descriptors.LayoutComponent;
import com.sun.jsftemplating.layout.descriptors.LayoutComposition;
@@ -58,6 +59,7 @@
import com.sun.jsftemplating.layout.descriptors.LayoutFacet;
import com.sun.jsftemplating.layout.descriptors.LayoutInsert;
import com.sun.jsftemplating.layout.descriptors.Resource;
+import com.sun.jsftemplating.util.LayoutElementUtil;
import com.sun.jsftemplating.util.LogUtil;
import com.sun.jsftemplating.util.SimplePatternMatcher;
import com.sun.jsftemplating.util.fileStreamer.Context;
@@ -473,19 +475,29 @@
               // NOTE: LayoutFacets that aren't JSF facets aren't
               // NOTE: meaningful in this context
           } else if (childElt instanceof LayoutComposition) {
-               String template = ((LayoutComposition) childElt).getTemplate();
+               LayoutComposition compo = ((LayoutComposition) childElt);
+               String template = compo.getTemplate();
               if (template != null) {
                   // Add LayoutComposition to the stack
                   Stack<LayoutElement> stack =
                       LayoutComposition.getCompositionStack(context);
                   stack.push(childElt);

-                   // build tree from the LD of the template...
+                   // Check to see if this is a non-trimming composition
+                   // If so, we want to wrap it in a NamingContainer to
+                   // avoid namespace problems.
+                   if (!compo.isTrimming()) {
+                       // Create a new NamingContainer to use as a parent to
+                       // avoid the same component from being inserted 2x or
+                       // other namespace collisions.
+                       NamingContainer cont = new NamingContainer();
+                       cont.setId(LayoutElementUtil.getGeneratedId("nc"));
+                       parent.getChildren().add(cont);
+                       parent = cont;
+                   }
+
                   try {
-// FIXME: Compositions may need to add a fake UIComponent to the tree
-// FIXME: that is naming container in order to allow multiple UICompositions
-// FIXME: to exist side-by-side... otherwise it will find existing component
-// FIXME: instances and re-use them.
+                       // Add the template here.
                       buildUIComponentTree(context, parent,
                           LayoutDefinitionManager.getLayoutDefinition(
                               context, template));
@@ -495,6 +507,11 @@
                       }
                   }

+                   if (!compo.isTrimming()) {
+                       // Restore the parent (b/c of added NamingContainer)
+                       parent = parent.getParent();
+                   }
+
                   // Remove the LayoutComposition from the stack
                   stack.pop();
               }
@@ -509,6 +526,14 @@
                           + "'ui:composition' was used!");
               }

+               // Create a new NamingContainer to use as a parent to
+               // avoid the same component from being inserted 2x or
+               // other namespace collisions.
+               NamingContainer cont = new NamingContainer();
+               cont.setId(LayoutElementUtil.getGeneratedId("nc"));
+               parent.getChildren().add(cont);
+               parent = cont;
+
               // Get associated UIComposition
               String insertName = ((LayoutInsert) childElt).getName();
               if (insertName == null) {
@@ -530,6 +555,9 @@
                       buildUIComponentTree(context, parent, def);
                   }
               }
+
+               // Restore the parent (b/c of added NamingContainer)
+               parent = parent.getParent();
           } else if (childElt instanceof LayoutComponent) {
               // Calling getChild will add the child UIComponent to tree
               child = ((LayoutComponent) childElt).


cvs diff: src/java/com/sun/jsftemplating/component/NamingContainer.java is a new entry, no comparison available
cvs diff: src/java/com/sun/jsftemplating/component/factory/basic/NamingContainerFactory.java is a new entry, no comparison available
cvs diff: src/java/com/sun/jsftemplating/component/layout/renderChildren.jsf is a new entry, no comparison available