Binding collections with embedded types (sort of GRAILS-5582)

[longwa]

We ran into a problem in Grails 2.1.0 where if you have a command
object with a collection of model objects, any embedded or associated
properties are not being bound correctly. I tracked the issue down to
the the GrailsDataBinder.autoCreatePropertyIfPossible() method.

Basically, once it sets up an indexed type, it needs to create an
instance of the specific index in order to return as the context for
the next property lookup. If the object isn't a domain object, it
doesn't even try to return a specific object, instead just returning
the whole list. That prevents any embedded types from working.

It seems like, in most cases, you have a LazyList in your command
object anyway. In that case, you can just access the "index" value and
let the LazyList factory return a new instance if needed. Since Groovy
2.0 will support lazy lists natively, this will probably be even more
prominent.

The following pull request fixes the issue. I've added a couple new
tests to the GrailsDataBinderTests suite as well to shakeout
collections and embedded types (probably not a bad idea either way).

https://github.com/grails/grails-core/pull/250

Should I comment and post as a patch/pull request against ticket 5582
or just open a new ticket? There are a ton of data binding tickets so
there might already be a specific one for this case that I just didn't
see. BTW, I'd love to see this added to 2.1.1, if possible, as we use
embedded types heavily and this is a real problem for us.

-Aaron

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[pledbrook]

> The following pull request fixes the issue. I've added a couple new
> tests to the GrailsDataBinderTests suite as well to shakeout
> collections and embedded types (probably not a bad idea either way).
>
> https://github.com/grails/grails-core/pull/250

Is this fix specific to Grails 2.2, or does it apply to 2.1 and 2.0 as
well? If the latter, I recommend resubmitting the pull request against
the 2.1.x branch.

> Should I comment and post as a patch/pull request against ticket 5582
> or just open a new ticket? There are a ton of data binding tickets so
> there might already be a specific one for this case that I just didn't
> see.

If you can't find a specific JIRA issue for this problem, just create a new one.

> BTW, I'd love to see this added to 2.1.1, if possible, as we use
> embedded types heavily and this is a real problem for us.

See above about submitting the pull request to the 2.1.x branch.

Thanks,

Peter

PS Graeme is back next week (with some catching up to do) so it may be
at least a week and a bit before someone gets to this.

--
Peter Ledbrook
Developer Advocate
VMware

t: @pledbrook

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[longwa]

Peter,
The fix in the pull request was already made against the 2.1.x branch.
I haven't even looked at 2.2 to see if anything would need to be
changed. As for a ticket, I'll dig around some more and see if I can
find a specific ticket for embedded/nested types in a collection. If
not, I'll just open and comment with the pull request as you said (I
just hate opening duplicate tickets and I didn't have a chance
initially to sift through all the binding tickets).

Rgds,
Aaron

On Tue, Aug 21, 2012 at 5:29 AM, Peter Ledbrook <[hidden email]> wrote:

>> The following pull request fixes the issue. I've added a couple new
>> tests to the GrailsDataBinderTests suite as well to shakeout
>> collections and embedded types (probably not a bad idea either way).
>>
>> https://github.com/grails/grails-core/pull/250
>
> Is this fix specific to Grails 2.2, or does it apply to 2.1 and 2.0 as
> well? If the latter, I recommend resubmitting the pull request against
> the 2.1.x branch.
>
>> Should I comment and post as a patch/pull request against ticket 5582
>> or just open a new ticket? There are a ton of data binding tickets so
>> there might already be a specific one for this case that I just didn't
>> see.
>
> If you can't find a specific JIRA issue for this problem, just create a new one.
>
>> BTW, I'd love to see this added to 2.1.1, if possible, as we use
>> embedded types heavily and this is a real problem for us.
>
> See above about submitting the pull request to the 2.1.x branch.
>
> Thanks,
>
> Peter
>
> PS Graeme is back next week (with some catching up to do) so it may be
> at least a week and a bit before someone gets to this.
>
> --
> Peter Ledbrook
> Developer Advocate
> VMware
>
> t: @pledbrook
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[pledbrook]

> The fix in the pull request was already made against the 2.1.x branch.

But the pull request was submitted to the 'master' branch, which is
perhaps why it won't auto-merge at the moment.

Peter

--
Peter Ledbrook
Developer Advocate
VMware

t: @pledbrook

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[longwa]

Ooop, my mistake. I just didn't select the correct target branch when
I submitted it. Sorry about that. I created a new pull request against
2.1.x and closed the other one.

https://github.com/grails/grails-core/pull/251

-Aaron

On Tue, Aug 21, 2012 at 8:53 AM, Peter Ledbrook <[hidden email]> wrote:

