Skip to content

fix: support symlinks in atomic file write methods#3324

Merged
jarhodes314 merged 1 commit intothin-edge:mainfrom
reubenmiller:fix-atomic-write-symlinks
Jan 14, 2025
Merged

fix: support symlinks in atomic file write methods#3324
jarhodes314 merged 1 commit intothin-edge:mainfrom
reubenmiller:fix-atomic-write-symlinks

Conversation

@reubenmiller
Copy link
Copy Markdown
Contributor

@reubenmiller reubenmiller commented Jan 9, 2025

Proposed changes

Support resolving symlink targets files when calling atomically_write_file_sync and atomically_write_file_async.

Note: A symlinks target file will only be resolved if the target file exists, otherwise the path will be the symlink itself.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3323

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@reubenmiller reubenmiller added the theme:configuration Theme: Configuration management label Jan 9, 2025
jarhodes314
jarhodes314 reviewed Jan 9, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jarhodes314 jarhodes314 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It appears permissions are ignored (/essentially don't exist) for symlinks on Linux, so I have updated this to use read_link, which will read a broken symlink as well as a working one. As far as I understand, this won't hold true on BSD or MacOS, but we aren't targeting those platforms for production, and it's also an extreme edge case that you'd have to go to some effort to find yourself in.

My original (incorrect) thoughts:

I believe this would still clobber the symlink in the case where:

  • we have write access to the directory containing the symlink
  • we don't have read access to the symlink itself

Is there a way we can use file metadata to establish if the file is a symlink and fail if we happen to be in this case? Or do we actually care about solving this particular edge case?

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 9, 2025 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
551 0 2 551 100 1h28m19.926018s

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 94.59459% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/common/tedge_utils/src/fs.rs 94.59% 2 Missing and 2 partials ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@reubenmiller
Copy link
Copy Markdown
Contributor Author

reubenmiller commented Jan 10, 2025

I believe this would still clobber the symlink in the case where:

  • we have write access to the directory containing the symlink
  • we don't have read access to the symlink itself

Is there a way we can use file metadata to establish if the file is a symlink and fail if we happen to be in this case? Or do we actually care about solving this particular edge case?

Yes you're suggestions are definitely valid (the part about symlinks which don't point to an existing file also surprised me). Would you be able to take over this PR and do the implementation as per your suggestion?

didier-wenzek
didier-wenzek approved these changes Jan 13, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved. Better to squash the commits.

@jarhodes314 jarhodes314 force-pushed the fix-atomic-write-symlinks branch from 4f5c086 to 88707fe Compare January 13, 2025 16:19
@jarhodes314
Copy link
Copy Markdown
Contributor

jarhodes314 commented Jan 13, 2025

Approved. Better to squash the commits.

Now squashed

@jarhodes314 jarhodes314 added this pull request to the merge queue Jan 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 14, 2025
@jarhodes314 jarhodes314 added this pull request to the merge queue Jan 14, 2025
Merged via the queue into thin-edge:main with commit 664065b Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jarhodes314 jarhodes314 jarhodes314 left review comments

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@albinsuresh albinsuresh Awaiting requested review from albinsuresh albinsuresh is a code owner

@rina23q rina23q Awaiting requested review from rina23q rina23q is a code owner

Assignees

No one assigned

Labels

theme:configuration Theme: Configuration management

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@reubenmiller @jarhodes314 @didier-wenzek