Use sane fallbacks when reading empty files

[sschuberth]

Fixes #3861.

Signed-off-by: Sebastian Schuberth [email protected]

[oheger-bosch]

This is a rather big change, and because File.readValue() can now return null, the API is harder to use.

Therefore, I would propose an alternative approach: I wonder if it is worth the effort to check for empty files globally. I would argue that in most places an empty input file is just invalid and should be treated as if it contained syntactically wrong JSON or YAML. Only in special places we may want to be more lenient, e.g. when optional configuration files are involved.

So to distinguish between these cases, there could be a strict readValue() function behaving as before (maybe with improved error logging that mentions an empty file), and a new readValueOrDefault() function. The latter would do the empty check and return the default value if necessary. Then the caller can decide which variant to use, but would not have to bother with null values and more complex error handling.

[sschuberth]

The [readValueOrDefault() function] would do the empty check and return the default value if necessary.

One "problem" is that there is not "the default value", but it'd need to be configurable, so basically a configurable value to return instead of null, which somewhat leads us back to the current approach, just in another function.

I'll play around with the idea, though.

[sschuberth]

I would argue that in most places an empty input file is just invalid and should be treated as if it contained syntactically wrong JSON or YAML.

I also gave this some though again, and I actually believe it's not "most places". Basically, all configuration files are ok to be empty, but the result files are not. So it's rather 50/50 or so.

Only in special places we may want to be more lenient, e.g. when optional configuration files are involved.

Please note that there are no mandatory configuration files per se.

So to distinguish between these cases, there could be a strict readValue() function behaving as before (maybe with improved error logging that mentions an empty file), and a new readValueOrDefault() function.

What I dislike about that approach is that you're not forced / reminded to make that choice for all current uses in the code base, so you might miss to call readValueOrDefault() where currently readValue() is used, and only find out when you run into the issue next time.

More opinions from others?

[sschuberth]

@oss-review-toolkit/kotlin-devs I'm unsure how to proceed here. On the one hand I like the fact that all callers of File.readValue() have to decide how to handle empty files with the current approach. That ensures this case is covered throughout the code base. On the other hand I can understand the wish to reduce the size of the diff by introducing another function instead of changing the semantics of the current function, and leave it to the caller to call the right function.

Any more opinions here?

[sschuberth]

To move things forward, I've split out a more minimal fix using @oheger-bosch's proposal: #3926