Updating SFTPSensor to properly handle scenario where file is missing

[jroachgolf84]

This PR aims to address the issue raised in #51151. When a file failed to exist on the SFTP site, the SFTPSensor was still returning True. This change checks if the file does in fact exist. If not, it returns an empty list, resulting in the Sensor returning False and another poke being executed.

closes: #51151

[jroachgolf84]

cc: @RNHTTR, do you mind taking a look at this?

[BBQing]

@jroachgolf84 I think the try expect block around hook.isfile, can be removed, because the exception is not raised from that call. Also I would suggest to update the test case for missing file - because at the moment its behavior depends on the the OSError, that never gets actually raised

[jroachgolf84]

@jroachgolf84 I think the try expect block around hook.isfile, can be removed, because the exception is not raised from that call. Also I would suggest to update the test case for missing file - because at the moment its behavior depends on the the OSError, that never gets actually raised

This is not the case; .isfile can raise the OSError exception. In fact, that is the only exception that can be raised. See here.

airflow/providers/sftp/src/airflow/providers/sftp/hooks/sftp.py

Lines 220 to 230 in 1439e1f

	def isfile(self, path: str) -> bool:
	"""
	Check if the path provided is a file.
	:param path: full path to the remote file to check
	"""
	with self.get_managed_conn() as conn:
	try:
	return stat.S_ISREG(conn.stat(path).st_mode) # type: ignore
	except OSError:
	return False

[BBQing]

airflow/tests/providers/sftp/sensors/test_sftp.py

Line 47 in d9ed7b9

def test_file_absent(self, sftp_hook_mock):

I am thinking about the testcase for file absent - it depends on get_mod_time, which is called with the for loop over the actual_files_present - but since it is iteration over empty list, you shoudl never get the oserror side effect in the test.

[jroachgolf84]

I am thinking about the testcase for file absent - it depends on get_mod_time, which is called with the for loop over the actual_files_present - but since it is iteration over empty list, you shoudl never get the oserror side effect in the test.

Can you be more specific? Are you talking about the existing test?

[BBQing]

For some reason I have been looking at tests for version 2.11 - the tests for 3.0.x are fine. My bad.

Maybe should there be a test case, when you receive False from the hook.isfile instead of OSError as last thing I can think of.

[jroachgolf84]

Test added!

[BBQing]

One more thing - does the newly added test fail against the code without the change applied?

[jroachgolf84]

One more thing - does the newly added test fail against the code without the change applied?

This has been validated.

[jason810496]

Thanks for the fix, I left a small nit for the files_pattern case.