Merge from develop and update source treatment to align with sample treatment for GitHub Issues 1309 and 1282

Author: labkey-susanh

api/src/org/labkey/api/ApiModule.java

@@ -46,6 +46,7 @@
 import org.labkey.api.data.AbstractForeignKey;
 import org.labkey.api.data.Aggregate;
 import org.labkey.api.data.AtomicDatabaseInteger;
+import org.labkey.api.data.BindingTestCase;
 import org.labkey.api.data.BooleanFormat;
 import org.labkey.api.data.BuilderObjectFactory;
 import org.labkey.api.data.CompareType;
@@ -508,6 +509,7 @@ public void registerServlets(ServletContext servletCtx)
             ApiKeyManager.TestCase.class,
             AppPropsTestCase.class,
             AtomicDatabaseInteger.TestCase.class,
+            BindingTestCase.class,
             BlockingCache.BlockingCacheTest.class,
             CompareType.TestCase.class,
             ContainerDisplayColumn.TestCase.class,

api/src/org/labkey/api/action/BaseApiAction.java

@@ -44,6 +44,7 @@
 import org.labkey.api.view.UnauthorizedException;
 import org.labkey.api.view.ViewContext;
 import org.springframework.beans.MutablePropertyValues;
+import org.springframework.beans.PropertyValues;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.validation.BindException;
 import org.springframework.validation.Errors;
@@ -339,7 +340,7 @@ private FormAndErrors<FORM> defaultPopulateForm() throws Exception
     {
         saveRequestedApiVersion(getViewContext().getRequest(), null);
 
-        BindException errors = defaultBindParameters(getCommand(), getPropertyValues());
+        BindException errors = defaultBindParameters(getPropertyValues());
         FORM form = (FORM)errors.getTarget();
 
         return new FormAndErrors<>(form, errors);
@@ -460,6 +461,16 @@ private FormAndErrors<FORM> populateJSONObjectForm() throws Exception
         }
         saveRequestedApiVersion(getViewContext().getRequest(), jsonObj);
 
+        // Records are immutable, so we can't instantiate the form up front and populate it; instead collect the JSON
+        // properties and construct the record via defaultBindParameters(). Note: record forms can't implement
+        // ApiJsonForm since that relies on mutating an existing instance.
+        if (getCommandClass().isRecord())
+        {
+            PropertyValues values = null == jsonObj ? new MutablePropertyValues() : new JsonPropertyValues(jsonObj);
+            BindException errors = defaultBindParameters(values);
+            return new FormAndErrors<>((FORM)errors.getTarget(), errors);
+        }
+
         FORM form = getCommand();
         BindException errors = populateForm(jsonObj, form);
         return new FormAndErrors<>(form, errors);

api/src/org/labkey/api/action/BaseViewAction.java

@@ -31,6 +31,7 @@
 import org.labkey.api.audit.TransactionAuditProvider;
 import org.labkey.api.data.Container;
 import org.labkey.api.data.ConvertHelper;
+import org.labkey.api.data.ObjectFactory;
 import org.labkey.api.security.User;
 import org.labkey.api.util.HelpTopic;
 import org.labkey.api.util.HttpUtil;
@@ -78,6 +79,9 @@
 import java.util.List;
 import java.util.Map;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
+
+import static org.labkey.api.action.SpringActionController.ERROR_MSG;
 
 public abstract class BaseViewAction<FORM> extends PermissionCheckableAction implements Validator, HasPageConfig, ContainerUser
 {
@@ -122,43 +126,36 @@ protected BaseViewAction()
             setCommandClass(typeBest);
     }
 
-
     protected abstract String getCommandClassMethodName();
 
-
     protected BaseViewAction(@NotNull Class<? extends FORM> commandClass)
     {
         setCommandClass(commandClass);
     }
 
-
     public void setProperties(PropertyValues pvs)
     {
         _pvs = pvs;
     }
 
-
     public void setProperties(Map<?,?> m)
     {
         _pvs = new MutablePropertyValues(m);
     }
 
-
     /* Doesn't guarantee non-null, non-empty */
     public Object getProperty(String key, String d)
     {
         PropertyValue pv = _pvs.getPropertyValue(key);
         return pv == null ? d : pv.getValue();
     }
 
-
     public Object getProperty(Enum<?> key)
     {
         PropertyValue pv = _pvs.getPropertyValue(key.name());
         return pv == null ? null : pv.getValue();
     }
 
-
     public Object getProperty(String key)
     {
         PropertyValue pv = _pvs.getPropertyValue(key);
@@ -170,7 +167,6 @@ public PropertyValues getPropertyValues()
         return _pvs;
     }
 
-
     public static PropertyValues getPropertyValuesForFormBinding(PropertyValues pvs, @NotNull Predicate<String> allowBind)
     {
         if (null == pvs)
@@ -184,7 +180,6 @@ public static PropertyValues getPropertyValuesForFormBinding(PropertyValues pvs,
         return ret;
     }
 
-
     /// Some characters can be mishandled by the browser in multipart/formdata requests (e.g. doublequote and backslask).
     /// We support an encoding from fields to avoid these characters, see {@link PageFlowUtil#encodeFormName} and {@link PageFlowUtil#decodeFormName}.
     static public class ViewActionParameterPropertyValues extends ServletRequestParameterPropertyValues
@@ -218,7 +213,6 @@ public ModelAndView handleRequest(@NotNull HttpServletRequest request, @NotNull
         return handleRequest();
     }
 
-
     private void handleSpecialProperties()
     {
         _robot = PageFlowUtil.isRobotUserAgent(getViewContext().getRequest().getHeader("User-Agent"));
@@ -260,55 +254,47 @@ private boolean hasStringValue(String propertyName)
 
     public abstract ModelAndView handleRequest() throws Exception;
 
-
     @Override
     public void setPageConfig(PageConfig page)
     {
         _pageConfig = page;
     }
 
-
     @Override
     public Container getContainer()
     {
         return getViewContext().getContainer();
     }
 
-
     @Override
     public User getUser()
     {
         return getViewContext().getUser();
     }
 
-
     @Override
     public PageConfig getPageConfig()
     {
         return _pageConfig;
     }
 
-
     public void setTitle(String title)
     {
         assert null != getPageConfig() : "action not initialized property";
         getPageConfig().setTitle(title);
     }
 
-
     public void setHelpTopic(String topicName)
     {
         setHelpTopic(new HelpTopic(topicName));
     }
 
-
     public void setHelpTopic(HelpTopic topic)
     {
         assert null != getPageConfig() : "action not initialized properly";
         getPageConfig().setHelpTopic(topic);
     }
 
-
     protected Object newInstance(Class<?> c)
     {
         try
@@ -324,7 +310,6 @@ protected Object newInstance(Class<?> c)
         }
     }
 
-
     protected @NotNull FORM getCommand(HttpServletRequest request) throws Exception
     {
         FORM command = (FORM) createCommand();
@@ -335,13 +320,11 @@ protected Object newInstance(Class<?> c)
         return command;
     }
 
-
     protected @NotNull FORM getCommand() throws Exception
     {
         return getCommand(getViewContext().getRequest());
     }
 
-
     //
     // PARAMETER BINDING
     //
@@ -353,6 +336,45 @@ protected Object newInstance(Class<?> c)
         return defaultBindParameters(form, getCommandName(), params);
     }
 