>> The fix in the pull request was already made against the 2.1.x branch.
>
> But the pull request was submitted to the 'master' branch, which is
> perhaps why it won't auto-merge at the moment.
>
> Peter
>
> --
> Peter Ledbrook
> Developer Advocate
> VMware
>
> t: @pledbrook
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[longwa]

Graeme,
Thanks for merging this into 2.1.x branch, that's a big help for us.
After looking at http://jira.grails.org/browse/GRAILS-5582 a little
more closely, I think my pull request will fix this issue. The example
in the ticket needs to use a LazyList so that the binding can create a
new list element, but other than that the use case is similar to
embedded types. I have commented in the ticket with the same remarks.

Just wanted to let you know in case you could cross a ticket off the list.

Rgds,
Aaron

On Tue, Aug 21, 2012 at 9:05 AM, Aaron Long <[hidden email]> wrote:

> Ooop, my mistake. I just didn't select the correct target branch when
> I submitted it. Sorry about that. I created a new pull request against
> 2.1.x and closed the other one.
>
> https://github.com/grails/grails-core/pull/251
>
> -Aaron
>
> On Tue, Aug 21, 2012 at 8:53 AM, Peter Ledbrook <[hidden email]> wrote:
>>> The fix in the pull request was already made against the 2.1.x branch.
>>
>> But the pull request was submitted to the 'master' branch, which is
>> perhaps why it won't auto-merge at the moment.
>>
>> Peter
>>
>> --
>> Peter Ledbrook
>> Developer Advocate
>> VMware
>>
>> t: @pledbrook
>>
>> ---------------------------------------------------------------------
>> To unsubscribe from this list, please visit:
>>
>>     http://xircles.codehaus.org/manage_email
>>
>>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Jeff Brown-3]

On Aug 30, 2012, at 1:03 PM, Aaron Long wrote:

> Graeme,
> Thanks for merging this into 2.1.x branch, that's a big help for us.
> After looking at http://jira.grails.org/browse/GRAILS-5582 a little
> more closely, I think my pull request will fix this issue. The example
> in the ticket needs to use a LazyList so that the binding can create a
> new list element, but other than that the use case is similar to
> embedded types. I have commented in the ticket with the same remarks.
>
> Just wanted to let you know in case you could cross a ticket off the list.



I haven't looked at the patch yet but I am not sure why the example in the ticket would need to use a LazyList.  Are you saying that this (from the ticket) is not compatible with the change?.

class UserCommand {
    List <User> users
}


Thanks for the help.



jb
--
Jeff Brown
SpringSource
http://www.springsource.com/

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


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[longwa]

Jeff,
The problem the GrailsDataBinder has is that, for a collection binding
like books[3].foo.bar, it needs to be able to create a new, empty
instance of a Book in the collection at the given index. Previously,
the code only tried to do this if the object containing the books list
was a domain object (such as for a hasMany). This is because, for a
domain object, we have a little more metadata about what we are
binding into so we know what type of objects are in the List and how
to create a new one of them.

However, for a Command object with a List<Book> books, it's not
immediately obvious how to return a new instance in order to populate
any nested properties.

By using the lazy list, the binding code can just attempt to access
any element in the list via get(index) and either return the current
value or a new instance of whatever the list contains, which is very
handy. If we try to access the element and it fails, the behavior will
just be the same as it was before this patch, i.e. the
autoCreatePropertyIfPossible() method returns null and we stop
recursing and creating BeanWrapperImpl's.

Most of the time, I find that I need to use a LazyList anyway when
binding via the [index] notation since it needs to handle cases where
there are gaps in the list, etc. so this doesn't seem like a huge
downside to me.

-Aaron

On Thu, Aug 30, 2012 at 3:01 PM, Jeff Brown <[hidden email]> wrote:

>
> On Aug 30, 2012, at 1:03 PM, Aaron Long wrote:
>
>> Graeme,
>> Thanks for merging this into 2.1.x branch, that's a big help for us.
>> After looking at http://jira.grails.org/browse/GRAILS-5582 a little
>> more closely, I think my pull request will fix this issue. The example
>> in the ticket needs to use a LazyList so that the binding can create a
>> new list element, but other than that the use case is similar to
>> embedded types. I have commented in the ticket with the same remarks.
>>
>> Just wanted to let you know in case you could cross a ticket off the list.
>
>
>
> I haven't looked at the patch yet but I am not sure why the example in the ticket would need to use a LazyList.  Are you saying that this (from the ticket) is not compatible with the change?.
>
> class UserCommand {
>     List <User> users
> }
>
>
> Thanks for the help.
>
>
>
> jb
> --
> Jeff Brown
> SpringSource
> http://www.springsource.com/
>
> Autism Strikes 1 in 166
> Find The Cause ~ Find The Cure
> http://www.autismspeaks.org/
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Jeff Brown-3]

On Aug 30, 2012, at 2:26 PM, Aaron Long wrote:

