You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cayenne.apache.org by Lachlan Deck <la...@gmail.com> on 2009/11/10 08:49:20 UTC
[suggestion] unmodifiable toMany lists
Hi there,
given some stuff we've seen in our own code (and general better
practices for dealing with collections in general) I thought I'd
suggest the following changes for the default cayenne entity templates:
- private ivars rather than protected (if they're not already)
- return Collections.unmodifiableList(foo) rather than returning the
actual list that's modified via addTo/removeFrom etc.
Thoughts? Philosophies?
with regards,
--
Lachlan Deck
Re: [suggestion] unmodifiable toMany lists
Posted by Andrus Adamchik <an...@objectstyle.org>.
In regards to mutability and thread-safety relationship lists are no
different from default Java lists (ArrayList), so I don't think we are
presenting an inconsistent picture. And I'd actually like server-side
objects to behave like ROP objects, not the other way around. (BTW,
CayenneDataObject is thread UNsafe on a more basic level - the map
that stores all values is not synchronized).
Adding immutable wrappers will also make CDO even heavier. In this
regard, here is another can of worms: to reconcile ROP and server-side
Cayenne and to make DO's lighter, some kind of POJO is the way to go
(which also solves DO thread-safety issue). This of course involves
class enhancement, etc.
Andrus
On Nov 12, 2009, at 12:09 AM, Aristedes Maniatis wrote:
> On 12/11/09 2:44 AM, Kevin Menard wrote:
>> The "problem" exists outside the context of an iterator, too. What
>> would you expect the semantics to be of the following?
>>
>> foo.allBars.remove(for.allBars.get(0));
>>
>> Should it just modify the in-memory list or should it represent the
>> backing DB and represent a DELETE operation?
>>
>> It gets a little worse when you add your own custom collection
>> methods
>> (i.e., not DB backed) and it's not clear what type you're working
>> with.
>>
>> Don't get me wrong, it's wholly a human problem. But, false
>> expectations can lead to tedious debugging sessions and adoptions of
>> seemingly tenuous programming habits.
>
>
> Yes, that's very nicely explained. And in addition, the collections
> which are returned are not thread safe. So that's another issue to
> have to remember. As you say, this issue is purely about developers
> making mistakes in their code and not remembering that a list which
> was passed through 15 function calls actually has a special purpose
> that rewrites the database.
>
> Even that special purpose is not completely consistent. Although it
> looks like a List, Cayenne treats it like a Set: reordering the list
> doesn't cause a database operation.
>
>
> On 12/11/09 3:20 AM, Andrus Adamchik wrote:
>> This is why I am in favor of a single strategy that can be clearly
>> explained in the docs. IMO the only problem today is a mismatch
>> between
>> server-side and ROP behavior (if I am not mistaken ROP allows
>> collection.add/remove, while server-side requires you to use
>> generated
>> methods addTo/removeFrom). Otherwise I think we are consistent.
>
> The proposal is actually to make that consistent: that is, require
> the user to use generated addTo/removeFrom methods in both server
> and client. We can do that by making the default templates generate
> immutable collections.
>
>> Wrapping collections into immutable wrappers is not something I'd
>> like
>> Cayenne to do. Let the users do it if they want to, either in the
>> code,
>> or via a very simple customized cgen template.
>
> Since the read-only approach is somewhat less prone to foot-
> shooting, should we make that the default template (and maybe have a
> simple commented out block in the template for the alternative
> approach)?
>
> Ari Maniatis
>
>
> --
>
> -------------------------->
> Aristedes Maniatis
> GPG fingerprint CBFB 84B4 738D 4E87 5E5C 5EFA EF6A 7D2E 3E49 102A
>
Re: [suggestion] unmodifiable toMany lists
Posted by Aristedes Maniatis <ar...@maniatis.org>.
On 12/11/09 2:44 AM, Kevin Menard wrote:
> The "problem" exists outside the context of an iterator, too. What
> would you expect the semantics to be of the following?
>
> foo.allBars.remove(for.allBars.get(0));
>
> Should it just modify the in-memory list or should it represent the
> backing DB and represent a DELETE operation?
>
> It gets a little worse when you add your own custom collection methods
> (i.e., not DB backed) and it's not clear what type you're working
> with.
>
> Don't get me wrong, it's wholly a human problem. But, false
> expectations can lead to tedious debugging sessions and adoptions of
> seemingly tenuous programming habits.
Yes, that's very nicely explained. And in addition, the collections which are returned are not thread safe. So that's another issue to have to remember. As you say, this issue is purely about developers making mistakes in their code and not remembering that a list which was passed through 15 function calls actually has a special purpose that rewrites the database.
Even that special purpose is not completely consistent. Although it looks like a List, Cayenne treats it like a Set: reordering the list doesn't cause a database operation.
On 12/11/09 3:20 AM, Andrus Adamchik wrote:
> This is why I am in favor of a single strategy that can be clearly
> explained in the docs. IMO the only problem today is a mismatch between
> server-side and ROP behavior (if I am not mistaken ROP allows
> collection.add/remove, while server-side requires you to use generated
> methods addTo/removeFrom). Otherwise I think we are consistent.
The proposal is actually to make that consistent: that is, require the user to use generated addTo/removeFrom methods in both server and client. We can do that by making the default templates generate immutable collections.
> Wrapping collections into immutable wrappers is not something I'd like
> Cayenne to do. Let the users do it if they want to, either in the code,
> or via a very simple customized cgen template.
Since the read-only approach is somewhat less prone to foot-shooting, should we make that the default template (and maybe have a simple commented out block in the template for the alternative approach)?
Ari Maniatis
--
-------------------------->
Aristedes Maniatis
GPG fingerprint CBFB 84B4 738D 4E87 5E5C 5EFA EF6A 7D2E 3E49 102A
Re: [suggestion] unmodifiable toMany lists
Posted by Kevin Menard <ni...@gmail.com>.
Sorry, I meant to simplify the remove call, but you get the idea.
--
Kevin
On Wed, Nov 11, 2009 at 10:44 AM, Kevin Menard <ni...@gmail.com> wrote:
>
> The "problem" exists outside the context of an iterator, too. What
> would you expect the semantics to be of the following?
>
> foo.allBars.remove(for.allBars.get(0));
>
> Should it just modify the in-memory list or should it represent the
> backing DB and represent a DELETE operation?
>
> It gets a little worse when you add your own custom collection methods
> (i.e., not DB backed) and it's not clear what type you're working
> with.
>
> Don't get me wrong, it's wholly a human problem. But, false
> expectations can lead to tedious debugging sessions and adoptions of
> seemingly tenuous programming habits.
>
> --
> Kevin
>
Re: [suggestion] unmodifiable toMany lists
Posted by Dirk Olmes <di...@xanthippe.ping.de>.
Kevin Menard wrote:
> On Wed, Nov 11, 2009 at 1:13 AM, Dirk Olmes <di...@xanthippe.ping.de> wrote:
>
>>> Right. They are...but this (I believe) is too easily exposed to user
>>> code. e.g.,
>>> for (Bar relation : foo.allBars()) {
>>> if (some condition) {
>>> foo.removeFromAllBars(relation); // will throw exception
>>> }
>>> }
>> Err, why is this? Because you're modifying the collection you're
>> iterating over? I'd rather fix the iterator then.
>
> The "problem" exists outside the context of an iterator, too. What
> would you expect the semantics to be of the following?
>
> foo.allBars.remove(for.allBars.get(0));
>
> Should it just modify the in-memory list or should it represent the
> backing DB and represent a DELETE operation?
You're absolutely right. I was thinking only in terms of ObjEntity based
relationship collections but in any non-trivial setup I'd expect mapped
classes to have other methods returning collections (e.g. ones that
filter on relationships). It would be hard for users to know when the
underlying DB is modified upon modification of the collection and when not.
So I guess returning unmodifiable collections and providing add/remove
custom methods from the generator templates is the better way to go.
-dirk
Re: [suggestion] unmodifiable toMany lists
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Nov 11, 2009, at 5:44 PM, Kevin Menard wrote:
>
> Don't get me wrong, it's wholly a human problem. But, false
> expectations can lead to tedious debugging sessions and adoptions of
> seemingly tenuous programming habits.
This is why I am in favor of a single strategy that can be clearly
explained in the docs. IMO the only problem today is a mismatch
between server-side and ROP behavior (if I am not mistaken ROP allows
collection.add/remove, while server-side requires you to use generated
methods addTo/removeFrom). Otherwise I think we are consistent.
Wrapping collections into immutable wrappers is not something I'd like
Cayenne to do. Let the users do it if they want to, either in the
code, or via a very simple customized cgen template.
Andrus
Re: [suggestion] unmodifiable toMany lists
Posted by Kevin Menard <ni...@gmail.com>.
On Wed, Nov 11, 2009 at 1:13 AM, Dirk Olmes <di...@xanthippe.ping.de> wrote:
>> Right. They are...but this (I believe) is too easily exposed to user
>> code. e.g.,
>> for (Bar relation : foo.allBars()) {
>> if (some condition) {
>> foo.removeFromAllBars(relation); // will throw exception
>> }
>> }
>
> Err, why is this? Because you're modifying the collection you're
> iterating over? I'd rather fix the iterator then.
The "problem" exists outside the context of an iterator, too. What
would you expect the semantics to be of the following?
foo.allBars.remove(for.allBars.get(0));
Should it just modify the in-memory list or should it represent the
backing DB and represent a DELETE operation?
It gets a little worse when you add your own custom collection methods
(i.e., not DB backed) and it's not clear what type you're working
with.
Don't get me wrong, it's wholly a human problem. But, false
expectations can lead to tedious debugging sessions and adoptions of
seemingly tenuous programming habits.
--
Kevin
Re: [suggestion] unmodifiable toMany lists
Posted by Dirk Olmes <di...@xanthippe.ping.de>.
>> - implement custom collections that are ObjEntity aware and "do the
>> right thing" when touched.
>>
>> I haven't looked at the Cayenne source for quite some time now but IIRC
>> the code currently does the second option above ...
>
> Right. They are...but this (I believe) is too easily exposed to user
> code. e.g.,
> for (Bar relation : foo.allBars()) {
> if (some condition) {
> foo.removeFromAllBars(relation); // will throw exception
> }
> }
Err, why is this? Because you're modifying the collection you're
iterating over? I'd rather fix the iterator then.
-dirk
Re: [suggestion] unmodifiable toMany lists
Posted by Lachlan Deck <la...@gmail.com>.
On 10/11/2009, at 7:57 PM, Dirk Olmes wrote:
> Lachlan Deck wrote:
>> Hi there,
>>
>> given some stuff we've seen in our own code (and general better
>> practices for dealing with collections in general) I thought I'd
>> suggest
>> the following changes for the default cayenne entity templates:
>> - private ivars rather than protected (if they're not already)
>>
>> - return Collections.unmodifiableList(foo) rather than returning the
>> actual list that's modified via addTo/removeFrom etc.
>>
>> Thoughts? Philosophies?
>
> There's actually two schools of thoughts here:
>
> - lock down access to the collections as you propose. However, given
> that you cannot detect if a Collection is immutable or not (thanks to
> Collections' broken interface design, think of NSArray vs
> NSMutableArray) this is only a weak workaround.
True.
> - implement custom collections that are ObjEntity aware and "do the
> right thing" when touched.
>
> I haven't looked at the Cayenne source for quite some time now but
> IIRC
> the code currently does the second option above ...
Right. They are...but this (I believe) is too easily exposed to user
code. e.g.,
for (Bar relation : foo.allBars()) {
if (some condition) {
foo.removeFromAllBars(relation); // will throw exception
}
}
with regards,
--
Lachlan Deck
Re: [suggestion] unmodifiable toMany lists
Posted by Dirk Olmes <di...@xanthippe.ping.de>.
Lachlan Deck wrote:
> Hi there,
>
> given some stuff we've seen in our own code (and general better
> practices for dealing with collections in general) I thought I'd suggest
> the following changes for the default cayenne entity templates:
> - private ivars rather than protected (if they're not already)
>
> - return Collections.unmodifiableList(foo) rather than returning the
> actual list that's modified via addTo/removeFrom etc.
>
> Thoughts? Philosophies?
There's actually two schools of thoughts here:
- lock down access to the collections as you propose. However, given
that you cannot detect if a Collection is immutable or not (thanks to
Collections' broken interface design, think of NSArray vs
NSMutableArray) this is only a weak workaround.
- implement custom collections that are ObjEntity aware and "do the
right thing" when touched.
I haven't looked at the Cayenne source for quite some time now but IIRC
the code currently does the second option above ...
-dirk
Re: [suggestion] unmodifiable toMany lists
Posted by Lachlan Deck <la...@gmail.com>.
Hey Kevin,
yeah, so probably a better way to 'fix' this whilst maintaining
compatibility for those who like this behaviour would be a system
property that tells the framework itself (when fetching) to expose
unmodifiable lists only perhaps...
How do other people handle this?
On 10/11/2009, at 11:56 PM, Kevin Menard wrote:
> Hi Lachlan,
>
> I think I proposed this once before a few years back. The issue as I
> recall was that there are enough people out there relying on the list
> modification behavior that changing the default would be a
> non-starter. I don't necessarily mind supporting a second set of
> generator templates, however.
Sure - but this is velocity right :-) It could be a property switch
within the same template. Doing this within the templates I'm thinking
is a bit of a work around in the end ... I'm thinking the better long-
term solution would be within the framework itself as mentioned above.
> --
> Kevin
>
> On Tue, Nov 10, 2009 at 2:49 AM, Lachlan Deck
> <la...@gmail.com> wrote:
>> Hi there,
>>
>> given some stuff we've seen in our own code (and general better
>> practices
>> for dealing with collections in general) I thought I'd suggest the
>> following
>> changes for the default cayenne entity templates:
>> - private ivars rather than protected (if they're not already)
>>
>> - return Collections.unmodifiableList(foo) rather than returning
>> the actual
>> list that's modified via addTo/removeFrom etc.
>>
>> Thoughts? Philosophies?
>>
>> with regards,
>> --
>>
>> Lachlan Deck
>>
>>
>>
>>
with regards,
--
Lachlan Deck
Re: [suggestion] unmodifiable toMany lists
Posted by Kevin Menard <ni...@gmail.com>.
Hi Lachlan,
I think I proposed this once before a few years back. The issue as I
recall was that there are enough people out there relying on the list
modification behavior that changing the default would be a
non-starter. I don't necessarily mind supporting a second set of
generator templates, however.
--
Kevin
On Tue, Nov 10, 2009 at 2:49 AM, Lachlan Deck <la...@gmail.com> wrote:
> Hi there,
>
> given some stuff we've seen in our own code (and general better practices
> for dealing with collections in general) I thought I'd suggest the following
> changes for the default cayenne entity templates:
> - private ivars rather than protected (if they're not already)
>
> - return Collections.unmodifiableList(foo) rather than returning the actual
> list that's modified via addTo/removeFrom etc.
>
> Thoughts? Philosophies?
>
> with regards,
> --
>
> Lachlan Deck
>
>
>
>