Grails

Named parameter in Domain Objects should throw MissingPropertyException

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.0.3
  • Fix Version/s: 1.1-beta2
  • Component/s: Commons
  • Labels:
    None

Description

This is a suggestion to make the behaviour of Grails Domain Classes more like Groovy POGOs.

In Groovy: 

 
class Tester {
}
new Tester(myProp:'hello')

leads to the expected Exception: groovy.lang.MissingPropertyException: No such property: myProb for class: Tester

But when defining the Tester class as Domain class (grails-app/domain) and instantiate it as above, everything
works fine: No exception and the object is created.

It would be much more save throwing a MissingPropertyException, if a non existing property is used in the
named parameter list of a constructor.
Without, it gets very hard to detect wrong parameter lists and could lead to an error-prone application.

Currently new Tester(params) is used to bind request parameters.
The second alternative for this is tester.properties=params.

Suggestion:

  • new Tester(params) would throw an MissingPropertyException
  • tester.properties=params works unchanged. This doesn't conflict to Groovy because tester.properties is read-only and doesn't allow any assignments at all

Workaround for 1.0.x:

  • Use something like Tester.metaClass.constructor = { Map m -> // your code here } to check for missing properties

For discussion on this topic check out:
http://www.nabble.com/Grails-doesn%27t-check-%27named-parameter%27-when-constructing-a-Domain-Object-td18256758.html#a18258465

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Lee Breisacher added a comment -

It seems to me there should be a difference between constructor-with-one-unnamed-map-param versus constructor-with-several-named-params, e.g.

new Tester(params) – works as it does now (no warnings)
new Tester(param1:value1, param2:value2, param3:value3) – works as desired by this JIRA, warning for missing properties

In addition, I think the latter syntax should invoke individual setter methods on each param. See http://www.nabble.com/constructor-setter-override-td18852761.html

Show
Lee Breisacher added a comment - It seems to me there should be a difference between constructor-with-one-unnamed-map-param versus constructor-with-several-named-params, e.g. new Tester(params) – works as it does now (no warnings) new Tester(param1:value1, param2:value2, param3:value3) – works as desired by this JIRA, warning for missing properties In addition, I think the latter syntax should invoke individual setter methods on each param. See http://www.nabble.com/constructor-setter-override-td18852761.html
Hide
Permalink
Graeme Rocher added a comment -

This can't be changed, data binding takes parameters from the request, so a user could just fiddle with the request parameters and get your application to blow up with a MissingPropertyException, besides that it is a breaking change

Show
Graeme Rocher added a comment - This can't be changed, data binding takes parameters from the request, so a user could just fiddle with the request parameters and get your application to blow up with a MissingPropertyException, besides that it is a breaking change
Hide
Permalink
Mos added a comment -

Ok, it's a pity. Sure it is one more breaking change for the next (major) release, but would result in much more faultless code. People could still use full data binding by using: domain.properties=params (as described above).

For people who are interested. In our projects we redefine the domain object behavior as follow:

/**
     * This one enhances the Grails Domain Objects and make
     * same behave like POGOs regarding name parameteres in a constructor:
     * Whenver an non existing property is used, ab MissingPropertyException
     * is thrown.
     */
    static void changeNamedParameterInDomainObjects() {
        ApplicationHolder.application.domainClasses.each { dc ->
              dc.metaClass.constructor = { Map m ->
                 def obj = delegate.newInstance()
                 def props = obj.metaClass.properties.name
                 def paramKeys = m.keySet()
                 paramKeys.each {
                     if (!props.contains(it)) {
                         throw new MissingPropertyException("Property '$it' doesn't exist in domain object!")
                     }
                 }
                 obj.properties=m
                 obj
             }
         }
    }
Show
Mos added a comment - Ok, it's a pity. Sure it is one more breaking change for the next (major) release, but would result in much more faultless code. People could still use full data binding by using: domain.properties=params (as described above). For people who are interested. In our projects we redefine the domain object behavior as follow: 
/**
     * This one enhances the Grails Domain Objects and make
     * same behave like POGOs regarding name parameteres in a constructor:
     * Whenver an non existing property is used, ab MissingPropertyException
     * is thrown.
     */
    static void changeNamedParameterInDomainObjects() {
        ApplicationHolder.application.domainClasses.each { dc ->
              dc.metaClass.constructor = { Map m ->
                 def obj = delegate.newInstance()
                 def props = obj.metaClass.properties.name
                 def paramKeys = m.keySet()
                 paramKeys.each {
                     if (!props.contains(it)) {
                         throw new MissingPropertyException("Property '$it' doesn't exist in domain object!")
                     }
                 }
                 obj.properties=m
                 obj
             }
         }
    }
Hide
Permalink
Mos added a comment -

In discussion for Grails 1.2. Please check:
http://markmail.org/message/hrhpa23tp6qlw5ty

Can somebody please move the target to Grails 1.2. Thanks.

Show
Mos added a comment - In discussion for Grails 1.2. Please check: http://markmail.org/message/hrhpa23tp6qlw5ty Can somebody please move the target to Grails 1.2. Thanks.
Hide
Permalink
Graeme Rocher added a comment -

Please don't re-open issues until it has been agreed, the explanation was pretty clear. This is a breaking change and is not in consideration for 1.2

Show
Graeme Rocher added a comment - Please don't re-open issues until it has been agreed, the explanation was pretty clear. This is a breaking change and is not in consideration for 1.2
Hide
Permalink
Mos added a comment -

For the record. Discussion from mailinglist:

>> *Ted*
>> I agree, the current behavior feels like a feature and not a bug. I can
>> now pass a parameter map (that might have other things in it) into a
>> constructor without needing to do some pre-filtering on it.
>> I've never run into a situation where this change would have saved me
>> work, and I can think of a number where it would have costed me time.

> mos
> The problem with GRAILS-3265 is that it leads to undetected errors.
> I'm confident that Grails should be aspire after a "fail-fast system".
> Especially regarding the persistence layer:
> http://en.wikipedia.org/wiki/Fail-fast
>
> Typically you have some constructs like this in your code
> new MyObject(name1:'xxx',nameSpellError:'yyy')
>
> If 'nameSpellError' has a spelling error Grails just ignores it
> and the developer has no chance (without writing strict tests)
> to recognize this.
> In Groovy you would get a MissingPropertyException for this.
>
> And as I wrote. If we change the behavior here, we don't
> lose a feature because we still can do a
> myObject.properties = params
> for a multi assignment.
>
> And as always. If we want to be backward compatible with
> old versions, we can define a config parameter for reactivate
> the old behavior.

Show
Mos added a comment - For the record. Discussion from mailinglist: >> *Ted* >> I agree, the current behavior feels like a feature and not a bug. I can >> now pass a parameter map (that might have other things in it) into a >> constructor without needing to do some pre-filtering on it. >> I've never run into a situation where this change would have saved me >> work, and I can think of a number where it would have costed me time. > mos > The problem with GRAILS-3265 is that it leads to undetected errors. > I'm confident that Grails should be aspire after a "fail-fast system". > Especially regarding the persistence layer: > http://en.wikipedia.org/wiki/Fail-fast > > Typically you have some constructs like this in your code > new MyObject(name1:'xxx',nameSpellError:'yyy') > > If 'nameSpellError' has a spelling error Grails just ignores it > and the developer has no chance (without writing strict tests) > to recognize this. > In Groovy you would get a MissingPropertyException for this. > > And as I wrote. If we change the behavior here, we don't > lose a feature because we still can do a > myObject.properties = params > for a multi assignment. > > And as always. If we want to be backward compatible with > old versions, we can define a config parameter for reactivate > the old behavior.
Hide
Permalink
Aaron Zeckoski added a comment -

Was this issue fixed then? It is marked as fixed but I think from looking at this issue that it was not actually fixed and should be marked as "won't fix"

Show
Aaron Zeckoski added a comment - Was this issue fixed then? It is marked as fixed but I think from looking at this issue that it was not actually fixed and should be marked as "won't fix"

People

Vote (2)
Watch (2)

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.