Yes it looks like an empty file should be created; the comments added to the top of the file probably form some instructions for use / customization.
On Thu, Oct 27, 2022 at 5:21 AM Dieter Stüken - con terra GmbH via Geoserver-devel <geoserver-devel@lists.sourceforge.net> wrote: > 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> 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> > *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> 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 > > > > <https://www.google.com/maps/search/Martin-Luther-King-Weg%0D%0A+20+%0D%0A+48155%0D%0A+M%C3%BCnster?entry=gmail&source=g> > > 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 > 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 > 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 > -- -- Jody Garnett
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
