Thanks Gabriel. I replied to the feedback. I don't have too strong a preference here about making the MetadataMap a map or not. I would lean toward keeping it a map, but i could be persuaded the other way if your preference is a strong one?
Anyways, maybe Andrea can be a tie breaker. Gabriel Roldan wrote: > Justin Deoliveira wrote: >> I have gone and implemented the map as based on this thread. I would >> love a review: > > I've posted my feedback on the issue. Basically it looks good, I just > keep on thinking there's no need for MetadataMap to implementa > java.util.Map. Proposed change being it either just doesn't and uses an > internal Map as it's data structure, getting rid of all the not going to > be used Map methods, or it's abstracted out as in interface and > MetadataMapImpl is free to extend HashMap<String, Serializable>. > > My 2c.- > > Gabriel >> >> http://jira.codehaus.org/browse/GEOS-3164 >> >> -Justin >> >> Gabriel Roldan wrote: >>> Andrea Aime wrote: >>>> Gabriel Roldan ha scritto: >>>>> Andrea Aime wrote: >>>>>> Justin Deoliveira ha scritto: >>>>>>> Hi all, >>>>>>> >>>>>>> A few issues have popped up recently related to how maps (in >>>>>>> particular the metadata map for catalog objects) is >>>>>>> persisted/depersisted. The latest issue revolves around someone >>>>>>> trying ot configure a feature type via rest. >>>>>>> >>>>>>> The issue is that in an attempt to make xstream less verbose, i >>>>>>> made it so that someone trying to upload map data with rest can >>>>>>> do this: >>>>>>> >>>>>>> <map> >>>>>>> <key1>value1</key1> >>>>>>> <key2>value2</key2> >>>>>>> ... >>>>>>> </map> >>>>>>> >>>>>>> As opposed to the xstream default: >>>>>>> >>>>>>> <map> >>>>>>> <entry> >>>>>>> <string>key1</string> >>>>>>> <string>value1</string> >>>>>>> </entry> >>>>>>> <entry> >>>>>>> <string>key2</string> >>>>>>> <string>value2</string> >>>>>>> </entry> >>>>>>> </map> >>>>>>> >>>>>>> However this has caused some issues. The main issue is that type >>>>>>> information is lost. This pops up in a couple of places: >>>>>>> >>>>>>> * when a feature is persisted in geoserver, and then read back in >>>>>>> again it is always read back in as a string. >>>>>>> >>>>>>> * when a feature type is uploaded via rest, the values in the >>>>>>> metadata map are of type string. >>>>>>> >>>>>>> The end result is the same, ClassClassException for code that >>>>>>> assumes type information about the metadata map. >>>>>>> >>>>>>> The quick solution for now has been to replace this: >>>>>>> >>>>>>> (Integer) featureType.getMetadata().get( "foo" ); >>>>>>> >>>>>>> with: >>>>>>> >>>>>>> Converters.convert( featureType.getMetadata().get( "foo" ), >>>>>>> Integer.class ); >>>>>>> >>>>>>> The longer term solution is to amend the xstream persistence a >>>>>>> bit so that type information is not lost. This is however a bit >>>>>>> tricky though. And probably won't fix every REST case. >>>>>>> >>>>>>> So I am wondering. What do people think about making it practice >>>>>>> that whenever accessing data from a metadata map, that a >>>>>>> converter be used. >>>>>> >>>>>> I would be ok using the converters everywhere. But we should make it >>>>>> pretty explicit that the user map is made of strings, >>>>>> Map<String,String>. This way the need of using some kind of >>>>>> conversion >>>>>> is obvious (teaching people that they can use Converters instead of >>>>>> Integer.parseInt(xxx) is another story). >>>>> >>>>> It looks to me like MetadataMap claims to be an class itself rather >>>>> than a java.uti.Map >>>>> MetadataMap{ >>>>> private Map internalMap; >>>>> ... >>>>> boolean contains(Serializable key); >>>>> <T extends Serializable> get(Serialiable key, Class<T>); >>>>> ///same for put >>>>> } >>>>> >>>>> then isolate the use of converters inside it. >>>> >>>> Hum.... uh? >>>> I looked in CatalogInfo and found: >>>> >>>> Map<String,Serializable> getMetadata(); >>>> >>>> in ServiceInfo: >>>> >>>> Map<String,Serializable> getMetadata(); >>>> >>>> Or else... you mean we should create the MetadataMap >>>> class and replace those Map<String,Serializable> >>>> references? Sounds like a good idea to me. >>> yup >>> >>>> >>>> Cheers >>>> Andrea >>>> >>> >>> >> >> > > -- Justin Deoliveira OpenGeo - http://opengeo.org Enterprise support for open source geospatial. ------------------------------------------------------------------------------ Crystal Reports - New Free Runtime and 30 Day Trial Check out the new simplified licensing option that enables unlimited royalty-free distribution of the report engine for externally facing server and web deployment. http://p.sf.net/sfu/businessobjects _______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
