A lot of failing tests and functionalities after upgrade to 2.4.3 because of getters are not nullabl

[droggo]

Hi,

plenty of tests and functionalities of our apps fail after upgrade from 2.4.2 to 2.4.3.
This is mostly caused by command objects with methods which names start with "getXxx". They are treated as validateable not nullable properties. Not only command object has errors on those 'properties' but sometimes also database queries or other not trivial operations are called just to check if method returns something. This seems like a big change and probably not only for our apps. Is this not to dangerous?

Maybe 'transient' methods should be supported in @Validateable objects?

thanks, droggo

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/63fe4332-f91f-423f-9cfd-6a8fb776f4ee%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[droggo]

It seems that even @Validateable(nullable = true) will not help and will not revert to 2.3.x behaviour, because nullable:true is added and getter is called during validation

droggo

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/c7c40b37-404d-4650-a08c-6da69237ed27%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Jeff Scott Brown-3]

Please file a JIRA and describe the details. 

JSB

Sent from my iPhone

On Aug 1, 2014, at 3:05 PM, [hidden email] wrote:

Hi,

plenty of tests and functionalities of our apps fail after upgrade from 2.4.2 to 2.4.3.
This is mostly caused by command objects with methods which names start with "getXxx". They are treated as validateable not nullable properties. Not only command object has errors on those 'properties' but sometimes also database queries or other not trivial operations are called just to check if method returns something. This seems like a big change and probably not only for our apps. Is this not to dangerous?

Maybe 'transient' methods should be supported in @Validateable objects?

thanks, droggo

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/63fe4332-f91f-423f-9cfd-6a8fb776f4ee%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/662D07BE-4ADB-4372-A8F0-D340BCB1F7CB%40pivotal.io.
For more options, visit https://groups.google.com/d/optout.

[Jeff Scott Brown-3]

On August 1, 2014 at 3:05:44 PM, [hidden email] ([hidden email]) wrote:

Hi,

plenty of tests and functionalities of our apps fail after upgrade from 2.4.2 to 2.4.3.
This is mostly caused by command objects with methods which names start with "getXxx". They are treated as validateable not nullable properties. 

When I replied earlier I was reading/typing on my phone and had not read the original note carefully enough.  I apologize for that.

Command object properties are supposed to be nullable:false by default.  If you define a method like getFirstName(), you are defining a property (that is just 1 way to define a property).  There have been some bugs around this but that is the documented and intended behavior.  One of those bugs was recently resolved and I suspect that you were taking advantage of the bug before and now that it is fixed, your code might need to be addressed to deal with that.  

Not only command object has errors on those 'properties' but sometimes also database queries or other not trivial operations are called just to check if method returns something. 

I don’t think that the framework every sends a query to the database in order to check if a method returns something.  Can you elaborate on that?

Maybe 'transient' methods should be supported in @Validateable objects?

That does not make sense to me.  I don’t think transient methods should be supported.  Even if they were, that wouldn’t change how nullable properties are handled.

JSB

-- 
Jeff Scott Brown 
[hidden email]pivotal.io

Find The Cause ~ Find The Cure 
http://www.autismspeaks.org/ 

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/etPan.53dd2f4f.6b8b4567.323f%40JSB-MBP-August-2012.local.
For more options, visit https://groups.google.com/d/optout.

[droggo]

Hi Jeff,

thank you for your answers. I completely understand approach with validateable:false and properties. I also know that getFirstName defines a 'property' in groovy. For us the problem is the big difference of default behaviour between grails versions. I will try to make few examples below. I will also include database access problem.

Lets suppose we have a Manager and Employee domains. Manager has many employees Relation may be active/inactive. When we need to define a page which defines manager details we would create a controller with command object as argument of most methods. Command object would look like:

@Validateable
ManagerCommand {
    Manager manager

    Collection getEmployees(){
        Employee.findAllByManagerAndActive(manager, true)
    }

    int getEmployeeCount(){
        Employee.countAllByManagerAndActive(manager, true)
    }
   
    static constraints = {
        manager nullable:false
    }
}

