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.
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 May be this was related to: https://github.com/pmd/pmd/issues/3235 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) 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. Does it make sense to wrap the file using Files.asResource into a Resource here? Anyway: is the URL returned by Class.getResource always a File if it comes from a JAR instead? I think It's worth to open a separate subtask to refactor this at first. Dieter. Von: Jody Garnett <jody.garn...@gmail.com> Gesendet: Freitag, 21. Oktober 2022 21:58 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 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%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762577204%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=WS6HiPOGG7gg6E4SsnCg07apkqnWcU%2Bs04%2BSpwky9qQ%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%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762577204%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=dIAvutYZgvYtC9nsJSkaEHLDqIJb2sVEOO8aj1Tl4wg%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%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762577204%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=lGkmtaL5W5WHm32U9WtDe8dec%2FkwrSAK64nDJtfFPdY%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%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762577204%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=lGkmtaL5W5WHm32U9WtDe8dec%2FkwrSAK64nDJtfFPdY%3D&reserved=0> conterra.de<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.conterra.com%2F&data=05%7C01%7C%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762732808%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=aT5OrCwdh1MiQqge9k%2FVkJI2wrISHQajELfcioCl2Mw%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%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762732808%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=Rt770p58gYyd28xjGddi2GyWCxEnOrsAP6KKP%2B32kiM%3D&reserved=0> YouTube<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.youtube.com%2Fconterrachannel&data=05%7C01%7C%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762732808%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=NxsyuaZeNWG%2B1iMaSr3bV1x0cc5as7nyotgOsZ6vVts%3D&reserved=0> | Twitter<https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftwitter.com%2Fconterra&data=05%7C01%7C%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762732808%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=AC7o6gVa9OodCKBhJ22T2OBfkb7%2F%2BqyRWHrZL2UifvY%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%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762732808%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=CcKrpneV3CfU%2FBTtF7ylBP6n5VnIT2Jj5i5VyrpnxTg%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%7C2e008cb088634d28236808dab39e8a6a%7C6e0bfede3fcb4518a16565dc14fe5620%7C0%7C0%7C638019790762732808%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000%7C%7C%7C&sdata=Ghl%2F65FNsgDxlxsisQkicGhBQtKTFrtguFKi8Qyiqtk%3D&reserved=0> -- -- Jody Garnett
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
