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
