[GEOS-11050] Refactor Resources and Paths API to support alternative …

…ResourceStore implementations
geoserver · Feb 29, 2024 · 9afad98 · 9afad98
1 parent 6fec0cf
commit 9afad98
Show file tree

Hide file tree

Showing 19 changed files with 437 additions and 297 deletions.
diff --git a/doc/en/developer/source/programming-guide/config/resource.rst b/doc/en/developer/source/programming-guide/config/resource.rst
@@ -21,17 +21,21 @@ Reference:
 
    The methods in the Resource API use String parameter names consistently:
 
-   * ``path`` relative location of resource within the data directory.
-
-     Paths are represented similar to a File URL, ``file://`` prefix used internally and
-     by the resource REST API.
-
-   * ``file`` a java File reference.
-
-   * ``url`` a location resolved with respect to the data directory.
-
-     A large number of special cases developed over time distilled into  ``Resources.fromUrl(base,url)`` and ``Files.url(base,url)`` methods.
-
+ * ``resource path`` path to a resource in the resource store. (For instance in the case of the default FileSystemResourceStore, this is file path that is relative with respect to the data directory, but for perserving generic behaviour compatible with any resource store, developers should not assume this to be the case). Resource paths do not support the `.` and `..` relative directory names. Resource paths use forward slashes, similar to URL's and unix style file paths, and are OS-independent.
+ * ``file path`` absolute path to a file in the file system. While these are OS dependent (with regard to the root of the absolute path) but they must always forward slashes, as supported by all operating systems and compatible with resource paths as well as file URL's. Note that ``Resource.path()`` for resources obtained by ``Files.asResource(file)`` will return a file path rather than a resource path.  
+ * ``file`` a java File reference.  
+ * ``url`` a location resolved with respect to the resource store. A number of special cases developed over time distilled into ``Resources.fromUrl(base,url)`` and ``Files.url(base,url)`` methods.  
+
+General Guidelines
+------------------
+
+All geoserver developers should be wary of the following general principles when contributing or reviewing:
+
+ * Avoid as much as possible using the file system directly. The only acceptable exception is when third party libraries require this, and even then the Resources API should be used maximally.
+ * For custom configuration files with a fixed location, always use ``ResourceStore``. ``GeoServerResourceLoader`` and ``GeoServerDataDirectory`` are legacy and should not be used in new code.
+ * For URL's provided by user configuration (such as templates, style sheets, etc), use ``Resources.fromURL``.
+ * For input/output, always use ``Resource.in()`` and ``Resource.out()``.
+ * Avoid the usage of ``Resource.file()`` and ``Resource.directory()``. These methods are only necessary for third party libraries that require usage of the file system, and only for input.  They should never be used for permanent output: Since there are alternative implementations of the ResourceStore that do not use the file system as underlying storage device, modifying them does not necessarily have a lasting effect
 
 ResourceStore
 -------------
@@ -83,17 +87,19 @@ Resource contents are streamed using ``out()`` and ``in()`` methods. The entire
 
 Resource ``path()`` provides the complete path relative to the ``ResourceStore`` base directory. Use ``name()`` to retrieve the resource name (as the last component in the path name sequence).
 
-Resource creation is handled in a lazy fashion, use ``file()`` or ``out()`` and the resource will be created as required.
+Resource creation is handled in a lazy fashion, use ``out()`` and the resource will be created as required, including any required parent directories are created to produce the completed path.
 
-Use ``dir()`` to create a empty directory. Directories have the ability to ``list()`` their contents:
+Directory resources have the ability to ``list()`` their contents:
 
 .. code-block:: java
    
    for( Resource child : resource.list()) {
       ...    
    }
 
-When creating a resource with ``file()``, ``out()`` or ``dir()`` any required parent directories are created to produce the completed path).
+The method ``isInternal()`` returns whether the resource is part of the resource store or rather a wrapped file obtained by ``File.asResource``. If this method returns `false` then ``path()`` returns a file path rather than a resource path.
+
+The methods ``file()`` and ``dir()`` may be used to obtain a file system representation of the resource. Depending on the resource store implementation, this may be the underlying storage entity (in the case of the default FileSystemResourceStore), or merely a cached entity. Changes to these should not be assumed to be permanent. These methods should only be used for input when a third library requires a file and does not support passing on streams.
 
 Once created resources can be managed with ``delete()``, ``renameTo(resource)`` methods.
 
@@ -104,7 +110,7 @@ Resource ``lock()`` is also supported.
 Paths
 -----
 
-The ``Paths`` facade provides methods for working with the relative paths used by ResourceStore.
+The ``Paths`` facade provides methods for working with resource paths used by ResourceStore.
 
 Helpful methods are provided for working with paths and names:
 
@@ -114,14 +120,21 @@ Helpful methods are provided for working with paths and names:
 * ``sidecar(path, extension)``
 * ``names(path)`` processes the path into a list of names as discussed below.
 
-The definition of a path has been expanded to work with the external locations (with ``Paths.isAbsolute(path)`` and ``Paths.names(path)``).
-
 Paths are broken down into a sequence of names, as listed by ``Paths.names(path)``:
 
 * ``Path.names("data/tasmania/roads.shp")`` is represented as a list of ``data``, ``tasmania``, ``roads.shp``.
-* On linux ``Path.names("/src/gis/cadaster/district.geopkg")`` starts with a marker to indicate an absolute path, resulting in ``/``, ``src``, ``gis``, ``cadaster``, ``district.geopkg``.
-* On windows ``Path.names("D:/gis/cadaster/district.geopkg")`` starts with a marker to indicate an absolute path, resulting in ``D:/``, ``gis``, ``cadaster``, ``district.geopkg``.
 
+For file paths that are OS dependent, use FilePaths.names instead.
+
+FilePaths
+---------
+
+The ``FilePaths`` facade provides methods for working with file paths.
+
+Paths are broken down into a sequence of names, as listed by ``Paths.names(path)``:
+
+* On linux ``FilePath.names("/src/gis/cadaster/district.geopkg")`` starts with a marker to indicate an absolute path, resulting in ``/``, ``src``, ``gis``, ``cadaster``, ``district.geopkg``.
+* On windows ``FilePath.names("D:/gis/cadaster/district.geopkg")`` starts with a marker to indicate an absolute path, resulting in ``D:/``, ``gis``, ``cadaster``, ``district.geopkg``.
 
 
 Paths.convert
@@ -167,15 +180,21 @@ There are also method for working with directories recursively and filtering con
 Resources.fromUrl
 ^^^^^^^^^^^^^^^^^
 
-There is an important method ``Resources.fromURL( baseDirectory, url)`` that is used by a lot of code trying to understand data references:
+The interpretation of the URLs is as follows:
 
+* ``resource:`` prefix - interpreted as a resource path, returns resource from the resource store.
+* ``file:`` prefix with absolute path - interpreted as file path, returns resource created by Files.asResource that refers to file in the file system.
+* ``file:`` prefix with relative path (deprecated) - interpreted as a resource path, returns resource from the resource store.
+
+Examples:
+
+* ``Resources.fromURL( baseDirectory, "resource:images/image.png")`` - resource path
+* ``Resources.fromURL( baseDirectory, "file:images/image.png")`` - resource path (deprecated)
 * ``Resources.fromURL( null, "/src/gis/cadaster/district.geopgk")`` - absolute file path (linux)
 * ``Resources.fromURL( baseDirectory, "D:\\gis\\cadaster\\district.geopkg")`` - absolute file path (windows)
 * ``Resources.fromURL( baseDirectory, "file:///D:/gis/cadaster/district.geopkg")`` - absolute file url (windows)
 * ``Resources.fromURL( baseDirectory, "ftp://veftp.gsfc.nasa.gov/bluemarble/")`` - null (external reference)
 
-For the absolute file references above, see the next section on ``Files.url``.
-
 Files
 -----
 
@@ -187,6 +206,7 @@ Files.url
 ^^^^^^^^^
 
 The other key method is ``Files.url( baseDirectory, url)`` which is used to look up files based on a user provided URL (or path).
+This method is deprecated because resources should always be used over files.
 
 * ``Files.fromURL( null, "resource:styles/logo.svg")`` - internal url format restricted to data directory content
 * ``Files.fromURL( null, "/src/gis/cadaster/district.geopgk")`` - absolute file path (linux)

diff --git a/...ity/backup-restore/core/src/test/java/org/geoserver/backuprestore/ResourceWriterTest.java b/...ity/backup-restore/core/src/test/java/org/geoserver/backuprestore/ResourceWriterTest.java
@@ -55,25 +55,15 @@ public void testResourceInfoAdditionalResourceWriter() throws IOException {
         GeoServerDataDirectory td = new GeoServerDataDirectory(root);
 
         Resource srcTemplatesDir = BackupUtils.dir(dd.get(Paths.BASE), "templates");
-        File srcTitleFtl =
-                Resources.createNewFile(
-                        Files.asResource(new File(srcTemplatesDir.dir(), "title.ftl")));
+        File srcTitleFtl = srcTemplatesDir.get("title.ftl").file();
         File srcHeaderFtl =
-                Resources.createNewFile(
-                        Files.asResource(
-                                new File(
-                                        Paths.toFile(
-                                                dd.get(Paths.BASE).dir(),
-                                                Paths.path("workspaces", "gs", "foo", "t1")),
-                                        "header.ftl")));
+                dd.get(Paths.BASE)
+                        .get(Paths.path("workspaces", "gs", "foo", "t1", "header.ftl"))
+                        .file();
         File srcFakeFtl =
-                Resources.createNewFile(
-                        Files.asResource(
-                                new File(
-                                        Paths.toFile(
-                                                dd.get(Paths.BASE).dir(),
-                                                Paths.path("workspaces", "gs", "foo", "t1")),
-                                        "fake.ftl")));
+                dd.get(Paths.BASE)
+                        .get(Paths.path("workspaces", "gs", "foo", "t1", "fake.ftl"))
+                        .file();
 
         assertTrue(Resources.exists(Files.asResource(srcTitleFtl)));
         assertTrue(Resources.exists(Files.asResource(srcHeaderFtl)));
@@ -90,21 +80,11 @@ public void testResourceInfoAdditionalResourceWriter() throws IOException {
 
         assertTrue(Resources.exists(trgTemplatesDir));
 
-        Resource trgTitleFtl = Files.asResource(new File(trgTemplatesDir.dir(), "title.ftl"));
+        Resource trgTitleFtl = trgTemplatesDir.get("title.ftl");
         Resource trgHeaderFtl =
-                Files.asResource(
-                        new File(
-                                Paths.toFile(
-                                        td.get(Paths.BASE).dir(),
-                                        Paths.path("workspaces", "gs", "foo", "t1")),
-                                "header.ftl"));
+                td.get(Paths.BASE).get(Paths.path("workspaces", "gs", "foo", "t1", "header.ftl"));
         Resource trgFakeFtl =
-                Files.asResource(
-                        new File(
-                                Paths.toFile(
-                                        td.get(Paths.BASE).dir(),
-                                        Paths.path("workspaces", "gs", "foo", "t1")),
-                                "fake.ftl"));
+                td.get(Paths.BASE).get(Paths.path("workspaces", "gs", "foo", "t1", "fake.ftl"));
 
         assertTrue(Resources.exists(trgTitleFtl));
         assertTrue(Resources.exists(trgHeaderFtl));

diff --git a/...extension/web-resource/src/main/java/org/geoserver/web/resources/PageResourceBrowser.java b/...extension/web-resource/src/main/java/org/geoserver/web/resources/PageResourceBrowser.java
@@ -361,7 +361,7 @@ protected Component getContents(String id) {
                             for (int i = 1; Resources.exists(store().get(dest)); i++) {
                                 dest = getIthPath(i);
                             }
-                            editPanel = new PanelEdit(id, dest, true, "");
+                            editPanel = new PanelEdit(id, store().get(dest), true, "");
                             return editPanel;
                         }
 
@@ -380,11 +380,7 @@ private String getPath() {
                         protected boolean onSubmit(AjaxRequestTarget target, Component contents) {
                             editPanel.getFeedbackMessages().clear();
                             String resourcePath = editPanel.getResource();
-                            if (Paths.isAbsolute(resourcePath)) {
-                                // although ResourceStore.get(path) is limited to relative paths
-                                // out of an abundance of caution we will reject
-                                error(getLocalizer().getString("pathUnsupported", getPage()));
-                            } else if (!Paths.isValid(resourcePath)) {
+                            if (!Paths.isValid(resourcePath)) {
                                 try {
                                     Paths.valid(resourcePath);
                                 } catch (IllegalArgumentException reason) {
@@ -465,7 +461,7 @@ public void onClick(AjaxRequestTarget target) {
 
                             @Override
                             protected Component getContents(String id) {
-                                editPanel = new PanelEdit(id, resource.path(), false, contents);
+                                editPanel = new PanelEdit(id, resource, false, contents);
                                 return editPanel;
                             }
 

diff --git a/src/extension/web-resource/src/main/java/org/geoserver/web/resources/PanelEdit.java b/src/extension/web-resource/src/main/java/org/geoserver/web/resources/PanelEdit.java
@@ -11,7 +11,7 @@
 import org.apache.wicket.markup.html.panel.FeedbackPanel;
 import org.apache.wicket.markup.html.panel.Panel;
 import org.apache.wicket.model.Model;
-import org.geoserver.platform.resource.Paths;
+import org.geoserver.platform.resource.Resource;
 
 /**
  * Panel for editing.
@@ -22,16 +22,16 @@ public class PanelEdit extends Panel {
 
     private static final long serialVersionUID = -31594049414032328L;
 
-    public PanelEdit(String id, String resource, boolean isNew, String contents) {
+    public PanelEdit(String id, Resource resource, boolean isNew, String contents) {
         super(id);
-        if (Paths.isAbsolute(resource)) {
+        if (!resource.isInternal()) {
             // double check resource browser cannot be used to edit
-            // absolute path locations
+            // files outside of resource store
             throw new IllegalStateException("Path location not supported by Resource Browser");
         }
         add(new FeedbackPanel("feedback").setOutputMarkupId(true));
         add(
-                new TextField<String>("resource", new Model<>(resource)) {
+                new TextField<String>("resource", new Model<>(resource.toString())) {
                     private static final long serialVersionUID = 1019950718780805835L;
 
                     @Override

diff --git a/src/extension/web-resource/src/main/java/org/geoserver/web/resources/ResourceNode.java b/src/extension/web-resource/src/main/java/org/geoserver/web/resources/ResourceNode.java
@@ -9,7 +9,6 @@
 import java.util.Set;
 import java.util.TreeSet;
 import org.apache.wicket.model.IModel;
-import org.geoserver.platform.resource.Paths;
 import org.geoserver.platform.resource.Resource;
 import org.geoserver.platform.resource.Resources;
 import org.geoserver.web.treeview.TreeNode;
@@ -30,9 +29,9 @@ public class ResourceNode implements TreeNode<Resource>, Comparable<ResourceNode
     private String uniqueId;
 
     public ResourceNode(Resource resource, ResourceExpandedStates expandedStates) {
-        if (Paths.isAbsolute(resource.path())) {
+        if (!resource.isInternal()) {
             // double check resource browser cannot be used to edit
-            // absolute path locations
+            // files outside of resource store
             throw new IllegalStateException("Path location not supported by Resource Browser");
         }
         this.resource = Resources.serializable(resource);

diff --git a/...tension/web-resource/src/main/java/org/geoserver/web/resources/WicketResourceAdaptor.java b/...tension/web-resource/src/main/java/org/geoserver/web/resources/WicketResourceAdaptor.java
@@ -11,7 +11,6 @@
 import org.apache.wicket.util.resource.AbstractResourceStream;
 import org.apache.wicket.util.resource.ResourceStreamNotFoundException;
 import org.apache.wicket.util.time.Time;
-import org.geoserver.platform.resource.Paths;
 import org.geoserver.platform.resource.Resource;
 
 /**
@@ -26,9 +25,9 @@ public class WicketResourceAdaptor extends AbstractResourceStream {
     protected Resource resource;
 
     public WicketResourceAdaptor(Resource resource) {
-        if (Paths.isAbsolute(resource.path())) {
+        if (!resource.isInternal()) {
             // double check resource browser cannot be used to edit
-            // absolute path locations
+            // files outside of resource store
             throw new IllegalStateException("Path location not supported by Resource Browser");
         }
         this.resource = resource;

diff --git a/src/main/src/main/java/org/geoserver/config/GeoServerDataDirectory.java b/src/main/src/main/java/org/geoserver/config/GeoServerDataDirectory.java
@@ -33,6 +33,7 @@
 import org.geoserver.catalog.WorkspaceInfo;
 import org.geoserver.platform.GeoServerExtensions;
 import org.geoserver.platform.GeoServerResourceLoader;
+import org.geoserver.platform.resource.FilePaths;
 import org.geoserver.platform.resource.Paths;
 import org.geoserver.platform.resource.Resource;
 import org.geoserver.platform.resource.Resource.Type;
@@ -929,7 +930,7 @@ public URL locateResource(String uri) {
                     file = resource.file();
                 } else {
                     // GEOS-7025: Just get the path; don't try to create the file
-                    file = Paths.toFile(root(), resource.path());
+                    file = FilePaths.toFile(root(), resource.path());
                 }
 
                 URL u = fileToUrlPreservingCqlTemplates(file);

diff --git a/src/platform/src/main/java/org/geoserver/platform/GeoServerExtensions.java b/src/platform/src/main/java/org/geoserver/platform/GeoServerExtensions.java
@@ -15,7 +15,7 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.servlet.ServletContext;
-import org.geoserver.platform.resource.Paths;
+import org.geoserver.platform.resource.FilePaths;
 import org.geotools.util.SoftValueHashMap;
 import org.geotools.util.SuppressFBWarnings;
 import org.geotools.util.factory.FactoryRegistry;
@@ -567,7 +567,7 @@ public static File file(String path) {
                     return file;
                 }
             } else {
-                List<String> items = Paths.names(path);
+                List<String> items = FilePaths.names(path);
                 int index = 0;
                 if (index < items.size()) {