Skip to content

Refactor AbstractFileObject#getOutputStream() #151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
boris-petrov wants to merge 2 commits into from
Closed

Refactor AbstractFileObject#getOutputStream() #151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
093890c
Refactor to always calling `FileContent.getOutputStream`
boris-petrov Dec 16, 2020
ac6d276
Add a couple of TODOs
boris-petrov Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,9 @@ public void createFile() throws FileSystemException {
}

if (!exists()) {
getOutputStream().close();
endOutput();
try (FileContent content = getContent()) {
content.getOutputStream().close();
}
}
} catch (final RuntimeException re) {
throw re;
Expand Down Expand Up @@ -1227,6 +1228,7 @@ public FileName getName() {
return fileName;
}

// TODO: remove this method for the next major version as it is unused
Copy link
Member

@garydgregory garydgregory Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@boris-petrov boris-petrov Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@garydgregory garydgregory Dec 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@boris-petrov boris-petrov Dec 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* Prepares this file for writing. Makes sure it is either a file, or its parent folder exists. Returns an output
* stream to use to write the content of the file to.
Expand All @@ -1238,6 +1240,8 @@ public OutputStream getOutputStream() throws FileSystemException {
return getOutputStream(false);
}

// TODO: mark this method as `final` and package-private for the next major version because
// it shouldn't be used from anywhere other than `DefaultFileContent`
/**
* Prepares this file for writing. Makes sure it is either a file, or its parent folder exists. Returns an output
* stream to use to write the content of the file to.
Expand Down
10 changes: 1 addition & 9 deletions commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ram/RamFileOutputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
import java.io.IOException;
import java.io.OutputStream;

import org.apache.commons.vfs2.FileSystemException;

/**
* OutputStream to a RamFile.
*/
Expand Down Expand Up @@ -57,13 +55,7 @@ public void close() throws IOException {
if (exception != null) {
throw exception;
}
try {
this.closed = true;
// Close the
this.file.endOutput();
} catch (final Exception e) {
throw new FileSystemException(e);
}
this.closed = true;
}

@Override
Expand Down