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

[api] Refactor /move API with additional validation checks #3991

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

Harshg999
Copy link
Collaborator

What changes were proposed in this pull request?

  • Add extra validation checks for the /move API to catch edge case scenarios and improve error handling.

How was this patch tested?

  • Manually
  • Unit tests

@Harshg999 Harshg999 self-assigned this Feb 7, 2025
@Harshg999 Harshg999 changed the title [api] Improve /move API with additional validation checks [api] Refactor /move API with additional validation checks Feb 7, 2025
@Harshg999 Harshg999 enabled auto-merge (squash) February 7, 2025 16:34
Copy link
Contributor

@JohanAhlen JohanAhlen left a comment

Choose a reason for hiding this comment

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

LGTM


# Check if destination path is parent of source path
if _is_destination_parent_of_source(request, source_path, destination_path):
return HttpResponse('Destination cannot be the parent directory of source.', status=400)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not the same as identical paths above? I take it it's different for folders but the message could be the same "Source and destination must be different"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Somehow I feel from API perspective, the original error message makes sense here and clearly explains the issue.

Assume you are hitting the API with source and destination path which are different but still you get the message that Source and destination must be different which feels vague and will require extra effort to figure out that the parent path cannot be same because that'd make move operation in same directory.

What do you think?

@Harshg999 Harshg999 merged commit 96978e7 into master Feb 12, 2025
5 of 6 checks passed
@Harshg999 Harshg999 deleted the improve-move branch February 12, 2025 16:19
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