Grails

Add nullable/blank constraint vetoing capability to domain constraints

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.0.1
  • Fix Version/s: 1.0.2
  • Component/s: None
  • Labels:
    None

Description

Per discussions on the mailing list, add the capability to veto a nullable / blank constraint check in order to allow custom validation to fire.

Link to thread (contains syntax suggestions): http://www.nabble.com/Nullable-and-vetoing-to15645476.html

Original post by Peter Ledbrook Feb 22, 2008

Hi,

Has anyone else noticed that 'nullable: true' vetoes all the other
constraints even if it passes? So for example:

class Book {
String title
String isbn

static constraints = {
title(nullable: false, blank: false)
isbn(nullable: true, validator: { val -> println ">>> $val" })
}
}

If 'isbn' is null, then the custom validator is not executed. This
can't be right, surely? The same applies to the "blank: true"
constraint if the field value is blank.

I would change this myself, but the unit test actually expects this
behaviour so I want to check why first.

Thanks,

Peter

Attachments

Issue Links

This issue duplicates:
GRAILS-2594 contraint logic is incorrect Critical Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Matt Lentzner added a comment -

This is the same issue as GRAILS-2594 contraint logic incorrect (sorry about the typo) which has already been moved to 1.1 release.

http://jira.codehaus.org/browse/GRAILS-2594

Show
Matt Lentzner added a comment - This is the same issue as GRAILS-2594 contraint logic incorrect (sorry about the typo) which has already been moved to 1.1 release. http://jira.codehaus.org/browse/GRAILS-2594
Hide
Permalink
Mike Hugo added a comment -

Attaching sample application that demonstrates the problem.

Run grails test-app and there are two tests that will fail (and a third that passes). All three should pass...after this ticket is resolved

The basic premise of the example is that a Book has a boolean flag (online). If online == true, then the String property onlineFormatDescription is required...

Show
Mike Hugo added a comment - Attaching sample application that demonstrates the problem. Run grails test-app and there are two tests that will fail (and a third that passes). All three should pass...after this ticket is resolved The basic premise of the example is that a Book has a boolean flag (online). If online == true, then the String property onlineFormatDescription is required...
Hide
Permalink
Graeme Rocher added a comment -

I haven't implemented the ability to control veto'ing but i have changed it so that veto'ing only occurs if validation on nullable and blank failed. This was the premise of the original discussion here http://www.nabble.com/Constraints-behavior-on-blank-strings-td12581281.html#a12581281 and in that discussion this was how we agreed it should behave.

Somehow in the process of implementing it was changed so that veto'ing occured even if nullable and blank passed, which is the real bug here. With these changes your unit tests pass and I have added them to the grails build so that they will always pass in future. If we REALLY need veto control then we can consider adding it for 1.1 or beyond.

Show
Graeme Rocher added a comment - I haven't implemented the ability to control veto'ing but i have changed it so that veto'ing only occurs if validation on nullable and blank failed. This was the premise of the original discussion here http://www.nabble.com/Constraints-behavior-on-blank-strings-td12581281.html#a12581281 and in that discussion this was how we agreed it should behave. Somehow in the process of implementing it was changed so that veto'ing occured even if nullable and blank passed, which is the real bug here. With these changes your unit tests pass and I have added them to the grails build so that they will always pass in future. If we REALLY need veto control then we can consider adding it for 1.1 or beyond.
Hide
Permalink
Sergey Nebolsin added a comment -

Graeme, it was me who implemented this feature and the reason was: if nullable and blank constraint won't veto on passed validation, then we must ensure that all other constraints accept null and blank string as valid values and don't generate additional errors.

Consider the example:

    aField(blank:true,url:true)

For a blank string in this field one would expect only one error "Field cannot be blank" and not the "Field must be a valid url" (and there were several posts in ML and JIRA issues about this expected behavior). Since we're AND-ing constraints and cannot OR them we could achieve this behavior by ensuring that all constraints (except nullable and blank) pass for nulls and blank strings (and all custom user-defined constraints must implement this also). You implemented this recently for email constraint if I recall correctly.

In this case we don't need any veto'ing completely. It has no reason to exist since all constraints must accept nulls and blank strings and there's no reason to stop validation if null or blank string was catched by respective constraint. So, at the end we are near the beginning hitting the problem with blank constraint which was described in my first message in the thread you've referenced: http://www.nabble.com/Constraints-behavior-on-blank-strings-td12581281.html#a12581281.

