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

fileservice: ignore not found error in FileService.Delete #16134

Merged
merged 1 commit into from
May 15, 2024

Conversation

reusee
Copy link
Contributor

@reusee reusee commented May 15, 2024

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #15751

What this PR does / why we need it:

ignore not found error when deleting files.

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 15, 2024
@mergify mergify bot requested a review from sukki37 May 15, 2024 03:29
@matrix-meow
Copy link
Contributor

@reusee Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that the changes are related to ignoring "not found" errors in the FileService.Delete method.

Body:

The body of the pull request provides relevant information about the type of PR and the issue it fixes. It states that the PR is an improvement and addresses issue #15751 by ignoring "not found" errors when deleting files.

Changes:

  1. In file_service_test.go:

    • Additional test cases have been added to verify that deleting a non-existent file does not result in an error. This is a good practice to ensure the functionality works as expected.
  2. In local_etl_fs.go and local_fs.go:

    • The changes involve modifying the deleteSingle method to handle "not found" errors by ignoring them. This adjustment prevents the method from returning an error when attempting to delete a file that does not exist.

Feedback and Suggestions:

  1. Redundant Deletion in Test:

    • In the test case added in file_service_test.go, there is a redundant deletion of the same file immediately after the initial deletion. This redundant operation may not add significant value to the test and could be removed to streamline the test case.
  2. Error Handling Improvement:

    • While ignoring "not found" errors is a valid approach, it is essential to consider logging or handling other types of errors appropriately. Ignoring all errors, including potential permission issues or other unexpected errors, might lead to silent failures or issues being overlooked. Consider logging these errors for better visibility during debugging.
  3. Consistency in Error Handling:

    • The changes made in local_etl_fs.go and local_fs.go are similar. To maintain consistency and avoid duplication, consider refactoring the error handling logic into a shared function that can be reused across different file service implementations.
  4. Documentation Update:

    • It would be beneficial to update the relevant documentation or comments to reflect the change in behavior regarding how "not found" errors are handled in the deleteSingle method. This helps in maintaining code clarity and assists other developers in understanding the intent behind the changes.
  5. Security Consideration:

    • While ignoring "not found" errors may be appropriate for this specific scenario, ensure that sensitive file operations are handled securely. Validate user input and permissions to prevent potential security vulnerabilities, such as path traversal attacks or unauthorized file deletions.
  6. Optimization Opportunity:

    • Consider optimizing the code by avoiding unnecessary os.Stat calls before deletion. Since the goal is to delete the file regardless of its existence, directly attempting the deletion operation and handling any errors might be more efficient than performing a separate existence check.

By addressing the mentioned points and considering the suggestions provided, the pull request can be enhanced to improve code quality, maintainability, and security aspects.

@mergify mergify bot merged commit 083773f into matrixorigin:1.2-dev May 15, 2024
17 of 19 checks passed
@aylei aylei mentioned this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants