Details
-
Type:
Bug
-
Status:
Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 2.2-beta1
-
Fix Version/s: 2.2-beta2
-
Component/s: Configuration
-
Labels:
-
Testcase included:yes
-
Number of attachments :
Description
The following patch is all what's needed for the default Catalog to truly return immutable lists. All tests still pass except the one patched, which was asserting the immutable list was mutable.
--- a/src/main/src/main/java/org/geoserver/catalog/impl/ProxyList.java +++ b/src/main/src/main/java/org/geoserver/catalog/impl/ProxyList.java @@ -4,7 +4,6 @@ */ package org.geoserver.catalog.impl; -import java.lang.reflect.Proxy; import java.util.AbstractList; import java.util.List; @@ -35,12 +34,8 @@ public abstract class ProxyList extends AbstractList { @Override public Object set(int index, Object element) { - if ( !proxyInterface.isInstance( element ) || !(element instanceof Proxy) ) { throw new IllegalArgumentException( "Object is not a proxy, or not a proxy of the correct type"); - } - - return proxyList.set(index, unwrapProxy(element, proxyInterface)); } public int size() { diff --git a/src/main/src/test/java/org/geoserver/catalog/impl/CatalogImplTest.java b/src/main/src/test/java/org/geoserver/catalog/impl/CatalogImplTest.java index 6bee7b8..7a2410a 100644 --- a/src/main/src/test/java/org/geoserver/catalog/impl/CatalogImplTest.java +++ b/src/main/src/test/java/org/geoserver/catalog/impl/CatalogImplTest.java @@ -1386,16 +1386,22 @@ public class CatalogImplTest extends TestCase { List<StyleInfo> styles = catalog.getStyles(); assertEquals( 2 , styles.size() ); - assertEquals( s.getName(), styles.get( 0 ).getName() ); - assertEquals( "a"+s.getName(), styles.get( 1).getName() ); - - //test sorting - Collections.sort( styles, new Comparator<StyleInfo>() { + //test immutability + Comparator<StyleInfo> comparator = new Comparator<StyleInfo>() { public int compare(StyleInfo o1, StyleInfo o2) { return o1.getName().compareTo( o2.getName()); } - }); + }; + try { + Collections.sort(styles, comparator); + fail("Expected runtime exception, immutable collection"); + } catch (RuntimeException e) { + assertTrue(true); + } + + styles = new ArrayList<StyleInfo>(styles); + Collections.sort(styles, comparator); assertEquals( "a"+s.getName(), styles.get( 0 ).getName() ); assertEquals( s.getName(), styles.get( 1 ).getName() );
Looks good. In ProxyList.set() should we just remove the method alltogether and throw UnsupportedOperationException as per the parent implementation?
Also for this case the only thing i could being an issue is sorting the style table from the ui. Which might not be explicitly tested with a test case. Can you verify that still works after the change?