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 > > > > 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) > 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%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 > > > > 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 > 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 >
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
