GSIP 70 Reduce CatalogFacade API

GSIP 70 - Reduce CatalogFacade API

Overview

Run a deprecation cycle to save about 63 superfluous methods from CatalogFacade by making it more generic, and optionally remove the detach methods from Catalog. Depends on [GSIP 69](GSIP 69 - Catalog scalability enhancements).

Proposed By

Gabriel Roldan

Assigned to Release

This proposal depends on the acceptance and arrival of [GSIP 69 - Catalog scalability enhancements]. So target code base is trunk after that proposal arrives on trunk.

State

Under Discussion, In Progress, Completed, Rejected, Deferred

Motivation

Explain in decent detail why you are putting forth the proposal.

Proposal

The following proposal aims to reduce the CatalogFacade API, as long as reducing the number of methods in an interface is considered a good thing. Plan is to apply the following CatalogFacade API changes and through a one release deprecation cycle applying the proposed method removal.

Method Math: total methods added: 7 (2 to CatalogInfo, 5 to CatalogFacade). Total methods removed: 71 (8 from CatalogInfo, 63 from CatalogFacade). Result: 64 less API methods for implementers to deal with.

API reduction by generifying CRUD methods:

Currently, the CatalogFacade interface matches almost 1 - 1 the Catalog interface in terms of “CRUD” (Create, Read, Update, Delete) methods:

• 8 add (XInfo) methods could be synthesized into a single add (CatalogInfo)
• 8 remove (XInfo) methods could be synthesized into a single remove (CatalogInfo)
• 8 save (XInfo) methods could be synthesized into a single save (CatalogInfo)
• 18 query methods that return a single result could be reduced to the new <T extends CatalogInfo> get (Class<T> type, Predicate<T> filter) method. This includes: *** All the } methods; *** All the getXByName (String name) methods; *** Others like }, }; ** 13 query methods that return a List of catalog info objects could be reduced to the new <T extends CatalogInfo> Iterator<T> list (Class<T> of, Predicate<T> filter, Integer offset, Integer count) method.

This would reduce the CatalogFacade interface by 50 methods, making it more compact and scaling to the possible introduction of new CatalogInfo objects in the future without the need to expand the CatalogFacade interface for the general add/save/remove and query methods.

So, being a lower level API than Catalog, it would be good to reduce its API (i.e. going through a deprecation cycle) to the most basic and compact:

interface CatalogFacade{

   /*
    * Minimal generic API
    */

    <T extends CatalogInfo> T add(T);
    <T extends CatalogInfo> T remvoe(T);
    <T extends CatalogInfo> T save(T);
    /**
     * @return the single object of type {@code T} that matches the given filter,
     *         or {@code null} if no object matches the provided filter.
     * @throws IllegalArgumentExeption if more than one object of type {@code T}
     *         match the provided filter.
     */
    <T extends CatalogInfo> T get(Class<T> type, Predicate<T> filter) throws IllegalArgumentException;
    <T extends CatalogInfo> int count(Class<T> of, Predicate<T> filter);
    <T extends CatalogInfo> CloseableIterator<T> list(Class<T> of,
                                                   Predicate<T> filter, 
                                                   @Nullable Integer offset, 
                                                   @Nullable Integer count);


   /*
    * Additional non-generic methods
    */
    void dispose();
    Catalog getCatalog();
    void setCatalog(Catalog catalog);
    void syncTo(CatalogFacade other);
    DataStoreInfo getDefaultDataStore(WorkspaceInfo workspace);
    void setDefaultDataStore(WorkspaceInfo workspace, DataStoreInfo store);
    void setDefaultNamespace(NamespaceInfo defaultNamespace);
    NamespaceInfo getDefaultNamespace();
    void setDefaultWorkspace(WorkspaceInfo workspace);
    WorkspaceInfo getDefaultWorkspace();
}

CatalogFacade client code would need to be updated as follows:

import static org.geoserver.catalog.Predicates.*;
class CatalogImpl{
    //BEFORE
    public void add(LayerInfo layer) {
        ...
        facade.add(layer);//calls to CatalogFacade.add(LayerInfo)
    }

    public void add(LayerGroupInfo layer) {
        ...
        facade.add(layer);//calls to CatalogFacade.add(LayerGroupInfo)
    }

    //AFTER
    public void add(LayerInfo layer) {
        ...
        facade.add(layer);//calls to CatalogFacade.add(T)
    }

    public void add(LayerGroupInfo layerGroup) {
        ...
        facade.add(layerGroup);//calls to CatalogFacade.add(T)
    }


    //BEFORE
    public LayerInfo getLayer(String id) {
        return facade.getLayer(id);
    }

