I would like to proceed with the change proposed by Niels, to store paths
rather than Resources. I would like to catch this now before the release
goes out as the demo page is one of the last blockers we have on the books.
Rolling back the change of Resource to implement Serializable, and storing
paths instead follows the original design so I do not feel a proposal is
required here.
One thing that gives me concern is the reference to relative paths, it may
be that we have a confusion on when resources should be used (during
configuration) and when files should be used (when resolving data source
connection parameters, tile cache location, etc...). The Resource API is
that it is clear when we are refering to configuraiton resources (it uses
the Resource class) vs when we are referencing a file location on disk (it
uses the File class).
Here is the original message:
Hello,
While working on the Demo Request bug (
https://osgeo-org.atlassian.net/browse/GEOS-7513), I came across an issue
with Resources. The bug is caused by buggy serialisation of a Resource
instance stored in a Wicket Page.
It was me who made the Resource interface extend Serializable so that all
Resources could be used as serializables.
The idea behind that was that Files are serializable and that made it
easier to replace the use of "File" by the use of "Resource" in the webui
pages amongst other things.
However, now I realise that this was a wrong thing to do. Resources are
usually implemented as non-static inner classes, and it is an anti-pattern
to make those Serializable, see:
https://www.securecoding.cert.org/confluence/display/java/SER05-J.+Do+not+serialize+instances+of+inner+classes
Making the Resource implementation classes static is also not an option.
They need to reference their store, because there can be different stores
at once and resources need to call their store for certain operations.
Changing that would require rethinking the resource API altogether. It
can't be the case that we serialize the *store *and get multiple copies of
it. It is a spring bean that refers to other spring beans such as
LockProvider, FileWatcher etc. We can't make all of that serializable and
create new copies from all of them, just because we are serializing a webui
page.
Anyway, I want to roll back this change. As far as I can see there are only
two places in which the serialisation of resources is relied upon:
- the Demo Request page as mentioned,
- the importer module.
The alternative solution is to use a String with the path instead and rely
upon the Resources.fromPath method - which uses the datadirectorystore for
relative paths and Files.asResource for absolute paths.
I had the idea to simplify the multiple use of this pattern by creating a
Resources.serializable(Resource) method which wraps a serializable resource
around a non-serializable resource, relying on the Resources.fromPath
method to do the serialisation. This way we need minimal change to the
aforementioned modules.
Does anyone object to any of my suggestions?
I assume I will need to make a proposal?
Regards
Niels
--
Jody Garnett
------------------------------------------------------------------------------
Mobile security can be enabling, not merely restricting. Employees who
bring their own devices (BYOD) to work are irked by the imposition of MDM
restrictions. Mobile Device Manager Plus allows you to control only the
apps on BYO-devices by containerizing them, leaving personal data untouched!
https://ad.doubleclick.net/ddm/clk/304595813;131938128;j
_______________________________________________
Geoserver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
