Thanks! Dieter you have prepared a number of fixes now; may I add you to the correct jira group so I can credit you as the person doing the fix? This also let's you indicate when you are working on an issue (ie in progress). -- Jody Garnett
On Fri, Feb 3, 2023 at 6:24 PM Dieter Stüken - con terra GmbH < d.stue...@conterra.de> wrote: > Hi Jody, > > > > after the pull requests for [GEOS-10723] [GEOS-10724] and [GEOS-10743] > accepted, we may also close the issues, doo. > > I went back to [10705] again and started another PR #6570 > <https://github.com/geoserver/geoserver/pull/6570>. > > > > The more I dive into the Resource API the more questions arise. > > After reading the GSIPs about it, I think I should probably have made a > separate GSIP from it. > > > > Solving the Resource.in() problem, I noticed, that calling Resorce.file() > raises similar problems. > > Probably we need a hint whether the intention is to read the file or to > create the file. > > But this really breaks the current API. > > > > I also just detected the jdbcstore community module. I’m currently about > to understand it…. > > > > Then I think about the IllegalStateException thrown. > > This should possibly be an UncheckedIOException (since 1.8) which > definitely covers the underlying FileNotFound Exception. > > Don’t see if this concerns any error handling. > > > > And then the idea of using Optional or Consumer<InputStream> to hide the > sensitive parts of dealing with the Inputstream. > > This may also improve many of the deeply nested error handlers. > > > > Regards, 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
