Throwing an Exception seems to be the better solution, I think. So you can differ between an empty file and a not existing file. I discovered some possible bug at:
https://github.com/geoserver/geoserver/blob/main/src/main/src/main/java/org/geoserver/config/GeoServerPropertyConfigurer.java#L104 Here an IOException is expected if the file is missing. Currently an empty file is silently created and I think this was not intended. If an IllegalStateException is thrown, it does not work well, too. This is the reason for [GEOS-10724]. I just try to find a unit test for GeoServerPropertyConfigurer.loadProperties with missing files. The existence can also be checked before without depending on an IOException. Should empty config files also be replaced here? Dieter. Von: Jody Garnett <jody.garn...@gmail.com> Gesendet: Donnerstag, 27. Oktober 2022 01:22 An: Dieter Stüken - con terra GmbH <d.stue...@conterra.de> Cc: geoserver-devel@lists.sourceforge.net Betreff: Re: [Geoserver-devel] (GEOS-10705) Loading missing resources creates unsolicited empty files Good discussion, feedback inline: On Wed, 26 Oct 2022 at 03:13, Dieter Stüken - con terra GmbH via Geoserver-devel <geoserver-devel@lists.sourceforge.net<mailto:geoserver-devel@lists.sourceforge.net>> wrote: The primary change (throwing an Exception for missing files) is quite easy, but I found lots of unit tests failing thereafter. Some are quite easy to fix, others require some refactorings. So this is going to be a judgement call: - Is it more clear to have an exception (forcing client code to handle) or runtime exception? - Is returning an empty inputstream going to be kinder / easier for code to manage? I'm a bit in struggle with EchoParametersDao and RulesDao. Those commonly use already opened InputStreams to read. Unfortunately it unclear who has to close them again and if try/resource block can be used. In the context of GEOS-9882 the InputStream was turned into a Supplier<InputStream> https://github.com/geoserver/geoserver/commit/9a8e4c30#diff-4b4acded1754ed2027dfc585dcfe55d05348911be59e55624c70b65fb6a815bbR53<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgeoserver%2Fgeoserver%2Fcommit%2F9a8e4c30%23diff-4b4acded1754ed2027dfc585dcfe55d05348911be59e55624c70b65fb6a815bbR53&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574733485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=UZ7LiAmt5BGl1wO7XlmEEY%2BzFlST8nr7xyrm%2F%2B6fyiI%3D&reserved=0> May be this was related to: https://github.com/pmd/pmd/issues/3235<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpmd%2Fpmd%2Fissues%2F3235&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574733485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1Ap2RhZ9jwGdxcBz2wtOEdmIDESKzaPTJGYGPQC5CDk%3D&reserved=0> I think the Resource must be forwarded down to the read function itself to keep the try/catch block as small as possible. (Similar argument for OutputStream here) This is an example where returning an empty inputstream would smoothly allow the existing code to function. The resulting stream.available() would always return 0 producing the desired functionality. However I like your approach of using Resource in the method api: RulesDao getRules() / RulesDao.saveOrUpdate() / deleteRules() ... I found, that the InputStream becomes necessary by TestSupport. doWork only, which works on File instead of Resource. And here we find another try-resource block again. This shows, how fragile it is to pass an open stream. That is a good one to fix; I was less concerned with the test cases using Files/ Does it make sense to wrap the file using Files.asResource into a Resource here? Yes, it is why the adaptor was created, to help test API in isolation. Anyway: is the URL returned by Class.getResource always a File if it comes from a JAR instead? I think it is when working with resources from your own module; but when working with resources from a jar it is best to use the GeoTools TestSupport class (which can unpack jar resources into a tmp file if needed). I think It's worth to open a separate subtask to refactor this at first. Agreed, thanks for digging into this Dieter it is a good maintenance activity that will benefit the codebase. Jody Dieter. Von: Jody Garnett <jody.garn...@gmail.com<mailto:jody.garn...@gmail.com>> Gesendet: Freitag, 21. Oktober 2022 21:58 An: Dieter Stüken - con terra GmbH <d.stue...@conterra.de<mailto:d.stue...@conterra.de>> Cc: GeoServer <geoserver-devel@lists.sourceforge.net<mailto:geoserver-devel@lists.sourceforge.net>> Betreff: Re: [Geoserver-devel] (GEOS-10705) Loading missing resources creates unsolicited empty files It is worth proposing a refactoring here. The guideline during the api developement was to optimize for the most common patterns of File usage we found in the codebase; and utillity methods such as in() for common usage patterns. Where possible we provided method name compatibility so it would be easier to migrate code from File to Resource. I was expecting far more input on this API when it was first introduced; your input is welcome now. For the specific example you show: - in() should not accidentally create a file for UNDEFINED - returning an empty input stream would be okay - less for client code to handle. I understand that failed parsing expression is not so fun - throwing UncheckedIOException is okay - concerned it would lead to a bunch of boiler plate code try / catch being introduced Since we have the whole codebase available it should be easy to access what this change would require and answer the concerns above. Whatever change is made I ask that the javadocs be clearly updated. -- Jody On Fri, Oct 21, 2022 at 12:03 PM Dieter Stüken - con terra GmbH via Geoserver-devel <geoserver-devel@lists.sourceforge.net<mailto:geoserver-devel@lists.sourceforge.net>> wrote: Hi dev list, I frequently noticed, that Resource.in() just creates non existing files and then returns an empty input stream. This almost ever causes further problems like https://github.com/geoserver/geoserver/pull/6216<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgeoserver%2Fgeoserver%2Fpull%2F6216&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574733485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=e0dFZW%2F0ZzmZ2d9HsevtnRe%2Ff5Am7y%2BSKrTcsFh3rJs%3D&reserved=0>. I think the root cause is the implementation of Resource.in() for UNDEFINED resources. Looking at Files.ResourceAdaptor.in<https://eur04.safelinks.protection.outlook.com/?url=http%3A%2F%2Ffiles.resourceadaptor.in%2F&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574733485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=UsQYbwt5zviFWI7WWcpyqucE%2F3zgTDBx2S%2FskxTapmA%3D&reserved=0>() I find: final File actualFile = file(); if (!actualFile.exists()) { throw new IllegalStateException("Cannot access " + actualFile); } Seems, the initial intention was to throw an exception, but since file() creates the missing file, it does not work as intended. I analyzed the former GSIP 106, but I did not find any hints about the expected behavior here. This can be fixed easily and in think an Exception about a missing file is much more expressive than a message from a failed content parser. Most use cases check for Type.RESOURCE in advance or have some elaborated exception handlers and are thus unaffected. Some usages however try to handle this condition later on by using inputStream.available() like EchoParametersDao.getEchoParameters(in) does and will fail then. I'm working on a pull request to change the behavior this way, but this causes adaptions to other related areas. Even SpringResourceAdaptor is affected, since it relays on IOExcetion which have been wrapped into IllegalStateException (and for which I think an UncheckedIOException is the better choice). Other solutions are: return null (can be checked afterwards but not so nice) return an empty InputStream (Java 11 even has some: InputStream.nullInputStream()) Do you think it is worth to propose a refactoring here or should I better keep hands off? Dieter. -- Dieter Stüken Software Engineer Nature Environment and Resources T +49 251 59689 403 d.stue...@conterra.de<mailto:d.stue...@conterra.de> con terra GmbH Martin-Luther-King-Weg 20<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.google.com%2Fmaps%2Fsearch%2FMartin-Luther-King-Weg%2B20%2B%250D%250A%2B48155%2BM%25C3%25BCnster%3Fentry%3Dgmail%26source%3Dg&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574733485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=M0FivA5pfA9s7ABC%2Fn3uVHiesse39Pn8J%2F4NSD6DswA%3D&reserved=0> 48155 Münster<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.google.com%2Fmaps%2Fsearch%2FMartin-Luther-King-Weg%2B20%2B%250D%250A%2B48155%2BM%25C3%25BCnster%3Fentry%3Dgmail%26source%3Dg&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574733485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=M0FivA5pfA9s7ABC%2Fn3uVHiesse39Pn8J%2F4NSD6DswA%3D&reserved=0> conterra.de<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.conterra.com%2F&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574733485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1rvFZ0o8hhopsbA55TZn3ghOMbAKpP2MTNe78YuIYuQ%3D&reserved=0> Geschäftsführung: Karl Wiesmann, Uwe König HRB 4149, Amtsgericht Münster Privacy statements<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.con-terra.com%2Fprivacy-statements&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574733485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RoEAdVnjU%2BDzX4PTNqgAKMXxueIVRRhFKcO3PpTWJVU%3D&reserved=0> YouTube<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.youtube.com%2Fconterrachannel&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574733485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vzUHSbtO4LlROd6qnm%2FMA2m92SpBpwrkMOIkg%2B%2Fn2Go%3D&reserved=0> | Twitter<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftwitter.com%2Fconterra&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574889268%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5TlBOT1VQiyku%2B3QNzsQaJ%2FWPsleniX5bbEwf2C%2BKQ8%3D&reserved=0> | LinkedIn<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.linkedin.com%2Fcompany%2Fcon-terra-gmbh&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574889268%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=S9MvI3wkMHM6W6tKtwRy7iYrUvU%2F0nbhWkqEUp1JJBA%3D&reserved=0> _______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net<mailto:Geoserver-devel@lists.sourceforge.net> https://lists.sourceforge.net/lists/listinfo/geoserver-devel<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Fgeoserver-devel&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574889268%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NuEPjnEsq6pw%2F5cZfaIAnDMthJ0RknsfC9M5g2Fw%2BUo%3D&reserved=0> -- -- Jody Garnett _______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net<mailto:Geoserver-devel@lists.sourceforge.net> https://lists.sourceforge.net/lists/listinfo/geoserver-devel<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Fgeoserver-devel&data=05%7C01%7C%7Ca17399575fcf4142a46308dab7a8f61f%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638024233574889268%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NuEPjnEsq6pw%2F5cZfaIAnDMthJ0RknsfC9M5g2Fw%2BUo%3D&reserved=0>
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
