Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(server): restore modified at timestamp after upload, preserve when copying #7010

Merged
merged 1 commit into from Feb 12, 2024

Conversation

jextrevor
Copy link
Contributor

Created to address #2581.

This change makes it so that the asset service restores the mtime of the asset file using the fileModifiedAt parameter.

Also, if renaming the file fails and the storage template service falls back to the copy and delete path instead, the filesystem provider now preserves the atime and mtime from the old file when copying.

This is my first time contributing - let me know if I missed something or what I can do to make this cleaner. Thanks!

(Also, not sure if I should be classifying this as a feature or a fix, I can change the commit message if needed)

Comment on lines 84 to 86
await this.fixModifiedAtTimestamp(file, dto);
if (livePhotoFile) {
await this.fixModifiedAtTimestamp(livePhotoFile, dto);
Copy link
Contributor

Choose a reason for hiding this comment

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

These two can just go inside of the create method instead. Probably can move the sidecar one in there as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I've moved all timestamp updating into the create method

@@ -369,4 +375,8 @@ export class AssetService {
throw new BadRequestException('Quota has been exceeded!');
}
}

private async fixModifiedAtTimestamp(file: UploadFile, dto: CreateAssetDto) {
await utimes(file.originalPath, new Date(), new Date(dto.fileModifiedAt));
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't call fs methods directly in services. Can you just add this to the storage repository and use that here instead? It makes the tests simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I've added a utimes method to the storage repository/filesystem provider

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

LGTM

@alextran1502 alextran1502 merged commit d7437d3 into immich-app:main Feb 12, 2024
23 checks passed
@jextrevor jextrevor deleted the fix-modified-at-timestamps branch February 14, 2024 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants