Update usage of FileInput/OutputStream #1837
Conversation
| @@ -85,7 +86,8 @@ trait IoHelpers { | |||
| /** | |||
| * Read an entire file into an Array[Byte] | |||
| */ | |||
| def readWholeFile(file: File): Array[Byte] = readWholeStream(new FileInputStream(file)) | |||
| def readWholeFile(file: File): Array[Byte] = readWholeStream(Files.newInputStream(file.toPath)) | |||
| def readWholeFile(path: Path): Array[Byte] = readWholeStream(Files.newInputStream(path)) | |||
There was a problem hiding this comment.
Will the scaladoc appear for both methods if they're grouped like this or should we copy it to be above the second instance, too?
There was a problem hiding this comment.
I'm not sure. I'll try it locally and see what it looks like.
There was a problem hiding this comment.
It did not appear, so I copied it.
|
@eltimn Are we waiting on a first-hand testing report from you or is this good to merge? |
|
I did get our app upgraded to scala 2.12, but I'd need to publish a few lift modules compiled against 3.1 before I could test this. I did make these same changes to our app without issues, so I'm good with merging it. |
|
I added a spec to test |
|
Sweet. I say we ship this, then. |
| @@ -284,14 +284,14 @@ FileParamHolder(name, mimeType, fileName) | |||
| * @param localFile The local copy of the uploaded file | |||
| */ | |||
| class OnDiskFileParamHolder(override val name: String, override val mimeType: String, | |||
| override val fileName: String, val localFile: File) extends | |||
| override val fileName: String, val localFile: Path) extends | |||
There was a problem hiding this comment.
Whelp, I missed this guy. This will break all consumers of OnDiskFileParamHolder. Is there a way to get from Path to File? If not, I suggest we revert the changes to OnDiskFileParamHolder. If yes, I suggest we rename this parameter localPath and provide a localFile accessor that returns a File as existing code will expect. We can mark that one deprecated if we really want to.
There was a problem hiding this comment.
There's an apply function with File, so it shouldn't break anything. Also, there's a toPath function on File, and a toFile function on Path that can convert either way.
There was a problem hiding this comment.
See #1852 (comment) . Folks are using .localFile directly, and passing it to things that expect a File. Moreover, localFile is a poor name for something that isn't a File, but rather a Path, IMO.
I'll work up a PR to make this API-compatible, should only be a couple of lines.
There was a problem hiding this comment.
I just realized that's not a case class, though. Do I need to change that to a constructor?
There was a problem hiding this comment.
I see what I did wrong now. Thanks for fixing it.
There was a problem hiding this comment.
I'll probably add a constructor overload as well, good point. I also missed that it wasn't a case class hah. I doubt anyone is using the constructor directly, but better safe than sorry.
There was a problem hiding this comment.
Heh, so the reason FileInputStream and FileOutputStream are “considered harmful” in the Cloudbees post is because they use finalizers. But OnDiskFileParamHolder also has a finalizer. I think this is probably still a win, but worth mentioning.
There was a problem hiding this comment.
The finalize function is on OnDiskFileParamHolder, not on the FileStream class, though. I would assume that's a bit different, although I guess I don't know exactly how having finalizers affect GC.
There was a problem hiding this comment.
Yeah not sure either. Will keep it in mind in case anything around this comes up in the future.
Fixes #1835
I haven't been able to directly test this yet, but I started to upgrade our app to use Scala 2.12 and will as soon as that's done.