[Geoserver-devel] [JIRA] (GEOS-10705) Loading missing resources creates unsolicited empty files.

GeoServer GeoServer Developer
1

Dieter Stüken created an issue

GeoServer / ImprovementGEOS-10705

Loading missing resources creates unsolicited empty files.

Issue Type:

ImprovementImprovement

Assignee:

Unassigned

Components:

Main

Created:

12/Oct/22 7:08 PM

Environment:

Geoserver startup

Priority:

MediumMedium

Reporter:

Dieter Stüken

While reading its configuration various config.xml files are located as a Resource and then parsed by XStreamPersister.load(r.in()). If the resource does not exist calling Resource.in() turns the missing resource into an empty file. This causes successive exceptions.

One of these cases was addressed by GEOS-10694, but I think this is a general problem here.

I find several locations, where a configuration is loaded from a resource. Some are testing if the resource exists, others don’t.

GWCConfigPersister.loadConfig()

try/catch

GeoServerLoader.depersist()

none

XStreamServiceLoader.load()

if/else

LoggingStartupContextListener.getLogging()

if/else

GeoServerSecurityManager.loadConfig()

throws IOException

GeoServerSecurityManager.loadConfigFile()

throws IOException

To handle these cases consistently I suggest introducing a general method

public <T> T load(Resource res, Class<T> clazz) throws IOException

to handle missing files either by returning null (which is handled correctly in most cases) or throwing a FileNotFoundException (which again leads to confusing catch/try blocks again).

There are many other (61) examples where Resource.in() is called unchecked or handled by complicated catch/try blocks, like in:

JDBCConfigurationStorage.getJDBCDiskQuotaConfig()

IMHO: I can’t figure out any situation, where creating an empty input file really makes sense to me. Instead, some Exception should be thrown. Looking at FileSystemResourceStore.in() I think this was indeed intended. However, using File actualFile = file() already creates the file unintentionally. Using this.file instead should restore the expected behavior and also leads to a more consistent stack trace. (I would rather suggest UncheckedIOException instead of IllegalStateException).

I even think about introducing some Optional<T> to replace lots of those try/catch and if/else monsters by something simpler like:

resource.tryLoad(someLoader::load).orElse(someDefault)

Unfortunately, most of the load() functions throw checked exceptions :worried: .

Add Comment

Add Comment

Get Jira notifications on your phone! Download the Jira Cloud app for Android or iOS

This message was sent by Atlassian Jira (v1001.0.0-SNAPSHOT#100208-sha1:399748d)

Atlassian logo