replace OpenmrsUtil.copyFile with IOUtils.copy

,

Hello!

would you be in favor of replacing all occurrences of

OpenmrsUtil.copyFile(inputStream, outputStream)

in openmrs-core with the apache commons

IOUtils.copy(inputStream, outputStream)

version, so we could maybe deprecate OpenmrsUtil.copyFile ?

One difference is that the outputStream is not closed by IOUtils, but this can easily be handled with a try-with-resource block.

Just a thought to make the :camping: side a little cleaner :slight_smile:

OpenmrsUtil.copyFile is not tested, just adds to the codebase that has to be maintained and

How about not deprecating it and hence not replacing any of its occurrences, but simply changing its implementation to delegate to IOUtils.copy(inputStream, outputStream) with the necessary try with resource block in one place instead of leaving it with every caller? :slight_smile:

sure, thats an easy and good step :slight_smile:

but why provide methods that are already done in well known/tested libraries? (also in openmrs-core we already use apache commons)

What you are suggesting makes a lot of sense! Though for cases like this particular one, where each caller would have to duplicate the try with resource block, wrapping it in a utility method looks … :smile:

Good point :wink:

ticket is here

I’m not 100% sure but I think in Java 8 streams are closed automatically, so the try … finally block is not needed.

Several methods dealing with files in OpenmrsUtil can be replaced with Apache ones.

OpenmrsUtil.getFileAsString => FileUtils.readFileToString OpenmrsUtil.getFileAsBytes => FileUtils.readFileToByteArray OpenmrsUtil.copyFile => FileUtils.copyFile

I think the OutputStream needs to be closed. Are you talking about the Java 8 java.util.stream? IO based streams still need to be closed.

You are right that there is more to clean up :slight_smile:

It seems that to allow autoclosing you need to use a special try block. Never tried it though.

http://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

Both InputStream and OutputStream implement AutoCloseable, and its close method says:

void close() throws Exception Closes this resource, relinquishing any underlying resources. This method is invoked automatically on objects managed by the try-with-resources statement.

Thanks, I know :slight_smile: I said it in my first post. try-with-resource block came in java 7.

We probably need to deprecate OpenmrsUtil.copyFile(InputStream, InputStream) , the method name is misleading since it doesn’t really copy a file. And add a new one named OpenmrsUtil.copy(InputStream, InputStream)

Sounds good! Should I just update the ticket I created to reflect that?

I’d personally say yes, don’t know about the others

Sounds good to me too!

Peeking at github, I see a lot of places where OpenmrsUtil.copyFile is used outside of openmrs-core.

What exactly is the point of deprecating (and then removing) this method, and then having to make sure that all these modules change their code in time for Platform 3.0?

I vote for just changing the implementation of OpenmrsUtil.copyFile to delegate to the apache version, so we’re not maintaining our own logic, but otherwise we let people who have been calling this continue to do so.

Im ok with that, as that’s my main concern.

wyclif just said the method name is misleading which I think too but yes this means work for others too.