Very nice. The patch looks good to me. +1 for trunk.
For 2.3.x, not sure. I would certainly say let's let the patch sit on trunk
for a bit and if no known issues after a month or so then backport.
On Sun, Jun 2, 2013 at 3:31 AM, Andrea Aime <[email protected]>wrote:
> On Tue, May 28, 2013 at 3:03 PM, Justin Deoliveira
> <[email protected]>wrote:
>
>> Thinking about it... the metadata map already requires that the objects
>> be put in there are Serializable,
>>
>>> so maybe the easiest thing to do is to deep clone them by serialization
>>> (maybe using XStream?) and
>>> be done with it.
>>>
>>
>> True, if this works then it i am all for it.
>>
>
> I went down this path, trying to keep cloning to the bare minimum.
>
> The wrap/clone logic is here, adopted several strategies to keep the
> overhead low and try to wrap
> what we know is wrappable using just ModificationProxy, and to copy by
> clone() or
> copy constructor when possible (also added copy constructors to a couple
> of objects that I saw were
> being copied over and over). Also for the copy by serialization I tried to
> use binary serialization
> (which gives a true copy) when possible, falling back on XStream only as a
> last resort:
> https://github.com/geoserver/geoserver/pull/245/files#L4R46
>
> Changes to ModificationProxy are minimal, although I'm not sure about the
> shallow/deep cloning
> choices, in this case, deep for the collection that's returned, shallow
> for the one that's kept as the original state:
> https://github.com/geoserver/geoserver/pull/245/files#L3L106
>
> In the process I've found a couple of things in WFS that basically relied
> on the collections being modified
> directly, and just kept a permanent reference to the config objects:
> https://github.com/geoserver/geoserver/pull/245/files#L7L76
> https://github.com/geoserver/geoserver/pull/245/files#L8R254
>
> With these changes I can no longer reproduce the GUI editing issues (and
> oh, added a test to cover
> the unwanted change to metadata links contents).
>
> I'd like to commit this on trunk. On 2.3.x, don't know....
> For sure several bits of the GUI are broken with the current situation,
> but I'm not sure what side
> effects might happen by switching to wrapping/cloning. As the patch shows,
> some bits of code
> existed that relied on that bug, those are fixed, but there might be
> others...
> Screwed if you do, screwed if you don't kind of thing. Given that I
> reported the issue and users
> have not complained about it yet, I'd keep it on trunk only.
> What do you think?
>
> Cheers
> Andrea
>
> --
> ==
> GeoServer training in Milan, 6th & 7th June 2013! Visit
> http://geoserver.geo-solutions.it for more information.
> ==
>
> Ing. Andrea Aime
> @geowolf
> Technical Lead
>
> GeoSolutions S.A.S.
> Via Poggio alle Viti 1187
> 55054 Massarosa (LU)
> Italy
> phone: +39 0584 962313
> fax: +39 0584 1660272
> mob: +39 339 8844549
>
> http://www.geo-solutions.it
> http://twitter.com/geosolutions_it
>
> -------------------------------------------------------
>
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite
It's a free troubleshooting tool designed for production
Get down to code-level detail for bottlenecks, with <2% overhead.
Download for free and get started troubleshooting in minutes.
http://p.sf.net/sfu/appdyn_d2d_ap2
_______________________________________________
Geoserver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
