I agree Niels, Let’s try and get this in before the RC which is (checks watch next Thursday).
My priorities are that the api be clear (confusing is terrible); that path be handled the same way internally everywhere (a strong response to earlier confusion); and that ResourceStore make no assumption about the fikesystem and be limited to “data directory” content as you say. I think we can compromise about resource store accepting a strong with a leading slash, I just ask that it convert it to a path immediately (and it can do so without a warning). Jody On Wed, Sep 13, 2023 at 11:38 AM Niels Charlier <ni...@scitus.be> wrote: > Hello Jody, > > We have a very different way of looking at things, but what matters most > is to find a solution moving forward even if we cannot convince each other. > So let's try to focus on what we each find most important and try to find a > compromise that respects both sides. > > The resource store is an abstract entity that is part of geoserver that > makes no assumption on file system or OS, the syntax is the same regardless > of OS, so to me resource paths are not file paths and vice versa, they live > in separate worlds. What is most important to me here is that resource > paths are OS independent. > > As far as point (3) concerns, the javadoc at the moment is confused and > confusing so that needs to be resolved one way or another. It starts with > talking about resource paths, but then suddenly it starts talking about > differences between operating systems. Resource paths should remain to be > OS independent, so we need to be clear about this. My preference would be > that resource paths and OS-dependent file paths are clearly separated from > each other, and therefore these methods should not be part of the same > utility class, but the bare minimum for me would be that the javadoc is at > least clear about the distinction. > > As far as point (1) concerns. The user has already been able to use > "resource:" paths for many years, but it is in no way obligated to so so, > their experience will indeed not change at all. (Admittedly, the only > application I could find though are image references in SLD files. ) The > javadoc from Resources.fromURL specifies: "File URL - will support relative > file references - this is deprecated, use resource: instead". This was the > original idea and the reasoning is 1) to make the distinction between OS > dependent file paths and OS independent resource paths; 2) relative file > paths violate the URL standard. So from my perspective, this would be the > logical way forward but since we cannot agree on this I would suggest to > not provide any warning and continue to allow both. On the same token, I do > not consider resource paths with a leading slash to be 'relative', in fact > to me they are absolute and make perfect sense and as long as they are part > of a resource: URL or used directly with the resource store they cannot be > misinterpreted, so I would prefer not to generate a warning for them. > > So the way forward I suggest would be to have no warnings and make sure > that the API functionality itself cannot be misinterpreted or misused, > whichever way you want to look at it. > > Concerning (4), there is indeed a page about the resources API. But for > new developers, this could be interpreted as an API that is free to use or > ignore. I mean to write some documentation with guidelines that all > developers should know to keep geoserver generic with respect to usage of > the data directory. > > Kind Regards > > Niels > > > > > On 13/09/2023 16:34, Jody Garnett wrote: > > You are arriving with this right as we try and find volunteers to make the > RC :) > > Please make a PR immediately and we can try and get it in. > > I am unclear about (1) - Can I confirm that users would not see resource: > URLs, only developers? I do not think relative paths should produce a > warning as they are the vast majority of cases - and the point of what we > are trying to do? > > The end user experience is what matters here and should not change. It is > slightly better if the rest api changes as (I think it could produce > warnings if absolute path is sent and ignored - indeed it may already). > > I strongly agree with (2). It can provide a warning for the leading slash > please, and correct it. The point is a path string should be consistent > everywhere in the codebase. > > I do not agree with (3) the point is to break resource stor me paths so > that we have consistency in our code base internals. > > (4) the page exists and can be clarified. > > Niels I am not understanding why you wish paths to be different? Each > opportunity to convert between paths produced problems in our codebase that > we are seeking to resolve by being strict? What am I missing … > > On Wed, Sep 13, 2023 at 5:30 AM Niels Charlier <ni...@scitus.be> wrote: > >> Hello Jody and Gabriel, >> >> Sorry for my late response. I have a suggestion to resolve the issue, >> please let me know what you think, >> >> My suggestion is that I would make a pull request with the following >> proposed changes, and you would review it. >> >> 1. The methods Resources.fromPath as well as Resources.fromURL with a >> "file:" URL follow your guidelines: relative paths are interpreted as >> resource store paths, absolute paths are interpreted as file system paths. >> However, I suggest that a relative "file:" URL produces a warning message >> that says that relative "file:" URLs are deprecated and that one should use >> a "resource:" URL instead. >> >> 2. The resource store implementation is separate from the two methods as >> specified above, and only provides access to files that are either in the >> data directory (file system resource store) or part of an external >> emulation of a file/folder hierarchy (such as jdbc database for the jdbc >> resource store). It has its own "root", and therefore the >> ResourceStore.get(...) method returns the same result with or without >> leading slash. A unit tests should check that the store does not provide >> any access to absolute paths on the file system. Javadoc should explain >> that Resources.fromPath or Resources.fromURL should be used for access to >> paths and URLs in general, and that direct usage of the resource store is >> only for access to configuration files and directories with a hard coded >> location (relative to the 'data directory'). >> >> 3. The Paths class javadoc is fixed or the class is refactored so that >> there is a clear distinction between resource store paths and file paths. >> >> 4. A page is added to the developer guide that specifies the above >> guidelines as well as general guidelines on how geoserver developers should >> deal with resources, URLs, files, etc. so that the generic nature of the >> resource store API is preserved in the future. >> >> I look forward to your feedback. >> >> Kind Regards >> >> Niels >> On 18/08/2023 17:35, Jody Garnett wrote: >> >> Niels: >> >> I checked if we were having a work party today, and put a little time >> into the activity. >> >> And I finally have a useful answer for you: Why did Resource absolute >> path change from being relative to the data directory? >> >> The answer is ... the concept of a Path was used all over our codebase, >> with different conversions happening, especially around absolute paths. >> This caused problems with a Java 11 change for windows which revealed how >> many different conventions we had in the codebase. >> >> So something had to change - paths was made consistent - so if you >> stopped with in a debugger, at any point, and saw a "path" you could have >> confidence in what it means. >> >> To that end a relative path (anywhere in the codebase) is now relative to >> the data directory location. >> >> There is a little bit of code in the REST Resource API that is forgiving >> for external scripts that use an absolute path. But it is changed to be >> consistent (ie relative path) as soon as possible. >> >> If you find any other special cases we can make note of them. >> -- >> Jody Garnett >> -- >> -- >> Jody Garnett >> >>
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
