excel-streaming-reader icon indicating copy to clipboard operation
excel-streaming-reader copied to clipboard

Workbook open doesn't declare any possible thrown errors?

Open mauro-rizzi-DSP opened this issue 1 year ago • 3 comments

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

mauro-rizzi-DSP avatar Jan 18 '24 20:01 mauro-rizzi-DSP

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.

pjfanning avatar Jan 18 '24 20:01 pjfanning

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,

mauro-rizzi-DSP avatar Jan 18 '24 21:01 mauro-rizzi-DSP

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).

pjfanning avatar Jan 19 '24 12:01 pjfanning