replace OpenmrsUtil.copyFile with IOUtils.copy

teleivo · October 9, 2016, 9:50am

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 side a little cleaner

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

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/util/OpenmrsUtil.java#L274


		}
		
		return null;
	}
	
	/**
	 * Copy file from inputStream onto the outputStream inputStream is not closed in this method
	 * outputStream /is/ closed at completion of this method
	 * 
	 * @param inputStream Stream to copy from
	 * @param outputStream Stream/location to copy to
	 * @throws IOException thrown if an error occurs during read/write
	 * @should not copy the outputstream if outputstream is null
	 * @should not copy the outputstream if inputstream is null
	 * @should copy inputstream to outputstream and close the outputstream
	 */
	public static void copyFile(InputStream inputStream, OutputStream outputStream) throws IOException {
		
		if (inputStream == null || outputStream == null) {
			if (outputStream != null) {
				IOUtils.closeQuietly(outputStream);

dkayiwa · October 9, 2016, 10:02am

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?

teleivo · October 9, 2016, 10:04am

sure, thats an easy and good step

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

dkayiwa · October 9, 2016, 10:22am

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 …

teleivo · October 9, 2016, 10:54am

Good point

teleivo · October 9, 2016, 1:44pm

ticket is here

lluismf · October 9, 2016, 6:26pm

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

teleivo · October 9, 2016, 6:58pm

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

lluismf · October 9, 2016, 7:30pm

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

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.

teleivo · October 9, 2016, 7:38pm

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

wyclif · October 10, 2016, 3:35pm

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)

teleivo · October 10, 2016, 3:57pm

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

wyclif · October 10, 2016, 4:30pm

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

dkayiwa · October 10, 2016, 4:36pm

Sounds good to me too!

darius · October 10, 2016, 5:16pm

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.

teleivo · October 10, 2016, 9:17pm

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.