Grails
  1. Grails
  2. GRAILS-9895 Improve Data Binding
  3. GRAILS-6710

A value rejected during the data binding gets lost after validation

    Details

    • Patch Submitted:
      Yes

      Description

      To reproduce the issue I created a simple project (attached) with one domain class Foo with two fields: Date 'bar' and String 'txt'. I changed only datePicker filed to textField with fieldValue in the edit.gsp

      When I open an entity for editing, put some non-parsable date like 'asdfasd' in the 'bar' field and press Update I get validation (actually it is databinding) error message at the top of the page, the input field gets red border and the text in the input field is 'asdfasd'.
      But when I change both fields (foo stil gets unparsable value) and press Update, the same happens except for the 'bar' field value that becomes the value currently stored in the database and not the rejected value.

      The underlying difference between two scenarios is that in the second case OpenSessionInViewInterceptor recognizes entity as a dirty one (new 'bar' value was rejected and didn't change the value of Foo.bar, but the 'txt' field has changed) when flushing the session and tries to save it. That in turn invokes validation on that entity which reinitializes errors object. During that process existing data binding errors get moved to the new Errors object but without rejected value:

      org.codehaus.groovy.grails.orm.hibernate.metaclass.AbstractDynamicPersistentMethod:
      101: errors.rejectValue(fe.getField(), fe.getCode(), fe.getArguments(), fe.getDefaultMessage());

      I believe it should crete new FieldError object instead abd call errors.addError()

      Going to submit a patch in few days

      1. patch.txt
        9 kB
        Denis Yudin

        Activity

        Hide
        Denis Yudin added a comment -

        attached a patch:
        changed

        org.codehaus.groovy.grails.orm.hibernate.metaclass.AbstractDynamicPersistentMethod.setupErrorsProperty(Object)

        to add a FieldError instead of rejecting a value

        Show
        Denis Yudin added a comment - attached a patch: changed org.codehaus.groovy.grails.orm.hibernate.metaclass.AbstractDynamicPersistentMethod.setupErrorsProperty(Object) to add a FieldError instead of rejecting a value
        Hide
        Graeme Rocher added a comment -

        will review patch

        Show
        Graeme Rocher added a comment - will review patch
        Hide
        boillod manuel added a comment -

        Attach a simpliest test case which shows that :

        • first call to validate() lost some attributes of binding errors
        • second call to validate() lost all binding errors
        • first call to validate() clear all error manually added

        I think that the two first cases are easy to resolve. The attached patch should be sufficient (see below).

        But the second is harder. However, it's easiest to live with when we know it.

        The fix proposed should also be applied to the same code in ValidationSupport class.

        Regards

        Show
        boillod manuel added a comment - Attach a simpliest test case which shows that : first call to validate() lost some attributes of binding errors second call to validate() lost all binding errors first call to validate() clear all error manually added I think that the two first cases are easy to resolve. The attached patch should be sufficient (see below). But the second is harder. However, it's easiest to live with when we know it. The fix proposed should also be applied to the same code in ValidationSupport class. Regards
        Hide
        boillod manuel added a comment - - edited

        The most common way to update a domain from a controller is:

        • bind params: def d = new Domain(params)
        • save domain and check for errors: {{if (!d.save()) ... }}

        So, I think that the best solution is to keep unchanged binding errors

        BindingResult errors = new ValidationErrors(target);
        ...
        FieldError fe = (FieldError)o;
        if (fe.isBindingFailure()) {
          errors.addError(fe);
        }
        

        And if we want to remove binding errors, we can call clearErrors(), as described is documentation:http://grails.org/doc/latest/ref/Domain%20Classes/clearErrors.html

        What do you think about this simple patch ?

        Show
        boillod manuel added a comment - - edited The most common way to update a domain from a controller is: bind params: def d = new Domain(params) save domain and check for errors: {{if (!d.save()) ... }} So, I think that the best solution is to keep unchanged binding errors BindingResult errors = new ValidationErrors(target); ... FieldError fe = (FieldError)o; if (fe.isBindingFailure()) { errors.addError(fe); } And if we want to remove binding errors, we can call clearErrors() , as described is documentation: http://grails.org/doc/latest/ref/Domain%20Classes/clearErrors.html What do you think about this simple patch ?
        Hide
        Serge P. Nekoval added a comment -

        For those waiting for 2.3 to see this fixed, the WORKAROUND is:

        When you see scaffolded code like this:

        if (!entityInstance.save(flush: true)) {
           ...
        }
        

        prepend an additional check here:

        if (entityInstance.hasErrors() || !entityInstance.save(flush: true)) {
           ...
        }
        

        when you add this, save() does not get called, therefore you won't get your errors copied, and — your rejected text does not corrupted!

        Consider it a hack, because the code will look really weird.

        Show
        Serge P. Nekoval added a comment - For those waiting for 2.3 to see this fixed, the WORKAROUND is: When you see scaffolded code like this: if (!entityInstance.save(flush: true )) { ... } prepend an additional check here: if (entityInstance.hasErrors() || !entityInstance.save(flush: true )) { ... } when you add this, save() does not get called, therefore you won't get your errors copied, and — your rejected text does not corrupted! Consider it a hack, because the code will look really weird.
        Hide
        boillod manuel added a comment -

        I developed a "smarter" version which retains the binding errors after validation.

        Example:

        //bind request parameters to a new Person
        Person p = new Person(params)
        if (!Domains.save(p)){
            //handle errors
        }
        

        Source code:

        Domains.java
        /**
         * Utility class for domains
         *
         * WORKAROUND for GRAILS-6710
         *
         * @author Manuel Boillod
         *
         */
        public final class Domains {
        
            private static final String ERRORS_PROPERTY = "errors";
            private static final String SAVE_METHOD = "save";
            private static final String VALIDATE_METHOD = "validate";
        
            /**
             * Save domain and merge binding errors and validation errors.
             * @param domain
             * @return the result of call of save() method
             */
            public static <T> T save(T domain){
                return saveOrValidate(domain, SAVE_METHOD);
            }
        
            /**
             * Validate domain and merge binding errors and validation errors
             * @param domain
             * @return the result of call of validate() method
             */
            //TEMP GRAILS-6710
            public static <T> T validate(T domain){
                return saveOrValidate(domain, VALIDATE_METHOD);
            }
        
            /**
             * Save or validate method and merge errors of binding and save.
             * @param domain
             * @param saveOrValidateMethod
             * @return the result of call of save() or validate() method
             */
            private static <T> T saveOrValidate(T domain, String saveOrValidateMethod){
                MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(domain.getClass());
        
                Errors originalErrors = (Errors) mc.getProperty(domain, ERRORS_PROPERTY);
        
                //If domain has already errors, merge existing binding errors with new validation errors
                if (originalErrors.hasErrors()){
                    //Call save() method
                    callSaveOrValidate(mc, domain, saveOrValidateMethod);
        
                    Errors saveErrors = (Errors) mc.getProperty(domain, ERRORS_PROPERTY);
        
                    //Merge errors from original and save errors
                    BindingResult mergedErrors = new ValidationErrors(domain);
        
                    //List of fields with a binding error. We don't add save error for that fields
                    List<String> fieldsWithBindingErrors = new ArrayList<String>();
        
                    //Add binding errors from original errors
                    for (Object o : originalErrors.getFieldErrors()) {
                        FieldError fe = (FieldError)o;
                        if (fe.isBindingFailure()) {
                            mergedErrors.addError(fe);
                            fieldsWithBindingErrors.add(fe.getField());
                        }
                    }
        
                    //Add binding errors from original errors
                    for (Object o : saveErrors.getFieldErrors()) {
                        FieldError fe = (FieldError)o;
                        if (!fieldsWithBindingErrors.contains(fe.getField())) {
                            mergedErrors.addError(fe);
                        }
                    }
        
                    //Assign merged errors
                    mc.setProperty(domain, ERRORS_PROPERTY, mergedErrors);
        
                    //Return null to indicate errors
                    return null;
                } else {
                    //If not yet errors, call simple save or validate
                    return callSaveOrValidate(mc, domain, saveOrValidateMethod);
                }
            }
        
            /**
             * Do call "save()" or "validate" method on given domain
             * @param mc Domain's metaclass
             * @param domain Domain object
             * @param saveOrValidateMethod Method's name
             * @return The results of method called
             */
            private static <T> T callSaveOrValidate(MetaClass mc, T domain, String saveOrValidateMethod) {
                MetaMethod method = mc.getMetaMethod(saveOrValidateMethod, null);
                return (T)method.invoke(domain, null);
            }
        
        }
        
        
        Show
        boillod manuel added a comment - I developed a "smarter" version which retains the binding errors after validation. Example: //bind request parameters to a new Person Person p = new Person(params) if (!Domains.save(p)){ //handle errors } Source code: Domains.java /** * Utility class for domains * * WORKAROUND for GRAILS-6710 * * @author Manuel Boillod * */ public final class Domains { private static final String ERRORS_PROPERTY = "errors" ; private static final String SAVE_METHOD = "save" ; private static final String VALIDATE_METHOD = "validate" ; /** * Save domain and merge binding errors and validation errors. * @param domain * @ return the result of call of save() method */ public static <T> T save(T domain){ return saveOrValidate(domain, SAVE_METHOD); } /** * Validate domain and merge binding errors and validation errors * @param domain * @ return the result of call of validate() method */ //TEMP GRAILS-6710 public static <T> T validate(T domain){ return saveOrValidate(domain, VALIDATE_METHOD); } /** * Save or validate method and merge errors of binding and save. * @param domain * @param saveOrValidateMethod * @ return the result of call of save() or validate() method */ private static <T> T saveOrValidate(T domain, String saveOrValidateMethod){ MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(domain.getClass()); Errors originalErrors = (Errors) mc.getProperty(domain, ERRORS_PROPERTY); //If domain has already errors, merge existing binding errors with new validation errors if (originalErrors.hasErrors()){ //Call save() method callSaveOrValidate(mc, domain, saveOrValidateMethod); Errors saveErrors = (Errors) mc.getProperty(domain, ERRORS_PROPERTY); //Merge errors from original and save errors BindingResult mergedErrors = new ValidationErrors(domain); //List of fields with a binding error. We don't add save error for that fields List< String > fieldsWithBindingErrors = new ArrayList< String >(); //Add binding errors from original errors for ( Object o : originalErrors.getFieldErrors()) { FieldError fe = (FieldError)o; if (fe.isBindingFailure()) { mergedErrors.addError(fe); fieldsWithBindingErrors.add(fe.getField()); } } //Add binding errors from original errors for ( Object o : saveErrors.getFieldErrors()) { FieldError fe = (FieldError)o; if (!fieldsWithBindingErrors.contains(fe.getField())) { mergedErrors.addError(fe); } } //Assign merged errors mc.setProperty(domain, ERRORS_PROPERTY, mergedErrors); //Return null to indicate errors return null ; } else { //If not yet errors, call simple save or validate return callSaveOrValidate(mc, domain, saveOrValidateMethod); } } /** * Do call "save()" or "validate" method on given domain * @param mc Domain's metaclass * @param domain Domain object * @param saveOrValidateMethod Method's name * @ return The results of method called */ private static <T> T callSaveOrValidate(MetaClass mc, T domain, String saveOrValidateMethod) { MetaMethod method = mc.getMetaMethod(saveOrValidateMethod, null ); return (T)method.invoke(domain, null ); } }
        Hide
        Jeff Scott Brown added a comment -
        Show
        Jeff Scott Brown added a comment - This was a validation problem more than a data binding problem. https://github.com/SpringSource/grails-data-mapping/commit/4540ded5cf99a3c045b9f9310021261c389de2a7

          People

          • Assignee:
            Jeff Scott Brown
            Reporter:
            Denis Yudin
          • Votes:
            12 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Last Reviewed:

              Development