Major gem architecture rewrite for improved maintainability and performance #183
Conversation
Major refactoring: - Replaced entity/client structure with resources/clients architecture - Consolidated file list operations into main file client - Integrated chunks client into multipart uploader - Added comprehensive upload client with parallel multipart support - Improved error handling and retry logic - Enhanced backwards compatibility (v4.4.3 methods preserved) Testing & Quality: - All 306 specs passing (2 pending for large file tests) - Fixed Ruby 3.4 compatibility (added base64 dependency) - Fixed all rubocop offenses - Verified examples work with new structure API Coverage: - Full API coverage maintained from old architecture - Enhanced with new features (parallel uploads, better polling) - Cleaner separation between upload and REST APIs
WalkthroughReplaces the ApiStruct/Entity architecture with a Zeitwerk-autoloaded client/resource design: adds Uploadcare.configure/Uploadcare.configuration, Faraday-backed RestClient/UploadClient, new clients and resource wrappers (File, Group, Project, Uploader, converters, Add‑Ons, Webhook), Authenticator/ErrorHandler/ThrottleHandler, many example scripts and tests, removes legacy Param/Entity/Concern layers, and bumps version to 5.0.0. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–90 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR represents a major architectural rewrite of the Uploadcare Ruby gem (v5.0.0), transitioning from an entity/client structure to a cleaner resources/clients pattern. The changes maintain full backwards compatibility with v4.4.3 while adding significant performance improvements including parallel multipart uploads and enhanced error handling. The rewrite consolidates file operations, unifies multipart handling, and simplifies the inheritance structure. All 306 specs pass with 100% RuboCop compliance.
Key Changes
- Complete architectural refactoring from entities to resources pattern
- Addition of Ruby 3.4 compatibility with base64 dependency
- Consolidated client architecture with unified multipart handling
- Parallel multipart upload support for improved performance
Reviewed changes
Copilot reviewed 196 out of 198 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uploadcare-ruby.gemspec | Updated dependencies: removed mimemagic/parallel/retries/uploadcare-api_struct, added base64/faraday/faraday-multipart/mime-types/zeitwerk; simplified require_paths |
| spec/uploadcare/webhook_signature_verifier_spec.rb | Enhanced test coverage with additional test cases for environment variable fallback and v4.4.3 compatibility namespace |
| spec/uploadcare/version_spec.rb | New test file verifying version constant equals '5.0.0' |
| spec/uploadcare/uploader_spec.rb | New comprehensive test suite for Uploader class covering URL detection, file path handling, arrays, and error scenarios |
| spec/uploadcare/resources/*_spec.rb | New test files for all resource classes (webhook, video_converter, uploader, project, group, file, file_metadata, document_converter, batch_file_result, add_ons) |
| spec/uploadcare/clients/*_spec.rb | New test files for all client classes with comprehensive HTTP mocking and error handling |
| spec/uploadcare/configuration_spec.rb | New configuration test verifying all default values and setters |
| spec/uploadcare/concerns/throttle_handler_spec.rb | Updated module name from Concerns to ThrottleHandler |
| spec/uploadcare/cname_generator_spec.rb | Updated all references from Uploadcare.config to Uploadcare.configuration |
| spec/uploadcare/authenticator_spec.rb | New test file for authentication header generation |
| spec/integration/upload_spec.rb | New comprehensive integration test suite covering complete upload workflows |
| spec/support/* | Updated VCR configuration to use Uploadcare.configuration; removed reset_config.rb and hashie.rb |
| spec/spec_helper.rb | Removed dry/monads and api_struct dependencies; added before(:all) configuration block |
| spec/fixtures/vcr_cassettes/* | Updated VCR cassettes with new request/response formats |
| lib/uploadcare/webhook_signature_verifier.rb | New implementation with v4.4.3 compatibility namespace alias |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (41)
lib/uploadcare/resources/file_metadata.rb-20-20 (1)
20-20: Typo in TODO comments: "opeartion" → "operation".This typo appears on lines 20, 30, 39, and 47.
- # TODO - Remove uuid if the opeartion is being perfomed on same file + # TODO - Remove uuid if the operation is being performed on same filelib/uploadcare/clients/multipart_uploader_client.rb-10-12 (1)
10-12: Fix incorrect comment about chunk size.The comment states "10MB" but the constant value is 5,242,880 bytes (5MB). This documentation inconsistency could confuse future maintainers.
Apply this diff:
- # Default chunk size for multipart uploads (10MB) + # Default chunk size for multipart uploads (5MB) class MultipartUploaderClient < UploadClient CHUNK_SIZE = 5_242_880api_examples/upload_api/put_multipart_part.rb-1-7 (1)
1-7: Missingrequire 'fileutils'for cleanup.
FileUtils.rm_fis called on line 69 butfileutilsis not explicitly required. While it may be autoloaded in some Ruby environments, explicit requires are more reliable for standalone scripts.# frozen_string_literal: true require_relative '../../lib/uploadcare' +require 'fileutils' # Load environment variables from .env file env_file = File.expand_path('../../.env', __dir__)lib/uploadcare/configuration.rb-9-16 (1)
9-16: Documentation usesUploadcare.configbut API usesUploadcare.configuration.The examples reference
Uploadcare.config.public_key = ...but based on the codebase, the correct method isUploadcare.configuration. This inconsistency could confuse users.# @example Configure via environment variables # ENV['UPLOADCARE_PUBLIC_KEY'] = 'your_public_key' # ENV['UPLOADCARE_SECRET_KEY'] = 'your_secret_key' # # @example Configure in code -# Uploadcare.config.public_key = 'your_public_key' -# Uploadcare.config.secret_key = 'your_secret_key' -# Uploadcare.config.upload_timeout = 120 +# Uploadcare.configuration.public_key = 'your_public_key' +# Uploadcare.configuration.secret_key = 'your_secret_key' +# Uploadcare.configuration.upload_timeout = 120examples/large_file_upload.rb-44-97 (1)
44-97: File handle may leak on exception beforefile.close.If
multipart_uploadraises an exception, the file handle opened on line 46 will not be closed. Therescueblock at line 94 catches the error but doesn't close the file.begin upload_client = Uploadcare::UploadClient.new file = File.open(file_path, 'rb') start_time = Time.now # Upload with multipart and parallel threads result = upload_client.multipart_upload(file, store: true, threads: threads, metadata: { source: 'large_file_example', upload_method: 'multipart' }) do |progress| # ... progress callback end - file.close elapsed = Time.now - start_time # ... output results rescue StandardError => e puts puts "✗ Upload failed: #{e.message}" exit 1 +ensure + file&.close endlib/uploadcare/clients/file_client.rb-7-9 (1)
7-9: Add leading slash tolistmethod path for consistency.The
listmethod uses'files/'while all other FileClient methods use paths with leading slashes (e.g.,'/files/storage/'). Both formats work correctly with Faraday, but the inconsistency should be resolved by changing'files/'to'/files/'to match the rest of the codebase.spec/uploadcare/resources/video_converter_spec.rb-56-56 (1)
56-56: Method name inconsistency between subject definition and test usage.Line 56 defines
subjectasvideo_converter.status(token), but lines 63 and 68 callvideo_converter.fetch_status(token). This mismatch suggests the tests aren't actually exercising thesubjectdefinition on line 56.Either update line 56 to match the method being tested:
- subject { video_converter.status(token) } + subject { video_converter.fetch_status(token) }Or update the test calls to use
subject:- result = video_converter.fetch_status(token) + result = subjectAlso applies to: 62-68
spec/fixtures/vcr_cassettes/file_info_success.yml-1-62 (1)
1-62: Misleading fixture filename.The file is named
file_info_success.ymlbut records a 403 Forbidden error response with body "pub_key is required." This filename doesn't match the content, which represents an authentication failure scenario.Consider renaming to better reflect the error case, such as:
file_info_auth_failure.ymlfile_info_missing_pub_key.ymlfile_info_403_forbidden.ymlspec/uploadcare/resources/video_converter_spec.rb-74-74 (1)
74-74: Fix typo in test description."attributecorrectly" should be "attribute correctly".
- it 'assigns attributecorrectly' do + it 'assigns attribute correctly' doCommittable suggestion skipped: line range outside the PR's diff.
spec/uploadcare/resources/document_converter_spec.rb-74-74 (1)
74-74: Fix typo in test description."attributecorrectly" should be "attribute correctly" (same typo appears in video_converter_spec.rb:74).
- it 'assigns attributecorrectly' do + it 'assigns attribute correctly' doapi_examples/upload_api/uploader_demo.rb-8-11 (1)
8-11: Silent failure when API keys are missing.Using
ENV.fetchwithnildefaults prevents early detection of missing credentials.Apply this diff:
-Uploadcare.configure do |config| - config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) - config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) -end +public_key = ENV['UPLOADCARE_PUBLIC_KEY'] +secret_key = ENV['UPLOADCARE_SECRET_KEY'] + +if public_key.nil? || public_key.empty? || secret_key.nil? || secret_key.empty? + puts 'Error: UPLOADCARE_PUBLIC_KEY and UPLOADCARE_SECRET_KEY must be set' + exit 1 +end + +Uploadcare.configure do |config| + config.public_key = public_key + config.secret_key = secret_key +endapi_examples/upload_api/post_multipart_start.rb-17-19 (1)
17-19: Silent failure when UPLOADCARE_PUBLIC_KEY is missing.Using
ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil)will returnnilif the key is not set, causing the script to continue and fail later with a confusing error during the API call.Consider one of these approaches:
Option 1: Fail fast with descriptive error
-Uploadcare.configure do |config| - config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) -end +public_key = ENV['UPLOADCARE_PUBLIC_KEY'] +if public_key.nil? || public_key.empty? + puts 'Error: UPLOADCARE_PUBLIC_KEY environment variable is not set' + puts 'Please set it in your .env file' + exit 1 +end + +Uploadcare.configure do |config| + config.public_key = public_key +endOption 2: Let ENV.fetch raise KeyError
Uploadcare.configure do |config| - config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) + config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY') endapi_examples/upload_api/get_group_info_example.rb-8-11 (1)
8-11: Missing validation for required API credentials.The
ENV.fetchcalls withnildefaults will not alert users when credentials are missing, causing API authentication failures with unclear error messages.Apply this diff:
-Uploadcare.configure do |config| - config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) - config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) -end +public_key = ENV['UPLOADCARE_PUBLIC_KEY'] +secret_key = ENV['UPLOADCARE_SECRET_KEY'] + +if public_key.nil? || public_key.empty? || secret_key.nil? || secret_key.empty? + puts 'Error: UPLOADCARE_PUBLIC_KEY and UPLOADCARE_SECRET_KEY must be set' + puts 'Please configure them in your .env file or as environment variables' + exit 1 +end + +Uploadcare.configure do |config| + config.public_key = public_key + config.secret_key = secret_key +endapi_examples/upload_api/uploader_demo.rb-79-81 (1)
79-81: Potential file handle leak.If
Uploadcare::Uploader.uploadraises an exception on line 80, the file opened on line 79 will not be closed, potentially leaking the file handle.Apply this diff to ensure the file is always closed:
- file = File.open('spec/fixtures/kitten.jpeg', 'rb') - response = Uploadcare::Uploader.upload(file, store: true, metadata: { source: 'demo_script' }) - file.close + file = File.open('spec/fixtures/kitten.jpeg', 'rb') + begin + response = Uploadcare::Uploader.upload(file, store: true, metadata: { source: 'demo_script' }) + ensure + file.close + endexamples/url_upload.rb-11-14 (1)
11-14: Silent failure when API keys are missing.Using
ENV.fetchwithnilas the default will allow the script to continue even when required credentials are not set, leading to confusing authentication errors during API calls.Apply this diff to fail early with a clear error message:
-Uploadcare.configure do |config| - config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) - config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) -end +public_key = ENV['UPLOADCARE_PUBLIC_KEY'] +secret_key = ENV['UPLOADCARE_SECRET_KEY'] + +if public_key.nil? || public_key.empty? || secret_key.nil? || secret_key.empty? + puts 'Error: UPLOADCARE_PUBLIC_KEY and UPLOADCARE_SECRET_KEY must be set' + puts 'Please configure them in your .env file' + exit 1 +end + +Uploadcare.configure do |config| + config.public_key = public_key + config.secret_key = secret_key +endexamples/group_creation.rb-43-51 (1)
43-51: Validate upload response structure.Line 48 uses
response.values.firstassuming the response is a hash with at least one value. If the API response structure changes or an error occurs, this will raise a crypticNoMethodErroror returnnil, causing issues downstream.Apply this diff to add validation:
file_paths.each_with_index do |path, index| file = File.open(path, 'rb') response = upload_client.upload_file(file, store: true) file.close - uuid = response.values.first + uuid = response.values.first if response.is_a?(Hash) && !response.empty? + unless uuid + puts "Error: Invalid upload response for #{File.basename(path)}" + exit 1 + end uuids << uuid puts " #{index + 1}. #{File.basename(path)} → #{uuid}" endapi_examples/upload_api/uploader_real_test.rb-20-32 (1)
20-32: Resource leak: File not closed on exception.Same issue as in comprehensive_demo.rb. If an exception occurs between line 21 (file open) and line 23 (file close), the file handle leaks.
Use
File.openwith a block to ensure automatic cleanup.api_examples/upload_api/uploader_real_test.rb-52-82 (1)
52-82: Resource leak in large file test.File opened at line 56 won't be closed if an exception occurs before line 68.
Use block form for automatic resource cleanup.
api_examples/upload_api/comprehensive_demo.rb-149-168 (1)
149-168: Resource leak in metadata test (same issue).File handle opened at line 150 won't be closed on exception before line 159.
Use block form as suggested in earlier comments.
api_examples/upload_api/uploader_real_test.rb-87-105 (1)
87-105: Resource leak in batch upload test.Files opened in array (lines 88-91) won't be closed if an exception occurs before line 95.
Wrap file operations in a
begin/ensureblock as suggested in comprehensive_demo.rb review.api_examples/upload_api/comprehensive_demo.rb-79-117 (1)
79-117: Same resource leak issue in large file test.The file handle opened at line 83 won't be closed if an exception occurs before line 100.
Use a block form as suggested in the previous comment.
api_examples/upload_api/comprehensive_demo.rb-37-52 (1)
37-52: Resource leak: File not closed on exception.If an exception occurs between opening the file (line 38) and closing it (line 42), the file handle will leak. This pattern is repeated in Tests 3 and 5 as well.
Apply this pattern consistently across all file operations:
- file = File.open('spec/fixtures/kitten.jpeg', 'rb') - file_size = (file.size / 1024.0).round(2) - - result = Uploadcare::Uploader.upload(file, store: true) - file.close + File.open('spec/fixtures/kitten.jpeg', 'rb') do |file| + file_size = (file.size / 1024.0).round(2) + result = Uploadcare::Uploader.upload(file, store: true) - - print_success('Upload successful', { - 'UUID' => result.uuid, - 'Filename' => result.original_filename, - 'Size' => "#{file_size} KB", - 'Method' => 'Base upload (auto-detected)' - }) + print_success('Upload successful', { + 'UUID' => result.uuid, + 'Filename' => result.original_filename, + 'Size' => "#{file_size} KB", + 'Method' => 'Base upload (auto-detected)' + }) + endapi_examples/upload_api/comprehensive_demo.rb-124-142 (1)
124-142: Potential resource leak in batch upload.If an exception occurs between opening the files (lines 125-128) and closing them (line 134), file handles will leak.
Consider this safer pattern:
- files = [ - File.open('spec/fixtures/kitten.jpeg', 'rb'), - File.open('spec/fixtures/another_kitten.jpeg', 'rb') - ] - - puts " Uploading #{files.length} files..." - - results = Uploadcare::Uploader.upload(files, store: true) - - files.each(&:close) + file_paths = [ + 'spec/fixtures/kitten.jpeg', + 'spec/fixtures/another_kitten.jpeg' + ] + + files = file_paths.map { |path| File.open(path, 'rb') } + begin + puts " Uploading #{files.length} files..." + results = Uploadcare::Uploader.upload(files, store: true) + ensure + files.each(&:close) + endlib/uploadcare/error_handler.rb-20-21 (1)
20-21: Typo in comment."apropriate" should be "appropriate".
# Upload API returns its errors with code 200, and stores its actual code and details within response message - # This methods detects that and raises apropriate error + # This method detects that and raises appropriate errorlib/uploadcare/error_handler.rb-22-28 (1)
22-28: JSON::ParserError incatch_upload_errorsis not caught.If
response[:body]contains invalid JSON in a 200 response,JSON.parseat line 25 will raiseJSON::ParserError, which bypasses the rescue clause at line 14 (since the exception occurs beforehandle_errorreaches its ownJSON.parse). Consider adding defensive handling here.def catch_upload_errors(response) return unless response[:status] == 200 - parsed_response = JSON.parse(response[:body].to_s) - error = parsed_response['error'] if parsed_response.is_a?(Hash) - raise RequestError, error if error + begin + parsed_response = JSON.parse(response[:body].to_s) + error = parsed_response['error'] if parsed_response.is_a?(Hash) + raise RequestError, error if error + rescue JSON::ParserError + # Not JSON or invalid JSON in 200 response - let handle_error continue + nil + end endspec/integration/upload_spec.rb-193-205 (1)
193-205: Test description doesn't match implementation.The test is titled "handles 1-byte files" but uses the standard
kitten.jpegfixture which is not a 1-byte file. Either update the test description or create/use an actual 1-byte fixture.context 'Very small files' do - it 'handles 1-byte files', :vcr do - # Use existing fixture file instead of creating invalid file + it 'handles small image files', :vcr do file = File.open(file_path, 'rb')lib/uploadcare/clients/webhook_client.rb-20-32 (1)
20-32: Missingversionparameter in webhook creation payload.The YARD docs mention a
versionparameter (line 17), but it's not included in the payload. According to the Uploadcare API,versioncontrols the webhook payload format.payload = { target_url: options[:target_url], event: options[:event] || 'file.uploaded', is_active: options[:is_active].nil? || options[:is_active] } # Add signing_secret if provided (compact removes nil values like v4.4.3) - payload.merge!({ signing_secret: options[:signing_secret] }.compact) + payload.merge!({ signing_secret: options[:signing_secret], version: options[:version] }.compact) post('/webhooks/', payload)lib/uploadcare/resources/webhook.rb-45-50 (1)
45-50: Inconsistent config handling inupdate.Same issue as
create— theupdatemethod doesn't accept or pass a custom configuration.- def self.update(id, options = {}) + def self.update(id, options = {}, config = Uploadcare.configuration) # v4.4.3 compatible: id is positional, rest are in options hash - client = Uploadcare::WebhookClient.new + client = Uploadcare::WebhookClient.new(config) response = client.update_webhook(id, options) - new(response) + new(response, config) endlib/uploadcare/resources/webhook.rb-57-60 (1)
57-60: Inconsistent config handling indelete.Same issue — the
deletemethod should also accept an optional config parameter for consistency.- def self.delete(target_url) - client = Uploadcare::WebhookClient.new + def self.delete(target_url, config = Uploadcare.configuration) + client = Uploadcare::WebhookClient.new(config) client.delete_webhook(target_url) endspec/integration/upload_spec.rb-207-216 (1)
207-216: Test description doesn't match implementation.Similar issue — test claims to handle "filenames with special characters" but uses a standard fixture without special characters. The test doesn't actually validate special character handling.
context 'Files with special characters' do - it 'handles filenames with special characters', :vcr do - # Use existing fixture file instead of creating invalid file + it 'uploads standard fixture file', :vcr do file = File.open(file_path, 'rb')Alternatively, create a fixture with special characters (e.g.,
test file (1).jpeg) to properly test this edge case.lib/uploadcare/resources/webhook.rb-30-35 (1)
30-35: Inconsistent config handling increatecompared tolist.The
listmethod accepts aconfigparameter and passes it through, butcreateuses the default configuration only. This prevents users from using a custom configuration when creating webhooks.- def self.create(options = {}) + def self.create(options = {}, config = Uploadcare.configuration) # v4.4.3 compatible: expects hash with target_url, event, etc. - client = Uploadcare::WebhookClient.new + client = Uploadcare::WebhookClient.new(config) response = client.create_webhook(options) - new(response) + new(response, config) endCommittable suggestion skipped: line range outside the PR's diff.
lib/uploadcare/resources/video_converter.rb-28-32 (1)
28-32: Documentation: token parameter type should beString, notInteger.The
@param tokendocumentation states[Integer], but API tokens are typically strings. This could mislead callers.# Fetches the status of a video conversion job by token - # @param token [Integer] The job token + # @param token [String] The job token # @return [Hash] The response containing the job status # @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/Conversion/operation/videoConvertStatusapi_examples/upload_api/get_from_url_status.rb-27-32 (1)
27-32: Missing error handling for async upload result.If
upload_from_urlfails or returns an unexpected response, accessingresult['token']on line 29 could raise an error or returnnil, causing the subsequentupload_from_url_statuscall to fail with a confusing message.Consider adding a guard:
client = Uploadcare::UploadClient.new result = client.upload_from_url(source_url, async: true) token = result['token'] +unless token + puts "Error: Failed to get upload token" + puts "Response: #{result.inspect}" + exit 1 +end + puts "Upload token: #{token}"lib/uploadcare/resources/video_converter.rb-30-36 (1)
30-36: Documentation:@returntype is incorrect.The method returns
self(aVideoConverterinstance), not aHash.# Fetches the status of a video conversion job by token # @param token [String] The job token - # @return [Hash] The response containing the job status + # @return [Uploadcare::VideoConverter] self with updated status attributes # @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/Conversion/operation/videoConvertStatusCommittable suggestion skipped: line range outside the PR's diff.
spec/uploadcare/resources/uploader_spec.rb-11-17 (1)
11-17: Test expectations don't match implementation error messages.The tests expect
ArgumentErrorwith message matchingExpected input to be a file/Array/URL, but the implementation inlib/uploadcare/uploader.rbraises:
'source cannot be nil'for nil input (line 50)"Unsupported source type: #{source.class}"for unsupported types (line 62)These tests will fail. Either update the tests to match the implementation or vice versa.
it 'raises ArgumentError for unsupported object types' do - expect { described_class.upload(123) }.to raise_error(ArgumentError, %r{Expected input to be a file/Array/URL}) - expect { described_class.upload({ invalid: 'object' }) }.to raise_error(ArgumentError, %r{Expected input to be a file/Array/URL}) - expect { described_class.upload(nil) }.to raise_error(ArgumentError, %r{Expected input to be a file/Array/URL}) + expect { described_class.upload(123) }.to raise_error(ArgumentError, /Unsupported source type/) + expect { described_class.upload({ invalid: 'object' }) }.to raise_error(ArgumentError, /Unsupported source type/) + expect { described_class.upload(nil) }.to raise_error(ArgumentError, /source cannot be nil/) endspec/uploadcare/resources/uploader_spec.rb-261-270 (1)
261-270: Cannot instantiate a module - test will fail.
Uploadcare::Uploaderis defined as a module (inlib/uploadcare/uploader.rb), but this test attempts to instantiate it withdescribed_class.new. This will raise aNoMethodError.Either update the implementation to be a class, or revise this test to match the module-based design:
describe 'configuration and initialization' do - it 'initializes with default configuration' do - uploader = described_class.new - expect(uploader.instance_variable_get(:@uploader_client)).to be_kind_of(Uploadcare::UploaderClient) - end - it 'uses class-level uploader_client when not instantiated' do - expect(described_class.send(:uploader_client)).to be_kind_of(Uploadcare::UploaderClient) + # Test the module's class method approach + client = Uploadcare::UploadClient.new + expect(client).to be_kind_of(Uploadcare::UploadClient) end endCommittable suggestion skipped: line range outside the PR's diff.
lib/uploadcare/resources/document_converter.rb-13-21 (1)
13-21: Incorrect return type in documentation.The
@returnannotation saysUploadcare::Documentbut the method returnsself, which is an instance ofUploadcare::DocumentConverter.# Fetches information about a document's format and possible conversion formats # @param uuid [String] The UUID of the document - # @return [Uploadcare::Document] An instance of Document with API response data + # @return [Uploadcare::DocumentConverter] An instance of DocumentConverter with API response data # @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/Conversion/operation/documentConvertInfolib/uploadcare/resources/document_converter.rb-43-52 (1)
43-52: Incorrect type annotation fortokenparameter.The
@param token [Integer]annotation appears incorrect. The token is likely a String (UUID/token identifier) returned from the conversion API, not an Integer.# Fetches document conversion job status by its token - # @param token [Integer] The job token + # @param token [String] The job token # @return [Uploadcare::DocumentConverter] An instance of DocumentConverter with status data # @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/Conversion/operation/documentConvertStatuslib/uploadcare/resources/paginated_collection.rb-25-37 (1)
25-37: YARD return types are too specific.The
@return [Uploadcare::FileList]annotations fornext_pageandprevious_pageare incorrect. SincePaginatedCollectionis generic and can hold any resource type, the return type should bePaginatedCollectionornil.# Fetches the next page of resources # Returns [nil] if next_page_url is nil - # @return [Uploadcare::FileList] + # @return [Uploadcare::PaginatedCollection, nil] def next_page fetch_page(@next_page_url) end # Fetches the previous page of resources # Returns [nil] if previous_page_url is nil - # @return [Uploadcare::FileList] + # @return [Uploadcare::PaginatedCollection, nil] def previous_pageREADME.md-686-760 (1)
686-760: Fix markdown heading structure.The section from "Storing Files" onwards has inconsistent heading levels and code blocks mixed with non-code-block text. Lines 688-722 have Ruby code and comments outside of code blocks, and headings jump levels (h4 after content that's not under h3).
The structure should be:
-``` - -#### Storing Files - -# Store a single file -``` ruby +#### Storing Files + +Store a single file: + +```ruby file = Uploadcare::File.new(uuid: "FILE_UUID") stored_file = file.store +puts stored_file.datetime_stored +# => "2024-11-05T09:13:40.543471Z" +``` -puts stored_file.datetime_stored -# => "2024-11-05T09:13:40.543471Z" -``` - -# Batch store files using their UUIDs -``` ruby +Batch store files using their UUIDs: + +```ruby uuids = ['uuid1', 'uuid2', 'uuid3'] batch_result = Uploadcare::File.batch_store(uuids) -``` - -# Check the status of the operation -``` ruby puts batch_result.status # => "success"Apply similar fixes to lines 724-760 for consistent markdown formatting. </blockquote></details> <details> <summary>lib/uploadcare/clients/uploader_client.rb-39-48 (1)</summary><blockquote> `39-48`: **Remove dead code and address missing options handling.** 1. The commented-out signature logic (lines 46-47) should be removed or replaced with a TODO if it's planned work. 2. The method doesn't handle documented options like `check_URL_duplicates`, `filename`, `save_URL_duplicates`, or `metadata` mentioned in the `upload_from_url` docstring (lines 23-27). ```diff def upload_from_url_body(url, options = {}) - { + body = { 'pub_key' => Uploadcare.configuration.public_key, 'source_url' => url, 'store' => store_value(options[:store]) } - # opts.merge!(Param::Upload::SignatureGenerator.call) if Uploadcare.config.sign_uploads - # options.merge(opts) + body['check_URL_duplicates'] = options[:check_URL_duplicates] if options.key?(:check_URL_duplicates) + body['save_URL_duplicates'] = options[:save_URL_duplicates] if options.key?(:save_URL_duplicates) + body['filename'] = options[:filename] if options[:filename] + body.merge!(generate_metadata_params(options[:metadata])) + body endCommittable suggestion skipped: line range outside the PR's diff.
| # Configure Uploadcare | ||
| Uploadcare.configure do |config| | ||
| config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) | ||
| config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) | ||
| end |
There was a problem hiding this comment.
Validate required configuration values.
Lines 12-13 use ENV.fetch with nil as the default. If either key is missing, authentication will fail with cryptic errors. For an example script, it's better to fail fast with a clear message.
Apply this diff to add validation:
# Configure Uploadcare
Uploadcare.configure do |config|
config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil)
config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil)
end
+
+# Validate configuration
+if Uploadcare.configuration.public_key.nil? || Uploadcare.configuration.public_key.empty?
+ puts 'Error: UPLOADCARE_PUBLIC_KEY environment variable is required'
+ exit 1
+end
+
+if Uploadcare.configuration.secret_key.nil? || Uploadcare.configuration.secret_key.empty?
+ puts 'Error: UPLOADCARE_SECRET_KEY environment variable is required'
+ exit 1
+end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Configure Uploadcare | |
| Uploadcare.configure do |config| | |
| config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) | |
| config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) | |
| end | |
| # Configure Uploadcare | |
| Uploadcare.configure do |config| | |
| config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) | |
| config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) | |
| end | |
| # Validate configuration | |
| if Uploadcare.configuration.public_key.nil? || Uploadcare.configuration.public_key.empty? | |
| puts 'Error: UPLOADCARE_PUBLIC_KEY environment variable is required' | |
| exit 1 | |
| end | |
| if Uploadcare.configuration.secret_key.nil? || Uploadcare.configuration.secret_key.empty? | |
| puts 'Error: UPLOADCARE_SECRET_KEY environment variable is required' | |
| exit 1 | |
| end |
🤖 Prompt for AI Agents
In examples/group_creation.rb around lines 10 to 14, the code uses
ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) and ENV.fetch('UPLOADCARE_SECRET_KEY',
nil) which allows missing keys to slip through and cause cryptic auth errors;
change to require the env vars and fail fast by either using
ENV.fetch('UPLOADCARE_PUBLIC_KEY') and ENV.fetch('UPLOADCARE_SECRET_KEY') (no
default) or explicitly validate after reading and raise/abort with a clear
message like "Missing UPLOADCARE_PUBLIC_KEY" / "Missing UPLOADCARE_SECRET_KEY"
so the script exits with a user-friendly error when keys are not set.
| def id | ||
| return @id if @id | ||
|
|
||
| # If initialized from URL, extract ID | ||
| if @cdn_url | ||
| extracted_id = @cdn_url.gsub('https://ucarecdn.com/', '') | ||
| extracted_id.gsub(%r{/.*}, '') | ||
| else | ||
| @id | ||
| end | ||
| end |
There was a problem hiding this comment.
Bug in id method: missing return statement.
The id method extracts an ID from @cdn_url but doesn't return the result. Lines 106-107 compute extracted_id but the method falls through to line 109 which returns @id (likely nil in this branch).
def id
return @id if @id
# If initialized from URL, extract ID
if @cdn_url
extracted_id = @cdn_url.gsub('https://ucarecdn.com/', '')
- extracted_id.gsub(%r{/.*}, '')
+ extracted_id.gsub(%r{/.*}, '')
else
- @id
+ nil
end
endWait, I see the issue more clearly - the method should return the extracted_id:
def id
return @id if @id
# If initialized from URL, extract ID
if @cdn_url
extracted_id = @cdn_url.gsub('https://ucarecdn.com/', '')
- extracted_id.gsub(%r{/.*}, '')
- else
- @id
+ extracted_id.gsub(%r{/.*}, '')
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def id | |
| return @id if @id | |
| # If initialized from URL, extract ID | |
| if @cdn_url | |
| extracted_id = @cdn_url.gsub('https://ucarecdn.com/', '') | |
| extracted_id.gsub(%r{/.*}, '') | |
| else | |
| @id | |
| end | |
| end | |
| def id | |
| return @id if @id | |
| # If initialized from URL, extract ID | |
| if @cdn_url | |
| extracted_id = @cdn_url.gsub('https://ucarecdn.com/', '') | |
| extracted_id.gsub(%r{/.*}, '') | |
| end | |
| end |
🤖 Prompt for AI Agents
In lib/uploadcare/resources/group.rb around lines 101 to 111, the id method
computes extracted_id from @cdn_url but never returns or sets it, so the method
falls back to returning @id (nil); change the method to return the extracted_id
(and preferably memoize it by setting @id = extracted_id) when @cdn_url is
present so the extracted value is actually returned on subsequent calls.
| --- | ||
| http_interactions: | ||
| - request: | ||
| method: post | ||
| uri: https://upload.uploadcare.com/multipart/start/ | ||
| body: | ||
| encoding: UTF-8 | ||
| string: UPLOADCARE_PUB_KEY=<uploadcare_public_key>&UPLOADCARE_STORE=1&content_type=image%2Fjpeg&filename=big.jpeg&part_size=5242880&size=10487050 | ||
| headers: | ||
| User-Agent: | ||
| - Faraday v2.14.0 | ||
| Content-Type: | ||
| - application/x-www-form-urlencoded | ||
| Accept-Encoding: | ||
| - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 | ||
| Accept: | ||
| - "*/*" | ||
| response: | ||
| status: | ||
| code: 400 | ||
| message: Bad Request | ||
| headers: | ||
| Date: | ||
| - Tue, 25 Nov 2025 08:44:50 GMT | ||
| Content-Type: | ||
| - text/plain; charset=utf-8 | ||
| Content-Length: | ||
| - '32' | ||
| Connection: | ||
| - keep-alive | ||
| Server: | ||
| - nginx | ||
| Vary: | ||
| - Origin | ||
| Access-Control-Allow-Origin: | ||
| - "*" | ||
| Access-Control-Allow-Methods: | ||
| - POST, OPTIONS | ||
| Access-Control-Allow-Headers: | ||
| - DNT, X-UC-User-Agent, X-PINGOTHER | ||
| Access-Control-Max-Age: | ||
| - '1' | ||
| Access-Control-Allow-Credentials: | ||
| - 'true' | ||
| Access-Control-Expose-Headers: | ||
| - Warning, Retry-After | ||
| X-Xss-Protection: | ||
| - 1; mode=block | ||
| X-Content-Type-Options: | ||
| - nosniff | ||
| Warning: | ||
| - '199 Miscellaneous warning: You should use secure requests to Uploadcare' | ||
| X-Uploadcare-Request-Id: | ||
| - 043a003e-f0c5-46fd-9d83-42bc0ebed71a | ||
| body: | ||
| encoding: UTF-8 | ||
| string: File size exceeds project limit. | ||
| recorded_at: Tue, 25 Nov 2025 08:44:50 GMT | ||
| recorded_with: VCR 6.3.1 |
There was a problem hiding this comment.
Performance test fixture only records a failed request.
This VCR cassette records a 400 "File size exceeds project limit" error, which cannot test actual upload performance or compare parallel vs. sequential upload speeds. A performance comparison test requires successful upload operations with timing data, not a single failed initialization request.
Consider:
- Using a smaller test file that stays within project limits
- Removing this cassette entirely if the test is meant to remain pending
- Adding a comment explaining why this fixture exists if it's intentionally recording the limitation
Based on PR objectives noting "2 pending (large multipart tests requiring >10MB files)", this test appears to be one of those pending cases. However, committing a VCR cassette that only captures a quota error is misleading for test infrastructure and doesn't validate the performance improvements claimed in the PR.
| let(:full_url) { "#{rest_api_root}#{path}" } | ||
|
|
||
| subject { client.list(params) } | ||
|
|
||
| context 'when the request is successful' do | ||
| let(:response_body) do | ||
| { | ||
| 'next' => nil, | ||
| 'previous' => nil, | ||
| 'per_page' => 10, | ||
| 'results' => [ | ||
| { | ||
| 'id' => 'group_uuid_1~2', | ||
| 'datetime_created' => '2023-11-01T12:49:10.477888Z', | ||
| 'files_count' => 2, | ||
| 'cdn_url' => 'https://ucarecdn.com/group_uuid_1~2/', | ||
| 'url' => "#{rest_api_root}groups/group_uuid_1~2/" | ||
| }, | ||
| { | ||
| 'id' => 'group_uuid_2~3', | ||
| 'datetime_created' => '2023-11-02T12:49:10.477888Z', | ||
| 'files_count' => 3, | ||
| 'cdn_url' => 'https://ucarecdn.com/group_uuid_2~3/', | ||
| 'url' => "#{rest_api_root}groups/group_uuid_2~3/" | ||
| } | ||
| ], | ||
| 'total' => 2 | ||
| } | ||
| end | ||
|
|
||
| before do | ||
| stub_request(:get, full_url) | ||
| .with(query: params) | ||
| .to_return(status: 200, body: response_body.to_json, headers: { 'Content-Type' => 'application/json' }) | ||
| end | ||
|
|
||
| it { is_expected.to eq(response_body) } | ||
| end | ||
|
|
||
| context 'when the request returns an error' do | ||
| before do | ||
| stub_request(:get, full_url) | ||
| .with(query: params) | ||
| .to_return(status: 400, body: { 'detail' => 'Bad Request' }.to_json, headers: { 'Content-Type' => 'application/json' }) | ||
| end | ||
|
|
||
| it 'raises an InvalidRequestError' do | ||
| expect { client.list(params) }.to raise_error(Uploadcare::InvalidRequestError, 'Bad Request') | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe '#info' do | ||
| let(:uuid) { 'group_uuid_1~2' } | ||
| let(:path) { "/groups/#{uuid}/" } | ||
| let(:full_url) { "#{rest_api_root}#{path}" } | ||
|
|
||
| subject { client.info(uuid) } | ||
|
|
||
| context 'when the request is successful' do | ||
| let(:response_body) do | ||
| { | ||
| 'id' => uuid, | ||
| 'datetime_created' => '2023-11-01T12:49:10.477888Z', | ||
| 'files_count' => 2, | ||
| 'cdn_url' => "https://ucarecdn.com/#{uuid}/", | ||
| 'url' => "#{rest_api_root}groups/#{uuid}/", | ||
| 'files' => [ | ||
| { | ||
| 'uuid' => 'file_uuid_1', | ||
| 'datetime_uploaded' => '2023-11-01T12:49:09.945335Z', | ||
| 'is_image' => true, | ||
| 'mime_type' => 'image/jpeg', | ||
| 'original_filename' => 'file1.jpg', | ||
| 'size' => 12_345 | ||
| } | ||
| ] | ||
| } | ||
| end | ||
|
|
||
| before do | ||
| stub_request(:get, full_url) | ||
| .to_return(status: 200, body: response_body.to_json, headers: { 'Content-Type' => 'application/json' }) | ||
| end | ||
|
|
||
| it { is_expected.to eq(response_body) } | ||
| end | ||
|
|
||
| context 'when the request returns an error' do | ||
| before do | ||
| stub_request(:get, full_url) | ||
| .to_return(status: 404, body: { 'detail' => 'Not Found' }.to_json, headers: { 'Content-Type' => 'application/json' }) | ||
| end | ||
|
|
||
| it 'raises a NotFoundError' do | ||
| expect { client.info(uuid) }.to raise_error(Uploadcare::NotFoundError, 'Not Found') | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Critical: Filename typo prevents test discovery.
The filename group_client_sepc.rb contains a typo—the letters 's' and 'p' are swapped. RSpec searches for files matching the pattern *_spec.rb, so this file will not be discovered or executed, resulting in silently skipped tests and reduced test coverage.
Rename the file to correct the typo:
# Rename the file to fix the typo
mv spec/uploadcare/clients/group_client_sepc.rb spec/uploadcare/clients/group_client_spec.rbAfter renaming, verify the tests are discovered:
# Search for the test file to confirm it exists with the correct name
fd -e rb "group_client_spec" spec/🤖 Prompt for AI Agents
In spec/uploadcare/clients/group_client_sepc.rb lines 1-111: the filename has a
typo ("sepc" instead of "spec") so RSpec won't discover the tests; rename the
file to spec/uploadcare/clients/group_client_spec.rb (or update the test
filename pattern) and commit the change, then run your test discovery (rspec or
fd) to confirm the spec is found.
- Add missing require 'thread' for multipart uploader - Fix spec filename typo: group_client_sepc.rb -> group_client_spec.rb - Use dotenv gem instead of manual .env parsing - Fix divide by zero errors in progress tracking - Fix comment discrepancy (10MB -> 5MB) - Implement proper mutex synchronization for thread safety - Fix retry loop condition (< instead of <=) - Fix typos in documentation comments - Update error class references in specs to use Uploadcare::Exception::RequestError
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
api_examples/upload_api/post_from_url.rb (1)
6-9: Add validation for required configuration.Using
ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil)on line 8 allows anilvalue, which will cause cryptic authentication errors later in the upload flow. The configuration should validate that the public key is present and non-empty.Apply this diff to validate the configuration:
# Configure Uploadcare Uploadcare.configure do |config| - config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) + config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY') end + +# Validate configuration +if Uploadcare.configuration.public_key.nil? || Uploadcare.configuration.public_key.empty? + puts 'Error: UPLOADCARE_PUBLIC_KEY environment variable is required' + exit 1 +endlib/uploadcare/resources/file_metadata.rb (1)
5-8: ATTRIBUTES defines file properties instead of arbitrary metadata keys.This issue was flagged in previous review: The ATTRIBUTES constant lists file properties (
datetime_removed,is_image,mime_type, etc.), but the file metadata API returns arbitrary user-defined key-value pairs. Whenindex(uuid)on line 23 callsassign_attributes(response), BaseResource only creates setters for attributes in ATTRIBUTES—custom metadata keys like{"author": "John", "version": "1.0"}are silently dropped because they lack corresponding setters.FileMetadata should store arbitrary key-value pairs via a dedicated hash attribute (e.g.,
@metadata = {}) or implement dynamic accessor methods usingmethod_missingto preserve all user-defined metadata.lib/uploadcare/clients/upload_client.rb (1)
690-708: Critical bug: Unused expression and incorrect FilePart parameter.Line 695 computes a
content_typevia a ternary expression but discards the result. Line 703 then passes the rawmime_typeobject (which could be nil) toFaraday::Multipart::FilePart.new, which expects a string content type as the second parameter.Apply this diff to fix:
def form_data_for(file, params) file_path = file.path filename = file.original_filename if file.respond_to?(:original_filename) filename ||= ::File.basename(file_path) mime_type = MIME::Types.type_for(file.path).first - mime_type ? mime_type.content_type : 'application/octet-stream' + content_type = mime_type ? mime_type.content_type : 'application/octet-stream' # if filename already exists, add a random number to the filename # to avoid overwriting the file filename = "#{SecureRandom.random_number(100)}#{filename}" if params.key?(filename) params[filename] = Faraday::Multipart::FilePart.new( file_path, - mime_type, + content_type, filename ) params endlib/uploadcare/clients/multipart_uploader_client.rb (2)
3-6: Critical: Remove debug require and clean up commented code.Line 4 contains
require 'byebug'which should never be committed to production code and will cause runtime errors in environments without byebug installed. Line 3'srequire 'thread'is redundant in modern Ruby. Lines 5-6 contain commented-out requires that should be removed.Apply this diff:
-require 'thread' -require 'byebug' -# require 'client/multipart_upload/chunks_client' -# require_relative 'upload_client'
78-97: Missing retry logic for chunk uploads.The method calls
put()directly without retry logic. The parentUploadClientprovidesmultipart_upload_part(with exponential backoff and up to 3 retries), but this implementation bypasses it. Given the PR objectives mention "Enhanced retry logic and error reporting," this gap should be addressed.Consider using the parent's retry-enabled method:
def process_chunk(object, links, link_index) offset = link_index * CHUNK_SIZE chunk = ::File.read(object, CHUNK_SIZE, offset) - put(links[link_index], chunk) + multipart_upload_part(links[link_index], chunk) return unless block_given? yield( chunk_size: CHUNK_SIZE, object: object, offset: offset, link_index: link_index, links: links, links_count: links.count ) - rescue StandardError => e - # Log error and re-raise for now - could implement retry logic here - Uploadcare.configuration.logger&.error("Chunk upload failed for link_id #{link_index}: #{e.message}") - raise end
🧹 Nitpick comments (6)
spec/uploadcare/clients/group_client_spec.rb (1)
1-111: Add test coverage for thedeletemethod.The spec file tests
listandinfomethods comprehensively, but according to the AI summary,GroupClientalso includes adelete(uuid)method that lacks test coverage here. To ensure complete test coverage for this new client, consider adding specs for the delete functionality covering both success and error scenarios.Example test structure to add:
describe '#delete' do let(:uuid) { 'group_uuid_1~2' } let(:path) { "/groups/#{uuid}/" } let(:full_url) { "#{rest_api_root}#{path}" } subject { client.delete(uuid) } context 'when the request is successful' do before do stub_request(:delete, full_url) .to_return(status: 204, body: '', headers: {}) end it 'returns successfully' do expect { subject }.not_to raise_error end end context 'when the request returns an error' do before do stub_request(:delete, full_url) .to_return(status: 404, body: { 'detail' => 'Not Found' }.to_json, headers: { 'Content-Type' => 'application/json' }) end it 'raises a RequestError' do expect { subject }.to raise_error(Uploadcare::Exception::RequestError, 'Not Found') end end endapi_examples/upload_api/post_from_url.rb (2)
11-24: Consider adding error handling for better example quality.While the sync upload logic is correct, adding basic error handling would make this a more robust example and help users understand potential failure modes (network errors, API errors, invalid URLs, etc.).
Consider wrapping the upload call in a begin/rescue block:
begin result = client.upload_from_url(source_url, store: true) puts 'Upload complete!' puts "File UUID: #{result['uuid']}" puts "Original filename: #{result['original_filename']}" puts "File size: #{result['size']} bytes" rescue StandardError => e puts "Upload failed: #{e.message}" exit 1 end
37-53: Consider demonstrating a polling loop for realistic async status checking.The status check example is correct but checks immediately after starting the async upload. In real-world usage, you'd typically poll with a delay until the upload completes.
Consider adding a polling loop to demonstrate realistic async handling:
# Example 3: Poll upload status until completion puts 'Example 3: Poll upload status until completion' puts '=' * 50 max_attempts = 30 attempt = 0 loop do status = client.upload_from_url_status(token) case status['status'] when 'success' puts 'Upload complete!' puts "File UUID: #{status['uuid']}" break when 'progress' puts 'Upload in progress...' when 'waiting' puts 'Upload waiting to start...' when 'error' puts "Upload failed: #{status['error']}" break end attempt += 1 break if attempt >= max_attempts sleep 2 endlib/uploadcare/resources/file_metadata.rb (1)
59-96: Class methods create new FileMetadataClient instances on every call.Each class method (
self.index,self.show,self.update,self.delete) instantiates a newFileMetadataClienton lines 60, 71, 83, and 94. If these methods are called frequently, repeated instantiation could cause overhead and prevent connection reuse, especially if the client maintains HTTP connections via Faraday connection pools.Consider caching client instances per configuration or using a shared client:
def self.client(config) @clients ||= {} @clients[config] ||= Uploadcare::FileMetadataClient.new(config) end def self.index(uuid, config = Uploadcare.configuration) client(config).index(uuid) endNote: Thread-safety should be considered if caching is implemented in a multi-threaded environment.
lib/uploadcare/clients/upload_client.rb (2)
208-228: Clarify retry semantics to avoid off-by-one confusion.The retry logic uses
retries < max_retrieswhich meansmax_retries: 3actually allows 3 retries (4 total attempts). The error message should reflect the actual retry count. Consider renaming tomax_attemptsor adjusting the logic to make the behavior clearer.Apply this diff to clarify the semantics:
- max_retries = options.fetch(:max_retries, 3) - retries = 0 + max_attempts = options.fetch(:max_retries, 3) + 1 + attempts = 0 begin upload_part_to_url(presigned_url, part_data) true rescue StandardError => e - retries += 1 - raise "Failed to upload part after #{max_retries} retries: #{e.message}" unless retries < max_retries + attempts += 1 + raise "Failed to upload part after #{attempts} attempts: #{e.message}" unless attempts < max_attempts - sleep(2**retries) # Exponential backoff + sleep(2**attempts) # Exponential backoff retry end
473-495: LGTM: Solid polling implementation with proper timeout handling.The polling logic correctly handles all status states and includes appropriate timeout protection.
Minor optimization: Check timeout before sleep to avoid unnecessary waiting:
when 'waiting', 'progress' elapsed = Time.now - start_time raise "Upload from URL polling timed out after #{poll_timeout} seconds" if elapsed > poll_timeout sleep(poll_interval)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
api_examples/upload_api/post_from_url.rb(1 hunks)examples/upload_with_progress.rb(1 hunks)lib/uploadcare/clients/multipart_uploader_client.rb(1 hunks)lib/uploadcare/clients/upload_client.rb(1 hunks)lib/uploadcare/resources/file_metadata.rb(1 hunks)spec/uploadcare/clients/group_client_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/upload_with_progress.rb
🧰 Additional context used
🧬 Code graph analysis (4)
api_examples/upload_api/post_from_url.rb (2)
lib/uploadcare.rb (1)
configure(19-21)lib/uploadcare/clients/upload_client.rb (2)
upload_from_url(112-123)upload_from_url_status(150-157)
lib/uploadcare/resources/file_metadata.rb (3)
lib/uploadcare.rb (1)
configuration(23-25)lib/uploadcare/resources/file.rb (1)
uuid(187-197)lib/uploadcare/resources/base_resource.rb (1)
assign_attributes(20-25)
lib/uploadcare/clients/multipart_uploader_client.rb (6)
lib/uploadcare/resources/file.rb (2)
uuid(187-197)file(32-36)lib/uploadcare/clients/upload_client.rb (1)
post(48-50)lib/uploadcare.rb (1)
configuration(23-25)lib/uploadcare/resources/paginated_collection.rb (1)
each(21-23)lib/uploadcare/clients/rest_client.rb (1)
put(44-46)lib/uploadcare/clients/upload_group_client.rb (1)
file_params(47-53)
spec/uploadcare/clients/group_client_spec.rb (2)
lib/uploadcare.rb (1)
configuration(23-25)lib/uploadcare/resources/file.rb (1)
uuid(187-197)
🪛 GitHub Actions: Ruby
lib/uploadcare/clients/multipart_uploader_client.rb
[warning] 3-3: rubocop: Lint/RedundantRequireStatement: Remove unnecessary require statement. Command: rubocop
[warning] 52-52: rubocop: Lint/UnusedMethodArgument: Unused method argument - block. Command: rubocop
[warning] 58-58: rubocop: Style/RedundantBegin: Redundant begin block detected. Command: rubocop
🔇 Additional comments (15)
spec/uploadcare/clients/group_client_spec.rb (3)
1-8: LGTM! Clean setup.The spec setup follows standard RSpec conventions with proper frozen string literal directive, appropriate let blocks, and clear describe block.
9-62: LGTM! Comprehensive list method testing.The tests properly cover both success and error scenarios for the
listmethod. The use of query parameters in stubs is correct, and the assertions appropriately verify raw response body equality for the client layer.
64-110: LGTM! Thorough info method testing.The tests appropriately cover both success and error scenarios for the
infomethod, including the nestedfilesarray structure. Error handling correctly verifies thatRequestErroris raised with the expected message.api_examples/upload_api/post_from_url.rb (1)
26-35: LGTM!The async upload example correctly demonstrates the
async: trueoption and token extraction. The client instance is properly reused from Example 1.lib/uploadcare/clients/upload_client.rb (6)
15-30: LGTM: Clean initialization and Faraday connection setup.The initialization properly configures the Faraday connection with multipart support and appropriate middleware for the Upload API.
76-81: LGTM: Proper validation and clean upload logic.The method correctly validates the file object and delegates to helper methods for parameter construction.
112-123: LGTM: Well-designed URL upload with sync/async support.The method properly handles both synchronous and asynchronous upload modes with appropriate validation.
281-306: LGTM: Well-designed convenience method for multipart uploads.The method provides a clean high-level interface that handles the complete multipart upload flow with optional parallelization and progress tracking.
327-347: LGTM: Robust group creation with flexible file parameter handling.The method properly handles both string UUIDs and file objects while building the correct indexed parameter structure.
650-661: LGTM: Clean helper for store parameter normalization.The conversion logic properly handles boolean, string, and nil values for the store parameter.
lib/uploadcare/clients/multipart_uploader_client.rb (5)
17-28: LGTM: Clean orchestration of multipart upload flow.The method properly coordinates the three-phase multipart upload process and returns a consistent response format.
52-72: Implementation correctly uses multi-threading.The implementation properly creates worker threads for concurrent chunk uploads (lines 56-68) and synchronizes progress callbacks with a mutex. The pipeline warning about unused
blockargument on line 52 is incorrect—the block is used on line 60 for progress callbacks.Note: The past review comment about sequential execution appears to reference an outdated version of this code. The current implementation correctly uses
Thread.newfor parallel uploads.
114-131: LGTM: Clean parameter generation with proper nil handling.The method properly constructs upload parameters with signature and metadata support, using
compactto remove nil values.
135-148: LGTM: Robust signature generation with graceful degradation.The method properly checks for SignatureGenerator availability and handles errors gracefully, allowing uploads to continue without signatures when necessary.
151-161: LGTM: Correct MIME type handling.This method correctly extracts and assigns the
content_typefrom the MIME type object (line 154), unlike the bug inupload_client.rb'sform_data_formethod. This is the correct pattern.
| def upload_parts_parallel(file, presigned_urls, part_size, threads, &block) | ||
| total_size = file.respond_to?(:size) ? file.size : ::File.size(file.path) | ||
| uploaded = 0 | ||
| mutex = Mutex.new | ||
| queue = Queue.new | ||
|
|
||
| # Read all parts into memory (for thread safety) | ||
| parts = [] | ||
| presigned_urls.each_with_index do |presigned_url, index| | ||
| file.seek(index * part_size) | ||
| part_data = file.read(part_size) | ||
|
|
||
| break if part_data.nil? || part_data.empty? | ||
|
|
||
| parts << { url: presigned_url, data: part_data, index: index } | ||
| end | ||
|
|
||
| # Add parts to queue | ||
| parts.each { |part| queue << part } | ||
|
|
||
| # Create worker threads | ||
| workers = threads.times.map do | ||
| Thread.new do | ||
| until queue.empty? | ||
| part = begin | ||
| queue.pop(true) | ||
| rescue StandardError | ||
| nil | ||
| end | ||
| next unless part | ||
|
|
||
| multipart_upload_part(part[:url], part[:data]) | ||
|
|
||
| mutex.synchronize do | ||
| uploaded += part[:data].bytesize | ||
| block&.call({ uploaded: uploaded, total: total_size, part: part[:index] + 1, | ||
| total_parts: parts.length }) | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| # Wait for all threads to complete | ||
| workers.each(&:join) | ||
| end |
There was a problem hiding this comment.
Memory safety concern: Loading all parts into memory defeats streaming for large files.
Line 586 reads all file parts into memory at once "for thread safety," but this can cause out-of-memory errors for large files. For example, a 5GB file with 5MB chunks would load ~5GB into RAM. Consider using a per-thread file handle (opened in each thread with independent file descriptors) or a mutex-protected seek/read pattern instead of pre-loading all chunks.
Alternative approach using per-thread file handles:
- # Read all parts into memory (for thread safety)
- parts = []
- presigned_urls.each_with_index do |presigned_url, index|
- file.seek(index * part_size)
- part_data = file.read(part_size)
-
- break if part_data.nil? || part_data.empty?
-
- parts << { url: presigned_url, data: part_data, index: index }
- end
-
- # Add parts to queue
- parts.each { |part| queue << part }
+ # Add part metadata to queue (read data in threads)
+ presigned_urls.each_with_index do |presigned_url, index|
+ queue << { url: presigned_url, index: index, offset: index * part_size, size: part_size }
+ end
+
+ file_path = file.path
# Create worker threads
workers = threads.times.map do
Thread.new do
+ # Each thread opens its own file handle
+ thread_file = ::File.open(file_path, 'rb')
until queue.empty?
part = begin
queue.pop(true)
rescue StandardError
nil
end
next unless part
- multipart_upload_part(part[:url], part[:data])
+ thread_file.seek(part[:offset])
+ part_data = thread_file.read(part[:size])
+ next if part_data.nil? || part_data.empty?
+
+ multipart_upload_part(part[:url], part_data)
mutex.synchronize do
uploaded += part[:data].bytesize
block&.call({ uploaded: uploaded, total: total_size, part: part[:index] + 1,
- total_parts: parts.length })
+ total_parts: presigned_urls.length })
end
end
+ thread_file.close
end
endCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/uploadcare/clients/upload_client.rb around lines 580 to 624, avoid
reading all parts into memory (currently done at line ~586) because it causes
OOM for large files; instead change the worker logic to open the file per thread
or use a shared file with a mutex-protected seek/read so each thread reads only
its own part on demand, and push only part descriptors (url, index, size) into
the queue; when a worker pops a descriptor it should open a separate File handle
(or lock, seek and read) to read that single chunk, call multipart_upload_part
with the chunk, and update uploaded bytes inside the mutex – ensure File handles
are closed and errors are propagated/logged.
- Remove byebug debug require from multipart_uploader_client.rb (CRITICAL) - Fix method naming consistency in AddOns: remove 'check_' prefix from aws_rekognition_detect_moderation_labels_status - Update corresponding test to match renamed method All tests passing (310 examples, 0 failures)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/uploadcare/clients/multipart_uploader_client.rb (1)
77-96: Implement retry logic for transient chunk upload failures.The error handler logs and re-raises without retrying transient failures. The PR objectives explicitly mention "Enhanced retry logic and error reporting" as a key improvement, yet
process_chunkcallsput()directly without leveraging any retry mechanism. While the parentUploadClientmay include retry-enabled methods, this implementation bypasses them.Implement exponential backoff retry for chunk uploads:
def process_chunk(object, links, link_index) + max_retries = 3 + retry_count = 0 + + begin offset = link_index * CHUNK_SIZE chunk = ::File.read(object, CHUNK_SIZE, offset) put(links[link_index], chunk) return unless block_given? yield( chunk_size: CHUNK_SIZE, object: object, offset: offset, link_index: link_index, links: links, links_count: links.count ) rescue StandardError => e - # Log error and re-raise for now - could implement retry logic here - Uploadcare.configuration.logger&.error("Chunk upload failed for link_id #{link_index}: #{e.message}") - raise + retry_count += 1 + if retry_count <= max_retries + backoff = 2 ** (retry_count - 1) + Uploadcare.configuration.logger&.warn("Chunk #{link_index} failed (attempt #{retry_count}/#{max_retries}), retrying in #{backoff}s: #{e.message}") + sleep(backoff) + retry + else + Uploadcare.configuration.logger&.error("Chunk upload failed for link_id #{link_index} after #{max_retries} retries: #{e.message}") + raise + end + end end
🧹 Nitpick comments (1)
lib/uploadcare/clients/multipart_uploader_client.rb (1)
132-147: Consider failing fast when required signature generation fails.When
sign_uploadsis enabled but signature generation fails, the method logs and continues by returningnil. This defers the failure to the API server, resulting in less clear error messages. If signing is explicitly enabled in configuration, signature generation failure likely indicates a configuration error that should fail immediately.Consider this approach for clearer error handling:
def generate_upload_signature if defined?(Uploadcare::Param::Upload::SignatureGenerator) - Uploadcare::Param::Upload::SignatureGenerator.call + return Uploadcare::Param::Upload::SignatureGenerator.call else - Uploadcare.configuration.logger&.warn('Upload signing is enabled but SignatureGenerator is not available') - nil + raise 'Upload signing is enabled but SignatureGenerator is not available' end rescue StandardError => e - Uploadcare.configuration.logger&.error("Failed to generate upload signature: #{e.message}") - nil + raise "Failed to generate upload signature: #{e.message}" endThis makes configuration errors explicit rather than allowing them to propagate as API failures.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
lib/uploadcare/clients/multipart_uploader_client.rb(1 hunks)lib/uploadcare/resources/add_ons.rb(1 hunks)spec/uploadcare/resources/add_ons_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/uploadcare/resources/add_ons_spec.rb
- lib/uploadcare/resources/add_ons.rb
🧰 Additional context used
🪛 GitHub Actions: Ruby
lib/uploadcare/clients/multipart_uploader_client.rb
[warning] 3-3: RuboCop offense: Lint/RedundantRequireStatement - Remove unnecessary require statement. (require 'thread')
[warning] 51-51: RuboCop offense: Lint/UnusedMethodArgument - Unused method argument - block. If it's necessary, use _ or _block as an argument name to indicate that it won't be used.
[warning] 57-57: RuboCop offense: Style/RedundantBegin - Redundant begin block detected.
| # Split file into chunks and upload those chunks into respective Amazon links | ||
| # @param object [File] | ||
| # @param links [Array] of strings; by default list of Amazon storage urls | ||
| def upload_chunks(object, links, &block) |
There was a problem hiding this comment.
Fix method signature to properly declare block parameter.
The block parameter should be explicitly declared or omitted entirely since it's only used via block_given?.
Apply this diff to address the RuboCop warning:
- def upload_chunks(object, links, &block)
+ def upload_chunks(object, links)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def upload_chunks(object, links, &block) | |
| def upload_chunks(object, links) |
🧰 Tools
🪛 GitHub Actions: Ruby
[warning] 51-51: RuboCop offense: Lint/UnusedMethodArgument - Unused method argument - block. If it's necessary, use _ or _block as an argument name to indicate that it won't be used.
🤖 Prompt for AI Agents
In lib/uploadcare/clients/multipart_uploader_client.rb around line 51, the
method signature def upload_chunks(object, links, &block) improperly declares a
block param while the method only checks block_given?; remove the explicit
&block from the signature so it reads def upload_chunks(object, links) (or
alternatively, if the method intends to capture the block, update internal uses
to reference the block variable), and run RuboCop to ensure the warning is
resolved.
- Fix batch operations test mocking issues (use correct method names and string keys) - Fix rubocop offenses (remove redundant require, fix method signatures, refactor complexity) - Refactor WebhookSignatureVerifier to reduce cyclomatic complexity - Add proper block parameter to upload_chunks method - Auto-correct code style issues (indentation, trailing whitespace, etc.) All tests passing: 310 examples, 0 failures Rubocop: 2 minor metric violations remaining (acceptable)
- Extract multipart upload helpers into separate module to reduce class length - Refactor upload_chunks method to reduce ABC complexity - Split complex methods into smaller, focused methods - Fix all rubocop offenses (no violations remaining) - Maintain all functionality and tests passing All tests passing: 310 examples, 0 failures Rubocop clean: 134 files inspected, no offenses detected
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
lib/uploadcare/resources/file_metadata.rb (1)
24-28: Critical: Custom metadata from API response is lost.The
indexmethod fetches metadata via@file_metadata_client.index(uuid)but then callsassign_attributes(response)on line 26. Based on BaseResource's implementation,assign_attributesonly sets attributes with defined setters (those in ATTRIBUTES). Since the file metadata API returns arbitrary user-defined key-value pairs, any custom metadata keys will be silently discarded.Additionally, the
@metadatahash initialized on line 17 is never populated with the API response, defeating its stated purpose (line 11 comment).Apply this fix to preserve custom metadata:
def index(uuid) response = @file_metadata_client.index(uuid) - assign_attributes(response) + @metadata = response self endOr if ATTRIBUTES should remain for file properties:
def index(uuid) response = @file_metadata_client.index(uuid) - assign_attributes(response) + assign_attributes(response.slice(*ATTRIBUTES)) + @metadata = response.except(*ATTRIBUTES) self end</comment_end>
examples/simple_upload.rb (1)
36-38: Use block form of File.open or ensure block to guarantee file closure.The file is opened and closed explicitly without protection against exceptions during upload. If
Uploadcare::Uploader.uploadraises an error, the file handle will leak.Apply this diff to use the block form:
- # Open and upload the file - file = File.open(file_path, 'rb') - result = Uploadcare::Uploader.upload(file, store: true) - file.close + # Open and upload the file + result = File.open(file_path, 'rb') do |file| + Uploadcare::Uploader.upload(file, store: true) + endlib/uploadcare/resources/group.rb (1)
101-111: Critical bug: Missing return statement in id method.The
extracted_idis computed on lines 106-107 but never returned. The method falls through to line 109, returning@idwhich is likelynilwhen the group is initialized from a URL.Apply this diff to fix and memoize the extracted ID:
def id return @id if @id # If initialized from URL, extract ID if @cdn_url extracted_id = @cdn_url.gsub('https://ucarecdn.com/', '') - extracted_id.gsub(%r{/.*}, '') - else - @id + @id = extracted_id.gsub(%r{/.*}, '') end + + @id endexamples/group_creation.rb (1)
16-19: Add validation for required configuration values.Using
ENV.fetchwithnilas the default allows missing keys to slip through, causing cryptic authentication errors later. For an example script, it's better to fail fast with a clear message.Apply this diff to add validation:
# Configure Uploadcare Uploadcare.configure do |config| config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) end + +# Validate required configuration +unless Uploadcare.configuration.public_key && Uploadcare.configuration.secret_key + puts 'Error: UPLOADCARE_PUBLIC_KEY and UPLOADCARE_SECRET_KEY environment variables are required' + puts 'Please set them in your environment or .env file' + exit 1 +endlib/uploadcare/clients/upload_client.rb (1)
586-598: Critical: Loading all parts into memory causes OOM for large files.Lines 586-595 read all file parts into memory at once. For large files, this defeats the purpose of chunked uploads and can cause out-of-memory errors. A 5GB file with 5MB chunks would load ~5GB into RAM.
Consider using per-thread file handles instead:
- # Read all parts into memory (for thread safety) - parts = [] + # Prepare part metadata (read data in threads) + part_descriptors = [] presigned_urls.each_with_index do |presigned_url, index| - file.seek(index * part_size) - part_data = file.read(part_size) - - break if part_data.nil? || part_data.empty? - - parts << { url: presigned_url, data: part_data, index: index } + part_descriptors << { url: presigned_url, index: index, offset: index * part_size, size: part_size } end - # Add parts to queue - parts.each { |part| queue << part } + # Add descriptors to queue + part_descriptors.each { |desc| queue << desc } + + file_path = file.path # Create worker threads workers = threads.times.map do Thread.new do + # Each thread opens its own file handle + thread_file = ::File.open(file_path, 'rb') until queue.empty? part = begin queue.pop(true) rescue StandardError nil end next unless part + thread_file.seek(part[:offset]) + part_data = thread_file.read(part[:size]) + next if part_data.nil? || part_data.empty? + multipart_upload_part(part[:url], part_data) mutex.synchronize do uploaded += part[:data].bytesize block&.call({ uploaded: uploaded, total: total_size, part: part[:index] + 1, - total_parts: parts.length }) + total_parts: part_descriptors.length }) end end + thread_file.close end endapi_examples/upload_api/post_from_url.rb (1)
4-4: Inconsistent dotenv handling - use begin/rescue pattern like other examples.This file unconditionally requires
dotenv/load, but other example files (e.g.,examples/simple_upload.rb,examples/group_creation.rb) wrap it in a begin/rescue block to handle cases where dotenv isn't available.Apply this diff to align with the pattern used in other examples:
-require 'dotenv/load' +# Load environment variables from .env file if dotenv is available +begin + require 'dotenv/load' +rescue LoadError + # dotenv not available, skip loading .env file +endlib/uploadcare/clients/multipart_uploader_client.rb (1)
89-99: Missing retry logic for chunk upload failures.The
process_chunkmethod callsput()directly without retry logic, bypassing the parent class'smultipart_upload_partmethod that includes exponential backoff. When chunk uploads fail due to transient network issues or throttling, this implementation will immediately fail rather than retry, reducing upload reliability.Consider implementing retry logic with exponential backoff (e.g., up to 3 attempts) or refactor to use the parent's retry-enabled method for chunk uploads.
🧹 Nitpick comments (2)
lib/uploadcare/resources/file_metadata.rb (1)
62-99: Consider caching client instances in class methods.Each class method (
index,show,update,delete) instantiates a newFileMetadataClientfor every call. While this ensures proper isolation, it may be inefficient for repeated operations.Since these are compatibility methods for v4.4.3, the current approach is acceptable. However, if performance becomes a concern, consider caching client instances per configuration.
Example for caching:
def self.client_for(config) @clients ||= {} @clients[config] ||= Uploadcare::FileMetadataClient.new(config) end def self.index(uuid, config = Uploadcare.configuration) client_for(config).index(uuid) end</comment_end>
spec/uploadcare/resources/file_spec.rb (1)
198-252: Consider using RSpec shared examples to reduce test duplication.The comments on lines 198-199 and 233-234 acknowledge duplication between instance and class method tests. You could use shared examples to DRY this up.
Example approach:
shared_examples 'local_copy behavior' do it { is_expected.to be_a(Uploadcare::File) } it { expect(subject.uuid).to eq(source) } it { expect(subject.original_filename).to eq('copy_of_file.jpg') } it { expect(subject.size).to eq(12_345) } end describe '#local_copy' do # ... setup ... subject { file.local_copy(options) } include_examples 'local_copy behavior' end describe '.local_copy' do # ... setup ... subject { described_class.local_copy(source, options) } include_examples 'local_copy behavior' end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
api_examples/upload_api/post_from_url.rb(1 hunks)examples/batch_upload.rb(1 hunks)examples/group_creation.rb(1 hunks)examples/large_file_upload.rb(1 hunks)examples/simple_upload.rb(1 hunks)examples/upload_with_progress.rb(1 hunks)examples/url_upload.rb(1 hunks)lib/uploadcare/clients/multipart_upload_helpers.rb(1 hunks)lib/uploadcare/clients/multipart_uploader_client.rb(1 hunks)lib/uploadcare/clients/upload_client.rb(1 hunks)lib/uploadcare/resources/file.rb(1 hunks)lib/uploadcare/resources/file_metadata.rb(1 hunks)lib/uploadcare/resources/group.rb(1 hunks)lib/uploadcare/webhook_signature_verifier.rb(1 hunks)spec/uploadcare/resources/file_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/upload_with_progress.rb
- lib/uploadcare/webhook_signature_verifier.rb
- examples/url_upload.rb
- examples/batch_upload.rb
- lib/uploadcare/resources/file.rb
🧰 Additional context used
🧬 Code graph analysis (5)
api_examples/upload_api/post_from_url.rb (2)
lib/uploadcare.rb (2)
configure(19-21)configuration(23-25)lib/uploadcare/clients/upload_client.rb (2)
upload_from_url(112-123)upload_from_url_status(150-157)
spec/uploadcare/resources/file_spec.rb (1)
lib/uploadcare/resources/file.rb (11)
uuid(187-197)list(44-61)file(32-36)store(66-71)delete(76-81)info(21-25)info(86-91)local_copy(96-100)local_copy(150-155)remote_copy(106-109)remote_copy(163-167)
lib/uploadcare/resources/file_metadata.rb (2)
lib/uploadcare.rb (1)
configuration(23-25)lib/uploadcare/resources/base_resource.rb (1)
assign_attributes(20-25)
examples/large_file_upload.rb (3)
lib/uploadcare.rb (1)
configure(19-21)lib/uploadcare/resources/file.rb (1)
file(32-36)lib/uploadcare/clients/upload_client.rb (1)
multipart_upload(281-306)
examples/simple_upload.rb (4)
lib/uploadcare.rb (1)
configure(19-21)lib/uploadcare/resources/file.rb (2)
file(32-36)uuid(187-197)lib/uploadcare/clients/multipart_uploader_client.rb (1)
upload(19-29)lib/uploadcare/resources/group.rb (1)
uuid(115-117)
🔇 Additional comments (5)
lib/uploadcare/clients/multipart_uploader_client.rb (5)
1-14: LGTM!The file header, module structure, and constants are well-defined. The 5MB chunk size and 4-thread concurrency limit are sensible defaults for multipart uploads.
31-44: LGTM!The
upload_startandupload_completemethods correctly implement the multipart upload lifecycle endpoints with appropriate parameter construction.
46-87: Well-implemented thread pooling for parallel uploads.The implementation correctly limits concurrency to
MAX_CONCURRENT_UPLOADS, uses a work queue pattern, and ensures thread-safe progress callbacks via mutex synchronization. TheThreadErrorhandling for queue exhaustion is appropriate, and error propagation via re-raise maintains proper failure semantics.
101-116: LGTM!The helper methods
chunk_progressandlog_errorare cleanly implemented with appropriate null-safe logging.
118-121: LGTM!The
form_data_foroverride appropriately delegates tomultipart_file_paramsfor multipart-specific file parameter construction.
| # Upload a big file by splitting it into parts | ||
| # @param object [File] File to upload | ||
| # @param options [Hash] Upload options | ||
| # @return [Hash] Response with uuid | ||
| def upload(object, options = {}, &block) |
There was a problem hiding this comment.
Fix parameter type mismatch between documentation and implementation.
The documentation specifies @param object [File] File to upload, but the implementation at line 92 calls ::File.read(object, CHUNK_SIZE, offset), which expects object to be a String file path, not a File object. If a user passes a File object (as documented), the code will fail at runtime.
Apply this diff to correct the documentation:
- # @param object [File] File to upload
+ # @param object [String] File path to uploadAlternatively, if File objects should be supported, refactor process_chunk to handle both file paths and File objects by checking the type and seeking/reading accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Upload a big file by splitting it into parts | |
| # @param object [File] File to upload | |
| # @param options [Hash] Upload options | |
| # @return [Hash] Response with uuid | |
| def upload(object, options = {}, &block) | |
| # Upload a big file by splitting it into parts | |
| # @param object [String] File path to upload | |
| # @param options [Hash] Upload options | |
| # @return [Hash] Response with uuid | |
| def upload(object, options = {}, &block) |
🤖 Prompt for AI Agents
In lib/uploadcare/clients/multipart_uploader_client.rb around lines 15 to 19 and
the process_chunk call at line 92, the YARD doc says the parameter is a File
object but the implementation calls ::File.read(object, CHUNK_SIZE, offset)
which requires a String path; either change the documentation to "@param object
[String] Path to file to upload" or modify the chunk-reading logic to accept
both types by checking the parameter type (if object is a String, use
::File.read(path, CHUNK_SIZE, offset); if it is a File/IO, seek to offset and
read CHUNK_SIZE from the object, handling binary mode and preserving position),
and update/ add tests accordingly.
| ATTRIBUTES = %i[ | ||
| datetime_removed datetime_stored datetime_uploaded is_image is_ready mime_type original_file_url | ||
| original_filename size url uuid variations content_info appdata source | ||
| ].freeze | ||
|
|
||
| attr_accessor(*ATTRIBUTES) | ||
| # Custom metadata is handled separately to allow for arbitrary key-value pairs | ||
| attr_accessor :metadata |
There was a problem hiding this comment.
ATTRIBUTES still contains file properties rather than supporting arbitrary metadata keys.
Despite the past review comment being marked as addressed, the ATTRIBUTES constant still defines file resource properties (datetime_removed, is_image, mime_type, etc.) rather than arbitrary metadata keys. According to the API documentation and the comment on line 11, file metadata should support arbitrary user-defined key-value pairs. These fixed attributes appear to belong to a File resource, not FileMetadata.
While a separate @metadata attribute was added (line 12), it's never populated (see next comment). Consider removing these file-property ATTRIBUTES entirely and using only the metadata hash to store arbitrary key-value pairs from the API.
</comment_end>
🤖 Prompt for AI Agents
In lib/uploadcare/resources/file_metadata.rb around lines 5 to 12, the
ATTRIBUTES constant currently lists fixed File properties (datetime_removed,
is_image, mime_type, etc.) which prevents arbitrary user-defined metadata from
being captured; remove these file-property symbols from ATTRIBUTES (leaving only
metadata accessors), and ensure the instance uses a single @metadata hash to
store all key/value pairs returned by the API by populating @metadata with the
response (or merging response minus any reserved keys) instead of relying on
assign_attributes to set unknown keys.
| def update(uuid, key, value) | ||
| @file_metadata_client.update(uuid, key, value) | ||
| end | ||
|
|
||
| # Retrieves the value of a specific metadata key for the file | ||
| # @param key [String] The metadata key to retrieve | ||
| # @return [String] The value of the metadata key | ||
| # @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/File-metadata/operation/fileMetadata | ||
| # TODO - Remove uuid if the operation is being performed on same file | ||
| def show(uuid, key) | ||
| @file_metadata_client.show(uuid, key) | ||
| end | ||
|
|
||
| # Deletes a specific metadata key for the file | ||
| # @param key [String] The metadata key to delete | ||
| # @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/File-metadata/operation/deleteFileMetadata | ||
| # TODO - Remove uuid if the operation is being performed on same file | ||
| def delete(uuid, key) | ||
| @file_metadata_client.delete(uuid, key) | ||
| end |
There was a problem hiding this comment.
Instance methods still require uuid parameter, indicating incomplete design.
All three instance methods (update, show, delete) require a uuid parameter despite being instance methods. The TODO comments on lines 30, 39, and 47 acknowledge this design issue, but it remains unresolved. This forces users to repeatedly provide the uuid when calling methods on a FileMetadata instance.
This pattern is inconsistent with typical resource design where an instance represents a specific entity. As noted in previous reviews, consider refactoring so FileMetadata instances are bound to a specific file uuid at initialization, allowing instance methods to use the bound uuid internally.
Do you want me to generate a refactored implementation that binds FileMetadata instances to a specific file uuid, eliminating the need for uuid parameters in instance methods?
</comment_end>
- Fix error handler to not raise on empty error strings - Fix configuration to read ENV variables at runtime - Fix uploader_spec tests to match actual implementation - Add Tempfile require for specs - Update test expectations for upload behavior
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
lib/uploadcare/error_handler.rb (2)
9-21: Address RuboCop ABC complexity warning.The
handle_errormethod has multiple responsibilities: response extraction, error detection, JSON parsing, error message construction, and raising exceptions. While functional, this contributes to the ABC complexity flagged by RuboCop.Consider extracting message construction to a helper method:
def handle_error(error) response = error.response catch_upload_errors(response) parsed_response = JSON.parse(response[:body].to_s) raise RequestError, parsed_response.to_s unless parsed_response.is_a?(Hash) raise RequestError, parsed_response['detail'] if parsed_response['detail'] - error_messages = parsed_response.map { |k, v| "#{k}: #{v}" }.join('; ') - raise RequestError, error_messages + raise RequestError, build_error_message(parsed_response) rescue JSON::ParserError raise RequestError, response[:body].to_s end + +private + +def build_error_message(parsed_response) + parsed_response.map { |k, v| "#{k}: #{v}" }.join('; ') +end
27-32: Consider consolidating JSON parsing to avoid duplication.Both
catch_upload_errorsandhandle_errorparse the response body as JSON. This duplication could lead to inconsistencies if parsing logic needs to change.Consider passing the parsed response to
catch_upload_errorsor consolidating the parsing:def handle_error(error) response = error.response parsed_response = parse_response(response) catch_upload_errors(response, parsed_response) # ... rest of logic end private def parse_response(response) JSON.parse(response[:body].to_s) rescue JSON::ParserError nil end def catch_upload_errors(response, parsed_response) return unless response[:status] == 200 && parsed_response.is_a?(Hash) error = parsed_response['error'] raise RequestError, error if error && !error.to_s.empty? endlib/uploadcare/configuration.rb (1)
120-128: Consider simplifying cdn_base to avoid unnecessary lambda.Returning a lambda that immediately evaluates based on
use_subdomainsadds indirection. Unless this lambda is called multiple times with changinguse_subdomainsvalues, consider returning the value directly or clarifying why lazy evaluation is needed.def cdn_base if use_subdomains CnameGenerator.cdn_base_postfix else default_cdn_base end endIf lazy evaluation is required for a specific reason, consider adding a comment explaining why.
spec/uploadcare/error_handler_spec.rb (1)
299-309: Consider handling nil response gracefully in the implementation.This test documents that a
NoMethodErroris raised whenresponseis nil. While this is valid edge-case documentation, the error message will be cryptic for users. If this scenario is possible in production, consider adding a guard clause inhandle_errorto raise a more descriptiveRequestError.spec/uploadcare/concerns/throttle_handler_spec.rb (1)
35-37: Remove unusedlet(:call_count)definition.This
letblock is never used becausecall_countis redefined as a local variable inside eachitblock (e.g., lines 39, 51, 65). Remove the dead code.context 'when block is throttled once then succeeds' do - let(:call_count) { 0 } - it 'retries and returns success result' doexamples/large_file_upload.rb (1)
15-19: Consider providing clearer error for missing API keys.When
UPLOADCARE_PUBLIC_KEYorUPLOADCARE_SECRET_KEYare not set,nilis passed to the configuration. The upload will fail later with a potentially unclear error. For a better user experience in this example script, consider validating keys upfront.# Configure Uploadcare +public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) +secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) + +unless public_key && secret_key + puts 'Error: UPLOADCARE_PUBLIC_KEY and UPLOADCARE_SECRET_KEY environment variables are required' + puts 'Set them in your environment or create a .env file' + exit 1 +end + Uploadcare.configure do |config| - config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) - config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) + config.public_key = public_key + config.secret_key = secret_key endexamples/upload_with_progress.rb (1)
16-18: Consider simplifying environment variable access.Using
ENV.fetchwithnilas the default is equivalent toENV['KEY']. Either useENV.fetchwithout a default to raise an error immediately if keys are missing, or use direct hash access for clarity.Apply this diff:
Uploadcare.configure do |config| - config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) - config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) + config.public_key = ENV['UPLOADCARE_PUBLIC_KEY'] + config.secret_key = ENV['UPLOADCARE_SECRET_KEY'] endOr use
ENV.fetchwithout default for fail-fast behavior:Uploadcare.configure do |config| - config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY', nil) - config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY', nil) + config.public_key = ENV.fetch('UPLOADCARE_PUBLIC_KEY') + config.secret_key = ENV.fetch('UPLOADCARE_SECRET_KEY') end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
Gemfile(1 hunks)api_examples/upload_api/post_from_url.rb(1 hunks)examples/group_creation.rb(1 hunks)examples/large_file_upload.rb(1 hunks)examples/simple_upload.rb(1 hunks)examples/upload_with_progress.rb(1 hunks)lib/uploadcare/clients/multipart_upload_helpers.rb(1 hunks)lib/uploadcare/configuration.rb(1 hunks)lib/uploadcare/error_handler.rb(1 hunks)lib/uploadcare/resources/file_metadata.rb(1 hunks)spec/spec_helper.rb(2 hunks)spec/uploadcare/clients/multipart_upload_helpers_spec.rb(1 hunks)spec/uploadcare/concerns/throttle_handler_spec.rb(1 hunks)spec/uploadcare/configuration_spec.rb(1 hunks)spec/uploadcare/error_handler_spec.rb(1 hunks)spec/uploadcare/exception/auth_error_spec.rb(1 hunks)spec/uploadcare/exception/configuration_error_spec.rb(1 hunks)spec/uploadcare/exception/conversion_error_spec.rb(1 hunks)spec/uploadcare/exception/request_error_spec.rb(1 hunks)spec/uploadcare/exception/retry_error_spec.rb(1 hunks)spec/uploadcare/exception/throttle_error_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- spec/uploadcare/exception/configuration_error_spec.rb
🚧 Files skipped from review as they are similar to previous changes (6)
- spec/uploadcare/configuration_spec.rb
- examples/group_creation.rb
- api_examples/upload_api/post_from_url.rb
- examples/simple_upload.rb
- lib/uploadcare/resources/file_metadata.rb
- lib/uploadcare/clients/multipart_upload_helpers.rb
🧰 Additional context used
🧬 Code graph analysis (6)
spec/uploadcare/clients/multipart_upload_helpers_spec.rb (2)
lib/uploadcare.rb (1)
configuration(23-25)lib/uploadcare/resources/file.rb (1)
file(32-36)
spec/spec_helper.rb (1)
lib/uploadcare.rb (1)
configure(19-21)
examples/large_file_upload.rb (3)
lib/uploadcare.rb (1)
configure(19-21)lib/uploadcare/resources/file.rb (1)
file(32-36)lib/uploadcare/clients/upload_client.rb (1)
multipart_upload(281-306)
examples/upload_with_progress.rb (4)
lib/uploadcare.rb (1)
configure(19-21)lib/uploadcare/resources/file.rb (2)
file(32-36)uuid(187-197)lib/uploadcare/clients/multipart_uploader_client.rb (1)
upload(19-29)lib/uploadcare/resources/group.rb (1)
uuid(115-117)
spec/uploadcare/error_handler_spec.rb (1)
lib/uploadcare/error_handler.rb (1)
handle_error(9-21)
spec/uploadcare/concerns/throttle_handler_spec.rb (2)
lib/uploadcare/throttle_handler.rb (1)
handle_throttling(8-19)lib/uploadcare.rb (1)
configuration(23-25)
🪛 GitHub Actions: Ruby
lib/uploadcare/error_handler.rb
[error] 9-9: RuboCop: Metrics/ABCSize for handle_error is too high.
spec/uploadcare/clients/multipart_upload_helpers_spec.rb
[error] 245-245: NameError: uninitialized constant RSpec::Support::Differ. This indicates a missing or incompatible RSpec support component required by the test.
spec/uploadcare/exception/throttle_error_spec.rb
[error] 68-68: RuboCop: Style/MultilineBlockChain: Avoid multi-line chains of blocks.
[error] 75-75: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 83-83: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 89-89: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 97-97: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 105-105: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
spec/uploadcare/error_handler_spec.rb
[error] 264-265: Uploadcare::ErrorHandler error message formatting with complex nested JSON errors formats nested errors correctly. Expected a raised Uploadcare::Exception::RequestError with detailed message, but a different error was raised (structure mismatch) during handle_error.
lib/uploadcare/configuration.rb
[error] 98-98: RuboCop: Layout/TrailingWhitespace detected. Trailing whitespace is autocorrectable.
[error] 102-102: RuboCop: Layout/TrailingWhitespace detected. Trailing whitespace is autocorrectable.
[error] 105-105: RuboCop: Layout/TrailingWhitespace detected. Trailing whitespace is autocorrectable.
spec/uploadcare/concerns/throttle_handler_spec.rb
[error] 42-42: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 56-56: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 68-68: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 85-85: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 87-87: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 105-105: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 107-107: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
🔇 Additional comments (16)
spec/uploadcare/exception/conversion_error_spec.rb (1)
5-186: LGTM! Comprehensive test coverage for ConversionError.This test suite provides excellent coverage across inheritance, initialization, error scenarios (document, video, image, job, API-specific), message formatting, and backtrace handling. The organization by context is clear and follows RSpec best practices.
spec/uploadcare/exception/auth_error_spec.rb (1)
5-100: LGTM! Well-structured test suite for AuthError.The test suite provides good coverage of inheritance, initialization, raising/catching, error scenarios, message formatting, and backtrace handling. The organization is clear and follows RSpec conventions.
spec/uploadcare/exception/request_error_spec.rb (1)
5-256: LGTM! Comprehensive and well-organized test suite.This test suite provides excellent coverage of RequestError across HTTP status codes, API-specific errors, network issues, validation scenarios, upload-specific cases, message formatting, and backtrace handling. The nested rescue/re-raise test (lines 241-254) properly validates backtrace preservation.
spec/uploadcare/exception/retry_error_spec.rb (1)
5-237: LGTM! Comprehensive coverage of retry error scenarios.This test suite provides excellent coverage of RetryError across various retry scenarios (service unavailability, rate limiting, uploads, conversions), retry strategies (exponential, linear, jittered backoff), context-aware messaging, and backtrace handling. Well-organized and thorough.
spec/uploadcare/exception/throttle_error_spec.rb (2)
210-231: Verify acceptance of invalid timeout values is intentional.Tests document that
ThrottleErroraccepts negative, nil, and string timeout values without validation (lines 212-230). While this provides flexibility, it could lead to unexpected behavior if calling code expects numeric timeouts.Consider whether validation should be added to ensure timeout is a non-negative number, or document this flexibility clearly if intentional.
Does the
ThrottleErrorimplementation intentionally accept any timeout value type, or should validation be added to ensure type safety?
5-323: Overall: well-structured test suite with comprehensive throttling scenarios.The test suite provides good coverage of ThrottleError behavior across initialization, timeout handling, various throttling scenarios (API, upload, conversion), real-world backoff patterns, and backtrace preservation. Address the style issues flagged by RuboCop for consistency.
lib/uploadcare/configuration.rb (2)
17-129: LGTM! Well-documented configuration class with sensible defaults.The Configuration class provides comprehensive configuration options with clear documentation, environment variable support, and sensible defaults. The structure is clean and follows Ruby conventions. Address the minor formatting and verification items noted.
114-128: CnameGenerator is properly defined and accessible.The class is explicitly required in lib/uploadcare.rb and is available in the Uploadcare namespace alongside Configuration. Both
generate_cname(line 23) andcdn_base_postfix(line 13) methods exist as defined class methods in lib/uploadcare/cname_generator.rb.spec/uploadcare/clients/multipart_upload_helpers_spec.rb (2)
6-454: LGTM! Comprehensive test coverage for multipart upload helpers.This test suite provides excellent coverage of the MultipartUploadHelpers module across parameter generation, signing, metadata handling, file parameter extraction, and various integration scenarios. The tests properly verify:
- Private method visibility
- Configuration integration
- Error propagation
- Edge cases (nil values, various file types, MIME detection)
Well-organized with clear test descriptions. Address the RSpec::Support::Differ issue to ensure tests run successfully.
245-246: Verify ifcontain_exactlymatcher is actually failing in the test pipeline.The test code at line 245 is syntactically correct and follows standard RSpec 3+ patterns. The
contain_exactlymatcher is a built-in RSpec matcher and the spec_helper.rb is properly configured. However, without access to actual pipeline error logs, the reportedNameError: uninitialized constant RSpec::Support::Differcannot be confirmed. If this error does occur, it would indicate a gem version mismatch between rspec-core, rspec-expectations, and rspec-support. Check the actual CI/pipeline logs to confirm whether the error exists, and if it does, verify that all RSpec gems are compatible versions by runningbundle installor checkingbundle show rspec rspec-core rspec-expectations rspec-support.spec/uploadcare/concerns/throttle_handler_spec.rb (1)
6-14: Good test isolation with dummy class.The dummy class correctly includes the module and stubs
sleepto avoid real delays. The expectation-based approach (expect(instance).to receive(:sleep)) works correctly with this setup.Gemfile (1)
9-9: LGTM!Correct use of
require: falsefor SimpleCov - it needs to be loaded explicitly in spec_helper before the application code to properly track coverage.spec/spec_helper.rb (2)
3-7: LGTM!SimpleCov is correctly configured before other requires, with appropriate filters to exclude spec and vendor directories from coverage reports.
28-35: Fixauth_typeconfiguration value: use'Uploadcare'instead of'Uploadcare.Simple'.The gem configuration defines auth_type with a default of
'Uploadcare', not'Uploadcare.Simple'. The latter is the HTTP authorization scheme name used in API requests (e.g., in Authorization headers), not a valid configuration parameter value. Updatec.auth_type = 'Uploadcare.Simple'toc.auth_type = 'Uploadcare'to match the gem's expected configuration format.Likely an incorrect or invalid review comment.
examples/large_file_upload.rb (1)
54-77: File handle leak issue addressed.The code now correctly uses
File.open(file_path, 'rb') do |file| ... endblock form, which guarantees the file is closed even if an exception occurs during the upload. This resolves the previous review concern.examples/upload_with_progress.rb (1)
44-75: LGTM! Resource leak resolved.The previous resource leak issue has been fixed by using the block form of
File.open. The file handle is now automatically closed when the block exits, even if an exception is raised during upload. The progress tracking logic is well-implemented with proper guards against division by zero.
|
|
||
| # Override with ENV variables if present (always check ENV, not just if key exists) | ||
| config[:public_key] = ENV.fetch('UPLOADCARE_PUBLIC_KEY', config[:public_key]) | ||
| config[:secret_key] = ENV.fetch('UPLOADCARE_SECRET_KEY', config[:secret_key]) | ||
|
|
||
| # Override with options passed in | ||
| config.merge!(options) | ||
|
|
There was a problem hiding this comment.
Fix trailing whitespace as flagged by RuboCop.
Lines 98, 102, and 105 have trailing whitespace. Remove it for consistency with project style.
Apply this diff:
def initialize(options = {})
# Start with defaults
config = DEFAULTS.dup
-
+
# Override with ENV variables if present (always check ENV, not just if key exists)
config[:public_key] = ENV.fetch('UPLOADCARE_PUBLIC_KEY', config[:public_key])
config[:secret_key] = ENV.fetch('UPLOADCARE_SECRET_KEY', config[:secret_key])
-
+
# Override with options passed in
config.merge!(options)
-
+
# Set all attributes🧰 Tools
🪛 GitHub Actions: Ruby
[error] 98-98: RuboCop: Layout/TrailingWhitespace detected. Trailing whitespace is autocorrectable.
[error] 102-102: RuboCop: Layout/TrailingWhitespace detected. Trailing whitespace is autocorrectable.
[error] 105-105: RuboCop: Layout/TrailingWhitespace detected. Trailing whitespace is autocorrectable.
🤖 Prompt for AI Agents
In lib/uploadcare/configuration.rb around lines 98 to 105, remove trailing
whitespace at the ends of lines 98, 102, and 105; edit those lines to have no
spaces or tabs after the final characters so they conform to RuboCop/style
guidelines and then run the linter to verify no trailing whitespace remains.
|
|
||
| module Uploadcare | ||
| module ErrorHandler | ||
| include Exception |
There was a problem hiding this comment.
Critical: include Exception is incorrect and will cause issues.
Line 5 attempts to include Exception, which is Ruby's base exception class, not a module. This will either fail at runtime or produce unexpected behavior.
Based on the PR context mentioning exception classes under Uploadcare::Exception, you likely meant to reference exception types rather than include them. If you need to reference exception classes like RequestError, simply use them directly without including anything, or include a proper module if one exists.
Apply this diff if no module inclusion is needed:
module Uploadcare
module ErrorHandler
- include Exception
-
# Catches failed API errorsIf a specific module should be included, please clarify which one.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| include Exception | |
| module Uploadcare | |
| module ErrorHandler | |
| # Catches failed API errors |
🤖 Prompt for AI Agents
In lib/uploadcare/error_handler.rb around line 5, the line "include Exception"
is wrong because Exception is a class not a module; remove that include line and
instead reference exception classes directly (e.g.,
Uploadcare::Exception::RequestError) or, if you intended to mix in a module,
replace it with the correct module name (provide the actual module) and update
any usages accordingly so the file does not attempt to include Ruby's Exception
class.
| call_count = 0 | ||
| result = instance.handle_throttling do | ||
| call_count += 1 | ||
| raise Uploadcare::Exception::ThrottleError.new(1.0) if call_count == 1 |
There was a problem hiding this comment.
Suppress RuboCop Style/RaiseArgs for ThrottleError with custom constructor.
ThrottleError accepts a timeout value in its constructor, not a message string. The standard raise Class, message form won't work here since it would pass the timeout as a message. Disable the cop inline for these lines.
Apply this pattern throughout the file where ThrottleError.new(timeout) is used:
- raise Uploadcare::Exception::ThrottleError.new(1.0) if call_count == 1
+ raise Uploadcare::Exception::ThrottleError.new(1.0) if call_count == 1 # rubocop:disable Style/RaiseArgsAlternatively, add a file-level disable at the top of the spec:
# rubocop:disable Style/RaiseArgsAnd re-enable at the end:
# rubocop:enable Style/RaiseArgs🧰 Tools
🪛 GitHub Actions: Ruby
[error] 42-42: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
🤖 Prompt for AI Agents
In spec/uploadcare/concerns/throttle_handler_spec.rb around line 42, RuboCop's
Style/RaiseArgs cop is flagging the use of ThrottleError.new(timeout) because
raise Class, message would misapply — suppress the cop for these usages by
adding an inline disable comment on the offending lines where
ThrottleError.new(timeout) is raised (or alternatively add a file-level disable
at the top and re-enable at the end of the spec) so the custom constructor
remains intact.
| describe 'error message formatting' do | ||
| context 'with complex nested JSON errors' do | ||
| let(:error_response) do | ||
| double('error', response: { | ||
| status: 400, | ||
| body: '{"validation": {"file": ["Required", "Invalid format"], "size": ["Too large"]}}' | ||
| }) | ||
| end | ||
|
|
||
| it 'formats nested errors correctly' do | ||
| expected = 'validation: {"file"=>["Required", "Invalid format"], "size"=>["Too large"]}' | ||
| expect { instance.handle_error(error_response) } | ||
| .to raise_error(Uploadcare::Exception::RequestError, expected) | ||
| end | ||
| end |
There was a problem hiding this comment.
Fix the expected error message format to match implementation output.
The pipeline failure indicates the expected message format doesn't match the actual output from handle_error. When parsed_response.map { |k, v| "#{k}: #{v}" } is called on a nested hash, Ruby's Hash#to_s output may vary slightly. Verify the exact output format by running the implementation manually or adjust the test to be more flexible.
Consider using a regex matcher or include matcher instead of exact string matching:
it 'formats nested errors correctly' do
- expected = 'validation: {"file"=>["Required", "Invalid format"], "size"=>["Too large"]}'
- expect { instance.handle_error(error_response) }
- .to raise_error(Uploadcare::Exception::RequestError, expected)
+ expect { instance.handle_error(error_response) }
+ .to raise_error(Uploadcare::Exception::RequestError) do |error|
+ expect(error.message).to include('validation:')
+ expect(error.message).to include('Required')
+ expect(error.message).to include('Invalid format')
+ expect(error.message).to include('Too large')
+ end
end🧰 Tools
🪛 GitHub Actions: Ruby
[error] 264-265: Uploadcare::ErrorHandler error message formatting with complex nested JSON errors formats nested errors correctly. Expected a raised Uploadcare::Exception::RequestError with detailed message, but a different error was raised (structure mismatch) during handle_error.
🤖 Prompt for AI Agents
In spec/uploadcare/error_handler_spec.rb around lines 253 to 267, the test
expects an exact error message string that doesn't match the actual output of
handle_error for nested hashes; update the example to assert flexibly (e.g., use
a regex or include matcher) or change the expected string to the exact output
produced by the implementation after verifying it locally; replace the strict
equality raise_error(expected) check with raise_error(..., /validation:
\{.*"file".*\}/) or expect the error message to include the key/value substring
so the test tolerates Ruby's Hash string formatting variations.
| it 'can be initialized with a message and cause' do | ||
| StandardError.new('Original error') | ||
| error = described_class.new('Invalid API key') | ||
| error = error.exception('Invalid API key') | ||
| expect(error.message).to eq('Invalid API key') | ||
| end |
There was a problem hiding this comment.
Clarify or fix the cause propagation test.
This test appears incomplete. Line 23 creates a StandardError that is never used, and Line 25 calls .exception('Invalid API key') which is redundant and doesn't demonstrate cause chaining.
If testing exception chaining, consider:
it 'can be initialized with a message and cause' do
original_error = StandardError.new('Original error')
error = described_class.new('Invalid API key')
error.set_backtrace(original_error.backtrace)
# Or use Ruby 2.1+ implicit cause chaining via raise/rescue
expect(error.message).to eq('Invalid API key')
end🤖 Prompt for AI Agents
In spec/uploadcare/exception/auth_error_spec.rb around lines 22 to 27, the test
currently creates an unused StandardError and calls .exception redundantly;
replace that with a proper cause propagation check by creating original_error =
StandardError.new('Original error') and then either (A) if the AuthError
initializer accepts a cause, initialize error = described_class.new('Invalid API
key', original_error) and assert error.cause == original_error and error.message
== 'Invalid API key', or (B) use Ruby's raise/rescue to chain the cause: raise
original_error; rescue => e; error = described_class.new('Invalid API key');
raise error from e; rescue => chained; expect(chained.cause).to eq(e) and
expect(chained.message).to eq('Invalid API key') — pick the approach that
matches the class API and update the test accordingly.
| expect do | ||
| raise described_class | ||
| end.to raise_error(described_class) do |error| | ||
| expect(error.timeout).to eq(10.0) | ||
| end | ||
| end | ||
|
|
||
| it 'can be raised and caught with custom timeout' do | ||
| expect do | ||
| raise described_class.new(45.0) | ||
| end.to raise_error(described_class) do |error| | ||
| expect(error.timeout).to eq(45.0) | ||
| end |
There was a problem hiding this comment.
Address RuboCop multiline block chain warning.
Line 68 has a multiline block chain flagged by RuboCop. Consider reformatting:
it 'can be raised and caught with default timeout' do
expect do
raise described_class
end.to raise_error(described_class)
# Then verify timeout in a separate expectation or use a local variable
begin
raise described_class
rescue described_class => error
expect(error.timeout).to eq(10.0)
end
endOr use rescue-style as in other tests.
🧰 Tools
🪛 GitHub Actions: Ruby
[error] 68-68: RuboCop: Style/MultilineBlockChain: Avoid multi-line chains of blocks.
[error] 75-75: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
🤖 Prompt for AI Agents
In spec/uploadcare/exception/throttle_error_spec.rb around lines 66 to 78, the
test uses a multiline block chain with expect { raise described_class }.to
raise_error(described_class) { |error| ... } which RuboCop flags; refactor the
test to avoid the chained multiline block by either splitting into two steps
(first assert raise with .to raise_error(described_class) without a block, then
re-raise and rescue to assert error.timeout) or use the rescue-style pattern
consistent with other tests (begin; raise described_class; rescue
described_class => error; expect(error.timeout).to eq(10.0); end), ensuring the
test still checks default timeout 10.0 and the custom timeout 45.0 without
multiline block chaining.
| raise described_class.new(45.0) | ||
| end.to raise_error(described_class) do |error| | ||
| expect(error.timeout).to eq(45.0) | ||
| end | ||
| end | ||
|
|
||
| it 'can be caught as StandardError' do | ||
| expect do | ||
| raise described_class.new(15.0) | ||
| end.to raise_error(StandardError) | ||
| end | ||
|
|
||
| it 'can be caught as Uploadcare::Exception::ThrottleError' do | ||
| expect do | ||
| raise described_class.new(20.0) | ||
| end.to raise_error(Uploadcare::Exception::ThrottleError) | ||
| end | ||
| end | ||
|
|
||
| describe 'throttling scenarios' do | ||
| it 'handles API rate limiting' do | ||
| expect do | ||
| raise described_class.new(60.0) | ||
| end.to raise_error(described_class) do |error| | ||
| expect(error.timeout).to eq(60.0) | ||
| end | ||
| end | ||
|
|
||
| it 'handles short throttle periods' do | ||
| expect do | ||
| raise described_class.new(1.5) |
There was a problem hiding this comment.
Address RuboCop RaiseArgs style violations.
Lines 75, 83, 89, 97, and 105 use raise described_class.new(timeout) which RuboCop flags. For consistency with project style, consider using two-argument raise or rescue-style tests:
# Instead of:
raise described_class.new(45.0)
# Use rescue-style:
error = described_class.new(45.0)
begin
raise error
rescue described_class => e
expect(e.timeout).to eq(45.0)
end🧰 Tools
🪛 GitHub Actions: Ruby
[error] 75-75: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 83-83: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 89-89: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 97-97: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
[error] 105-105: RuboCop: Style/RaiseArgs: Provide an exception class and message as arguments to raise.
🤖 Prompt for AI Agents
In spec/uploadcare/exception/throttle_error_spec.rb around lines 75 to 105,
tests call raise described_class.new(timeout) which triggers RuboCop RaiseArgs
style warnings; replace those direct raise calls with creating the error object
then raising inside a begin/rescue (or use two-argument raise form) and assert
on the rescued exception: instantiate error = described_class.new(timeout), then
begin; raise error; rescue described_class => e; expect(e.timeout).to
eq(timeout); end (apply this pattern for the cases at lines 75, 83, 89, 97 and
105).
- Fix critical UUID extraction bug in File#uuid method - Add API key validation to all example scripts - Verify spec filename typo already fixed - Clean up debug requires and comments (none found) - Test coverage at 89.46% with 25 failures remaining in uploader_spec
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/uploadcare/resources/file.rb (1)
52-60: Critical bug still present: Incorrect resource_class assignment.This issue was flagged in a previous review but remains unresolved. In a class method,
selfis already the class (Uploadcare::File), soself.classreturnsClass. This will causePaginatedCollectionto receive the wrong class reference.Apply this diff to fix:
PaginatedCollection.new( resources: files, next_page: response['next'], previous_page: response['previous'], per_page: response['per_page'], total: response['total'], client: file_client, - resource_class: self.class + resource_class: self )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
examples/batch_upload.rb(1 hunks)examples/group_creation.rb(1 hunks)examples/large_file_upload.rb(1 hunks)examples/simple_upload.rb(1 hunks)examples/upload_with_progress.rb(1 hunks)examples/url_upload.rb(1 hunks)lib/uploadcare/resources/file.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/group_creation.rb
- examples/large_file_upload.rb
- examples/url_upload.rb
🧰 Additional context used
🧬 Code graph analysis (2)
examples/simple_upload.rb (4)
lib/uploadcare.rb (2)
configure(19-21)configuration(23-25)lib/uploadcare/resources/file.rb (2)
file(32-36)uuid(187-198)lib/uploadcare/clients/multipart_uploader_client.rb (1)
upload(19-29)lib/uploadcare/resources/group.rb (1)
uuid(115-117)
lib/uploadcare/resources/file.rb (3)
lib/uploadcare.rb (1)
configuration(23-25)lib/uploadcare/resources/base_resource.rb (1)
assign_attributes(20-25)lib/uploadcare/configuration.rb (1)
cdn_base(120-128)
🔇 Additional comments (6)
examples/batch_upload.rb (2)
7-26: LGTM!The configuration setup follows the same pattern as other examples, with proper dotenv handling and configuration validation.
28-43: LGTM!Input validation is thorough with clear error messages.
examples/simple_upload.rb (2)
7-35: LGTM!Configuration and input validation follow the established pattern and handle edge cases appropriately.
41-58: LGTM!The block form of
File.openproperly ensures the file handle is closed even if an exception occurs during upload. This correctly addresses the previous review feedback about resource leaks.examples/upload_with_progress.rb (2)
7-49: LGTM!The setup, configuration, and user guidance about the 10MB threshold for progress tracking are clear and helpful.
51-99: LGTM!The block form of
File.openproperly ensures resource cleanup, addressing the previous review feedback. The progress tracking implementation is well-designed with:
- Safe division-by-zero checks
- Clear visual progress bar
- Helpful metrics (speed, ETA, parts)
| # Open all files | ||
| files = file_paths.map { |path| File.open(path, 'rb') } | ||
|
|
||
| begin | ||
| # Upload all files | ||
| results = Uploadcare::Uploader.upload(files, store: true) | ||
|
|
||
| # Close files | ||
| files.each(&:close) | ||
|
|
||
| # Display results | ||
| puts '✓ Batch upload complete!' | ||
| puts | ||
| puts 'Results:' | ||
| puts '-' * 50 | ||
|
|
||
| results.each_with_index do |file, index| | ||
| puts "#{index + 1}. #{file.original_filename}" | ||
| puts " UUID: #{file.uuid}" | ||
| puts " CDN URL: https://ucarecdn.com/#{file.uuid}/" | ||
| puts | ||
| end | ||
|
|
||
| puts "Successfully uploaded #{results.length} files" | ||
| rescue StandardError => e | ||
| files.each(&:close) | ||
| puts "✗ Batch upload failed: #{e.message}" | ||
| exit 1 | ||
| end |
There was a problem hiding this comment.
Resource leak: file handles not guaranteed to close on exception.
Files are opened on line 50 outside the begin block. If an exception occurs between line 50 and line 52, the file handles will leak. Additionally, closing files in both success and rescue paths (lines 57 and 74) is error-prone.
Apply this diff to ensure proper cleanup using an ensure block:
-# Open all files
-files = file_paths.map { |path| File.open(path, 'rb') }
-
begin
+ # Open all files
+ files = file_paths.map { |path| File.open(path, 'rb') }
+
# Upload all files
results = Uploadcare::Uploader.upload(files, store: true)
- # Close files
- files.each(&:close)
-
# Display results
puts '✓ Batch upload complete!'
puts
puts 'Results:'
puts '-' * 50
results.each_with_index do |file, index|
puts "#{index + 1}. #{file.original_filename}"
puts " UUID: #{file.uuid}"
puts "CDN URL: https://ucarecdn.com/#{file.uuid}/"
puts
end
puts "Successfully uploaded #{results.length} files"
rescue StandardError => e
- files.each(&:close)
puts "✗ Batch upload failed: #{e.message}"
exit 1
+ensure
+ files&.each(&:close)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Open all files | |
| files = file_paths.map { |path| File.open(path, 'rb') } | |
| begin | |
| # Upload all files | |
| results = Uploadcare::Uploader.upload(files, store: true) | |
| # Close files | |
| files.each(&:close) | |
| # Display results | |
| puts '✓ Batch upload complete!' | |
| puts | |
| puts 'Results:' | |
| puts '-' * 50 | |
| results.each_with_index do |file, index| | |
| puts "#{index + 1}. #{file.original_filename}" | |
| puts " UUID: #{file.uuid}" | |
| puts " CDN URL: https://ucarecdn.com/#{file.uuid}/" | |
| puts | |
| end | |
| puts "Successfully uploaded #{results.length} files" | |
| rescue StandardError => e | |
| files.each(&:close) | |
| puts "✗ Batch upload failed: #{e.message}" | |
| exit 1 | |
| end | |
| begin | |
| # Open all files | |
| files = file_paths.map { |path| File.open(path, 'rb') } | |
| # Upload all files | |
| results = Uploadcare::Uploader.upload(files, store: true) | |
| # Display results | |
| puts '✓ Batch upload complete!' | |
| puts | |
| puts 'Results:' | |
| puts '-' * 50 | |
| results.each_with_index do |file, index| | |
| puts "#{index + 1}. #{file.original_filename}" | |
| puts " UUID: #{file.uuid}" | |
| puts " CDN URL: https://ucarecdn.com/#{file.uuid}/" | |
| puts | |
| end | |
| puts "Successfully uploaded #{results.length} files" | |
| rescue StandardError => e | |
| puts "✗ Batch upload failed: #{e.message}" | |
| exit 1 | |
| ensure | |
| files&.each(&:close) | |
| end |
🤖 Prompt for AI Agents
In examples/batch_upload.rb around lines 49 to 77, file handles are opened
before the begin block so an exception can leak them and closing is duplicated
in success and rescue paths; fix by moving the creation of the files array into
the begin block (or wrap the opening in a begin) and add an ensure block that
closes all file handles (files.each(&:close) guarded by nil/empty check) so
files are always closed once regardless of success or error, and remove the
duplicated close from the rescue/success sections.
| # Loads file metadata, if it's initialized with url or uuid (v4.4.3 compatibility) | ||
| # @return [Uploadcare::File] | ||
| def load | ||
| response = info | ||
| assign_attributes(response.instance_variables.each_with_object({}) do |var, hash| | ||
| hash[var.to_s.delete('@').to_sym] = response.instance_variable_get(var) | ||
| end) | ||
| self | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Simplify redundant logic in load method.
The info method already updates the current instance via assign_attributes (line 89) and returns self. Lines 219-221 redundantly extract instance variables from self and reassign them back to self, which has no effect since the instance was already updated by the info call.
Apply this diff to simplify:
def load
- response = info
- assign_attributes(response.instance_variables.each_with_object({}) do |var, hash|
- hash[var.to_s.delete('@').to_sym] = response.instance_variable_get(var)
- end)
- self
+ info
end🤖 Prompt for AI Agents
In lib/uploadcare/resources/file.rb around lines 215 to 223, the load method
calls info which already updates the instance via assign_attributes and returns
self, so the subsequent block that extracts instance variables from the response
and reassigns them back is redundant; remove the extraction/assignment block and
simplify load to just call info and return self (or simply return the result of
info) to eliminate the no-op reassignment.
Summary
This PR introduces a complete architectural rewrite of the Uploadcare Ruby gem, moving from an entity/client structure to a cleaner resources/clients architecture. The rewrite maintains full API compatibility while adding significant performance improvements and cleaner code organization.
Key Changes
Architecture Improvements
New Features
API Coverage Verification
All APIs from the previous version are fully covered in the new architecture:
entity/file.rbresources/file.rbentity/group.rbresources/group.rbclient/multipart_upload_client.rbclients/multipart_uploader_client.rbclient/file_list_client.rbclients/file_client.rbclients/*_converter_client.rbTesting
Test Results
Examples Verified
All example scripts have been updated and verified to work with the new architecture:
Migration Notes
The gem maintains backwards compatibility, so existing code should continue to work. However, the internal structure has changed significantly:
require 'uploadcare'entry point is unchangedPerformance Improvements
Breaking Changes
None for public APIs. Internal APIs have been completely restructured, but these were not part of the public interface.
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.