FTPHook methods should not change working directory #35105
Conversation
…ore_file methods, they no change working path anymore
|
This should be idone via try/finally or (better) following context_manager (and tests shoudl be fixed) |
airflow/providers/ftp/hooks/ftp.py
Outdated
| current_path = conn.pwd() | ||
| conn.cwd(path) | ||
|
|
||
| files = conn.nlst() | ||
| conn.cwd(current_path) |
There was a problem hiding this comment.
same here (https://pythontic.com/ftplib/ftp/nlst):
| current_path = conn.pwd() | |
| conn.cwd(path) | |
| files = conn.nlst() | |
| conn.cwd(current_path) | |
| files = conn.nlst(path=path) |
There was a problem hiding this comment.
Thank a lot. It's look better. Cant commit your suggestion because of typo: nlst method has no path name argument
There was a problem hiding this comment.
@hussein-awala can you see the changes you requested?
airflow/providers/ftp/hooks/ftp.py
Outdated
| @@ -182,10 +185,12 @@ def write_to_file_with_progress(data): | |||
| callback = output_handle.write | |||
|
|
|||
| remote_path, remote_file_name = os.path.split(remote_full_path) | |||
| current_path = conn.pwd() | |||
| conn.cwd(remote_path) | |||
| self.log.info("Retrieving file from FTP: %s", remote_full_path) | |||
| conn.retrbinary(f"RETR {remote_file_name}", callback, block_size) | |||
There was a problem hiding this comment.
Here you can provide the path of the file instead of its name to the command.
There was a problem hiding this comment.
You're right. The old implementation confused me, I thought that this was impossible - so I tried to get around it by calling the cwd method again. Perhaps there was a reason for this approach?
airflow/providers/ftp/hooks/ftp.py
Outdated
| @@ -214,8 +219,10 @@ def store_file( | |||
| else: | |||
| input_handle = local_full_path_or_buffer | |||
| remote_path, remote_file_name = os.path.split(remote_full_path) | |||
| current_path = conn.pwd() | |||
| conn.cwd(remote_path) | |||
| conn.storbinary(f"STOR {remote_file_name}", input_handle, block_size) | |||
There was a problem hiding this comment.
and here you can provide the path instead of the name too
There was a problem hiding this comment.
Thank you. Commit with your suggestion:
86f92ca
I don't seem to understand your proposal. Are you expecting this? remote_path, remote_file_name = os.path.split(remote_full_path)
current_path = conn.pwd()
try:
conn.cwd(remote_path)
conn.storbinary(f"STOR {remote_file_name}", input_handle, block_size)
finally:
conn.cwd(current_path)
I can't understand what it means. Can you point to an example please? |
…9b8d3aaef440471b115c108a5c2e#r1367884193 provide path directly to mlsd method Co-authored-by: Hussein Awala <[email protected]>
|
Much nicer indeed (after fixing static checks). BTW. Highly recommend installing pre-commit, it will even fix your static checks for you automatically. |
| conn.cwd(remote_path) | ||
| conn.storbinary(f"STOR {remote_file_name}", input_handle, block_size) | ||
|
|
||
| conn.storbinary(f"STOR {remote_full_path}", input_handle, block_size) |
There was a problem hiding this comment.
This is probably already an issue previously; do we need to quote the path here?
There was a problem hiding this comment.
I got ftplib.error_perm: 550 Failed to open file when surround path with quotes. But your proposal is understandable. Now there should be problems if there are spaces in the path
There was a problem hiding this comment.
Try to retrieve and store file to path with spaces without quoting - it works. Probably ftplib somehow handles this internally
There was a problem hiding this comment.
The RFC does not allow quoting for STOR and simply STOR expects filename that ends with EOL so spaces are perfectly fine there:
|
This is actually a change in behavior in |
This methods change current directory. And second call with same directory will raise
no such file or directoryerror. So I addcwdcall to change back to origin directorycloses: #35015