Skip to content

fix(storage): Fix TM download dir corner case#14142

Merged
krishnamd-jkp merged 2 commits intogoogleapis:mainfrom
krishnamd-jkp:tm_download_fix
Mar 10, 2026
Merged

fix(storage): Fix TM download dir corner case#14142
krishnamd-jkp merged 2 commits intogoogleapis:mainfrom
krishnamd-jkp:tm_download_fix

Conversation

@krishnamd-jkp
Copy link
Copy Markdown
Contributor

This PR fixes the corner case where download file path is resolved exactly to the download directory. The fix now checks for the parent folder of the final resolved file path and checks the relative path against the local directory.

@krishnamd-jkp krishnamd-jkp requested review from a team as code owners March 9, 2026 10:30
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Mar 9, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to fix a corner case in the download directory validation. While the test case fix and addition are correct and valuable, the change to the isSubPath function logic introduces a critical regression. It prevents downloading files directly into the specified download directory and also causes the newly added test to fail. My review includes a comment with a detailed explanation and a suggestion to revert the implementation change while keeping the test improvements.

Comment on lines +997 to +1000
absFilePathFolder := filepath.Dir(absFilePath)
// The relative path from the local directory to the file path.
// ex: if localDirectory is /tmp/foo and filePath is /tmp/foo/bar, rel will be "bar".
rel, err := filepath.Rel(absLocalDirectory, absFilePath)
rel, err := filepath.Rel(absLocalDirectory, absFilePathFolder)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The original implementation of this function was correct, and the issue was a faulty test case which this PR correctly fixes. However, this change to the implementation introduces a regression.

  1. With this change, isSubPath will incorrectly return false for files located directly in the localDirectory (e.g., localDirectory: /tmp/d, filePath: /tmp/d/file.txt). This is because filepath.Dir will result in absFilePathFolder being the same as absLocalDirectory, causing filepath.Rel to return ".", which then fails the check on line 1005 (rel != ".").

  2. Additionally, this change causes the new test case local directory is / to fail for the same reason.

Please revert the changes in this file. The original logic correctly handles all cases, including the one this PR aims to fix.

Suggested change
absFilePathFolder := filepath.Dir(absFilePath)
// The relative path from the local directory to the file path.
// ex: if localDirectory is /tmp/foo and filePath is /tmp/foo/bar, rel will be "bar".
rel, err := filepath.Rel(absLocalDirectory, absFilePath)
rel, err := filepath.Rel(absLocalDirectory, absFilePathFolder)
// The relative path from the local directory to the file path.
// ex: if localDirectory is /tmp/foo and filePath is /tmp/foo/bar, rel will be "bar".
rel, err := filepath.Rel(absLocalDirectory, absFilePath)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no such condition as rel != "."

@krishnamd-jkp
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request attempts to fix a corner case in download path validation. While the change addresses the specified case for non-root directories, it introduces an inconsistency where the check fails for the root directory (/). Additionally, a code comment has become outdated due to the logic change. I've added comments to highlight the logical flaw by suggesting a failing test case and to point out the stale comment for correction.

Comment thread storage/transfermanager/downloader_test.go
// The relative path from the local directory to the file path.
// ex: if localDirectory is /tmp/foo and filePath is /tmp/foo/bar, rel will be "bar".
rel, err := filepath.Rel(absLocalDirectory, absFilePath)
rel, err := filepath.Rel(absLocalDirectory, absFilePathFolder)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The comment on lines 998-999 is now misleading due to this change. For the example given (localDirectory of /tmp/foo and filePath of /tmp/foo/bar), rel will now be . instead of bar. Please update the comment to reflect the new logic and avoid future confusion.

@krishnamd-jkp krishnamd-jkp changed the title chore(storage): Fix TM download dir corner case fix(storage): Fix TM download dir corner case Mar 9, 2026
@krishnamd-jkp krishnamd-jkp merged commit 87cdcc9 into googleapis:main Mar 10, 2026
10 checks passed
krishnamd-jkp added a commit that referenced this pull request Mar 10, 2026
PR created by the Librarian CLI to initialize a release. Merging this PR
will auto trigger a release.

Librarian Version: v0.8.3
Language Image:
us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/librarian-go@sha256:19bb93e8f1f916c61b597db2bad65dc432f79baaabb210499d7d0e4ad1dffe29
<details><summary>storage: v1.61.0</summary>

##
[v1.61.0](storage/v1.60.0...storage/v1.61.0)
(2026-03-10)

### Features

* add bucket encryption enforcement configuration (#13874)
([245c8d7](245c8d76))

* add multistream options to MRD (#13758)
([4557675](4557675e))

* add a DeleteFolderRecursive API definition (PiperOrigin-RevId:
866471251)
([6f31019](6f310199))

* add multistream support to MRD (#13792)
([ffa7268](ffa7268c))

### Bug Fixes

* Fix TM download dir corner case (#14142)
([87cdcc9](87cdcc9f))

* Omit auto checksum in final request when MD5 is given (#14024)
([d404777](d4047774))

* optimize gRPC writer with zero-copy and lazy allocation (#13481)
([df64147](df641476))

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants