[Geoserver-devel] Fun with Wrapper(s)

GeoServer GeoServer Developer
1

Hi,
in the security implementation work I have to create a ton of wrappers.
These classes are needed in order to make the current user do only
what he's supposed to do, that is, if the current user can only read
a feature type, the wrappers make sure the service really see a read only object, be it a FeatureTypeInfo, a FeatureSource, DataStore,
FeatureCollection and whatnot (man, too many ways to get a write
enabled object!).

On the outbound this works fine, but it might happen some of these
object to come back and be used to query, before passing them
to the actual catalog, unwrapping is needed (since the catalog is
working on object identity comparisons, we have no real "keys"
around).

The java.sql.Wrapper interface is an inspiration, but there are
a couple of gotchas:
(http://java.sun.com/javase/6/docs/api/java/sql/Wrapper.html)
* it is in the wrong package (what I'm doing has nothing to do
   with sql)
* it is in the wrong JDK (1.6 onwards)

What about implementing it in GeoTools instead? We could benefit
from using it in the many wrappers we have around (it would
make it possible to walk them for example).

The Wrapper javadoc is also lacking about best practices on
how to implement the wrappers:
* what about equality, when are two wrappers equal? When
   they have the same delegate? Or when unwrap(targetClass)
   returns the same object (deep or shallow equality?)
* what about hashcodes, shall we use the one of the delegate
   or create a new one?

Also wrapper creation is not trivial when two objects
candidate for wrapping point one to the other, like
FeatureSource and DataStore. By using just constructors
to create the wrappers, new wrappers can be created each
time a certain object, say a DataStore, is required, and
that _might_ break some code.
Should we use a factory and a canonicalizer (a weak
hash map using the delegate as the key) to avoid that?
Or I'm just thinking too much? :wink: (probably so!).

Cheers
Andrea

2

Hi Andrea,

I recently ran into some of the same concerns with the new catalog implementation. As you know there is currently no real DTO layer, which means that changes made via the UI or wherever else are instantly made live. Now.. this if course has issues with synchronization, beign able to revert changes, etc... etc..

So to solve this I used a dynamic proxy, not quite the same as but similar to the wrapper idea. However I too ran into the question of how to deal with equals and hashcode. And the answer was to have the objects being proxied to define equality based on interface, not on implementation.

So perhaps the two ideas line up? Not sure, just wanted to give my 2c.

-Justin

Andrea Aime wrote:

Hi,
in the security implementation work I have to create a ton of wrappers.
These classes are needed in order to make the current user do only
what he's supposed to do, that is, if the current user can only read
a feature type, the wrappers make sure the service really see a read only object, be it a FeatureTypeInfo, a FeatureSource, DataStore,
FeatureCollection and whatnot (man, too many ways to get a write
enabled object!).

On the outbound this works fine, but it might happen some of these
object to come back and be used to query, before passing them
to the actual catalog, unwrapping is needed (since the catalog is
working on object identity comparisons, we have no real "keys"
around).

The java.sql.Wrapper interface is an inspiration, but there are
a couple of gotchas:
(http://java.sun.com/javase/6/docs/api/java/sql/Wrapper.html)
* it is in the wrong package (what I'm doing has nothing to do
   with sql)
* it is in the wrong JDK (1.6 onwards)

What about implementing it in GeoTools instead? We could benefit
from using it in the many wrappers we have around (it would
make it possible to walk them for example).

The Wrapper javadoc is also lacking about best practices on
how to implement the wrappers:
* what about equality, when are two wrappers equal? When
   they have the same delegate? Or when unwrap(targetClass)
   returns the same object (deep or shallow equality?)
* what about hashcodes, shall we use the one of the delegate
   or create a new one?

Also wrapper creation is not trivial when two objects
candidate for wrapping point one to the other, like
FeatureSource and DataStore. By using just constructors
to create the wrappers, new wrappers can be created each
time a certain object, say a DataStore, is required, and
that _might_ break some code.
Should we use a factory and a canonicalizer (a weak
hash map using the delegate as the key) to avoid that?
Or I'm just thinking too much? :wink: (probably so!).

Cheers
Andrea

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

!DSPAM:4007,4847fa2b39291096210785!

--
Justin Deoliveira
The Open Planning Project
jdeolive@anonymised.com

3

Justin Deoliveira ha scritto:

Hi Andrea,

I recently ran into some of the same concerns with the new catalog implementation. As you know there is currently no real DTO layer, which means that changes made via the UI or wherever else are instantly made live. Now.. this if course has issues with synchronization, beign able to revert changes, etc... etc..

So to solve this I used a dynamic proxy, not quite the same as but similar to the wrapper idea.

Yeah, I was tempted by this approach as well, that would
have spared me quite some pure forwarding delegation calls... but
at the same time it would have made the decorated calls handling
a a bit cumbersome, since I have no generic rule for decoration.
See for example:

http://svn.codehaus.org/geoserver/trunk/geoserver/main/src/main/java/org/geoserver/security/decorators/ReadOnlyDataStore.java
http://svn.codehaus.org/geoserver/trunk/geoserver/main/src/main/java/org/geoserver/security/decorators/DecoratingDataStore.java

It would be nice to be able and just say

class DecoratingDataStore extends MySmartProxy<DataStore> implements DataStore {
   public DecoratingDataStore(DataStore delegate) {
     super(delegate);
   }
}

and then leave ReadOnlyDataStore as is, but I don't see how this can be implemented using the java dynamic proxies.

However I too ran into the question of how to deal with equals and hashcode. And the answer was to have the objects being proxied to define equality based on interface, not on implementation.

I lost you there. I looked into the ModificationProxy class:

http://svn.codehaus.org/geoserver/trunk/geoserver/main/src/main/java/org/geoserver/catalog/impl/ModificationProxy.java

and found it has no overrides for equals/hashcode, so the contract
defined by the java Proxy class applies:

"An invocation of the hashCode, equals, or toString methods declared in java.lang.Object on a proxy instance will be encoded and dispatched to the invocation handler's invoke method in the same manner as interface method invocations are encoded and dispatched, as described above. The declaring class of the Method object passed to invoke will be java.lang.Object. Other public methods of a proxy instance inherited from java.lang.Object are not overridden by a proxy class, so invocations of those methods behave like they do for instances of java.lang.Object. "

Looked into the implementations of objects being proxies, and it seems
equals and hashcode have been generated by Eclipse. For example:

http://svn.codehaus.org/geoserver/trunk/geoserver/main/src/main/java/org/geoserver/catalog/impl/StyleInfoImpl.java

Ah, but in the equals method you cast to StyleInfo, not to StyleInfoImpl, that's what you mean by interface
based equality, right?

So perhaps the two ideas line up? Not sure, just wanted to give my 2c.

Yes, I mean, they forcefully line up as long as the catalog objects
are concerned, since the equality used is the same, they do not when it
comes to the gt2 objects I wrapped (FeatureSource, FeatureCollection, DataStore) because there the equality concept is not the same.

Anyways, I got enough information to:
* define a Wrapper interface that mimics the java.sql.Wrapper one,
   which could be used by ModificationProxy too, since the unwrap
   method is bit by bit compatible
* define a generic decorator superclass that implements Wrapper
   and consistently delegates equals and hascode to the wrapped
   object
* unfortunately, unless someone has a bright idea, keep the DecoratingXXX objects around as they are (but extending the
   generic decorator class)

Cheers
Andrea

4

Yeah, I was tempted by this approach as well, that would
have spared me quite some pure forwarding delegation calls... but
at the same time it would have made the decorated calls handling
a a bit cumbersome, since I have no generic rule for decoration.
See for example:

http://svn.codehaus.org/geoserver/trunk/geoserver/main/src/main/java/org/geoserver/security/decorators/ReadOnlyDataStore.java
http://svn.codehaus.org/geoserver/trunk/geoserver/main/src/main/java/org/geoserver/security/decorators/DecoratingDataStore.java

It would be nice to be able and just say

class DecoratingDataStore extends MySmartProxy<DataStore> implements DataStore {
   public DecoratingDataStore(DataStore delegate) {
     super(delegate);
   }
}

and then leave ReadOnlyDataStore as is, but I don't see how this can be implemented using the java dynamic proxies.

Yeah, the case for datastore is a little hard to sell proxies. However for regular java beans its like the classes in the new configuration model its a little bit easier to sell. You can put in some generic logic based on get/set method naming conventions. For instance the code in LayerGroupInfo that thats the collections of layers and wraps them in read only layers. There is code in ModificationProxy that does this exact same thing.

So the short of it is I think we could save some code by using proxies, and break out full blown decorators when need be... like with DataStore. However as you say programming with proxies is a level of indirection as all calls you make are reflective calls.

The only thing I fear with the pure decorator approach as that we are following down the same road as with DTO's and the config... where modifying one class does not mean modifying one class.

However I too ran into the question of how to deal with equals and hashcode. And the answer was to have the objects being proxied to define equality based on interface, not on implementation.

I lost you there. I looked into the ModificationProxy class:

http://svn.codehaus.org/geoserver/trunk/geoserver/main/src/main/java/org/geoserver/catalog/impl/ModificationProxy.java

and found it has no overrides for equals/hashcode, so the contract
defined by the java Proxy class applies:

"An invocation of the hashCode, equals, or toString methods declared in java.lang.Object on a proxy instance will be encoded and dispatched to the invocation handler's invoke method in the same manner as interface method invocations are encoded and dispatched, as described above. The declaring class of the Method object passed to invoke will be java.lang.Object. Other public methods of a proxy instance inherited from java.lang.Object are not overridden by a proxy class, so invocations of those methods behave like they do for instances of java.lang.Object. "

Looked into the implementations of objects being proxies, and it seems
equals and hashcode have been generated by Eclipse. For example:

http://svn.codehaus.org/geoserver/trunk/geoserver/main/src/main/java/org/geoserver/catalog/impl/StyleInfoImpl.java

Ah, but in the equals method you cast to StyleInfo, not to StyleInfoImpl, that's what you mean by interface
based equality, right?

Correct, I meant that the objects themselves, not the proxy implement equals in terms of the interface.

So perhaps the two ideas line up? Not sure, just wanted to give my 2c.

Yes, I mean, they forcefully line up as long as the catalog objects
are concerned, since the equality used is the same, they do not when it
comes to the gt2 objects I wrapped (FeatureSource, FeatureCollection, DataStore) because there the equality concept is not the same.

Anyways, I got enough information to:
* define a Wrapper interface that mimics the java.sql.Wrapper one,
   which could be used by ModificationProxy too, since the unwrap
   method is bit by bit compatible
* define a generic decorator superclass that implements Wrapper
   and consistently delegates equals and hascode to the wrapped
   object
* unfortunately, unless someone has a bright idea, keep the DecoratingXXX objects around as they are (but extending the
   generic decorator class)

Cheers
Andrea

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

!DSPAM:4007,4848fa04213634901796417!

--
Justin Deoliveira
The Open Planning Project
jdeolive@anonymised.com

5

Justin Deoliveira ha scritto:
...

and then leave ReadOnlyDataStore as is, but I don't see how this can be implemented using the java dynamic proxies.

Yeah, the case for datastore is a little hard to sell proxies. However for regular java beans its like the classes in the new configuration model its a little bit easier to sell. You can put in some generic logic based on get/set method naming conventions. For instance the code in LayerGroupInfo that thats the collections of layers and wraps them in read only layers. There is code in ModificationProxy that does this exact same thing.

So the short of it is I think we could save some code by using proxies, and break out full blown decorators when need be... like with DataStore. However as you say programming with proxies is a level of indirection as all calls you make are reflective calls.

I just find it quite a bit more error prone and it's not refactoring safe, but one can work around that limitation with more unit testing.

The only thing I fear with the pure decorator approach as that we are following down the same road as with DTO's and the config... where modifying one class does not mean modifying one class.

Yeah, seen your commit :slight_smile: Sorry, I consistently used the same
approach for all the wrappers, making decorators like this was a lot
easier given the dev env support for building delegate classes.
I guess the trade off depends a lot on how stable the catalog API
is going to be, if we need to change it often then it's probably
worth it to rewrite the decorators to use proxies, change their
unit tests and the client code accordingly.

Oh, btw, on Windows I cannot rename the following class:
http://svn.codehaus.org/geoserver/trunk/geoserver/main/src/main/java/org/geoserver/security/decorators/DecoratingLayerGroupINfo.java

because DecoratingLayerGroupINfo and DecoratingLayerGroupInfo are the same file name on windows. Can you do that for me? :wink:

Cheers
Andrea