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

Improve file storage list method #22

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kamiazya
Copy link
Owner

@kamiazya kamiazya commented Oct 29, 2023

This pull request makes several improvements to the file storage list method.

Key Modifications

  1. FileSystemStorageAdapter.js Changes:

    • Enhanced the list method.
    • Added checks for storage level read permission and directory permissions.
    • Implemented functionality to return an empty array if the directory does not exist.
    • Added handling for PermissionDeniedError when access is denied.
  2. New Utility Function - getGlobBase:

    • Added a new utility function, getGlobBase, which determines the base directory of a glob pattern.
    • This function is utilized in the FileSystemStorageAdapter.js to improve the handling of file paths and patterns.
  3. Test Suite Enhancements:

    • Updated tests for the FileSystemStorageAdapter to cover new scenarios and exceptions.
    • Added tests for the getGlobBase function to ensure its correct functionality across different glob patterns.

Impact on Project

These changes improve the robustness and flexibility of the file storage list method in Connectable IO. It enhances error handling and path resolution, leading to more reliable file system interactions.

Summary by CodeRabbit

  • New Features

    • Enhanced file storage plugin to support glob patterns for advanced file searching.
    • Introduced new validation to ensure storage readability before listing files.
  • Bug Fixes

    • Fixed an issue by requiring a prefix parameter in the file listing method to improve search accuracy.
  • Tests

    • Added tests to validate the existence of the base directory and proper handling of glob patterns.
    • Implemented unit tests for the new getGlobBase function.
  • Documentation

    • Provided an example for the getGlobBase function usage.

@kamiazya kamiazya added bug Something isn't working enhancement New feature or request labels Oct 29, 2023
@kamiazya kamiazya self-assigned this Oct 29, 2023

describe('getGlobBase', () => {
test.each([
{ pattrn: 'foo/bar/*.js', expected: 'foo/bar/' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo していました。

Suggested change
{ pattrn: 'foo/bar/*.js', expected: 'foo/bar/' },
{ pattern: 'foo/bar/*.js', expected: 'foo/bar/' },

Copy link
Owner Author

Choose a reason for hiding this comment

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

2f253cd で修正しました 🙏

Copy link

changeset-bot bot commented Dec 11, 2023

⚠️ No Changeset found

Latest commit: 2f253cd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kamiazya
Copy link
Owner Author

kamiazya commented Jan 8, 2024

@coderabbitai review

Copy link

coderabbitai bot commented Jan 8, 2024

Walkthrough

The updates to the Connectable IO's file storage plugin improve file listing capabilities by requiring a prefix parameter for glob pattern matching and ensuring the storage's readability. The addition of unit tests ensures the new functionality works correctly, including the verification of the base directory's existence. The list method now includes permission checks to verify storage accessibility, and a new utility function getGlobBase is introduced to extract the base directory from glob patterns.

Changes

File Path Change Summary
.../storage-plugin-file/src/adapters/FileSystemStorageAdapter.test.ts Added tests for base directory existence, glob patterns, and storage readability.
.../storage-plugin-file/src/adapters/FileSystemStorageAdapter.ts Updated list method with prefix parameter and permission checks.
.../storage-plugin-file/src/utils/get-glob-base.spec.ts Created file with unit tests for getGlobBase function.
.../storage-plugin-file/src/utils/get-glob-base.ts Introduced getGlobBase function for extracting base directory from glob patterns.
.../storage/src/models.ts Added exception for non-readable storage in listFiles method.

Poem

To list the files, a prefix we now need,
Through glob patterns, the paths we will lead.
🐇 CodeRabbit hopped in with a test,
Ensuring storage reads are the best.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f1013fd and 2f253cd.
Files selected for processing (5)
  • packages/@connectable-io/storage-plugin-file/src/adapters/FileSystemStorageAdapter.test.ts (4 hunks)
  • packages/@connectable-io/storage-plugin-file/src/adapters/FileSystemStorageAdapter.ts (2 hunks)
  • packages/@connectable-io/storage-plugin-file/src/utils/get-glob-base.spec.ts (1 hunks)
  • packages/@connectable-io/storage-plugin-file/src/utils/get-glob-base.ts (1 hunks)
  • packages/@connectable-io/storage/src/models.ts (1 hunks)
Additional comments: 9
packages/@connectable-io/storage-plugin-file/src/utils/get-glob-base.ts (1)
  • 14-22: The implementation of getGlobBase function looks correct and aligns with the PR objectives to improve path resolution.
packages/@connectable-io/storage-plugin-file/src/utils/get-glob-base.spec.ts (1)
  • 4-15: The test suite for getGlobBase function is comprehensive and covers a variety of glob patterns, ensuring the function's correctness.
packages/@connectable-io/storage/src/models.ts (1)
  • 88-88: The added documentation and exception cases for the listFiles method in the Storage interface are clear and enhance the error handling process.
packages/@connectable-io/storage-plugin-file/src/adapters/FileSystemStorageAdapter.ts (2)
  • 16-16: The import of getGlobBase is correctly added to be used within the list method.

  • 212-235: The changes to the list method, including the requirement of a prefix parameter and the implementation of permission checks, align with the PR objectives to enhance the file storage list functionality.

packages/@connectable-io/storage-plugin-file/src/adapters/FileSystemStorageAdapter.test.ts (4)
  • 3-3: The addition of constants from node:fs/promises is appropriate for the new tests that check for read permissions.

  • 125-131: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [128-140]

The test cases added for the list method correctly simulate the listing of files and the handling of non-existent directories.

  • 145-149: The test case that asserts an empty array is returned when the base directory does not exist is correct and aligns with the PR objectives.

  • 171-184: The test cases added for checking the glob base directory and storage readability are appropriate and ensure the new functionality works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants