Created attachment 31998 [details] Proposed patch This patch simply adds a public NPOIFSFileSystem(FileChannel channel, boolean readOnly) constructor, since NPOIFSFileSystem(FileChannel) only allows to create a read-write one. This is especially important because of this change: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java?r1=1590319&r2=1590556&diff_format=h Since that change, using NPOIFileSystem with a non-writable FileChannel went from working to failing with: java.nio.channels.NonWritableChannelException at sun.nio.ch.FileChannelImpl.map(FileChannelImpl.java:838) at org.apache.poi.poifs.nio.FileBackedDataSource.read(FileBackedDataSource.java:77) at org.apache.poi.poifs.filesystem.NPOIFSFileSystem.getBlockAt(NPOIFSFileSystem.java:439) at org.apache.poi.poifs.filesystem.NPOIFSFileSystem.readBAT(NPOIFSFileSystem.java:413) This new constructor will allow to avoid such regression in client code. I'm using the regression severity because of this (hopefully that's appropriate). On a related note, NPOIFSFileSystem(File) is implying read-only mode, while NPOIFSFileSystem(FileChannel) is implying read-write. Is this intended?
The File constructor is read only, as it mirrors the InputStream one. The InputStream one is read only to match that of POIFS, which is always read only when opened for reading as it doesn't support in-place updates. NPOIFS can do in-place updates if you want it to, which is why it needs to offer read+write options I've applied your patch in r1624226, along with a few related javadoc tweaks to hopefully make read or write clear, thanks!
Nick, thanks for applying the patch so quickly and for the added comments. If I understand your argument for the File constructor being read-only to match InputStream, shouldn't that apply to the FileChannel as well? I.e. consistently use the read-only mode in all constructors. This change would have the added benefit that applications that pass a read-only FileChannel would not break when upgrading from 3.10 to 3.11. On the other hand, that could potentially break existing code, but only code using trunk or a 3.11 beta, which sounds like a lesser evil than breaking code using a released version. Attaching the proposed further patch.
Created attachment 32002 [details] Change read-only mode to true by default for FileChannels
That makes sense to me. Second patch applied in r1624266, thanks!