    public DataStoreInfo getDataStore(String id) {
        return facade.getStore(id, DataStoreInfo.class);
    }

    //AFTER
    public LayerInfo getLayer(String id) {
        return facade.get(LayerInfo.class, propertyEquals("id", id));
    }

    public DataStoreInfo getDataStore(String id) {
        return facade.get(DataStoreInfo.class, propertyEquals("id", id));
    }


    //BEFORE
    public StyleInfo getStyleByName(String name) {
        return facade.getStyleByName(name);
    }

    public LayerInfo getLayerByName(String name) {
        ...
        return facade.getLayerByName(name);
    }

    //AFTER
    public StyleInfo getStyleByName(String name) {
        return facade.get(StyleInfo.class, propertyEquals("name", name));
    }

    public LayerInfo getLayerByName(String name) {
        ...
        return facade.get(LayerInfo.class, propertyEquals("name", name));
    }


    //BEFORE
    public List<LayerInfo> getLayers() {
        return facade.getLayers();
    }

    public List<WorkspaceInfo> getWorkspaces() {
        return facade.getWorkspaces(); 
    }

    //AFTER
    public List<LayerInfo> getLayers() {
        CloseableIterator<LayerInfo> it = facade.list(
                                     LayerInfo.class, acceptAll(), null, null);
        try{
            return Lists.newArrayList(it);
        }finally{
            it.close();
        }
    }

    public List<WorkspaceInfo> getWorkspaces() {
        CloseableIterator<WorkspaceInfo> it = facade.list(
                                  WorkspaceInfo.class, acceptAll(), null, null);
        try{
            return Lists.newArrayList(it);
        }finally{
            it.close();
        }
    }


    //BEFORE
    public <T extends ResourceInfo> List<T> getResourcesByNamespace(
                                       NamespaceInfo namespace, Class<T> clazz) {
        return facade.getResourcesByNamespace(namespace, clazz);
    }


    public <T extends ResourceInfo> List<T> getResourcesByStore(
            StoreInfo store, Class<T> clazz) {
        return facade.getResourcesByStore(store, clazz); 
    }

    //AFTER
    public <T extends ResourceInfo> List<T> getResourcesByNamespace(
                                       NamespaceInfo namespace, Class<T> clazz) {
        Predicate<T> filter = propertyEquals("namespace.id", namespace.getId());
        CloseableIterator<T> it = facade.list(clazz, filter, null, null);
        try{
            return Lists.newArrayList(it);
        }finally{
            it.close();
        }
    }


    public <T extends ResourceInfo> List<T> getResourcesByStore(
            StoreInfo store, Class<T> clazz) {
        Predicate<T> filter = propertyEquals("store.id", store.getId());
        CloseableIterator<T> it = facade.list(clazz, filter, null, null);
        try{
            return Lists.newArrayList(it);
        }finally{
            it.close();
        }
    }

    // and so forth
}

Get Rid of CatalogFacade.dettach (\*Info) methods:

It +seems to me+ that the CatalogFacade.dettach (LayerGroupInfo|LayerInfo|MapInfo|NamespaceInfo|WorkSpaceInfo|? extends StoreInfo|? extends ResourceInfo) family of methods were added in order to enable the [Hibernate based dbconfig module](DBConfig Module) to “dettach” a catalog object from the underlying storage mechanism so that it can be used when no Hibernate session is around for the current thread.

I’ve only seen this used on Wicket UI detachable models, and IMHO makes programming against the API more painful and error prone, as in my understanding, dettach () should be called by any Catalog client code that doesn’t fit on the Hibernate’s “session-per-request” implementation pattern (to mind come WPS async processes and GWC tile requests, or any other piece of code that is not triggered by the same thread than an origin HTTP request?).

So, if this reasoning is correct, and the dbconfig module is to be kept around, perhaps a different approach could be taken by it in order to both scale well and deliver dettached objects, so that the Catalog and CatalogFacade dettach (…) methods are no longer needed.

Such an approach could possibly be to query for the list of object ids to return, then then returning a decorating/transforming list that fetches each dettached object on demand when an object is acquired from the list.

In any case, feedback welcomed.

Feedback

This section should contain feedback provided by PSC members who may have a problem with the proposal.

Backwards Compatibility

State here any backwards compatibility issues.

Voting

Alessio Fabiani: Andrea Aime: Ben Caradoc Davies: Christian Mueller: Gabriel Roldan: Jody Garnett: +1 on the idea (note impact to backwards compatibility) Jukka Rahkonen: Justin Deoliveira: Phil Scadden: Simone Giannecchini:

Links

JIRA Task Email Discussion Wiki Page