Repository: primefaces/primefaces (java)

Issue:
Column: wrong parsing of fieldname from sortby/filterBy VE
**Describe the defect**
As we're migrating all of our columns to use `UIColumn#field` instead of sortBy/filterBy. I recently discovered a bug when sortBy contains an expression like: `#{i18n[row][property]}`, it resolves a fieldname `i18n[row][property]` which obvisouly turn into a bug later on. Same goes for `p:columns` with `#{i18n[row][column[property]]}` (`row][column[property]` being resolved). Basically, our regexes are not restrictive enough, it should handle two very basic cases:

- `#{car.name}` -> name (for static columns)
- `#{car[column.property]}` -> name (for dynamic columns)

Beyond that, it gets too complicated to resolve field

**Environment:**
 - PF Version: 10.0
 - JSF + version:  Mojarra 2.2.20
 - Affected browsers: Chrome

**To Reproduce**
Steps to reproduce the behavior:
1. For static columns: Use a VE like `#{i18n[row][property]}` for sortBy 
2. For dynamic columns: Use a VE like `#{i18n[row][column[property]]}` for sortBy 

**Expected behavior**
Where field is not set, resolving field from these expressions should return null


Pull request:
Fix #7607 - Column: wrong parsing of field from sortby/filterBy VE

Patch:
diff --git a/primefaces/src/main/java/org/primefaces/component/api/DynamicColumn.java b/primefaces/src/main/java/org/primefaces/component/api/DynamicColumn.java
index 77210f2163..258ade6f9e 100644
--- a/primefaces/src/main/java/org/primefaces/component/api/DynamicColumn.java
+++ b/primefaces/src/main/java/org/primefaces/component/api/DynamicColumn.java
@@ -181,7 +181,7 @@ public class DynamicColumn implements UIColumn {
 
     @Override
     public boolean isDynamic() {
-        return columns.isDynamic();
+        return true;
     }
 
     @Override
diff --git a/primefaces/src/main/java/org/primefaces/component/api/UIColumn.java b/primefaces/src/main/java/org/primefaces/component/api/UIColumn.java
index dc3b423403..19aed6e740 100644
--- a/primefaces/src/main/java/org/primefaces/component/api/UIColumn.java
+++ b/primefaces/src/main/java/org/primefaces/component/api/UIColumn.java
@@ -33,6 +33,7 @@ import javax.el.MethodExpression;
 import javax.el.ValueExpression;
 import javax.faces.FacesException;
 import javax.faces.component.UIComponent;
+import javax.faces.component.UIData;
 import javax.faces.context.FacesContext;
 
 import org.primefaces.component.celleditor.CellEditor;
@@ -41,48 +42,44 @@ import org.primefaces.util.LangUtils;
 public interface UIColumn {
 
     /**
-     * Currently there 3 expression type for attributes like filterBy, sortBy:
-     * 1) #{car[column.property]}
-     *    this is a valid expression and resolves to a real value
-     * 2) #{column.property}
-     *    this is a invalid expressions and would need modification before calling the VE
-     *    currently not supported anymore
-     * 3) #{testView.tableModel.getValue(row, column.keyId)}
-     *    this is a valid expression and resolves to a real value
+     * Used to extract bean's property from a value expression in dynamic columns
+     * (e.g #{car[column.property]} = name)
      */
-    Pattern VALUE_EXPRESSION_VALID_SYNTAX = Pattern.compile("^#\\{\\w+\\[(.+)]}$");
+    Pattern DYNAMIC_FIELD_VE_LEGACY_PATTERN = Pattern.compile("^#\\{\\w+\\[([\\w.]+)]}$");
 
     /**
-     * This method is used to extract the optional "field" of SortMeta and FieldMeta,
-     * based on the defined "sortBy" or "filterBy" expression.
+     * Used to extract bean's property from a value expression in static columns (e.g "#{car.year}" = year)
+     */
+    Pattern STATIC_FIELD_VE_LEGACY_PATTERN = Pattern.compile("^#\\{\\w+\\.([\\w.]+)}$");
+
+    /**
+     * Used to extract UIColumn#field if not defined. Supports strictly two kind of expressions:
      *
-     * #{car.name} -> name
-     * #{car[column.property]} -> name
-     * #{testView.tableModel.getValue(row, column.keyId)} -> null
+     * #{car.name} -> name (for static columns)
+     * #{car[column.property]} -> name (for dynamic columns)
      *
      * @param context the {@link FacesContext}
      * @param expression the {@link ValueExpression} like "filterBy" or "sortBy"
-     * @param dynamic if its a dynamic column
-     * @return the "field" name, can be null
+     * @return the "field" name if found, otherwise null
      */
-    static String extractFieldFromValueExpression(FacesContext context, ValueExpression expression, boolean dynamic) {
-        String expressionString = expression.getExpressionString();
+    default String resolveField(FacesContext context, ValueExpression expression) {
+        String exprStr = expression.getExpressionString();
 
-        if (dynamic) {
+        if (isDynamic()) {
             ELContext elContext = context.getELContext();
 
-            Matcher matcher = VALUE_EXPRESSION_VALID_SYNTAX.matcher(expressionString);
+            Matcher matcher = DYNAMIC_FIELD_VE_LEGACY_PATTERN.matcher(exprStr);
             if (matcher.find()) {
-                expressionString = matcher.group(1);
+                exprStr = matcher.group(1);
                 expression = context.getApplication().getExpressionFactory()
-                        .createValueExpression(elContext, "#{" + expressionString  + "}", String.class);
+                        .createValueExpression(elContext, "#{" + exprStr  + "}", String.class);
                 return (String) expression.getValue(elContext);
             }
         }
         else {
-            if (expressionString.startsWith("#{")) {
-                expressionString = expressionString.substring(2, expressionString.indexOf('}')); //Remove #{}
-                return expressionString.substring(expressionString.indexOf('.') + 1); //Remove var
+            Matcher matcher = STATIC_FIELD_VE_LEGACY_PATTERN.matcher(exprStr);
+            if (matcher.find()) {
+                return matcher.group(1);
             }
         }
 
@@ -90,10 +87,10 @@ public interface UIColumn {
     }
 
     /**
-     * This method is used to build a valid {@link ValueExpression} for sorting or filtering, if the "field"
-     * shortcut attribute is defined.
+     * Used to build a valid {@link ValueExpression} using {@link UIData#getVar()} and {@link UIColumn#getField()}
+     * Mostly used if sortBy and/or filterBy are not defined.
      *
-     * field="name" -> #{car.name}
+     * var="car" and field="name" -> #{car.name}
      *
      * @param context the {@link FacesContext}
      * @param var the "var" attribute of the parent table
diff --git a/primefaces/src/main/java/org/primefaces/model/FilterMeta.java b/primefaces/src/main/java/org/primefaces/model/FilterMeta.java
index e899055237..c2c5efb199 100644
--- a/primefaces/src/main/java/org/primefaces/model/FilterMeta.java
+++ b/primefaces/src/main/java/org/primefaces/model/FilterMeta.java
@@ -94,7 +94,7 @@ public class FilterMeta implements Serializable {
         }
 
         if (field == null) {
-            field = UIColumn.extractFieldFromValueExpression(context, filterByVE, column.isDynamic());
+            field = column.resolveField(context, filterByVE);
         }
         else if (filterByVE == null) {
             filterByVE = UIColumn.createValueExpressionFromField(context, var, field);
diff --git a/primefaces/src/main/java/org/primefaces/model/SortMeta.java b/primefaces/src/main/java/org/primefaces/model/SortMeta.java
index 7afc08f45d..7284a53e5d 100644
--- a/primefaces/src/main/java/org/primefaces/model/SortMeta.java
+++ b/primefaces/src/main/java/org/primefaces/model/SortMeta.java
@@ -88,7 +88,7 @@ public class SortMeta implements Serializable, Comparable<SortMeta> {
         }
 
         if (field == null) {
-            field = UIColumn.extractFieldFromValueExpression(context, sortByVE, column.isDynamic());
+            field = column.resolveField(context, sortByVE);
         }
         else if (sortByVE == null) {
             sortByVE = UIColumn.createValueExpressionFromField(context, var, field);


Test patch:
diff --git a/primefaces/src/test/java/org/primefaces/component/datatable/DataTableTest.java b/primefaces/src/test/java/org/primefaces/component/datatable/DataTableTest.java
index 9ea0dd9cdc..766a5b1ff3 100644
--- a/primefaces/src/test/java/org/primefaces/component/datatable/DataTableTest.java
+++ b/primefaces/src/test/java/org/primefaces/component/datatable/DataTableTest.java
@@ -26,6 +26,9 @@ package org.primefaces.component.datatable;
 import de.odysseus.el.util.SimpleContext;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.primefaces.component.api.DynamicColumn;
+import org.primefaces.component.column.Column;
+import org.primefaces.component.columns.Columns;
 import org.primefaces.el.MyBean;
 import org.primefaces.el.MyContainer;
 import org.primefaces.mock.FacesContextMock;
@@ -35,8 +38,6 @@ import javax.el.ValueExpression;
 import javax.faces.context.FacesContext;
 
 import static org.mockito.Mockito.*;
-import org.primefaces.component.api.UIColumn;
-import org.primefaces.component.api.UITable;
 
 public class DataTableTest {
 
@@ -49,20 +50,24 @@ public class DataTableTest {
     @Test
     public void testResolveStaticField() {
         FacesContext context = new FacesContextMock();
-        DataTable table = new DataTable();
+
+        Column column = new Column();
         ValueExpression exprVE = mock(ValueExpression.class);
 
         when(exprVE.getExpressionString()).thenReturn("#{car.year}");
-        Assertions.assertEquals("year", UIColumn.extractFieldFromValueExpression(context, exprVE, false));
+        Assertions.assertEquals("year", column.resolveField(context, exprVE));
 
         when(exprVE.getExpressionString()).thenReturn("#{car.wrapper.year}");
-        Assertions.assertEquals("wrapper.year", UIColumn.extractFieldFromValueExpression(context, exprVE, false));
+        Assertions.assertEquals("wrapper.year", column.resolveField(context, exprVE));
 
         when(exprVE.getExpressionString()).thenReturn("#{car}");
-        Assertions.assertEquals("car", UIColumn.extractFieldFromValueExpression(context, exprVE, false));
+        Assertions.assertNull(column.resolveField(context, exprVE));
 
         when(exprVE.getExpressionString()).thenReturn("car.year");
-        Assertions.assertNull(UIColumn.extractFieldFromValueExpression(context, exprVE, false));
+        Assertions.assertNull(column.resolveField(context, exprVE));
+
+        when(exprVE.getExpressionString()).thenReturn("#{i18n[row][property]}");
+        Assertions.assertNull(column.resolveField(context, exprVE));
     }
 
     @Test
@@ -70,6 +75,8 @@ public class DataTableTest {
         FacesContext context = new FacesContextMock();
         ExpressionFactory expFactory = context.getApplication().getExpressionFactory();
 
+        DynamicColumn column = new DynamicColumn(0, mock(Columns.class), context);
+
         MyBean bean = new MyBean();
         MyContainer container = new MyContainer();
         container.setValue("MyValue");
@@ -78,13 +85,23 @@ public class DataTableTest {
         ((SimpleContext)context.getELContext()).setVariable("column",
                 expFactory.createValueExpression(bean, MyBean.class));
 
-        // old syntax
+        // correct syntax
         ValueExpression exprVE = expFactory.createValueExpression(
                 context.getELContext(), "#{car[column.container.value]}", String.class);
-        Assertions.assertEquals("MyValue", UIColumn.extractFieldFromValueExpression(context, exprVE, true));
+        Assertions.assertEquals("MyValue", column.resolveField(context, exprVE));
+
+        // unsupported syntax
+        exprVE = expFactory.createValueExpression(
+                context.getELContext(), "#{i18n[column][property]}", String.class);
+        Assertions.assertNull(column.resolveField(context, exprVE));
 
-        // new syntax
+        // incorrect syntax
         exprVE = expFactory.createValueExpression(context.getELContext(), "#{column.container.value}", String.class);
-        //Assertions.assertEquals("MyValue", UITable.extractFieldForColumn(context, exprVE, true));
+        Assertions.assertNull(column.resolveField(context, exprVE));
+
+        // unsupported syntax
+        exprVE = expFactory.createValueExpression(
+                context.getELContext(), "#{i18n[row][column[property]]}", String.class);
+        Assertions.assertNull(column.resolveField(context, exprVE));
     }
 }