excel-streaming-reader
excel-streaming-reader copied to clipboard
Workbook open doesn't declare any possible thrown errors?
So i'm replacing standard poi workbooks with this to avoid a memory issue on my application and I went and replaced the workbook building to
workbook = StreamingReader.builder().open(inputStream);
This was inside a try catch block that would catch an IOException if the file couldn't be opened. Now the class won't compile because the builder open method doesn't declare any possible thrown errors.
This method is opening a file or interpreting an InputStream there's no way it's completely safe and unable to throw errors, why aren't they declared?
EDIT: The exceptions are thrown in the subclass StreamingWorkbookReader but I think they still should be declared as throws in the open methods of the builder
https://www.javadoc.io/static/com.github.pjfanning/excel-streaming-reader/4.2.1/com/github/pjfanning/xlsx/StreamingReader.Builder.html#open-java.io.InputStream-
The javadoc says the method can throw a ReadException. This is a RuntimeException. I guess this is not ideal. I didn't write the initial code and just took it on after the initial maintainer stopped doing releases. I would agree that RuntimeExceptions are not a good idea - but unfortunately if I change this, I can guarantee that I will have many more people complaining that the code is not backwardly compatible when they do their upgrades.
I do recommend that you do read the javadoc. Apache POI also has lots of RuntimeExceptions floating around. I would recommend that you catch RuntimeExceptions and handle them as you see appropriate.
My suggestion was because just looking at the following methods I didn't know what to try to catch and I expected them to just throw an IOException if it couldn't read the file (like poi does)
public Workbook open(InputStream is) {
StreamingWorkbookReader workbookReader = new StreamingWorkbookReader(this);
workbookReader.init(is);
return new StreamingWorkbook(workbookReader);
}
public Workbook open(File file) {
StreamingWorkbookReader workbookReader = new StreamingWorkbookReader(this);
workbookReader.init(file);
return new StreamingWorkbook(workbookReader);
}
After looking into it I would suggest to have the throws arguments added to them so the interface itself declares what errors it can throw and compilers complain when you don't try to catch them. As in having one of them be public Workbook open(File file) throws ReadException { ... for example
I'm not that experienced in java so please do take this issue with a grain of salt,
I've made some changes and may look at a 4.3.0 release in the next few weeks. A 4.3.0-SNAPSHOT is published.
The exceptions are still RuntimeExceptions as I don't want to break backward compatibility in 4.x releases. I have #228 open and a PR linked to it for a breaking set of changes that I might do for a 5.0.0 release (but no guarantee that I will pursue that).