[NET-642] FTPSClient execPROT removes proxy settings#90
Conversation
…xy Settings do not reset proxy settings when re-setting the socket factory create method identical to open _openDataConnection_ for FTPS where proxy is used and ssl socket is created from ssl context
|
Expecting a feedback. Also I'm not sure how to write a unit test here, any hints? |
|
You can start by looking at |
|
@YaniM ping. |
|
Yes, Gary. I see the hint but I don't know when I will make time for this. |
karnick
left a comment
There was a problem hiding this comment.
+1 We have been waiting for this fix.
|
@garydgregory |
|
We are blocked because of this issue to migrate to Commons NET. Will it be merged ? |
|
Still needs a unit test to fail without the main changes. |
| * @param sslSocket ssl socket | ||
| * @throws IOException closing sockets is not successful | ||
| */ | ||
| private void closeSockets(Socket socket, Socket sslSocket) throws IOException { |
There was a problem hiding this comment.
You should have a method that takes a single argument and call it for each input.
Use final when you can.
|
Hi, we are also blocked due to this issue and so far had to copy the entire FTP client to our project to get around it. We'll see if we can contribute the test, but I am not able to understand where the happy path unit test is either, so maybe it's better not to touch anything. A solution would be amazing! |
|
Hi, sooo... this PR allow to resolve a bug declared in 2017... but it's not merged because it lacks a unit test to fail without the main changes ? I wonder if every fork are made to use this PR as i'll do. |
|
There is at least one comment that has not been addressed. This puts the PR on the back burner for me. "Who would create a unit test with a socks proxy to prove it doesn't work as intended ?"
|
|
On the one hand, I agree with everything you said. On the other hand, i wonder why there is no test to prove it works in the first place. But maybe there is, and one case is just not covered ? In this case, maybe it's easier to implement the missing case. |
|
"i wonder why there is no test to prove it works in the first place" |
Lower visibility of method back
|
@YaniM this PR seems to break FTPS proxy support for HTTP type proxies - see https://issues.apache.org/jira/browse/NET-718 |
do not reset proxy settings when re-setting the socket factory
create method identical to open openDataConnection for FTPS where proxy is used and ssl socket is created from ssl context