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

ISSUE-675: sanitize filename in video recorder #683

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ersanin
Copy link
Collaborator

@ersanin ersanin commented Nov 13, 2024

No description provided.

@ersanin ersanin force-pushed the ISSUE-675-Filenames-Sanitization-In-VideoRecorder branch from 10c2bcb to debbb0b Compare November 13, 2024 13:37
@@ -12,7 +12,10 @@ class VideosImpl(
) : Videos {

override fun record(tag: String) {
val videoFile: File = resourceFilesProvider.provideVideoFile(tag)
val sanitizedVideoName = tag
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this be a breaking change or not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we believe the comment in the original isse, then this changes fix only the broken case

Copy link
Collaborator

@Nikitae57 Nikitae57 left a comment

Choose a reason for hiding this comment

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

Minor issues. May release as is, but I would change it in the future

@@ -12,7 +12,10 @@ class VideosImpl(
) : Videos {

override fun record(tag: String) {
val videoFile: File = resourceFilesProvider.provideVideoFile(tag)
val sanitizedVideoName = tag
.replace("[/:\"*?<>|]+".toRegex(), "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. The issue creator used backing slash to cover each special simbol. It's not needed?
  2. Please, replace with the undescrore. User expected a simbol there
  3. Please, if the video name changed, then leave a warning log with something like log("Can't record video with name $tag since it contains one of the following special characters [/:\"*?<>|]. Changing the name to $sanitizedVideoName")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. We don't need this. IDE marks these special symbols as redundant.
  2. Done
  3. Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants