Hello Gabriel and Jody,
I agree with most of what Gabriel says, it is mainly the same point as
mine except that I think it is unrealistic to deprecate the file()
method. First of all we would need to to move the resource API to
geotools because SLD files are stored in the resource store, but there
are still tertiary tools that require files like template and image
processors. The idea is that the file created on disk is only a cache
for these tertiary tools and manipulating it will have no effect.
Jody, most of the docs seem fine, except that the second blue box
explanation for 'path' is confusing paths with URLs: 'file:/' URLs are
not part of the resourcestore and resource rest API, and the other
problems is that docs are missing any mention of the "resource:" URLs.
We must first of all distinguish between (1) resourcestore API and (2)
Resources.fromURL method, I think there might be some confusion there.
Then the distinction between file: and resource: urls need to be in the
docs.
The (1) Resourcestore does not deal with URLs or absolute paths, only
relative paths. Internally, geoserver modules load their configuration
files in the resource store directly from this API.
The (2) Resources.fromURL deals with URLS and can handle both absolute
file paths and resources from the resourcestore using the 'resource:'
protocol prefix. This is for external use: the purpose is so that people
can refer to both files on disk and resources in the resource store
wherever they can specify a URL, for instance inside style files or in
the parameters of a data store. File: URLs is for the entire file
system, not the data directory and get 'converted' to a resource using
the Files.asResource wrapping method. Resource: URLs refer to anything
in the data directory or alternative resource store.
As legacy from pre-resourcestore times, geoserver supports (supported?)
'file:' URLs relative to the data directory (this is a little bit
'dodgy' since 'file:' urls are standardized and do not paths without
leading slash - https://en.wikipedia.org/wiki/File_URI_scheme ). Note
that the Resources.fromURL javadoc says this is deprecated and should be
replaced with resource: URLs.
Kind Regards
Niels
On 05/07/2023 15:06, Jody Garnett wrote:
Hey folks, I was tired and not in position to act clearly when these
gaps were noticed last year.
We can tighten up the api definition, at least with the changes made
we can now notice when an absolute path was used.
I never quite managed to communicate the separation between using a
file URL and File access (when DataStores wish to access local files)
as distinct from use of a resource for managing configuration files.
I suspect you both (Niels and Gabe) have met this distinction first
hand - but it is hard to explain the value.
Documentation is as added here:
https://docs.geoserver.org/latest/en/developer/programming-guide/config/resource.html
This could your review and input.
Jody
On Wed, Jul 5, 2023 at 2:34 PM Gabriel Roldan
<gabriel.rol...@gmail.com> wrote:
If "the codebase had drifted away from the intended use over time"
I think it's even more important to stick to the contract and not
the other way around.
As far as I can see, there are two abstractions, ResourceStore and
Resource, the former clearly says
"This abstraction assumes a unix like file system, all paths are
relative and use forward slash {@code /} as the separator",
the latter "Resource used for configuration storage.".
Only by adhering to this contract, we could provide alternative
implementations.
So if the changes were made to accommodate "common usage" on the
specific case of a filesystem-based ResourceStore
implementation, it should be the other way around, to find out
usages that don't adhere to the spec and fix them.
Concretely, I _think_ the only place where absolute URI's would be
requested to the ResourceStore, is where a data, not configuration,
file, is resolved, expecting the ResourceStore to be smart and
resolve "file:data/roads.shp" relative to the datadir, and
"file:/data/roads.shp"
relative to the file system.
Now, in doing so, we're asking the ResourceStore to do what it's
not intended to. The test case Niels mentioned used to check that
a leading "/"
had no meaning for the resource store (i.e. /data/roads.shp ==
data/roads.shp), and that was changed to mean the opposite.
So, IMHO, the responsibility of resolving files outside the
datadir shouldn't be on ResourceStore, but on the client code.
Something like:
String path = ...
File shp;
if(Paths.isAbsolute(path))
shp = new File(path);
else
shp = resourceStore.get(path).file();
As a matter of fact, Resource.file():java.io.File should be
deprecated and eventually removed. Resource is for config contents and
has Resource.in():InputStream and Resource.out():OutputStream.
It is hard enough already to adhere to lax interface contracts
(catalog's default workspace hello) as to keep on making it more
and more difficult.
So what do we do? can we get to an agreement that the contract in
the interfaces is mandatory and work from there?
cheers,
On Tue, 4 Jul 2023 at 17:49, Niels Charlier via Geoserver-devel
<geoserver-devel@lists.sourceforge.net> wrote:
Hello Jody,
Yes, I admit it's my own fault for missing this discussion at
the time.
I think it would be a shame to let the codebase further drift
away from the intended use of the resource store. We have done
the work to make the entirety of geoserver generic with
respect to the implementation of the data directory and it is
only a minimal effort to keep it this way. Even though jdbc
store is still a community module and the Redis based
geoserver project has been discontinued, it has been proven
that alternative implementations for the data directory work
and the jdbc store module is actually being used in production
successfully.
Now, interestingly, I have discovered that there are
contradictions in how it works / is documented now.
- I discovered that while 'theoryRootIsAbsolute' test is
successful, it is actually very misleading. At least on linux,
paths starting with '/' *still create a file relative to the
data directory*!. The only difference is the string returned
by the path() method. So while the path might seem absolute,
the file you are accessing is not. So the path() method is
misleading and in reality the leading slash is still being
ignored (again, at least on linux).
- Note that the javadoc of ResourceStore still says:
This abstraction assumes a unix like file system, *all paths
are relative*
There it says it: all paths are relative for the
resourcestore. Absolute paths have no meaning or function when
it comes to the resource store.
- I think this contradiction could be resolved in two possible
ways:
1) the test 'theoryIsRootSlashIsIgnored' should come back
instead of 'theoryRootIsAbsolute'. The root slash is ignored
and removed from the path.
2) The resource store throws an exception when you ask for an
absolute path.
Either way, all absolute paths should be handled _outside of_
the ResourceStore, for instance by calling Files.asResource().
So problems with absolute paths in the rest service should be
resolved in the rest service, or some other layer that is used
by the rest service.
Kind Regards
Niels
On 04/07/2023 21:59, Jody Garnett wrote:
Hello Niels,
The PRs for this activity contain extensive discussion.
The fundamental issue was the handling of absolute paths
which was done differently by different sections of code.
Specifically we found that the REST API endpoint was allowing
paths *//data* and */data *to both reference content in the
data directory, rather than treating the first one as an
absolute path. In response we tightened up the javadocs and
added test cases including the one you mentioned.
I agree that this goes against the goal of resource store,
but the codebase had drifted away from the intended use over
time.
Now that you are present in the discussion it would be a good
opportunity to discuss this with the parties involved.
--
Jody Garnett
On Jul 3, 2023 at 2:50:02 PM, Niels Charlier
<ni...@scitus.be> wrote:
Hello Jody and others,
I am having trouble understanding the changes that were made
about 6 months ago to the ResourceStore's expected behaviour.
In particular, in the class
'org.geoserver.platform.resource.ResourceTheoryTest', the
unit test 'theoryRootSlashIsIgnored' was replaced by
'theoryRootIsAbsolute'. I cannot make sense out of this
theory test at all.
This seems to be entirely contradictory to the whole reason
that the ResourceStore API was created, that is to make an
abstraction of the /Data Directory/, so that it can be
replaced by something else (such as jdbc store or other
implementations that have been made). There was already
support for absolute file paths in all circumstances by
using "file:" URLs. This will bypass the resource store and
call Files.asResource instead. But resource: URLs are for
the data directory or alternative resource store only. How
does it make sense to get absolute paths from the resource
store?
In order to make jdbc-config pass the tests, I will have to
turn off this particular method. But why should the test
even be there if the file resource store is the only one
that could ever support it? Programmers and users will rely
on this behaviour and support for all alternative
implementations of ResourceStore will be broken. In this
case we may as well do away with the API and just use the
file system directly again.
Kind Regards
Niels
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
--
Gabriel Roldán
--
--
Jody Garnett
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
