Bug 56956 - [PATCH] add constructor NPOIFSFileSystem(FileChannel channel, boolean readOnly)
Summary: [PATCH] add constructor NPOIFSFileSystem(FileChannel channel, boolean readOnly)
Alias: None
Product: POI
Classification: Unclassified
Component: POIFS (show other bugs)
Version: 3.11-dev
Hardware: PC Linux
: P2 regression (vote)
Target Milestone: ---
Assignee: POI Developers List
Depends on:
Reported: 2014-09-11 07:13 UTC by Daniel Bonniot
Modified: 2014-09-11 12:07 UTC (History)
1 user (show)

Proposed patch (1.32 KB, patch)
2014-09-11 07:13 UTC, Daniel Bonniot
Details | Diff
Change read-only mode to true by default for FileChannels (1.24 KB, patch)
2014-09-11 11:08 UTC, Daniel Bonniot
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bonniot 2014-09-11 07:13:37 UTC
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:


Since that change, using NPOIFileSystem with a non-writable FileChannel went from working to failing with:

	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?
Comment 1 Nick Burch 2014-09-11 08:48:47 UTC
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!
Comment 2 Daniel Bonniot 2014-09-11 11:07:31 UTC
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.
Comment 3 Daniel Bonniot 2014-09-11 11:08:27 UTC
Created attachment 32002 [details]
Change read-only mode to true by default for FileChannels
Comment 4 Nick Burch 2014-09-11 12:07:47 UTC
That makes sense to me. Second patch applied in r1624266, thanks!