Miretpl commented on code in PR #72162:
URL: https://github.com/apache/airflow/pull/72162#discussion_r4049248955
##########
providers/amazon/src/airflow/providers/amazon/aws/log/s3_task_handler.py:
##########
@@ -84,6 +84,18 @@ def upload(self, path: os.PathLike | str, ti: RuntimeTI |
None = None) -> None:
local_loc = self.base_log_folder.joinpath(path)
remote_loc = os.path.join(self.remote_base, path)
+ # The log path is supplied by the caller and is not guaranteed to stay
within
+ # ``base_log_folder``: ``joinpath`` and ``PurePath.relative_to`` are
purely lexical and
+ # do not normalise ``..``. Without this check a traversing path would
have its contents
+ # uploaded to the remote log store and, with ``delete_local_copy``,
its parent directory
+ # removed. Mirrors the containment check in
``CloudWatchRemoteLogIO.upload``.
Review Comment:
Let's say it is good enough.
> What they can't say is why the check has to exist: that the path comes
from the caller, and that joinpath and PurePath.relative_to are purely lexical
and will carry a .. through without normalising it. Nothing in the code below
hints at that, and it is the entire reason a lexical-looking comparison is not
enough here.
Usage of `joinpath` and `relative_to` hints that as it is how these
functions behave. I think that we should not document how Python works. If we
do, we will have a comment for every line of the code sooner or later, or most
of the codebase will be comments (not really readable for humans and noise for
AI - *I'm not including context cost factor here*). I believe that we should
not go in that direction.
> "comment sparingly, code says what, comments say why" — and then names
this exact carve-out: "Reserve longer explanation for genuinely complex or
non-obvious logic (e.g. a security check whose threat model isn't apparent)."
This instruction is not really precise. Following that, we can have a
comment explaining why we are doing e.g. `logging.info`. For me, most of the
comments which I see, generated by AI, follow this instruction but do not
follow the intent which should be carried by it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
