[Geoserver-devel] resource store and base directory discussion

GeoServer GeoServer Developer
1

Niels.

I got a bit lost in our Resources port discussion, where we last left off …

GeoServerResourceLoader was acquiring the base directory location using the following:

public GeoServerResourceLoader(ResourceStore resourceStore) {
this.baseDirectory = resourceStore.get(Paths.BASE).dir();
this.resources = resourceStore;
}

I agree that an API change is needed to prevent this from happening.

I also note that these constructors are a bit confusing to tell what is provided for testing purposes, and what is provided for use at runtime. We should try and take better notes in the javadocs.

The official way to find the location of the GEOSERVER_BASE_DIRECTORY is via GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext). Admittedly this location is not cached.

I will try and prepare a patch to clarify javadocs, and replace the above with something like:

public GeoServerResourceLoader(ResourceStore resourceStore) {

if( resourceStore instanceof FileSystemResourceStore){
FileSystemResourceStore files = (FileSystemResourceStore) resourceStore;
this.baseDirectory = files.getBaseDirectory();
}
else {
this.baseDirectory = null; // client code will supply via setBaseDirectory
}
this.resources = resourceStore;
}

···

–
Jody Garnett

2

On 17-11-15 02:45, Jody Garnett wrote:

    public GeoServerResourceLoader(ResourceStore resourceStore) {
        if( resourceStore instanceof FileSystemResourceStore){
            FileSystemResourceStore files = (FileSystemResourceStore) resourceStore;
            this.baseDirectory = files.getBaseDirectory();
        }
        else {
            this.baseDirectory = null; // client code will supply via setBaseDirectory
        }
        this.resources = resourceStore;
    }

So what is going to happen if you are not use the FileSystemResourcestore? The baseDirectory is really necessary, several pieces of code depend on it, including classes loaded by Spring as well that need it to be there. The GeoServerResourceLoader is created around the ResourceStore, so the ResourceStore itself cannot set a property of the ResourceLoader on its initialisation. So what's going to happen? If the ResourceStore doesn't provide it, what will? And if something else provides it, why the need for the FileSystemResourceStore still providing it?

I know that you think that the JDBCResourceStore should not advertise a directory, but the point is this: even then GeoServer still relies on a file system directory and the JDBCResourceStore knows that directory (for importing & caching).

In my opinion the only real alternative to having a getBaseDirectory for every resource store is to get rid of the GeoServerResourceLoader.baseDirectory altogether. That would be the only way to be truly consistent with your philosophy: that there should be no assumption by GeoServerResourceLoader that the store is file/directory based.

Then we could still cache the result of GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext) in a static variable, which was my second suggestion, and perhaps slowly moving away from using it altogether.

Regards
Niels

3

Here is a branch where I am exploring the ideas brought up in our conversation:

https://github.com/jodygarnett/geoserver/commits/base_dir

···

So what is going to happen if you are not use the FileSystemResourcestore?

The method GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext) should be used to obtain the GEOSERVER_DATA_DIRECTORY location, it should be retrieved from GeoServerResourceLoader.getBaseDirectory(). If we need to adjust method / wrappers / constructor order to make that happen so be it :slight_smile:

The baseDirectory is really necessary, several pieces of code depend on it, including classes loaded by Spring as well that need it to be there.

I understand, it would help if we can short-list those cases so we can use them to know “when we are done”.

The GeoServerResourceLoader is created around the ResourceStore, so the ResourceStore itself cannot set a property of the ResourceLoader on its initialisation. So what’s going to happen? If the ResourceStore doesn’t provide it, what will? And if something else provides it, why the need for the FileSystemResourceStore still providing it?

The only reason FileSystemResourceStore is providing something is to fake out the system for testing purposes. This is what I meant about separating out the “real use” from the “test case” use.

We use a subclass of FileSystemResourceStore that obtains the value from GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext) during a real run.

I know that you think that the JDBCResourceStore should not advertise a directory, but the point is this: even then GeoServer still relies on a file system directory and the JDBCResourceStore knows that directory (for importing & caching).

We can adjust the api for JDBCResourceStore to provide a directory, we just need to ensure it gets that value from GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext).

In my opinion the only real alternative to having a getBaseDirectory for every resource store is to get rid of the GeoServerResourceLoader.baseDirectory altogether. That would be the only way to be truly consistent with your philosophy: that there should be no assumption by GeoServerResourceLoader that the store is file/directory based.

That is fine, to pull that off we still need to find a home for the value (for the code that needs file access). We could take this responsibility away from ResourceLoader and keep the GeoServerDataDirectory.root() method alive (I am glad to see most of the other methods there are deprecated and being replaced).

We could also force each ResourceStore (including JDBCResourceStore) to provide a getBaseDirectory() method as outlined above.

Then we could still cache the result of GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext) in a static variable, which was my second suggestion, and perhaps slowly moving away from using it altogether.

I hope to quickly move away from it. Even for uses like geowebcache and importer they are mostly looking up a directory of their own choosing (rather than assuming a path relative to GEOSERVER_DATA_DIRECTORY).

4

Hi Jody,

I have made a few minor edits to https://github.com/geoserver/geoserver/wiki/Cleaning-up-File-References

Now we just need to decide which of the two possibilities we prefer. I really don't mind which one.

Regards
Niels

On 18-11-15 01:48, Jody Garnett wrote:

Here is a branch where I am exploring the ideas brought up in our conversation:

https://github.com/jodygarnett/geoserver/commits/base_dir

    So what is going to happen if you are not use the
    FileSystemResourcestore?

