Refactor AbstractFileObject#getOutputStream() #151
Conversation
|
Hi @boris-petrov |
boris-petrov
force-pushed
the
refactor-get-output-stream
branch
from
67ea3a7
to
c0c8f43
Compare
|
@garydgregory - done. |
|
Hi @boris-petrov |
boris-petrov
force-pushed
the
refactor-get-output-stream
branch
2 times, most recently
from
53d884e
to
8bc1acb
Compare
|
@garydgregory - I rebased and added TODOs instead of changing the methods so that we remember to do it for the next major version. |
| @@ -1232,6 +1233,7 @@ public FileName getName() { | |||
| return fileName; | |||
| } | |||
|
|
|||
| // TODO: remove this method for the next major version as it is unused | |||
There was a problem hiding this comment.
It's a public method, you do not know who uses it, so you can't say it's "unused". I think this comment can be dropped.
There was a problem hiding this comment.
It's a public method, right. Note, however, that users of VFS always use FileObject which is an interface and it doesn't have this method. It's only available if one casts explicitly to AbstractFileObject (which no one should do) and to sub-classes (which should not call it at all). The only place this will be called is in DefaultFileContent (actually not this but the other overload). That's why I've written that this can be removed - so that in the future a conversation like this is not done. 😃
There was a problem hiding this comment.
Hi @boris-petrov
It does not matter as AbstractFileObject is a public method.
if one casts explicitly to AbstractFileObject (which no one should do)
Again, i does not matter as the class is available for 3rd party providers to subclass. Remember that anyone can plugin their own provider(s) in the framework, it's designed to be extensible. There are two kinds of clients of the API: (1) Traditional consumers of the API, and (2) File System providers, like VFS itself.
For 3.0 we can consider breaking the APIs but we are not there yet.
There was a problem hiding this comment.
You are correct for the two usages. My point is that neither customers of the API, nor FS providers should be calling these methods. Hence they could be made final and package-private. But not before 3.0 as you said. And because we're not there, I've added the TODOs so this is not forgotten.
| } catch (final Exception e) { | ||
| throw new FileSystemException(e); | ||
| } | ||
| this.closed = true; |
There was a problem hiding this comment.
This is a change in behavior that is likely to break some apps since endOutput() is no longer called. Needs more study...
There was a problem hiding this comment.
endOutput will be called because the only way to get a RamFileOutputStream is through DefaultFileContent which wraps the output stream and on close calls endOutput.
There was a problem hiding this comment.
Hi @rgoers,
Any thoughts as to whether or not this can break existing users?
TY!
boris-petrov
force-pushed
the
refactor-get-output-stream
branch
from
8bc1acb
to
34229c0
Compare
boris-petrov
force-pushed
the
refactor-get-output-stream
branch
from
34229c0
to
1d5815f
Compare
... instead of `AbstractFileObject.getOutputStream`. This allows us to not call `endOutput` explicitly from anywhere other than `DefaultFileContent`
boris-petrov
force-pushed
the
refactor-get-output-stream
branch
from
1d5815f
to
ac6d276
Compare
garydgregory
changed the title
garydgregory
changed the title
Update patch to avoid some possible if unlikely NPEs.
|
Hi @boris-petrov |
I'm not sure whether changing the visibility of
AbstractFileObject::getOutputStreamand marking it asfinalis not a breaking change. This change is not strictly needed but I did it to protect from someone accidentally calling it from the outside like was done inRamFileSystem.AbstractFileObject::getOutputStreamis an "internal" method which users should not call because closing the stream returned from it doesn't call the needed hooks - likeendOutput. That was whyRamFileOutputStream::closedid that explicitly (which is not nice because when creating a new file in the RAM provider,endOutputwould be called twice).See also #150.