Grails
  1. Grails
  2. GRAILS-8345

Grails default controller implementation does not properly validate domain objects before attempting to persist to database

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.3.7
    • Fix Version/s: None
    • Labels:
      None

      Description

      When attempting to save a new domain object, the default implementation of the controller's save makes a new instance of the object and calls hibernate to save. If there are any errors (i.e. constraint validation fails), the page is rendered again w/ the list of failing constraints. However, if a user then proceeds to view a list of all saved objects via a call to the controller's list method, the invalid object is still persisted to the database and now the database is corrupted.

      1. constraints fail.jpg
        146 kB
      2. persistence success.jpg
        94 kB

        Activity

        Hide
        Michael Hatch added a comment -

        the default configuration of a scaffolded controller's save method is...

         def save = {
                def locationInstance = new Location(params)
                if (locationInstance.save(flush: true)) {
                    flash.message = "${message(code: 'default.created.message', args: [message(code: 'location.label', default: 'Location'), locationInstance.id])}"
                    redirect(action: "show", id: locationInstance.id)
                }
                else {
                    render(view: "create", model: [locationInstance: locationInstance])
                }
            }
        

        according to documentation: "The save method returns null if validation failed and the instance was not saved and the instance itself if successful..."

        Any call to save will persist no matter the constraints.
        The workaround is validating before calling save:

        def save = {
                def locationInstance = new Location(params)
                if (locationInstance.validate() && locationInstance.save(flush: true)) {
          ...remaining logic
        
        Show
        Michael Hatch added a comment - the default configuration of a scaffolded controller's save method is... def save = { def locationInstance = new Location(params) if (locationInstance.save(flush: true )) { flash.message = "${message(code: ' default .created.message', args: [message(code: 'location.label', default : 'Location'), locationInstance.id])}" redirect(action: "show" , id: locationInstance.id) } else { render(view: "create" , model: [locationInstance: locationInstance]) } } according to documentation: "The save method returns null if validation failed and the instance was not saved and the instance itself if successful..." Any call to save will persist no matter the constraints. The workaround is validating before calling save: def save = { def locationInstance = new Location(params) if (locationInstance.validate() && locationInstance.save(flush: true )) { ...remaining logic
        Hide
        Michael Hatch added a comment -

        not to nitpick, but on a sidenote: the documentation wording is confusing.

        "The save method returns null if validation failed and the instance was not saved and the instance itself if successful..."

        should read as:

        "The save method returns null if validation fails, else returns the instance itself if validation is successful..."

        Show
        Michael Hatch added a comment - not to nitpick, but on a sidenote: the documentation wording is confusing. "The save method returns null if validation failed and the instance was not saved and the instance itself if successful..." should read as: "The save method returns null if validation fails, else returns the instance itself if validation is successful..."
        Hide
        Jeff Scott Brown added a comment -

        I can't reproduce this problem.

        You should not have to call .validate() before calling .save(). The .save() method is supposed to run validation first and then only send the update to the database if validation passes.

        See the attached grails8345.zip app. If you try to create a Person with a name that begins with a lower case letter validation should fail. If you then visit the person list page, the invalid Person should not be there.

        Can you provide any more details on how to reproduce the problem?

        Show
        Jeff Scott Brown added a comment - I can't reproduce this problem. You should not have to call .validate() before calling .save(). The .save() method is supposed to run validation first and then only send the update to the database if validation passes. See the attached grails8345.zip app. If you try to create a Person with a name that begins with a lower case letter validation should fail. If you then visit the person list page, the invalid Person should not be there. Can you provide any more details on how to reproduce the problem?
        Hide
        Michael Hatch added a comment -

        page successfully identifies which constraints are violated

        Show
        Michael Hatch added a comment - page successfully identifies which constraints are violated
        Hide
        Michael Hatch added a comment - - edited

        domain objects is (incorrectly) successfully persisted to database. attempting to load this by id or delete by id causes hibernate errors; database is permanently corrupted.

        Show
        Michael Hatch added a comment - - edited domain objects is (incorrectly) successfully persisted to database. attempting to load this by id or delete by id causes hibernate errors; database is permanently corrupted.
        Hide
        Jeff Scott Brown added a comment -

        Michael,

        I see that constraints are violated. Are you saying that even though constraints are violated, the default scaffolding still saves the object in the database?

        Show
        Jeff Scott Brown added a comment - Michael, I see that constraints are violated. Are you saying that even though constraints are violated, the default scaffolding still saves the object in the database?
        Hide
        Jeff Scott Brown added a comment -

        I see now that you have relationships between domain classes, which may end up being relevant and help reproduce the problem.

        Can you attach an app which demonstrates the behavior?

        Show
        Jeff Scott Brown added a comment - I see now that you have relationships between domain classes, which may end up being relevant and help reproduce the problem. Can you attach an app which demonstrates the behavior?
        Hide
        Michael Hatch added a comment -

        yes. the constraints are violated (which is what I was originally testing). I decided to click on "Agent List" to make sure that nothing was saved. To my surprise, the object is still saved. The only way I could avoid corruption of the database was to first validate, and then save if there were no validation errors.

        Show
        Michael Hatch added a comment - yes. the constraints are violated (which is what I was originally testing). I decided to click on "Agent List" to make sure that nothing was saved. To my surprise, the object is still saved. The only way I could avoid corruption of the database was to first validate, and then save if there were no validation errors.
        Hide
        Michael Hatch added a comment -

        I have exported my project and attached it to this Jira. I'm not totally sure if that is what you were asking.
        However, yes, there are relationships between my domains in the screenshot, but I do not see why any hasMany relationships should affect persistence after constraints have been reported to be violated. I would imagine a domain w/ a single field would exhibit the same behavior.

        To reproduce
        1) attempt to save any domain object w/ constraints against it; be sure to attempt save with said constraints violated in any manner.
        2) after the constraints have been shown to be violated, skip fixing constraints and simply make a call to the controllers "list" method.

        expected behavior: no object was persisted to the database
        actual behavior: invalid object is persisted to the database

        Show
        Michael Hatch added a comment - I have exported my project and attached it to this Jira. I'm not totally sure if that is what you were asking. However, yes, there are relationships between my domains in the screenshot, but I do not see why any hasMany relationships should affect persistence after constraints have been reported to be violated. I would imagine a domain w/ a single field would exhibit the same behavior. To reproduce 1) attempt to save any domain object w/ constraints against it; be sure to attempt save with said constraints violated in any manner. 2) after the constraints have been shown to be violated, skip fixing constraints and simply make a call to the controllers "list" method. expected behavior: no object was persisted to the database actual behavior: invalid object is persisted to the database
        Hide
        Jeff Scott Brown added a comment -

        Michael,

        I have reproduced the problem with your app. That will be helpful.

        I don't see the problematic behavior in the default scaffolding but I do see it in your modified scaffolding. We will track down the specifics and get it resolved.

        Thanks again for the report.

        Show
        Jeff Scott Brown added a comment - Michael, I have reproduced the problem with your app. That will be helpful. I don't see the problematic behavior in the default scaffolding but I do see it in your modified scaffolding. We will track down the specifics and get it resolved. Thanks again for the report.
        Hide
        Jeff Scott Brown added a comment -

        Michael,

        You said "...but I do not see why any hasMany relationships should affect persistence after constraints have been reported to be violated.". The hasMany may very well be relevant as I cannot reproduce the problem with a simple domain class (see the original app I attached). One way that the hasMany may be relevant is it may be that the Agent isn't being saved to the db explicitly but because a Role hasMany Agent, there may be a bug that is causing the invalid Agent to be sent to the database because a valid Role is updated when the session is flushed. I am not saying that is what is happening as I don't know that yet, but it is certainly conceivable that the hasMany is relevant and that is one way it might be.

        Thanks again.

        Show
        Jeff Scott Brown added a comment - Michael, You said "...but I do not see why any hasMany relationships should affect persistence after constraints have been reported to be violated.". The hasMany may very well be relevant as I cannot reproduce the problem with a simple domain class (see the original app I attached). One way that the hasMany may be relevant is it may be that the Agent isn't being saved to the db explicitly but because a Role hasMany Agent, there may be a bug that is causing the invalid Agent to be sent to the database because a valid Role is updated when the session is flushed. I am not saying that is what is happening as I don't know that yet, but it is certainly conceivable that the hasMany is relevant and that is one way it might be. Thanks again.
        Hide
        Michael Hatch added a comment -

        I modified the scaffolding only for the AgentController so that the Agent object would not be persisted. I forgot to re-edit the controller back to the original scaffolding, wherein the "save" method does not first call "validate" before calling save via Hibernate.

        Feel free to replace my code w/ the scaffolded definition of "save".

        Show
        Michael Hatch added a comment - I modified the scaffolding only for the AgentController so that the Agent object would not be persisted. I forgot to re-edit the controller back to the original scaffolding, wherein the "save" method does not first call "validate" before calling save via Hibernate. Feel free to replace my code w/ the scaffolded definition of "save".
        Hide
        Michael Hatch added a comment -

        I have tested the attached Person app that has a simple domain and the bug is not there. I think you may be correct in your theory that the hasMany relationships shared between domain objects may be causing the invalid persistence.

        I'm just no good at hasMany relationships it'd seem.

        Show
        Michael Hatch added a comment - I have tested the attached Person app that has a simple domain and the bug is not there. I think you may be correct in your theory that the hasMany relationships shared between domain objects may be causing the invalid persistence. I'm just no good at hasMany relationships it'd seem.

          People

          • Assignee:
            Jeff Scott Brown
            Reporter:
            Michael Hatch
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Last Reviewed:

              Development