Review existing crawler client implementations #107
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…verage This commit introduces support for additional protocols and cloud storage services: ## New Clients Implemented: 1. **SftpClient** - SFTP (SSH File Transfer Protocol) support using JSch 2. **WebDavClient** - WebDAV protocol support using Sardine 3. **GitClient** - Git repository crawling using JGit 4. **AwsS3Client** - AWS S3 dedicated client using AWS SDK for Java v2 5. **AzureBlobClient** - Azure Blob Storage support using Azure Storage SDK 6. **GoogleCloudStorageClient** - Google Cloud Storage support using GCS client library ## Features: - All clients extend AbstractCrawlerClient for consistent behavior - Authentication support for each protocol (username/password, private keys, API keys) - Pattern-based authentication matching for flexible credential management - Directory/container listing with ChildUrlsException for hierarchical navigation - Content length validation and MIME type detection - Large file handling with temporary file storage - Comprehensive metadata extraction - Timeout management for all operations - Full test coverage for each client ## Dependencies Added: - com.jcraft:jsch:0.1.55 (SFTP) - com.github.lookfirst:sardine:5.12 (WebDAV) - org.eclipse.jgit:org.eclipse.jgit:6.10.0.202406032230-r (Git) - software.amazon.awssdk:s3:2.28.25 (AWS S3) - com.azure:azure-storage-blob:12.28.1 (Azure) - com.google.cloud:google-cloud-storage:2.44.1 (GCP) All implementations follow the existing patterns from FtpClient and StorageClient, ensuring consistency across the codebase.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the crawler framework by implementing support for six additional protocols and cloud storage services: SFTP, WebDAV, Git, AWS S3, Azure Blob Storage, and Google Cloud Storage. Each implementation follows established patterns from existing clients (FtpClient and StorageClient) and includes authentication support, large file handling, metadata extraction, and comprehensive test coverage.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| fess-crawler/pom.xml | Added dependencies for JSch (SFTP), Sardine (WebDAV), JGit, AWS SDK, Azure Storage SDK, and Google Cloud Storage SDK |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/sftp/SftpClient.java | SFTP client implementation with connection pooling and authentication support |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/sftp/SftpAuthentication.java | SFTP authentication configuration with private key support |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/sftp/SftpAuthenticationHolder.java | Manages SFTP authentication credentials with pattern matching |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/webdav/WebDavClient.java | WebDAV client implementation using Sardine library |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/webdav/WebDavAuthentication.java | WebDAV authentication configuration |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/webdav/WebDavAuthenticationHolder.java | Manages WebDAV authentication credentials |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/git/GitClient.java | Git repository client with JGit, supporting branch/path navigation |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/git/GitAuthentication.java | Git authentication configuration |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/git/GitAuthenticationHolder.java | Manages Git authentication credentials |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/aws/AwsS3Client.java | AWS S3 client using AWS SDK v2 |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/azure/AzureBlobClient.java | Azure Blob Storage client |
| fess-crawler/src/main/java/org/codelibs/fess/crawler/client/gcp/GoogleCloudStorageClient.java | Google Cloud Storage client |
| fess-crawler/src/test/java/org/codelibs/fess/crawler/client/sftp/SftpClientTest.java | Unit tests for SFTP client functionality |
| fess-crawler/src/test/java/org/codelibs/fess/crawler/client/webdav/WebDavClientTest.java | Unit tests for WebDAV client functionality |
| fess-crawler/src/test/java/org/codelibs/fess/crawler/client/git/GitClientTest.java | Unit tests for Git client functionality |
| fess-crawler/src/test/java/org/codelibs/fess/crawler/client/aws/AwsS3ClientTest.java | Unit tests for AWS S3 client functionality |
| fess-crawler/src/test/java/org/codelibs/fess/crawler/client/azure/AzureBlobClientTest.java | Unit tests for Azure Blob client functionality |
| fess-crawler/src/test/java/org/codelibs/fess/crawler/client/gcp/GoogleCloudStorageClientTest.java | Unit tests for GCP Storage client functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
366
to
374
| protected String getFileName(final String uri) { | ||
| if (uri == null) { | ||
| return ""; | ||
| } | ||
| final int index = uri.lastIndexOf('/'); | ||
| if (index >= 0 && index < uri.length() - 1) { | ||
| return uri.substring(index + 1); | ||
| } | ||
| return uri; |
There was a problem hiding this comment.
getFileName returns the entire URI when it ends with '/', but should return an empty string for consistency with the test expectations (test_getFileName line 101).
Comment on lines
472
to
486
| protected String getMimeType(final String filename) { | ||
| if (StringUtil.isBlank(filename)) { | ||
| return APPLICATION_OCTET_STREAM; | ||
| } | ||
| // Basic MIME type detection - can be enhanced | ||
| if (filename.endsWith(".txt")) { | ||
| return "text/plain"; | ||
| } else if (filename.endsWith(".java")) { | ||
| return "text/x-java"; | ||
| } else if (filename.endsWith(".xml")) { | ||
| return "application/xml"; | ||
| } else if (filename.endsWith(".json")) { | ||
| return "application/json"; | ||
| } | ||
| return APPLICATION_OCTET_STREAM; |
There was a problem hiding this comment.
The hardcoded MIME type mapping in getMimeType is duplicating functionality that exists in MimeTypeHelper. Consider using the MimeTypeHelper component instead for consistency and maintainability.
Comment on lines
+132
to
+134
| localRepoDir = File.createTempFile("git-crawler-", "-repos"); | ||
| if (!localRepoDir.delete() || !localRepoDir.mkdirs()) { | ||
| throw new CrawlerSystemException("Failed to create local repository directory: " + localRepoDir.getAbsolutePath()); |
There was a problem hiding this comment.
Creating a temp file and then deleting it to create a directory with the same name creates a race condition vulnerability. Use Files.createTempDirectory() instead or another secure method to create a temporary directory.
This commit addresses three issues identified in code review: 1. Fix unreachable statement in GitClient.java line 302 - Removed unreachable return statement after exception-throwing catch blocks - Both catch blocks throw exceptions, making the return statement unreachable 2. Fix WebDavClient.getFileName() to return empty string for trailing slash - Updated logic to check if URI ends with '/' and return empty string - Previously returned the entire URI when it ended with '/' - Matches test expectations 3. Refactor GitClient.getMimeType() to use MimeTypeHelper - Removed hardcoded MIME type mappings - Now uses MimeTypeHelper component for MIME type detection - Updated test to expect correct MIME types from Apache Tika - Changed .java file expectation from "text/x-java" to "text/x-java-source"
Changed getBytes(String) to getBytes(Charset) to avoid checked exception: - Use Constants.UTF_8_CHARSET instead of Constants.UTF_8 - Applies to private key and passphrase byte conversion - getBytes(Charset) doesn't throw UnsupportedEncodingException
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…verage
This commit introduces support for additional protocols and cloud storage services:
New Clients Implemented:
Features:
Dependencies Added:
All implementations follow the existing patterns from FtpClient and StorageClient, ensuring consistency across the codebase.