> Jeff,
> The problem the GrailsDataBinder has is that, for a collection binding
> like books[3].foo.bar, it needs to be able to create a new, empty
> instance of a Book in the collection at the given index. Previously,
> the code only tried to do this if the object containing the books list
> was a domain object (such as for a hasMany). This is because, for a
> domain object, we have a little more metadata about what we are
> binding into so we know what type of objects are in the List and how
> to create a new one of them.
>
> However, for a Command object with a List<Book> books, it's not
> immediately obvious how to return a new instance in order to populate
> any nested properties.
>
> By using the lazy list, the binding code can just attempt to access
> any element in the list via get(index) and either return the current
> value or a new instance of whatever the list contains, which is very
> handy. If we try to access the element and it fails, the behavior will
> just be the same as it was before this patch, i.e. the
> autoCreatePropertyIfPossible() method returns null and we stop
> recursing and creating BeanWrapperImpl's.
>
> Most of the time, I find that I need to use a LazyList anyway when
> binding via the [index] notation since it needs to handle cases where
> there are gaps in the list, etc. so this doesn't seem like a huge
> downside to me.
>
> -Aaron
>

It still isn't clear to me why we can't make this work...

class UserCommand {
   List <User> users
}


I will look at the patch as soon as I have time and maybe that will answer my question.

Thanks for your help!



jb

--
Jeff Brown
SpringSource
http://www.springsource.com/

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


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Lari Hotari]

This comment is a bit off-topic, but I've been using Spring's
AutoPopulatingList for binding collections in command objects,
example: http://stackoverflow.com/a/11834964/166062

Regards, Lari


30.08.2012 22:01, Jeff Brown wrote:

> On Aug 30, 2012, at 1:03 PM, Aaron Long wrote:
>
>> Graeme,
>> Thanks for merging this into 2.1.x branch, that's a big help for us.
>> After looking at http://jira.grails.org/browse/GRAILS-5582 a little
>> more closely, I think my pull request will fix this issue. The example
>> in the ticket needs to use a LazyList so that the binding can create a
>> new list element, but other than that the use case is similar to
>> embedded types. I have commented in the ticket with the same remarks.
>>
>> Just wanted to let you know in case you could cross a ticket off the list.
>
>
> I haven't looked at the patch yet but I am not sure why the example in the ticket would need to use a LazyList.  Are you saying that this (from the ticket) is not compatible with the change?.
>
> class UserCommand {
>     List <User> users
> }
>
>
> Thanks for the help.
>
>
>
> jb
> --
> Jeff Brown
> SpringSource
> http://www.springsource.com/
>
> Autism Strikes 1 in 166
> Find The Cause ~ Find The Cure
> http://www.autismspeaks.org/
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[longwa]

Given just:

class UserCommand {
   List <User> users
}

I'm just not sure how the Grails data binder would know how to create
an instance of whatever is being stored in the list. You might be able
to use reflection to get the parameterized type of the list (although
I think some of that information is only available at compile time)
and try to create instances using newInstance().

However, that still leaves the general problem that when you bind a
list in your form, you need flexibility in the initial size and order
that things are processed and the LazyList is already the best
solution anyway.

That said, it would be nice to have a solution which didn't have any
additional dependencies.

-Aaron

On Mon, Sep 3, 2012 at 1:43 PM, Lari Hotari <[hidden email]> wrote:

>
> This comment is a bit off-topic, but I've been using Spring's
> AutoPopulatingList for binding collections in command objects,
> example: http://stackoverflow.com/a/11834964/166062
>
> Regards, Lari
>
>
> 30.08.2012 22:01, Jeff Brown wrote:
>> On Aug 30, 2012, at 1:03 PM, Aaron Long wrote:
>>
>>> Graeme,
>>> Thanks for merging this into 2.1.x branch, that's a big help for us.
>>> After looking at http://jira.grails.org/browse/GRAILS-5582 a little
>>> more closely, I think my pull request will fix this issue. The example
>>> in the ticket needs to use a LazyList so that the binding can create a
>>> new list element, but other than that the use case is similar to
>>> embedded types. I have commented in the ticket with the same remarks.
>>>
>>> Just wanted to let you know in case you could cross a ticket off the list.
>>
>>
>> I haven't looked at the patch yet but I am not sure why the example in the ticket would need to use a LazyList.  Are you saying that this (from the ticket) is not compatible with the change?.
>>
>> class UserCommand {
>>     List <User> users
>> }
>>
>>
>> Thanks for the help.
>>
>>
>>
>> jb
>> --
>> Jeff Brown
>> SpringSource
>> http://www.springsource.com/
>>
>> Autism Strikes 1 in 166
>> Find The Cause ~ Find The Cure
>> http://www.autismspeaks.org/
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe from this list, please visit:
>>
>>     http://xircles.codehaus.org/manage_email
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email