Validation works fine in grails 2.4.2. After migrating from 2.3.x we no longer need nullable:false, because it is default in 2.4.x, but I will need it later
managerCommand.constraints will contain only "manager" with nullable:false constraint. If we use @Validateable(nullable=true), it will be the same.

After migration to 2.4.3 validation will cause getEmployees() and getEmployeeCount() to be executed though.
constraints will contain manager, employee and employeeCount properties with nullable:false constraint. If we use @Validateable(nullable:true) it will have manager with nullable:false and employeeCount and employees with nullable:true.
Because we have constraint defined for properties employees and employeeCount, getters will be called during validation which for us causes a lot of problems during migration.

Why we add such methods to command objects? Because command object is stateful, contains request binded data and was always a best place to keep business logic. If transaction is needed, service can be called from CO. This was our approach to build all apps, please let us know if this is invalid. It is also commonly described by other users, to use CO for business logic, for example: http://groovy.dzone.com/articles/grails-best-practices

What 'transient' would allow us to, is to have the same functionality from 2.3.x in 2.4.x, because we would just mark those methods which we don't want to be treated as properties, and we would not have issues with validation.

I can create JIRA with examples if this will help.

thanks, droggo

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/1a69c503-0f8b-4ba4-9604-d81367db3410%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[lmivan]

I completely agree with Droggo, we use the command objects the same way as he does and this bug fix is a breaking change for us.

I think you should reconsider this or at least provide a way such as transients or something like that. If not, the only solution would be to rename this methods:

Instead of using manager.employees, manager.employeeCount.  We can use manager.fetchEmployees(), manager.fetchEmployeeCount().
I prefer the first one.

Regards, Iván.

--
Sent from my Nexus 10

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/CABRkPEovqPo44NtqL4Jsr%2B8bbDXTbZndAWpDUOOpd%2B6Qjj%3DJMg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Jeff Scott Brown-3]

On August 3, 2014 at 2:56:37 AM, Iván López ([hidden email]) wrote:
> 
> I think you should reconsider this or at least provide a way such as
> transients or something like that. If not, the only solution would be to
> rename this methods:
>

You have both suggested using transient.  I don’t understand what transient has to do with this?  Are you both suggesting that only persistent properties be constrained?  If so, I don’t think that is a good idea.  Can you elaborate?



JSB

--
Jeff Scott Brown
[hidden email]

Autism Strikes 1 in 166  
Find The Cause ~ Find The Cure
http://www.autismspeaks.org/


--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/etPan.53de54f6.29fdfbb9.1029%40JSB-iMac-2011.local.
For more options, visit https://groups.google.com/d/optout.

[Jeff Scott Brown-3]

On August 3, 2014 at 2:56:37 AM, Iván López ([hidden email]) wrote:
> I completely agree with Droggo, we use the command objects the same way as
> he does and this bug fix is a breaking change for us.
>  
> I think you should reconsider this or at least provide a way such as
> transients or something like that. If not, the only solution would be to
> rename this methods:
> 

All of your command object properties are supposed to be nullable:false by default.  If you define something like this:

@Validateable
class SomeCommand{
    String propOne

    void setPropTwo(String s) {
       // ...
    }

    String getPropTwo() {
        // ...
    }

    String getPropThree() {
        // ...
    }
}

All 3 of those properties are supposed to be nullable:false.  Grails turns that into something like this:

@Validateable
class SomeCommand{
    String propOne

    void setPropTwo(String s) {
       // ...
    }

    String getPropTwo() {
        // ...
    }

    String getPropThree() {
        // ...
    }

    static constraints = {
        propOne nullable: false
        propTwo nullable: false
        propThree nullable: false
    }
}

If you write the class like this:

@Validateable(nullable=true)
class SomeCommand{
    String propOne

    void setPropTwo(String s) {
       // ...
    }

    String getPropTwo() {
        // ...
    }

    String getPropThree() {
        // ...
    }
}

