Hi Jody, I get the impression I just scratched the tip of the iceberg 🤔
Resource.dir() creates a directory or throws an error. Resource.file() creates a file from an external resource if it exists. For plain filesystem resources however an empty file is created (if missing), but I still think this is questionable. Using out() creates the necessary directory and the file. There are very few places, where file() is used as output file to be opened directly, where the parent directory is expected to exist. In general I think exposing file() and in() or out() must be avoided as much as possible. Many pattern are like: Resources.copy(input.file(), output) or Resources.copy(input.in(), output). Those might be replaced by Resource.copy(Resource in, Resource out), but there arises a problem: While copy(File, Resource) assumes its output resource to be a directory and creates an output file with the same name, Resource.copy(InputStream, Resource) takes the output resource as file resource directly. OK, we have Resource.copy(InputStream, Resource, name) to use a target directory instead. Resource.copy(Resource, Resource) claims to support directories, but only for input. Currently there is no easy way, to copy a file resource into a target directory. It might be possible, to check if the output resource is a directory, but this is still ambiguous if both are directories. May be we simply need an extra method copyInto(Resource any, Resource directory) to make it explicit. This way about 65 usages of r.file() and r.in() can be avoided (there is soooo much boilerplate code to find). You may also have a look at my Pull Request #6570<https://github.com/geoserver/geoserver/pull/6570> which addresses a similar problem with XStreamPersister.load(). Concerning the Resource.Lock: On FileSystemResource.in() the lock is created in advance and I worried about what happens on error. But I did not see the final catch (FileNotFoundException e), where the lock is released, again. But what happens on any other exception? Why not create the lock within the FileInputStream constructor AFTER calling super(file) succeeds. And I frequently see: finally() { lock.release(); } Why not Lock extends Closeable? Dieter. Von: Jody Garnett <jody.garn...@gmail.com> Gesendet: Samstag, 4. Februar 2023 17:11 An: Dieter Stüken - con terra GmbH <d.stue...@conterra.de> Cc: GeoServer <geoserver-devel@lists.sourceforge.net> Betreff: Re: [Geoserver-devel] (GEOS-10705) Loading missing resources creates unsolicited empty files Dieter: First up you are absolutely not "annoying" with all your visions. Indeed I am quite happy to have such a conversation and feel it it *excellent* to have such discussion with a) the initial migration completed so we can see how the API is used b) someone (you) passionate / interested / caring. You may wish to try and attend one of the bi-weekly meetings for a more interactive discussion. I will try and respond to some of your ideas "inline" as part of this reply. My IDE finds 163 usages. Most of them are clearly read attempts. What is missing is some hint about the intention (read or write). Indeed if these are clearly read attempts then it would be good if they used in() or checked if the resource is a FILE before attempting a read. We have the file() method for when we work with systems like freemarker templates which only understand a java File reference. Any GeoServer code should be possible to refactor to use in(). Currently we have no change, since the API promises to create the file. But I think this is misleading. I begin to understand the background of Resource related to JDBCResource. In this case a file is created on demand from a database resource. But the relevant part is creating any missing directory structure, I think. There are several relevant parts, creating the missing directory structure and creating the file avoid lots of code having to make such work. Is there any description about what JDBCResource exactly does? It should be an implementation of the Resource API backed by JDBC Blobs rather than files. In the case where a file is needed (for example freemarker template) the file contents are "unpacked" from the database blob into a file on disk for use. Indeed in a cluster each geoserver blob would unpack such a file for use. If a Resource on the database was found, creating the associated file seems OK. If not, does it make sense to create an empty file in advance? If the file is being created to write to then indeed it should be created. If the file Is written using out(), is it finally written back to the database on close()? Yes. A possible refactoring process might be to introduce a method like file(boolean create). I am not sure this is needed. We do have some utility methods for common cases that could be used. I wish to have the minimal number of methods for resource store api implementations to manage. At first we must define file() -> file(true), to retain the API. But we may change all those obvious read attempts to use file(false) in a first step. Later on we can analyze the remaining calls. Could we make a summary of how many can be refactored to use out(), and make the rest use: - Resources.find(resource) - Resources.find(resource, boolean) Which behave in the manner you are proposing. I observed, that FileSystemResource and ResourceAdaptor have a lot of duplicate code in common. Is this intended? ResourceAdaptor is intended to be quite independent, used for test cases and so forth. ResourceAdaptor got some improvements which are not propagated to FileSystemResource. I may have some suggestions to refactor the duplicate code… Adding some package visible methods to Files class would be an appropriate location. I also try to understand the lock() system. I am curious what you will find. Another thing: If you are brave, the first thing you will notice is a terrible simulation of the Java file watching system. We were using an older version of Java that did not yet have WatchService when the API was implemented. What is there now was designed to be thrown away and replaced with WatchService when we upgraded to Java 7 :) The style of events is similar. I read something about previous problems and I get the feeling there is still a race condition. But I have to analyze this a bit deeper…. And some suggestion: I like to replace the IllegalStateException by UncheckedIOException (since 1.8). I think that will be fine; it will not be so confusing for JDBC implementation. -- Jody
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