+    /**
+     * Bind request parameters to the action's command class, dispatching to record binding when that class is a Java
+     * record. Records are immutable (no setters), so instead of instantiating the form and populating it via Spring data
+     * binding we collect the property values into a map and construct the record via its {@link ObjectFactory}. Callers
+     * that previously invoked {@code defaultBindParameters(getCommand(), params)} directly should prefer this method so
+     * they transparently gain record support.
+     */
+    public @NotNull BindException defaultBindParameters(PropertyValues params) throws Exception
+    {
+        Class<?> commandClass = getCommandClass();
+        return commandClass.isRecord() ? bindParametersToRecord(commandClass, params, getCommandName()) : defaultBindParameters(getCommand(), params);
+    }
+
+    // Simple binding for Java records: provides binding errors for missing primitive parameters and type conversions
+    // failures. Currently, no support for array or list parameter types.
+    public static <R> BindException bindParametersToRecord(Class<R> recordClass, PropertyValues pvs, String commandName)
+    {
+        // Note: We don't support record-based forms implementing HasAllowBindParameter since we must populate all
+        // properties at record construction time and therefore can't invoke allowBindParameter() prior to that.
+        PropertyValues m = getPropertyValuesForFormBinding(pvs, HasAllowBindParameter.getDefaultPredicate());
+        ObjectFactory<R> factory = ObjectFactory.Registry.getFactory(recordClass);
+        Map<String, Object> map = m.stream()
+            .filter(pv -> pv.getValue() != null)
+            .collect(Collectors.toMap(PropertyValue::getName, PropertyValue::getValue));
+        BindException errors;
+        try
+        {
+            R record = factory.fromMap(map);
+            errors = new NullSafeBindException(record, commandName);
+        }
+        catch (IllegalArgumentException | ConversionException e)
+        {
+            // Missing primitive parameter or type conversion error. We have no instance to bind to, so report a global
+            // error with details.
+            errors = new NullSafeBindException(new Object(), commandName);
+            errors.reject(ERROR_MSG, e.getMessage());
+        }
+        return errors;
+    }
 
     public static @NotNull BindException defaultBindParameters(Object form, String commandName, PropertyValues params)
     {
@@ -398,28 +420,28 @@ protected Object newInstance(Class<?> c)
             // Maybe we should propagate exception and return SC_BAD_REQUEST (in ExceptionUtil.handleException())
             // most POST handlers check errors.hasErrors(), but not all GET handlers do
             BindException errors = new BindException(command, commandName);
-            errors.reject(SpringActionController.ERROR_MSG, "Error binding property: " + x.getPropertyName());
+            errors.reject(ERROR_MSG, "Error binding property: " + x.getPropertyName());
             return errors;
         }
         catch (NumberFormatException x)
         {
             // Malformed array parameter throws this exception, unfortunately. Just reject the request. #21931
             BindException errors = new BindException(command, commandName);
-            errors.reject(SpringActionController.ERROR_MSG, "Error binding array property; invalid array index (" + x.getMessage() + ")");
+            errors.reject(ERROR_MSG, "Error binding array property; invalid array index (" + x.getMessage() + ")");
             return errors;
         }
         catch (NegativeArraySizeException x)
         {
             // Another malformed array parameter throws this exception. #23929
             BindException errors = new BindException(command, commandName);
-            errors.reject(SpringActionController.ERROR_MSG, "Error binding array property; negative array size (" + x.getMessage() + ")");
+            errors.reject(ERROR_MSG, "Error binding array property; negative array size (" + x.getMessage() + ")");
             return errors;
         }
         catch (IllegalArgumentException x)
         {
             // General bean binding problem. #23929
             BindException errors = new BindException(command, commandName);
-            errors.reject(SpringActionController.ERROR_MSG, "Error binding property; (" + x.getMessage() + ")");
+            errors.reject(ERROR_MSG, "Error binding property; (" + x.getMessage() + ")");
             return errors;
         }
     }

api/src/org/labkey/api/action/ConfirmAction.java

@@ -112,7 +112,7 @@ protected String getCommandClassMethodName()
 
     public BindException bindParameters(PropertyValues m) throws Exception
     {
-        return defaultBindParameters(getCommand(), m);
+        return defaultBindParameters(m);
     }
 
     /**

api/src/org/labkey/api/action/FormApiAction.java

@@ -79,7 +79,7 @@ protected ModelAndView getPrintView(FORM form, BindException errors) throws Exce
 
     protected BindException bindParameters(PropertyValues pvs) throws Exception
     {
-        return SimpleViewAction.defaultBindParameters(getCommand(), getCommandName(), pvs);
+        return defaultBindParameters(pvs);
     }
 
     /**

api/src/org/labkey/api/action/FormViewAction.java

@@ -17,22 +17,18 @@
 package org.labkey.api.action;
 
 import org.jetbrains.annotations.NotNull;
-import org.labkey.api.data.ObjectFactory;
 import org.labkey.api.miniprofiler.MiniProfiler;
 import org.labkey.api.miniprofiler.Timing;
 import org.labkey.api.util.ExceptionUtil;
 import org.labkey.api.util.URLHelper;
 import org.labkey.api.view.HttpView;
-import org.springframework.beans.PropertyValue;
 import org.springframework.beans.PropertyValues;
 import org.springframework.validation.BindException;
 import org.springframework.validation.Errors;
 import org.springframework.validation.ObjectError;
 import org.springframework.web.servlet.ModelAndView;
 
 import java.util.List;
-import java.util.Map;
-import java.util.stream.Collectors;
 
 /**
  * Is this better than BaseCommandController?  Probably not, but it understands TableViewForm.
@@ -149,22 +145,7 @@ protected String getCommandClassMethodName()
 
     public BindException bindParameters(PropertyValues m) throws Exception
     {
-        Class<?> commandClass = getCommandClass();
-        return commandClass.isRecord() ? defaultBindParametersToRecord(commandClass, m) : defaultBindParameters(getCommand(), m);
-    }
-
-    // Simple binding for Java records: no support for binding errors, arrays, lists, etc.
-    private <R> BindException defaultBindParametersToRecord(Class<R> recordClass, PropertyValues pvs)
-    {
-        // Note: We don't support record-based forms implementing HasAllowBindParameter since we must populate all
-        // properties at record construction time and therefore can't invoke allowBindParameter() prior to that.
-        PropertyValues m = getPropertyValuesForFormBinding(pvs, HasAllowBindParameter.getDefaultPredicate());
-        ObjectFactory<R> factory = ObjectFactory.Registry.getFactory(recordClass);
-        Map<String, Object> map = m.stream()
-            .filter(pv -> pv.getValue() != null)
-            .collect(Collectors.toMap(PropertyValue::getName, PropertyValue::getValue));
-        R record = factory.fromMap(map);
-        return new NullSafeBindException(record, "Form");
+        return defaultBindParameters(m);
     }
 
     @Override

api/src/org/labkey/api/action/PermissionCheckableAction.java

@@ -258,7 +258,6 @@ private void _checkActionPermissions(Set<Role> contextualRoles) throws Unauthori
             throw new DeprecatedActionException(actionClass);
     }
 
-
     private void checkPermissionsAndTermsOfUse(Set<Role> contextualRoles, boolean isSendBasic) throws UnauthorizedException
     {
         checkActionPermissions(contextualRoles);
@@ -267,7 +266,6 @@ private void checkPermissionsAndTermsOfUse(Set<Role> contextualRoles, boolean is
             verifyTermsOfUse(isSendBasic);
     }
 
-
     /**
      * Check if terms of use are ever required for this request. If so, enumerate all the terms-of-use providers and ask
      * each to verify terms are set via its custom mechanism. If a provider's terms are active and the user hasn't yet
@@ -289,7 +287,6 @@ protected void verifyTermsOfUse(boolean isSendBasic) throws RedirectException
             provider.verifyTermsOfUse(context, isBasicAuth);
     }
 
-
     /**
      * Actions may provide a set of {@link Role}s used during permission checking
      * or null if no contextual roles apply.

api/src/org/labkey/api/action/SimpleViewAction.java

@@ -114,7 +114,7 @@ protected String getCommandClassMethodName()
 
     public BindException bindParameters(PropertyValues pvs) throws Exception
     {
-        return defaultBindParameters(getCommand(), pvs);
+        return defaultBindParameters(pvs);
     }
 
     public void validate(FORM form, BindException errors)