Grails turns that into something like this:

@Validateable
class SomeCommand{
    String propOne

    void setPropTwo(String s) {
       // ...
    }

    String getPropTwo() {
        // ...
    }

    String getPropThree() {
        // ...
    }

    static constraints = {
        propOne nullable: true
        propTwo nullable: true
        propThree nullable: true
    }
}

I think that is all valid and consistent with how things are supposed to work.  At validation time, the values of all of those properties need to be retrieved from the object in order to do the validation.  It sounds like you guys have properties for which you do not want their values retrieved at validation time.  One thing we can look at is if @Validateable(nullable=true) is used, then we can have the compiler not add the nullable:true constraint for the property and just leave it out altogether.  If that worked out, and you didn't apply any other constraints to the property, then the property value would not need to be retrieved at validation time.  If the framework behaved that way, would that work well of the scenarios you are concerned about?





JSB

--
Jeff Scott Brown
[hidden email]

Autism Strikes 1 in 166  
Find The Cause ~ Find The Cure
http://www.autismspeaks.org/


--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/etPan.53de585c.3ae3fb54.1029%40JSB-iMac-2011.local.
For more options, visit https://groups.google.com/d/optout.

[droggo]

@Validateable(nullable=true) which would not add "nullable:true" would do the trick, it would be consistent with the behaviour from 2.3.x and also 2.4.2.
In fact this is required to make following part of upgrade guide truth: "If you wish to retain the old behavior, you can do so on a per-command object basis by using the @Validateable constraint explicitly and passing the nullable: true argument" - http://grails.org/doc/2.4.x/guide/upgradingFrom23.html

"transient" is just a reference to functionality from domain, but I (probably we) don't mean persistent properties. I meant that such properties should not be nullable:false by default. It may be different keyword or something like @NotValidateable annotation. Still this annotation may be helpful to define properties have nothing in constraints even with @Validateable(nullable=false) option.

I can create JIRA with tests for above if this will help

thank you, droggo

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/d7cbc40f-71a9-4707-b60c-4ac8c0eaf77c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Jeff Scott Brown-3]

On August 3, 2014 at 11:00:48 AM, [hidden email] ([hidden email]) wrote:

I can create JIRA with tests for above if this will help

I started looking at this this afternoon.  See https://jira.grails.org/browse/GRAILS-11625 and https://github.com/grails/grails-core/commit/2423424909cf933eddd85aee503a9dbe551b0791?w=true.  If you want to work up any further tests to help validate the behavior, that would be great.

JSB

-- 
Jeff Scott Brown 
[hidden email]pivotal.io

Find The Cause ~ Find The Cure 
http://www.autismspeaks.org/ 

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/etPan.53de91df.6b8b4567.3b05%40JSB-MBP-August-2012.local.
For more options, visit https://groups.google.com/d/optout.

[droggo]

Thank you for the fast change Jeff. I added small pull request to the Spec. I also described one more concern in the pull request description:
If we add constraint "age validator:{val -> val > 0}" framework automatically adds "nullable:false" constraint. This happens both in 2.3.x and in 2.4.x. Albo for both @Validatateable and @Validateable(nullable = true). Theoretically it is not valid, because custom validator may not require nullable:false, and I couldn't find such functionality description in docs

droggo

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/1e7b122c-5167-468a-a46a-958b420b7bcc%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Jeff Scott Brown-3]

On August 4, 2014 at 1:48:38 AM, [hidden email] ([hidden email]) wrote:

If we add constraint "age validator:{val -> val > 0}" framework automatically adds "nullable:false" constraint. This happens both in 2.3.x and in 2.4.x. Albo for both @Validatateable and @Validateable(nullable = true). Theoretically it is not valid, because custom validator may not require nullable:false, and I couldn't find such functionality description in docs

If that is happening for @Validateable(nullable=true) then that is a bug and we can fix that but for @Validateable() (without nullable=true), I think that is the correct behavior.  All properties are by default supposed to be configured with nullable:false unless you express otherwise.

