Repository: primefaces/primefaces
Pull request #10784 (2023-10-07 22:30:38)

TreeTable: removal of cloneOnFilter property
My take on this, is that `TreeTable#cloneOnFilter` seems unecessary. As a reminder, cloneOnFilter:

> Defines if nodes should be cloned on filter via Cloneable interface or Copy-Constructor (CustomNode(CustomNode original) or CustomNode(String type, Object data, TreeNode parent)). Normally the filtered nodes are new instanceof of DefaultTreeNode.

How can we expect user to know that? A simpler description would be: whether or not custom nodes is being used. And we 
 don't need a property for that. 

Also, if cloning fails through Cloneable or Constructor: it fallbacks to built-in instance, meaning you'll start with a tree of C nodes and end-up having a tree of A nodes. So, sooner or later there will be problem... like #6173

What I changed is: if clonable fails, then fail. If fallback argument constructor: fails. No point to fallback to default/checkbox nodes


melloware: I agree this is not something we need to ask the user. I really like this refactoring and clean up!
tandraschko: +1 about the clone change; it was always weird, i just improved it a bit somewhere between the last years

not sure about making the treenodes generic, a parent node must actually not have the same datatype as the child.

would be better to split up your PR
Rapster: > not sure about making the treenodes generic, a parent node must actually not have the same datatype as the child.

I wouldn't say "must" hopefully they can have the same type, but I see what you mean child nodes data don't necessarely have the same type as parent node. I'll create a separate PR for it

Patch:
diff --git a/docs/14_0_0/components/treetable.md b/docs/14_0_0/components/treetable.md
index 537decc11e..0b281801f6 100644
--- a/docs/14_0_0/components/treetable.md
+++ b/docs/14_0_0/components/treetable.md
@@ -23,7 +23,6 @@ id | null | String | Unique identifier of the component
 allowUnsorting | false | Boolean | Defines whether columns are allowed to be unsorted. Default is false.
 binding | null | Object | An el expression that maps to a server side UIComponent instance in a backing bean
 cellSeparator | null | String | Separator text to use in output mode of editable cells with multiple components.
-cloneOnFilter | false | Boolean | Defines if nodes should be cloned on filter via Cloneable interface or Copy-Constructor (CustomNode(CustomNode original) or CustomNode(String type, Object data, TreeNode parent)). Normally the filtered nodes are new instanceof of DefaultTreeNode.
 currentPageReportTemplate | null | String | Template of the currentPageReport UI.
 dataLocale | null | Object | Locale to be used in features such as sorting, defaults to view locale.
 disabledTextSelection | true | Boolean | Disables text selection on row click.
diff --git a/docs/migrationguide/14_0_0.md b/docs/migrationguide/14_0_0.md
index 1446d9dda8..94f22873d6 100644
--- a/docs/migrationguide/14_0_0.md
+++ b/docs/migrationguide/14_0_0.md
@@ -33,3 +33,6 @@
 ## File Upload
   * `org.primefaces.model.file.UploadedFile` now required new overridden method `getWebkitRelativePath()` see: https://github.com/primefaces/primefaces/pull/10272
   * Apache Commons FileUpload deprecated
+
+## TreeTable
+  * Removal of `cloneOnFilter` property, when custom node is used, component will systematically clone node by using either 3 args constructor, copy constructor or `Cloneable#clone` method 
diff --git a/primefaces/src/main/java/org/primefaces/component/treetable/feature/FilterFeature.java b/primefaces/src/main/java/org/primefaces/component/treetable/feature/FilterFeature.java
index 815ad298ff..9022e2996a 100644
--- a/primefaces/src/main/java/org/primefaces/component/treetable/feature/FilterFeature.java
+++ b/primefaces/src/main/java/org/primefaces/component/treetable/feature/FilterFeature.java
@@ -23,18 +23,18 @@
  */
 package org.primefaces.component.treetable.feature;
 
-import javax.el.ELContext;
-import javax.faces.context.FacesContext;
-import javax.faces.event.PhaseId;
 import java.io.IOException;
 import java.lang.reflect.Constructor;
