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