JSB

-- 
Jeff Scott Brown 
[hidden email]pivotal.io

Find The Cause ~ Find The Cure 
http://www.autismspeaks.org/ 

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/etPan.53df7a82.6b8b4567.5ae5%40JSB-MBP-August-2012.local.
For more options, visit https://groups.google.com/d/optout.

[Jeff Scott Brown-3]

On August 4, 2014 at 1:48:38 AM, [hidden email] ([hidden email]) wrote:

Thank you for the fast change Jeff. I added small pull request to the Spec. I also described one more concern in the pull request description:
If we add constraint "age validator:{val -> val > 0}" framework automatically adds "nullable:false" constraint. This happens both in 2.3.x and in 2.4.x. Albo for both @Validatateable and @Validateable(nullable = true). Theoretically it is not valid, because custom validator may not require nullable:false, and I couldn't find such functionality description in docs

I think the assertion you added at https://github.com/grails/grails-core/commit/3dcab35ba5fcae0f38d1b97934077acfd0b0393b#diff-ad85ae603aa15d04fef1f20cb95c59deR166 is consistent with what happens but inconsistent with what should happen.  I think the age property should in fact be nullable since the class is marked with @Validateable(nullable=true).  Is that correct?

JSB

-- 
Jeff Scott Brown 
[hidden email]pivotal.io

Find The Cause ~ Find The Cure 
http://www.autismspeaks.org/ 

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/etPan.53df9487.66334873.5ae5%40JSB-MBP-August-2012.local.
For more options, visit https://groups.google.com/d/optout.

[Jeff Scott Brown-3]

On August 4, 2014 at 1:48:38 AM, [hidden email] ([hidden email]) wrote:

> Thank you for the fast change Jeff. I added small pull request to the Spec.
> I also described one more concern in the pull request description:
> If we add constraint "age validator:{val -> val > 0}" framework
> automatically adds "nullable:false" constraint. This happens both in 2.3.x
> and in 2.4.x. Albo for both @Validatateable and @Validateable(nullable =
> true). Theoretically it is not valid, because custom validator may not
> require nullable:false, and I couldn't find such functionality description
> in docs
>  
> droggo
> 

Let me summarize what I think should happen. 

In all cases, if nullable:true or nullable:false is explicitly expressed in the constraints block, it should be respected.

For classes marked @Validateable() or @Validateable(nullable=false):

- Any property that is not configured with a nullable constraint should be configured with nullable:false

For classes marked with @Validateable(nullable=true):

- Any constrained property that is not configured with a nullable constraint should be configured with nullable:true 
- Any unconstrained property should be left unconstrained and as such, that properties' value will not need to be retrieved at validation time.

What say ye?  I have code worked up that satisfies those requirements.



JSB

--  
Jeff Scott Brown  
[hidden email]

Find The Cause ~ Find The Cure  
http://www.autismspeaks.org/

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/etPan.53df983b.2ae8944a.5ae5%40JSB-MBP-August-2012.local.
For more options, visit https://groups.google.com/d/optout.

[droggo]

I think the assertion you added at https://github.com/grails/grails-core/commit/3dcab35ba5fcae0f38d1b97934077acfd0b0393b#diff-ad85ae603aa15d04fef1f20cb95c59deR166 is consistent with what happens but inconsistent with what should happen.  I think the age property should in fact be nullable since the class is marked with @Validateable(nullable=true).  Is that correct?

Yes, you are right. "constraints.age.nullable == false" this line was added because of current functionality

In all cases, if nullable:true or nullable:false is explicitly expressed in the constraints block, it should be respected.

For classes marked @Validateable() or @Validateable(nullable=false):

- Any property that is not configured with a nullable constraint should be configured with nullable:false

For classes marked with @Validateable(nullable=true):

- Any constrained property that is not configured with a nullable constraint should be configured with nullable:true 
- Any unconstrained property should be left unconstrained and as such, that properties' value will not need to be retrieved at validation time.

