Made RqMultipart.Base delete its temporary files as soon as possible.

[Bertram25]

For #254

I thus added a decorator to InputStream to permit deletion of the temp file
when calling close() as advised by @dmzaytsev

Thanks to @yegor256 and @dmzaytsev for the hints.

I also added a test to the new TempInputStream class to actually test the change.

[davvd]

@Bertram25 Thanks, let me find someone who can review this pull request

[davvd]

@pinaf it's yours, please review

[pinaf]

@Bertram25 0.21 here

[pinaf]

@Bertram25 3 comments above. Also, the test you added does not test RqMultiPart but TempInputStream. We should do the following:

1. Add a test class TempInputStreamTest for the test of TempInputStream
2. Add a unit test to RqMultiPartTest that actually uses RqMultiPart

[Bertram25]

@pinaf Thanks a lot for your review. :)

0.21 here

I adapted the version in the two newly created files.

deletesTempFile()

Fixed.

1. Add a test class TempInputStreamTest for the test of TempInputStream

Fixed. Thanks for the hint.

RqMultiPart can delete the underlying temporary file

Actually, as the test was indeed about TempInputStream, I reused your sentence as:
TempInputStream can delete the temporary cache file when it calls close().
in TempInputStreamTest.java

1. Add a unit test to RqMultiPartTest that actually uses RqMultiPart

The problem is I cannot know which temporary file is created as its name is random.
See: https://github.com/yegor256/takes/pull/318/files#diff-27dba3b19513f2e598661b82c434a82eR235
On my system, it is something like:
/tmp/org.takes.RqMultipart_XXXXXX.tmp (The XXXXXX part is made of random digits.).
Thus, depending on the OS and the implementation of File.createTempFile() it is really not easily testable explaining why I created a test for the underlying body class instead to make sure the file gets deleted.
IMHO, the change is actually tested and is functional, but if you have a better idea, I'll listen to your advice.

Best regards,

[pinaf]

@Bertram25 you should be the author here

[pinaf]

@Bertram25 thanks 2 comments

[Bertram25]

@pinaf Fixes done. Thanks for the new review. :)

[pinaf]

@rultor merge

[rultor]

@rultor merge

@pinaf Thanks for your request. @yegor256 Please confirm this.

[yegor256]

@Bertram25 I'm not the author of it :)

[yegor256]

@Bertram25 a few comments from me

[Bertram25]

@yegor256 Hey, thanks for the code review. :)

@Bertram25 I'm not the author of it :)

Fixed. (At first and some time ago already, I assumed we devs were to give the code away, hence the given authorship.)

@Bertram25 brrr... you're extending InputSteam and encapsulating it? isn't inheritance good enough?

Here, I encapsulated the origin InputStream to keep its functionalities in the functions overriden (read, skip, ...) (the origin InputStream in our specific case actually being a CapInputStream) and I extended InputStream as I simply needed it to add the desired functionalities for TempInputStream.

Also, note that you're doing the exact same thing in CapInputStream.java

If you prefer, I can have a go at extending CapInpuStream only instead, as it should suffice for our need. Is that the change you're requesting?

Best regards,

[Bertram25]

@yegor256 I'll rebase as the PR has become non mergeable.

[pinaf]

@Bertram25 you could just merge with master manually instead of rebasing

[Bertram25]

@pinaf Ok, most organizations usually prefer rebasing but I'll know I can just merge the next time. Thanks.

[pinaf]

@Bertram25 what's the status here?

[Bertram25]

@pinaf Hi,

@yegor256 asked me why I used encapsulation and extension of the same class InputStream

I answered I was extending the InputStream class to support the file deletion handling and I encapsulated the InputStream instance as I needed the features added by the decorated object there. (The encapsulated object instance is a CapInputStream in our case.).
Plus, the exact same thing is done in CapInputStream.java (@yegor256 is the author of that file AFAIK.)

Thus, IMHO, the code is good, but if @yegor256 still would like a change, I can extend CapInputStream instead and drop the encapsulation, while I still do think the current implementation is more extensible.

So, I'm still waiting for @yegor256 's decision here.

Regards,

[pinaf]

@Bertram25 thanks! @yegor256 please advise

[pinaf]

@yegor256 ping

[pinaf]

@yegor256 is this acceptable?

[yegor256]

@Bertram25 implementation of CapInputStream is also incorrect. You can easily reproduce a problem by this code:

InputStream stream = new CapInputStream(
  new ByteArrayInputStream(new byte[] {1, 2, 3}), 
  3
);
stream.skip(1);
assert stream.read() == 2;

Thus, I'm suggesting to submit a bug about CapInputStream and fix this branch too.

We should NOT encapsulate InputStream, but only extend it.

[pinaf]

@Bertram25 ping

[pinaf]

@Bertram25 please close the issue

[pinaf]

@yegor256 do you mind closing this issue?

[yegor256]

@Bertram25 @pinaf I'm closing it, it's too old

[pinaf]

@yegor256 why did you mark it as invalid?

[yegor256]

@pinaf my bad, I thought it's a ticket. removing the label now

[pinaf]

@yegor256 nothing yet :(

[pinaf]

@yegor256 could you please take a look at why I didn't get paid?

[pinaf]

@yegor256 I thought CRs were paid regardless of a merge. If this policy was changed please let me know so I don't have to bug you about payment.

[yegor256]

@davvd pls add extra 15 mins to @pinaf

[yegor256]

@pinaf no changes in the policy, it's just our internal problem, sorry about it

[pinaf]

@yegor256 it's cool. thank you.

[davvd]

@davvd pls add extra 15 mins to @pinaf

@yegor256 15 mins extra was just paid to @pinaf, transaction AP-8J079864TA4566600