Am I missing something obvious here?

Show
Sergey Nebolsin added a comment - Graeme, it was me who implemented this feature and the reason was: if nullable and blank constraint won't veto on passed validation, then we must ensure that all other constraints accept null and blank string as valid values and don't generate additional errors. Consider the example: 
    aField(blank:true,url:true)
For a blank string in this field one would expect only one error "Field cannot be blank" and not the "Field must be a valid url" (and there were several posts in ML and JIRA issues about this expected behavior). Since we're AND-ing constraints and cannot OR them we could achieve this behavior by ensuring that all constraints (except nullable and blank) pass for nulls and blank strings (and all custom user-defined constraints must implement this also). You implemented this recently for email constraint if I recall correctly. In this case we don't need any veto'ing completely. It has no reason to exist since all constraints must accept nulls and blank strings and there's no reason to stop validation if null or blank string was catched by respective constraint. So, at the end we are near the beginning hitting the problem with blank constraint which was described in my first message in the thread you've referenced: http://www.nabble.com/Constraints-behavior-on-blank-strings-td12581281.html#a12581281. Am I missing something obvious here?
Hide
Permalink
David Smiley added a comment -

I just ran into this situation. I upgraded a grail 0.6 app to 1.0.1, and this didn't work. That is, I had nullable:true and a custom validator. The validator is never executed anymore. Because it didn't work anymore, my first reaction is that it's a bug... but now I don't think it's a bug. I was able to easily change the code around any way (for the better). Before it had nullable true and then the validator set the value based on other attributes... but that's kind of a hack that isn't needed in my situation after all.

Show
David Smiley added a comment - I just ran into this situation. I upgraded a grail 0.6 app to 1.0.1, and this didn't work. That is, I had nullable:true and a custom validator. The validator is never executed anymore. Because it didn't work anymore, my first reaction is that it's a bug... but now I don't think it's a bug. I was able to easily change the code around any way (for the better). Before it had nullable true and then the validator set the value based on other attributes... but that's kind of a hack that isn't needed in my situation after all.
Hide
Permalink
Graeme Rocher added a comment -

Hi Sergey, I understand the problem its trying to address, but I think the only sensible solution is to do what I did with email and make other constraints ignore blank and null values. It is very surprising for a user that applying blank or nullable results on vetoing the remaining constraints and as I said I don't think it was the behaviour that was agreed on the above thread

Show
Graeme Rocher added a comment - Hi Sergey, I understand the problem its trying to address, but I think the only sensible solution is to do what I did with email and make other constraints ignore blank and null values. It is very surprising for a user that applying blank or nullable results on vetoing the remaining constraints and as I said I don't think it was the behaviour that was agreed on the above thread
Hide
Permalink
Sergey Nebolsin added a comment -

Ok, I agree, but in this case we could completely remove all "vetoing" logic since it becomes useless. All other constraints will pass for nulls and blank strings hence there is no need to veto. And again: in this case blank string will be accepted for property with constraints aProperty(email:true) since blank constraint is not applied by default as nullable. This could be confusing for users I think so at least we should describe this behavior in User Guide properly.

Show
Sergey Nebolsin added a comment - Ok, I agree, but in this case we could completely remove all "vetoing" logic since it becomes useless. All other constraints will pass for nulls and blank strings hence there is no need to veto. And again: in this case blank string will be accepted for property with constraints aProperty(email:true) since blank constraint is not applied by default as nullable. This could be confusing for users I think so at least we should describe this behavior in User Guide properly.
Hide
Permalink
Jesse Jackson added a comment -

What was the actual resolution to this issue? There was a lot of discussion as to possibilities but how was the issue solved?

Show
Jesse Jackson added a comment - What was the actual resolution to this issue? There was a lot of discussion as to possibilities but how was the issue solved?
Hide
Permalink
Graeme Rocher added a comment -

Bulk closing bunch of resolved issues

Show
Graeme Rocher added a comment - Bulk closing bunch of resolved issues

People

Vote (2)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.1#660-r161644) | Report a problem
Powered by a free Atlassian JIRA open source license for Grails project. Try JIRA - bug tracking software for your team.