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? 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 <grol...@opengeo.org> 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.
------------------------------------------------------------------------------
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