What say ye?  I have code worked up that satisfies those requirements.

This is fine for us. Our way of coding will not have any issue with that, because we always defined nullable:false if it was needed. This functionality is not compliant with 2.3.x though and may cause issue for some upgrades - when nullable:false was not added by user but is needed. 2.3.x added it always if property was constrained.

I can adapt Spec this evening, if you didn't do that already

thanks

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/7401ef1f-ff23-4de5-97e4-1be7c18c05af%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[longwa]

The original discussion around this change was to have a way to mimic the behavior of pre-2.4 in order to ease the transition. Prior to 2.4, if you had a property that was unconstrained in a command object, it was by default nullable: true. However, as soon as you added a constraint of any kind, it became nullable: false and you had to explicitly add the nullable: true constraint (IIRC). Personally, I never felt that behavior was very intuitive but it was easy enough to handle.

Many options were discussed including a config flag that would make it behave like 2.3 and earlier. Everyone agreed another config option would suck so we settled on this approach.

The problem now is that the @Validateable(nullable=true) is a much stronger statement than what we were originally shooting for (i.e. backwards compatibility) and implies a slightly different contract for things that are using it.

I think the proposal by Jeff above is the desired behavior long term, but it's worth pointing out that it does not provide any kind of backwards compatibility per the original concerns.

-Aaron

On Monday, August 4, 2014 11:15:02 AM UTC-4, [hidden email] wrote:

I think the assertion you added at <a href="https://github.com/grails/grails-core/commit/3dcab35ba5fcae0f38d1b97934077acfd0b0393b#diff-ad85ae603aa15d04fef1f20cb95c59deR166" target="_blank" onmousedown="this.href='https://www.google.com/url?q\75https%3A%2F%2Fgithub.com%2Fgrails%2Fgrails-core%2Fcommit%2F3dcab35ba5fcae0f38d1b97934077acfd0b0393b%23diff-ad85ae603aa15d04fef1f20cb95c59deR166\46sa\75D\46sntz\0751\46usg\75AFQjCNFNO7l8eKEms8ostGXvvv_1bhTTMw';return true;" onclick="this.href='https://www.google.com/url?q\75https%3A%2F%2Fgithub.com%2Fgrails%2Fgrails-core%2Fcommit%2F3dcab35ba5fcae0f38d1b97934077acfd0b0393b%23diff-ad85ae603aa15d04fef1f20cb95c59deR166\46sa\75D\46sntz\0751\46usg\75AFQjCNFNO7l8eKEms8ostGXvvv_1bhTTMw';return true;">https://github.com/grails/grails-core/commit/3dcab35ba5fcae0f38d1b97934077acfd0b0393b#diff-ad85ae603aa15d04fef1f20cb95c59deR166 is consistent with what happens but inconsistent with what should happen.  I think the age property should in fact be nullable since the class is marked with @Validateable(nullable=true).  Is that correct?

Yes, you are right. "constraints.age.nullable == false" this line was added because of current functionality

In all cases, if nullable:true or nullable:false is explicitly expressed in the constraints block, it should be respected.

For classes marked @Validateable() or @Validateable(nullable=false):

- Any property that is not configured with a nullable constraint should be configured with nullable:false

For classes marked with @Validateable(nullable=true):

- Any constrained property that is not configured with a nullable constraint should be configured with nullable:true 
- Any unconstrained property should be left unconstrained and as such, that properties' value will not need to be retrieved at validation time.

What say ye?  I have code worked up that satisfies those requirements.

This is fine for us. Our way of coding will not have any issue with that, because we always defined nullable:false if it was needed. This functionality is not compliant with 2.3.x though and may cause issue for some upgrades - when nullable:false was not added by user but is needed. 2.3.x added it always if property was constrained.

I can adapt Spec this evening, if you didn't do that already

thanks

--
You received this message because you are subscribed to the Google Groups "Grails Dev Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/grails-dev-discuss/4c19b0db-d627-4e98-ba67-184886ac55bd%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.