GSIP 132

GSIP 132 - Resource Store changes

Overview

These changes have been proposed while working on the JDBCResourceStore as well as a transition of the core modules from using files to using only resources.

This proposal makes deleting a resource consistent.

This proposal allows a new resource store to be provided by the jdbc-config module.

Proposed By

Niels Charlier

Assigned to Release

This proposal is for GeoServer 2.8-beta.

State

• Under Discussion
• In Progress
• Completed
• Rejected
• Deferred

Proposal

I propose to make a number of different changes to the resource store API:

1. Make Resource.delete() recursive and remove Resources.delete(Resource).

2. Let ResourceStore.remove(String) as well as Files.delete(String) return false when the file doesn't exist.

3. Let Resource.list() return an empty list when the resource is Undefined, throw an IllegalStateException when it is a file. Never return null.

4. The creation of a ResourceStoreProxy class which implements ResourceStore but delegates to another ResourceStore.

Motivation

1. Implementation of deleting full directories may differ per type of ResourceStore, recursion may or may not be necessary. In the case of the JDBCStore, looking up and deleting each file separately is highly inefficient, and can be optimised to use considerably less and more efficient SQL queries.

2. Consistency between Resource.delete and ResourceStore.remove in return value on non-existent files. Remove Resources.delete( resource ) as uncecessary.

   Goal is to match File.delete() to make it easier to migrate code.

3. The motivation for this is the currently very common use of GeoserverResourceLoader.findOrCreateDirectory. There is no equivalent for this method that avoids the use of the file system but enforces the creation of a directory in the resource store. Creation of files inside a non-existent directory will create that directory automatically. However, code like this PaletteManager.java:107 will produce a NPE if the directory does not exist yet. It is either necessary that the directory is already pre-existing in the resource system (which is only certain if at least one file is created in it) or we need to do a null check before each call to list(). Returning an empty list avoids this extra worry and confirms the principle that directories are created on the fly when necessary and new directories may be used as part of paths as if they already exist.

4. Spring configuration of the "officially" used resource store, whilst still remaining access to the DataDirectoryResourceStore if necessary (for example, to load the jdbc configuration in the first place).

API Change

1. The only "visible" change is in the Javadoc.

   interface Resource {      
     /**
      * Deletes a resource, if the resource is a directory contents will be recursively deleted.
      * 
      * @return true if resource(s) were deleted
      */
    boolean delete();
   
    /**
     * List of directory contents.
     * 
     * The listed files exist (and may be DIRECTORY or RESOURCE items).
     * 
     * @see File#listFiles()
     * @return List of directory contents, or an empty list for UNDEFINED or RESOURCE 
     */
     List<Resource> list();
   }
   

2. Visible changes to javadocs, and removal of Resources.delete( resources ).

   interface ResourceStore {
       ...
    /**
     * Remove resource at indicated path (including individual resources or directories).
     * <p>
     * Returns <code>true</code> if Resource was removed (or was never present). For read-only content
     * (or if a security check) prevents the resource from being removed <code>false</code> is returned.</p>
     * 
     * @param path Path of resource to remove
     * @return <code>false</code> if unable to remove and resource is still present, <code>true</code> if resource is now UNDEFINED.
     */
    boolean remove(String path);
   }
   

3. Previously, in the file system, we would do this

    File dir : resourceLoader.findOrCreateDirectory("my_config");
    for (File file :  dir.listFiles()) {
      //do something with each file
    }
   

   Without this change, this would need to be transformed to

    Resource dir : resourceStore.get("my_config");
    List list = dir.list();
    if (list != null) {
      for (Resource res :  list) {
        //do something with each file
      }
    }
   

   but with this change this can be simplified to:

    Resource dir : resourceStore.get("my_config");
    for (Resource res :  dir.list()) {
        //do something with each file
   }
   ```
   
   

4. Currently, applicationContext.xml in gs-main looks like this:

     <bean id="resourceStore" class="org.geoserver.platform.resource.DataDirectoryResourceStore">
       property name="lockProvider" ref="lockProvider"/>
     </bean>
   

   after this change, it'd look like this:

     <!-- resources -->
     <bean id="dataDirectoryResourceStore" class="org.geoserver.platform.resource.DataDirectoryResourceStore">
       <property name="lockProvider" ref="lockProvider"/>
     </bean>
    
     <bean id="resourceStore" class="org.geoserver.platform.resource.ResourceStoreProxy">
       <property name="delegate" ref="dataDirectoryResourceStore"/>
     </bean>
   

Feedback

Jody: Make a note to delete Resources.delete( resource ) as it will no longer be needed after API change.

• https://github.com/geoserver/geoserver/pull/1192

Backwards Compatibility

There is full backwards compatibility, except for the change in the ResourceStore.remove method. However analysis shows that nowhere in the Geoserver code where this method is used, an assumption is made on its return value for non-existing files.

Voting

• Alessio Fabiani
• Andrea Aime +0
• Ben Caradoc-Davies +0
• Christian Mueller
• Jody Garnett +1
• Jukka Rahkonen
• Phil Scadden
• Simone Giannecchini