The method GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext) should be used to obtain the GEOSERVER_DATA_DIRECTORY location, it should be retrieved from GeoServerResourceLoader.getBaseDirectory(). If we need to adjust method / wrappers / constructor order to make that happen so be it :slight_smile:

    The baseDirectory is really necessary, several pieces of code
    depend on it, including classes loaded by Spring as well that need
    it to be there.

I understand, it would help if we can short-list those cases so we can use them to know "when we are done".

    The GeoServerResourceLoader is created around the ResourceStore,
    so the ResourceStore itself cannot set a property of the
    ResourceLoader on its initialisation. So what's going to happen?
    If the ResourceStore doesn't provide it, what will? And if
    something else provides it, why the need for the
    FileSystemResourceStore still providing it?

The only reason FileSystemResourceStore is providing something is to fake out the system for testing purposes. This is what I meant about separating out the "real use" from the "test case" use.

We use a subclass of FileSystemResourceStore that obtains the value from GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext) during a real run.

    I know that you think that the JDBCResourceStore should not
    advertise a directory, but the point is this: even then GeoServer
    still relies on a file system directory and the JDBCResourceStore
    knows that directory (for importing & caching).

We can adjust the api for JDBCResourceStore to provide a directory, we just need to ensure it gets that value from GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext).

    In my opinion the only real alternative to having a
    getBaseDirectory for every resource store is to get rid of the
    GeoServerResourceLoader.baseDirectory altogether. That would be
    the only way to be truly consistent with your philosophy: that
    there should be no assumption by GeoServerResourceLoader that the
    store is file/directory based.

That is fine, to pull that off we still need to find a home for the value (for the code that needs file access). We could take this responsibility away from ResourceLoader and keep the GeoServerDataDirectory.root() method alive (I am glad to see most of the other methods there are deprecated and being replaced).

We could also force each ResourceStore (including JDBCResourceStore) to provide a getBaseDirectory() method as outlined above.

    Then we could still cache the result of
    GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext)
    in a static variable, which was my second suggestion, and perhaps
    slowly moving away from using it altogether.

I hope to quickly move away from it. Even for uses like geowebcache and importer they are mostly looking up a directory of their own choosing (rather than assuming a path relative to GEOSERVER_DATA_DIRECTORY).

5

Thanks Niels,

I think the responsible thing during application startup is to have GeoServerResourceLoader look up the correct value using lookupGeoServerDataDirectory(ServletContext) during application startup.

You have four examples of code wanting to do the file thing, they can call GeoServerResourceLoader.getBaseDirectory() and have a party.

This is better than doing anything with GeoServerDataDirectory.root() which is mostly deprecated - hopefully you can finish it off.

For test cases we can have the base directory value supplied by the injected FileSystemResourceLoader and short cut checking the ServletContext.

···

On 20 November 2015 at 01:27, Niels Charlier <niels@anonymised.com> wrote:

Hi Jody,

I have made a few minor edits to https://github.com/geoserver/geoserver/wiki/Cleaning-up-File-References

Now we just need to decide which of the two possibilities we prefer. I really don’t mind which one.

Regards
Niels

On 18-11-15 01:48, Jody Garnett wrote:

Here is a branch where I am exploring the ideas brought up in our conversation:

https://github.com/jodygarnett/geoserver/commits/base_dir

–
Jody Garnett

So what is going to happen if you are not use the FileSystemResourcestore?

The method GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext) should be used to obtain the GEOSERVER_DATA_DIRECTORY location, it should be retrieved from GeoServerResourceLoader.getBaseDirectory(). If we need to adjust method / wrappers / constructor order to make that happen so be it :slight_smile:

The baseDirectory is really necessary, several pieces of code depend on it, including classes loaded by Spring as well that need it to be there.

I understand, it would help if we can short-list those cases so we can use them to know “when we are done”.

The GeoServerResourceLoader is created around the ResourceStore, so the ResourceStore itself cannot set a property of the ResourceLoader on its initialisation. So what’s going to happen? If the ResourceStore doesn’t provide it, what will? And if something else provides it, why the need for the FileSystemResourceStore still providing it?

The only reason FileSystemResourceStore is providing something is to fake out the system for testing purposes. This is what I meant about separating out the “real use” from the “test case” use.

We use a subclass of FileSystemResourceStore that obtains the value from GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext) during a real run.

I know that you think that the JDBCResourceStore should not advertise a directory, but the point is this: even then GeoServer still relies on a file system directory and the JDBCResourceStore knows that directory (for importing & caching).

We can adjust the api for JDBCResourceStore to provide a directory, we just need to ensure it gets that value from GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext).

In my opinion the only real alternative to having a getBaseDirectory for every resource store is to get rid of the GeoServerResourceLoader.baseDirectory altogether. That would be the only way to be truly consistent with your philosophy: that there should be no assumption by GeoServerResourceLoader that the store is file/directory based.

That is fine, to pull that off we still need to find a home for the value (for the code that needs file access). We could take this responsibility away from ResourceLoader and keep the GeoServerDataDirectory.root() method alive (I am glad to see most of the other methods there are deprecated and being replaced).

We could also force each ResourceStore (including JDBCResourceStore) to provide a getBaseDirectory() method as outlined above.

Then we could still cache the result of GeoServerResourceLoader.lookupGeoServerDataDirectory(servletContext) in a static variable, which was my second suggestion, and perhaps slowly moving away from using it altogether.

I hope to quickly move away from it. Even for uses like geowebcache and importer they are mostly looking up a directory of their own choosing (rather than assuming a path relative to GEOSERVER_DATA_DIRECTORY).