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
