Community
Participate
Working Groups
The RSE implementation of EFS / IFileStore does not support the documented option EFS#APPEND in IFileStore#getOutputStream(). Because of that, it is not possible to open an output stream which appends to an existing file. This is a major limitation since clients should be able to expect full API conformance from any EFS implementation. Fixing this issue will likely require a change to the IFileService API.
Kevin - you've been dealing with EFS a lot, would you like to take a look at this one? I suggest writing a unit test first, then fixing the bug (will likely require and API change to support opening a stream on the remote which appends to an existing file)
Fixed in RSEFileStoreImpl, LocalFileService, FTPService, SftpService, DStoreService, DStoreOutputStream, FileServiceSubSystem, AbstractFileService, IRemoteFileSubSystem, and IFileService. New Unit Test added, FileAppendOutputStreamTestCase. This will fail for SSH till Bug #211374 is fixed. Added API to IRemoteFileSubSystem and IFileService. Both have a new getOutputStream method which takes a boolean argument for whether the output stream should be append or overwrite. Old getOutputStream methods have been deprecated. New API: IRemoteFileSubSystem.getOutputStream(String, String, boolean, boolean append, IProgressMonitor) IFileService.getOutputStream(String, String, boolean, boolean append, IProgressMonitor)
I think it would be better to use an int options parameter instead of boolean append that would make our APIs more in-sync with EFS, and would keep us from having to change API again in case we invent another option.
(In reply to comment #3) > I think it would be better to use an > int options > parameter instead of > boolean append > > that would make our APIs more in-sync with EFS, and would keep us from having > to change API again in case we invent another option. > Okay, I'll add 2 constants APPEND and OVERWRITE to IFileService that match EFS.APPEND and EFS.OVERWRITE and fix up the rest of the API.
I've changed the API to be int options instead of boolean append. I also added 2 constants to IFileService. IFileService.APPEND and IFileService.NONE. I did not create IFileService.OVERWRITE as we don't need it. If you want a stream that is for overwriting specify IFileService.NONE. This works the same way as EFS. New API methods: IFileService.getOutputStream(String, String, boolean, int options, IProgressMonitor) IRemoteFileSubSystem.getOutputStream(String, String, boolean, int options, IProgressMonitor) I also created a new testcase for testing append/overwrite streams. FileOutputStreamTestCase. The old test was removed and refactored in this one.
(1) Just wondering, if we're already breaking API: shouldn't we also fold the boolean isBinary argument into the new int options argument by means of a constant IFileService#TEXT_MODE such that binary is the default assumed? - The only counter argument I can think of, is that this makes the API inconsistent with the file-based upload() and download() calls; but, on the other hand, since APPEND is only supported by the streams-based API they are inconsistent anyways. (2) The other thing I've noticed is that I don't like the way the new API is implemented in FTPService, LocalFileService and SftpFileService -- there is unnecessary dupliation of code in there, which makes maintenance unnecessarily hard. I think that the now deprecated implementation should just forward the call to the new implementation with the default constant: isBinary ? IFileService.NONE : IFileService.TEXT_MODE (3) In terms of backward compatibility, a service implementation that was able to return streams in 2.0 is now no longer able because AbstractFileService does not forward the call to the old impl. I think that the new getOutputStream() in AbstractFileService should not return a null stream directly, but forward to the old impl in case IFileService#APPEND is not set.
I committed a fix for (3) - default impl now forwards to old deprecated impl
Created attachment 85196 [details] Folding of isBinary into options I like the idea of merging isBinary into options. Looking at IFileStore almost everything has an options even if no options are currently supported. Attached is the patch for removing isBinary in favor of the option IFileService.TEXT_MODE. Also made the old deprecated implementations of getOutputStream call the new implementations for easier maintenance. Martin do you see anything else before I commit this?
(In reply to comment #8) Patch looks good, except one thing: Since the @deprecated comments will persist after the release, they should not reference any particular milestone. Therefore, instead of As of 3.0M4, replaced by they should be like @deprecated use {@link xxxx} instead
Patch committed and released for 3.0M4: [208778][api] Change new Streams Services API to use int options instead of boolean binary
