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
