Hey Gabriel,
Thanks for the explanation. It is a lot to parse though but yes, the
original intent was to make any collections returned from the catalog
immutable, even though it is not explicitly stated in the javadocs. The
comments of not adding/removing the returned list were the attempt to do so
but i agree, it leaves things ambiguous.
So +1 on this change and truly enforcing immutability. As always though I
would do a thorough test run to figure out where all those assumptions lie
and deal with accordingly before proceeding. Afaik though any code that
needs to modify a list returned from the catalog makes there own copy. But
there could be cases where not. Tests will tell.
-Justin
On Sun, Apr 22, 2012 at 12:21 PM, Gabriel Roldan <groldan@anonymised.com>wrote:
On Fri, Apr 20, 2012 at 7:43 PM, Justin Deoliveira <jdeolive@anonymised.com>
wrote:
> Hey Gabriel,
>
> I think all the options make sense. If we ensure the returned list is
> immutable there is no need to return a defensive copy right?
I think there still is. Otherwise the returned list is wrapped by an
immutable decorator, but its state is live changing as styles are
added/removed from the catalog, Meaning we're still breaking
encapsulation.
Also, I'm not sure whether there's some code depending on the returned
lists being "sortable". There probably are. The fact is that ProxyList
is some place in the middle. It doesn't allow adding content only if
it's not of the proper proxy type.
So to clarify:
- ProxyList's javadoc states it's a "unmodifiable list proxy", yet
it's something in between.
- ProxyList is not part of the Catalog contract, just an
implementation detail. On the other hand, the Catalog method contracts
don't specify the returned lists are immutable, just that they're not
to be used to add/remove content from the catalog.
This leads to a situation where probably the returned lists are used
based on observed behavior instead of stated contract terms. Which for
the sake of all the current contracts it means the returned lists are
not unmodifiable, but they're implicitly detached from storage.
So for the sake of safety, and at least for the time being until we
revise the catalog api as a whole, I'd rather make getStyles() return
a wrapped safe copy like all other methods do. In the future, and once
GSIP 69 is applied, it'd be safer to constrain the contracts of
methods returning list to explicitly indicate they're immutable, if
that happens to be "a good thing" (like in facilitating Catalog
implementations, which actually would be the case), since if you want
a sorted result, you'll be able to use the new list methods that allow
for paging and sorting.
Makes sense?
(sorry I wasn't so clear before, it's not the same evaluating this
fresh in the morning than tired at night it seems).
Cheers,
Gabriel
> So I guess i
> would vote for b as it is consistent with the contract as described by
the
> javadoc.
>
> -Justin
>
> On Fri, Apr 20, 2012 at 2:20 PM, Gabriel Roldan <groldan@anonymised.com>
wrote:
>>
>> Hey,
>>
>> implementing an alternate catalog facade, found something weird.
>> CatalogImplTest.testProxyListBehaviour does the following:
>>
>> List<StyleInfo> styles = catalog.getStyles();
>> Collections.sort( styles, new Comparator<StyleInfo>(){...});
>>
>> Problem is two fold:
>> - the returned list is said to be immutable. The javadocs for
>> ProxyList say "An unmodifiable list proxy in which each element in the
>> list is wrapped in a proxy of its own". Yet the collection is being
>> sorted, hence changed.
>> - The internal state of the DefaultCatalogFacade is being changed as
>> a side effect. It's internal list of styles is being returned as is,
>> wrapped on a ProxyList, I suppose relying on the fact that the wrapper
>> should make it immutable.
>>
>> So, breaking the encapsulation of DefaultCatalogFacade.styles may lead
>> to unexpected issues. Either a ConcurrentModificationException or
>> phantom reads when multiple threads use it.
>> It is not clear whether the returned list shall be immutable by
>> contract or not. I would say yes. I'm pretty sure I've seen a test
>> that ensured ProxyList is, but don't seem to be able to find it now.
>> In any case, immutable is good.
>>
>> So what do we do?
>> a- return a defensive copy on getStyles()
>> b- return an actually immutable list? (Note javadoc for all the
>> methods returning a list say "The resulting list should not be used to
>> add or remove styles, the {@link #add(XXXInfo)} and {@link
>> #remove(XXXInfo)} are used for that purpose.
>> c- both?
>>
>>
>> Cheers,
>> Gabriel
>> --
>> Gabriel Roldan
>> OpenGeo - http://opengeo.org
>> Expert service straight from the developers.
>>
>>
>>
------------------------------------------------------------------------------
>> For Developers, A Lot Can Happen In A Second.
>> Boundary is the first to Know...and Tell You.
>> Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
>> http://p.sf.net/sfu/Boundary-d2dvs2
>> _______________________________________________
>> Geoserver-devel mailing list
>> Geoserver-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/geoserver-devel
>
>
>
>
> --
> Justin Deoliveira
> OpenGeo - http://opengeo.org
> Enterprise support for open source geospatial.
>
--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.