-import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.logging.Logger;
+import javax.el.ELContext;
+import javax.faces.FacesException;
+import javax.faces.context.FacesContext;
+import javax.faces.event.PhaseId;
 
 import org.primefaces.PrimeFaces;
 import org.primefaces.component.treetable.TreeTable;
@@ -110,7 +110,7 @@ public class FilterFeature implements TreeTableFeature {
         collectFilteredRowKeys(context, tt, root, root, filterBy, filterLocale, filteredRowKeys);
 
         // recreate tree node
-        TreeNode filteredValue = cloneTreeNode(tt, root, root.getParent());
+        TreeNode filteredValue = cloneTreeNode(root, root.getParent());
         createFilteredValueFromRowKeys(tt, root, filteredValue, filteredRowKeys);
 
         tt.updateFilteredValue(context, filteredValue);
@@ -199,7 +199,7 @@ public class FilterFeature implements TreeTableFeature {
 
             for (String rk : filteredRowKeys) {
                 if (rk.equals(rowKeyOfChildNode) || rk.startsWith(rowKeyOfChildNode + "_") || rowKeyOfChildNode.startsWith(rk + "_")) {
-                    TreeNode newNode = cloneTreeNode(tt, childNode, filteredNode);
+                    TreeNode newNode = cloneTreeNode(childNode, filteredNode);
                     if (rk.startsWith(rowKeyOfChildNode + "_")) {
                         newNode.setExpanded(true);
                     }
@@ -211,7 +211,7 @@ public class FilterFeature implements TreeTableFeature {
         }
     }
 
-    protected TreeNode cloneTreeNode(TreeTable tt, TreeNode<?> node, TreeNode<?> parent) {
+    protected TreeNode cloneTreeNode(TreeNode<?> node, TreeNode<?> parent) {
         TreeNode clone = null;
 
         // equals check instead of instanceof to allow subclassing
@@ -222,61 +222,47 @@ public class FilterFeature implements TreeTableFeature {
         else if (DefaultTreeNode.class.equals(node.getClass())) {
             clone = new DefaultTreeNode(node.getType(), node.getData(), parent);
         }
-
-        if (clone == null && tt.isCloneOnFilter()) {
-            if (node instanceof Cloneable) {
-                try {
-                    Method cloneMethod = node.getClass().getMethod("clone");
-                    if (cloneMethod != null) {
-                        cloneMethod.setAccessible(true);
-                        clone = (TreeNode) cloneMethod.invoke(node);
-                    }
-                }
-                catch (NoSuchMethodException e) {
-                    LOGGER.warning(node.getClass().getName() + " declares Cloneable but no clone() method found!");
+        else if (node instanceof Cloneable) {
+            try {
+                Method cloneMethod = node.getClass().getMethod("clone");
+                cloneMethod.setAccessible(true);
+                clone = (TreeNode) cloneMethod.invoke(node);
+            }
+            catch (ReflectiveOperationException e) {
+                throw new FacesException("Cloning using " + node.getClass().getSimpleName() + "#clone() failed", e);
+            }
+        }
+        else {
+            try {
+                Constructor<? extends TreeNode> ctor = node.getClass().getConstructor(node.getClass());
+                clone = ctor.newInstance(node);
+                if (parent != null) {
+                    parent.getChildren().add(clone);
                 }
-                catch (InvocationTargetException | IllegalAccessException e) {
-                    LOGGER.warning(node.getClass().getName() + "#clone() not accessible!");
+            }
+            catch (ReflectiveOperationException e) {
+                if (!(e instanceof NoSuchMethodException)) {
+                    throw new FacesException("Calling copy constructor of " + node.getClass().getSimpleName() + " failed: " + e.getMessage());
                 }
             }
-            else {
+
+            if (clone == null) {
                 try {
-                    Constructor<? extends TreeNode> ctor = node.getClass().getConstructor(node.getClass());
-                    clone = ctor.newInstance(node);
-                    if (parent != null) {
-                        parent.getChildren().add(clone);
-                    }
-                }
-                catch (NoSuchMethodException e) {
-                    // ignore
-                }
-                catch (InvocationTargetException | IllegalAccessException | InstantiationException e) {
-                    LOGGER.warning("Could not clone " + node.getClass().getName()
-                            + " via public " + node.getClass().getSimpleName() + "() constructor!");
+                    Constructor<? extends TreeNode> ctor = node.getClass().getConstructor(String.class, Object.class, TreeNode.class);
+                    clone = ctor.newInstance(node.getType(), node.getData(), parent);
                 }
-
-                if (clone == null) {
-                    try {
-                        Constructor<? extends TreeNode> ctor = node.getClass().getConstructor(String.class, Object.class, TreeNode.class);
-                        clone = ctor.newInstance(node.getType(), node.getData(), parent);
-                    }
-                    catch (NoSuchMethodException e) {
-                        // ignore
-                    }
-                    catch (InvocationTargetException | IllegalAccessException | InstantiationException e) {
-                        LOGGER.warning("Could not clone " + node.getClass().getName()
-                                + " via public " + node.getClass().getSimpleName() + "(String type, Object data, TreeNode parent) constructor!");
+                catch (ReflectiveOperationException e) {
+                    if (!(e instanceof NoSuchMethodException)) {
+                        throw new FacesException("Calling constructor " + node.getClass().getSimpleName()
+                                + "(String type, Object data, TreeNode parent) failed", e);
                     }
                 }
             }
-        }
 
-        if (clone == null) {
-            if (node instanceof CheckboxTreeNode) {
-                clone = new CheckboxTreeNode(node.getType(), node.getData(), parent);
-            }
-            else {
-                clone = new DefaultTreeNode(node.getType(), node.getData(), parent);
+            if (clone == null) {
+                throw new FacesException("Custom node requires to implement either Cloneable, "
+                        + "a constructor: " + node.getClass().getSimpleName() + "(String type, Object data, TreeNode parent), "
+                        + "or a copy contructor");
             }
         }
 


Test patch:
diff --git a/primefaces/src/test/java/org/primefaces/component/treetable/feature/TreeTableFilterFeatureTest.java b/primefaces/src/test/java/org/primefaces/component/treetable/feature/TreeTableFilterFeatureTest.java
index cde1c7543b..5c66301826 100644
--- a/primefaces/src/test/java/org/primefaces/component/treetable/feature/TreeTableFilterFeatureTest.java
+++ b/primefaces/src/test/java/org/primefaces/component/treetable/feature/TreeTableFilterFeatureTest.java
@@ -39,9 +39,23 @@ public class TreeTableFilterFeatureTest {
         DefaultTreeNode cloneRoot = new DefaultTreeNode();
         CustomNode<String> customNode = new CustomNode<>("Type", "Name", root);
         TreeTable treeTable = new TreeTable();
-        treeTable.setCloneOnFilter(true);
 
-        TreeNode cloneCustomNode = filterFeature.cloneTreeNode(treeTable, customNode, cloneRoot);
+        TreeNode cloneCustomNode = filterFeature.cloneTreeNode(customNode, cloneRoot);
+
+        Assertions.assertEquals(1, cloneRoot.getChildren().size());
+        Assertions.assertEquals(cloneCustomNode, cloneRoot.getChildren().get(0));
+    }
+
+    @Test
+    public void testCloneTreeNodeWithBuiltInNodeConstructor() {
+        FilterFeature filterFeature = new FilterFeature();
+
+        DefaultTreeNode root = new DefaultTreeNode();
+        DefaultTreeNode cloneRoot = new DefaultTreeNode();
+        DefaultTreeNode<String> regularNode = new DefaultTreeNode<>("Type", "Name", root);
+        TreeTable treeTable = new TreeTable();
+
+        TreeNode cloneCustomNode = filterFeature.cloneTreeNode(regularNode, cloneRoot);
 
         Assertions.assertEquals(1, cloneRoot.getChildren().size());
         Assertions.assertEquals(cloneCustomNode, cloneRoot.getChildren().get(0));