GH-3076: Add file existence check #3077
Conversation
|
@artembilan Maybe we should discuss about modifying With my first suggestion, the I suggest
|
|
Sounds like a plan, @EmmanuelRoux ! Although I'd like to see some unit tests anyway. Thank you for your effort! |
EmmanuelRoux
force-pushed
the
GH-3076
branch
3 times, most recently
from
59d0f51 to
187483b
Compare
There was a problem hiding this comment.
Please, find my review.
It might be slight nit-picking, but we develop here Open Source and our code should be in high quality 😉
Also, consider in the future not squashing commits for PR. First of all we get a notification about new commits. Secondly it is much easier to review a historical flow. We squash them during merge anyway.
Thanks for undertstanding
| * <p> | ||
| * Locks acquired through this method should be passed back to #closeChannelFor to prevent memory leaks. | ||
| * <p> | ||
| * Thread safe. | ||
| */ | ||
| public static FileLock tryLockFor(File fileToLock) throws IOException { | ||
| if (!fileToLock.exists()) { | ||
| throw new FileNotFoundException("Unable to get lock for non-existent file: " + fileToLock.getAbsolutePath()); |
There was a problem hiding this comment.
Wouldn't return null instead of this exception enough for the method contract?
As you see in the NioFileLocker, you just suppress such an exception any way.
So, let's make a return null; here, add a @Nullable for method return and so on!
There was a problem hiding this comment.
That's what I suggested as an alternative in #3076. In my mind, the semantics were different between a lock that failed because the file was already locked (or any other filesystem-related reason), and an attempt to lock a file that does not exist.
Anyway, I will push a new version returning null 😉
There was a problem hiding this comment.
Well, if there is nothing to lock, then we definitely not locked. Seems for me that's the point of that contract.
So, let's live with null for non-existing file as well!
...n-file/src/test/java/org/springframework/integration/file/locking/FileChannelCacheTests.java
Outdated
Show resolved
Hide resolved
...n-file/src/test/java/org/springframework/integration/file/locking/FileChannelCacheTests.java
Show resolved
Hide resolved
| public TemporaryFolder temp = new TemporaryFolder() { | ||
|
|
||
| @Override | ||
| public void create() throws IOException { |
There was a problem hiding this comment.
I don't see reason in such extra logic.
It looks like temp.newFile("test0"); is fully enough against a default public TemporaryFolder temp = new TemporaryFolder();
Although I would insist on JUnit 5 already in the tests.
I'll back-port it to JUnit 4 for version 4.3.x on merge
There was a problem hiding this comment.
Reason is that TemporaryFolder.newFile(String) calls File.createNewFile() which is not what is desired for test case of a non-existent file
There was a problem hiding this comment.
Well, you can do then this.temp.getRoot() for a new File().
My point is: do not over-engineer a TemporaryFolder if we really can rely on its default public API.
With JUnit 5 we definitely don't have a luxury to extend extensions on the fly.
There was a problem hiding this comment.
You're right, let's go for that
| } | ||
| }; | ||
|
|
||
| @Test(expected = FileNotFoundException.class) |
There was a problem hiding this comment.
It is much cleaner to use an Assertions.assertThatExceptionOfType() in the code instead.
Although when we agree about null instead, this will go away anyway.
...tion-file/src/test/java/org/springframework/integration/file/locking/NioFileLockerTests.java
Show resolved
Hide resolved
|
... and cherry-picked to Back-ported to Will clean up code a little bit and fix AssertJ on Thank you very much for contribution; looking forward for more! |
Fixes #3076