Grails
  1. Grails
  2. GRAILS-3396 Top level task: GORM Improvements
  3. GRAILS-559

Lack of fail fast on validation failure when calling MyDomainClass.save() is very annoying

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: 0.4
    • Fix Version/s: 1.2-M2
    • Component/s: None
    • Labels:
      None

      Description

      When working outside controllers, it is very annoying to find that some domain class has not saved but you did not notice because save() fails silently.

      What's more, if you're creating a class on the fly and call save, you cannot then inspect the errors anyway unless you copy the instance into a variable before calling save().

      This is OK within controllers because they will typically render a view showing the errors property of the object.

      A possible solution:

      When the errors property is populated, set a flag. When accessing the errors property (get) clear the flag. Then make the request/session cleanup code check the flag and if its still set dump out all the validation errors to the log?

        Issue Links

          Activity

          Hide
          Marc Palmer added a comment -

          This was discussed on the mailing list and a consensus reached.

          save() should throw a meaningful exception on validation failure if a convention property such as failFastSave = true is set, and this property should be set on all new generated domain classes, so that old applications do not break but all new classes protect the user from mistakes unless they knowingly disable it.

          Show
          Marc Palmer added a comment - This was discussed on the mailing list and a consensus reached. save() should throw a meaningful exception on validation failure if a convention property such as failFastSave = true is set, and this property should be set on all new generated domain classes, so that old applications do not break but all new classes protect the user from mistakes unless they knowingly disable it.
          Hide
          Mike Cantrell added a comment -

          Using this in the constraints would allow course or fine-grained control. I don't know if this is overkill or not but it might be interesting to consider... For instance:

          // Course Grained:
          static constraints =

          { fatal true // any validation error throws an exception }

          // Fine Grained
          static constraints =

          { email(email:true, fatal:true) // fatal lastName(blank:false) // non-fatal }
          Show
          Mike Cantrell added a comment - Using this in the constraints would allow course or fine-grained control. I don't know if this is overkill or not but it might be interesting to consider... For instance: // Course Grained: static constraints = { fatal true // any validation error throws an exception } // Fine Grained static constraints = { email(email:true, fatal:true) // fatal lastName(blank:false) // non-fatal }
          Hide
          Graeme Rocher added a comment -

          breaking change that maybe we should consider for 2.0 after community discussion

          Show
          Graeme Rocher added a comment - breaking change that maybe we should consider for 2.0 after community discussion
          Hide
          Sharad Jain added a comment -

          Mike's suggestion would work. However, it is sort of all or none approach. Instead it should be conditional and any validate() or save() method should take option on a case by case basis. Depending on use case, you may or may not expect your save to fail.
          For example, if expect validation errors when in controller but not when you are loading seed-data (assumed to be correct) using batch file.

          Show
          Sharad Jain added a comment - Mike's suggestion would work. However, it is sort of all or none approach. Instead it should be conditional and any validate() or save() method should take option on a case by case basis. Depending on use case, you may or may not expect your save to fail. For example, if expect validation errors when in controller but not when you are loading seed-data (assumed to be correct) using batch file.
          Hide
          Marc Palmer added a comment -

          Sharad - I don't understand why you want that. You can just call validate() if you don't want an exception on failure, even if you turn on the global failfast.

          I don't think per-property failfast makes sense as per Mike's suggestion - it is overkill IMO. You will not know whether or not you can trust fail fast on validation failure if some properties have it and some don't. It will be worse than now where there is none.

          Show
          Marc Palmer added a comment - Sharad - I don't understand why you want that. You can just call validate() if you don't want an exception on failure, even if you turn on the global failfast. I don't think per-property failfast makes sense as per Mike's suggestion - it is overkill IMO. You will not know whether or not you can trust fail fast on validation failure if some properties have it and some don't. It will be worse than now where there is none.
          Hide
          Marc Palmer added a comment -

          We had a lot of discussion about this problem in the past. The conclusion we agreed on was:

          1) Add a new Config property: grails.gorm.failfast

          2) By default if not specified, the value of this property is false.

          3) New project template for Config.groovy must set this value to TRUE

          4) Calling save() on any domain objects should throw an exception that displays the errors on the object, if failfast is set to TRUE in Config. Otherwise it should return null (current behaviour).

          This is a non-breaking change for existing users. For all new users this is sane, and saves them hours of WTF? and also prevents accidental flushing of non-discarded objects.

          Once introduced, most people will not turn this feature off - it just adds more code requirements.

          Show
          Marc Palmer added a comment - We had a lot of discussion about this problem in the past. The conclusion we agreed on was: 1) Add a new Config property: grails.gorm.failfast 2) By default if not specified, the value of this property is false. 3) New project template for Config.groovy must set this value to TRUE 4) Calling save() on any domain objects should throw an exception that displays the errors on the object, if failfast is set to TRUE in Config. Otherwise it should return null (current behaviour). This is a non-breaking change for existing users. For all new users this is sane, and saves them hours of WTF? and also prevents accidental flushing of non-discarded objects. Once introduced, most people will not turn this feature off - it just adds more code requirements.
          Hide
          Graeme Rocher added a comment -

          This has been implemented already with save(failOnError:true)

          Show
          Graeme Rocher added a comment - This has been implemented already with save(failOnError:true)

            People

            • Assignee:
              Graeme Rocher
              Reporter:
              Marc Palmer
            • Votes:
              8 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development