Niels: Comments inline -- Jody Garnett
On Aug 2, 2023 at 12:20:43 PM, Niels Charlier <ni...@scitus.be> wrote: > Hi Jody, > > I do support checking everything is handled correctly. > Thanks Niels, sorry I am a bit frustrated these days so am probably not explaining clearly. I just don't seem the point in breaking huge amounts of code for nothing. > And that is what will happen if we start throwing exceptions in the > resource store if the path starts with a "/". Which is actually normal, > because the resource store has a root. > ResourceStore no longer uses `/` as a root. The work done earlier this year removed the use leading / from the codebase (including the REST API Controller). > Please: define "absolute path". Is this OS dependent? Should the behaviour > of the resource store be OS dependent? What /exactly/ > I put the definitions into the document I shared and the javadocs (not sure why I cannot find the javadocs online?). When I say "path" I mean the strings used by the class Paths.isAbsolute(String) - https://github.com/geoserver/geoserver/blob/main/src/platform/src/main/java/org/geoserver/platform/resource/Paths.java#L550 - This has nothing to do with the ResourceStore API. This is covered in the developers guide here: https://docs.geoserver.org/latest/en/developer/programming-guide/config/resource.html#paths Paths are broken down into a sequence of names, as listed by Paths.names(path): - Path.names("data/tasmania/roads.shp") is represented as a list of data, tasmania, roads.shp. - On linux Path.names("/src/gis/cadaster/district.geopkg") starts with a marker to indicate an absolute path, resulting in /, src, gis, cadaster, district.geopkg. - On windows Path.names("D:/gis/cadaster/district.geopkg") starts with a marker to indicate an absolute path, resulting in D:/, gis, cadaster, district.geopkg. are you suggesting should be allowed or not allowed to access a resource > from the resource store? > I am suggesting paths that return true for this method be not allowed to access resources from the resource store. > Yes, "paths" are used across the application. But "path" is a vague term, > 'xpaths' are paths too, they have no relationship with file paths. Resource > paths are no different, they live in their own world and are not part of > the file system. The notion of a path being 'absolute' or 'relative' has no > meaning when you are talking about paths in general, this is file system > terminology. > Earlier in this email thread I shared the developer guide documentation which was written to define what Paths mean for GeoServer. > When someone gets a resource directly from the resource store, they are > using the resource store as a black box, and the "path" is always relative > to the _root_ of the resource store. > Correct, relative paths are with respect to the resource store location. Actually when I did he QA I found that there was at one point a search path that could be scanned to allow "data directory" to be spread across several folders (terrifying). > Resource.fromURL is a different story, these are not hard-coded URL's, > they are user provided, and then we allow both the file system as the > resource store to be accessed, where file: is for the file system and > resource: is for the resource store. > I have not seen users use "resource:", to my knowledge that was only used internally in the logic between in geoserver and geotools ( applied by a style visitor to help geotools resolve icons in the data directory.) See the examples Resource.fromURL(base, url) <https://docs.geoserver.org/latest/en/developer/programming-guide/config/resource.html#resources-fromurl> : - Resources.fromURL( null, "/src/gis/cadaster/district.geopgk") - absolute file path (linux) - Resources.fromURL( baseDirectory, "D:\\gis\\cadaster\\district.geopkg") - absolute file path (windows) - Resources.fromURL( baseDirectory, "file:///D:/gis/cadaster/district.geopkg") - absolute file url (windows) - Resources.fromURL( baseDirectory, "ftp://veftp.gsfc.nasa.gov/bluemarble/ ") - null (external reference) See the examples Paths.convert(base, filename) <https://docs.geoserver.org/latest/en/developer/programming-guide/config/resource.html#paths-convert> - Paths.convert(base,file) - uses URI relativize method to determine relative path (between file and base) - Paths.convert(base,folder, fileLocation) - can resolve relative location, limited to content within the base directory - Paths.convert(base, filename) See examples Files.fromURL( base, url) <https://docs.geoserver.org/latest/en/developer/programming-guide/config/resource.html#files-url> - Files.fromURL( null, "resource:styles/logo.svg") - internal url format restricted to data directory content - Files.fromURL( null, "/src/gis/cadaster/district.geopgk") - absolute file path (linux) - Files.fromURL( baseDirectory, "D:\\gis\\cadaster\\district.geopkg") - absolute file path (windows) - Files.fromURL( baseDirectory, "file:///D:/gis/cadaster/district.geopkg") - absolute file url (windows) - Files.fromURL( baseDirectory, "ftp://veftp.gsfc.nasa.gov/bluemarble/";) - null (external reference ignored as we cannot determine a file) - Files.fromURL( baseDirectory, "sde://user:pass@server:port") - null (custom strings are ignored as we cannot determine a file) Folks use Paths (in the sense defined above) here also; the is the motivation for fixing absolute paths on windows (as the definition had changed in Java 11) and where the inconsistency of use was found. > I just haven't seen any valid argument why the original design is flawed. > I do not believe the origional design was flawed, it was not consistently applied in the codebase. Where a path is used > Kind Regards > > Niels > Does it help that path and absolute path is defined strictly above? Thanks, Jody > On 02/08/2023 16:40, Jody Garnett wrote: > > Niels: > > We may be on the same page, you indicate ResoueceStore should never use > absolute paths for the file system. > > That is the decision I wish to make, and then we should go do the work to > enforce it. How much work? I am not sure, hence the work party. > > There is something subtle here though. You are focused on the > ResourceStore API, and I am asking you to consider the application as a > whole. > > We do have paths in the application including absolute paths. The QA > activity to check that these are handled correctly is the work. > > Paths are not only for ResourceStore. > > Jody > > > On Tue, Aug 1, 2023 at 11:55 PM Niels Charlier <ni...@scitus.be> wrote: > >> Hello Jody, >> >> Unfortunately I am not really on board with your plan. I really don't >> think we need to make a new branch and do a lot of work. This seems like a >> lot of unnecessary work. I think the original design is fine, and does not >> require breaking the code base. We just need to go back to how things were >> and perhaps fix a few cases where people have gone wrong since then. >> >> >> - >> >> Gabe and Niels are having trouble with the API changes made earlier >> this year >> - >> >> The definition of path was inconsistent for windows absolute paths >> >> I disagree. The definition of 'path' for the resource store is not in any >> way related to the OS, because the resource store lives in a separate world >> from the file system. The resource store and the file system have no joint >> concept of 'path'. >> >> - >> >> Early resource store work was not strict about use of paths, and when >> to use Resource and when to use File >> >> I disagree. It was always very clear about this. There should obviously >> never be hard coded absolute paths for the file system. In general the code >> should basically always use resources. Only where users can use URLs they >> have access to both the file system and the resource store, this is always >> how it was intended to work, as can be read from the javadoc. >> >> - >> >> The email conversation is going in circles because something has to >> break >> >> >> - >> >> We have to accept that something that break >> >> I disagree. I think nearly all of the code base is fine with how it was >> designed to work. >> >> Regards >> >> Niels >> On 01/08/2023 19:31, Jody Garnett wrote: >> >> Niel, Gabe: >> >> Our email thread on this topic has explored the topic by now, I think >> action rather than more communication is needed next. >> >> Can we setup a 1/2 day breakout work-party to get this activity underway >> (prior to the Bolsena code sprint and the next release cycle). Even a >> couple hours to get the topic started on a branch would be great. >> >> I am proposing two times below, and am willing to wake up early to see >> this topic move along: >> >> >> 1. August 8th >> >> <https://www.timeanddate.com/worldclock/meetingdetails.html?year=2023&month=8&day=8&hour=13&min=0&sec=0&p1=256&p2=45&p3=48> >> 15 >> CEST / 11 BRT / 6 PDT >> 2. August 15th >> >> <https://www.timeanddate.com/worldclock/meetingdetails.html?year=2023&month=8&day=15&hour=13&min=0&sec=0&p1=256&p2=45&p3=48>: >> 15 CEST / 11 BRT / 6 PDT >> >> >> Is this a good idea :) >> -- >> Jody Garnett >> >> -- > -- > Jody Garnett > >
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
