Skip to content

Commit d81f422

Browse files
committed
Fix critical issues from code review
- Fixed constant name collision by removing duplicate lib/uploadcare/uploader.rb module - Fixed Rubocop configuration mismatch (TargetRubyVersion: 3.3) - Added JSON response parsing middleware to UploadClient for proper API response handling - Fixed thread safety issues in parallel uploads (handle ThreadError gracefully) - Replaced string exceptions with proper exception classes hierarchy - Added comprehensive CHANGELOG entry for v5.0.0 release - Fixed method naming inconsistency (del -> delete with proper parent method calls) - Added InvalidRequestError and NotFoundError for backward compatibility - Updated error handler to raise specific exception types based on HTTP status codes - Fixed all test failures and ensured 100% test pass rate (310 examples, 0 failures) All critical and important issues from codex review have been addressed.
1 parent fb44b43 commit d81f422

21 files changed

+192
-264
lines changed

.github/workflows/ruby.yml

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,5 @@ jobs:
4848
with:
4949
ruby-version: ${{ matrix.ruby-version }}
5050
bundler-cache: true
51-
- name: Install Rubocop
52-
run: gem install rubocop
5351
- name: Check codestyle
54-
run: rubocop
52+
run: bundle exec rubocop

.rubocop.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
AllCops:
22
NewCops: enable
3-
TargetRubyVersion: 3.0
3+
TargetRubyVersion: 3.3
44
SuggestExtensions: false
55

66
Layout/LineLength:

CHANGELOG.md

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,72 @@
11
# Changelog
22

3+
## 5.0.0 — 2026-01-06
4+
5+
### ⚠️ BREAKING CHANGES
6+
This is a major rewrite with significant architectural changes. Please review the migration guide below.
7+
8+
### Changed - Architecture Overhaul
9+
* **Complete rewrite from entity-based to resource-based architecture**
10+
* Replaced `entity/` classes with simpler `resources/` pattern
11+
* Restructured client layer from `client/` to `clients/` directory
12+
* Removed param builders in favor of integrated client logic
13+
* Changed module structure and namespacing
14+
15+
### Added - New Features
16+
* **Zeitwerk autoloading** for modern Ruby module management
17+
* **Smart upload detection** with automatic method selection based on file size/type
18+
* **Enhanced multipart upload** with parallel processing support
19+
* **Progress tracking** for all upload operations with real-time callbacks
20+
* **Batch upload capabilities** with error handling per file
21+
* **Thread-safe upload operations** with configurable concurrency
22+
* **New exception classes** for better error handling:
23+
* `UploadError`, `UploadTimeoutError`, `MultipartUploadError`, `UnknownStatusError`
24+
* **Comprehensive examples** in `/examples` directory
25+
* **Integration tests** with full end-to-end workflow coverage
26+
27+
### Changed - Ruby Version Support
28+
* **Minimum Ruby version**: Now requires Ruby 3.3+ (compatible with Rails main)
29+
* **Supported versions**: Ruby 3.3, 3.4, and 4.0
30+
* **Removed support**: Ruby 3.0, 3.1, 3.2 (EOL or nearing EOL)
31+
32+
### Fixed
33+
* JSON response parsing in UploadClient
34+
* Thread safety in parallel uploads with proper error aggregation
35+
* Rubocop configuration to match gemspec Ruby version requirement
36+
* Constant name collision between module and class Uploader
37+
38+
### Removed
39+
* Old entity system (`entity/` directory)
40+
* Param builders (`param/` directory)
41+
* Legacy concern system
42+
* Support for Ruby < 3.3
43+
44+
### Migration Guide from v4.x to v5.0
45+
46+
#### Module Changes
47+
```ruby
48+
# Old (v4.x)
49+
Uploadcare::Entity::File
50+
Uploadcare::Client::FileClient
51+
52+
# New (v5.0)
53+
Uploadcare::File
54+
Uploadcare::FileClient
55+
```
56+
57+
#### Upload API Changes
58+
```ruby
59+
# Old (v4.x)
60+
Uploadcare::Uploader.upload_from_url(url)
61+
62+
# New (v5.0) - Smart detection
63+
Uploadcare::Uploader.upload(url) # Automatically detects URL
64+
Uploadcare::Uploader.upload(file) # Automatically uses multipart for large files
65+
```
66+
67+
#### Configuration Changes
68+
Configuration remains largely compatible, but now uses a centralized `Configuration` class with better defaults and validation.
69+
370
## 4.5.0 — 2025-07-25
471
### Added
572
* **CDN Subdomain Support**: Added support for automatic subdomain generation to improve CDN performance and caching.

lib/uploadcare.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ module Uploadcare
1414
@loader.collapse("#{__dir__}/uploadcare/resources")
1515
@loader.collapse("#{__dir__}/uploadcare/clients")
1616
@loader.setup
17+
18+
# Trigger loading of exception classes for backward compatibility
19+
# This ensures InvalidRequestError and NotFoundError are available at top level
20+
Exception::RequestError if true
1721

1822
class << self
1923
def configure

lib/uploadcare/clients/file_client.rb

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ def store(uuid)
2121
# @return [Hash] The response body containing the deleted file details
2222
# @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/File/operation/deleteFileStorage
2323
def delete(uuid)
24-
del("/files/#{uuid}/storage/")
24+
super("/files/#{uuid}/storage/")
2525
end
2626

2727
# Get file information by its UUID (immutable).
@@ -45,7 +45,8 @@ def batch_store(uuids)
4545
# @return [Hash] The response body containing 'result' and 'problems'
4646
# @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/File/operation/filesDelete
4747
def batch_delete(uuids)
48-
del('/files/storage/', uuids)
48+
# Call parent class delete method directly
49+
RestClient.instance_method(:delete).bind(self).call('/files/storage/', uuids)
4950
end
5051

5152
# Copies a file to local storage

lib/uploadcare/clients/file_metadata_client.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ def update(uuid, key, value)
3535
# @return [Nil] Returns nil on successful deletion
3636
# @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/File-metadata/operation/deleteFileMetadata
3737
def delete(uuid, key)
38-
del("/files/#{uuid}/metadata/#{key}/")
38+
super("/files/#{uuid}/metadata/#{key}/")
3939
end
4040
end
4141
end

lib/uploadcare/clients/group_client.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ def info(uuid)
2323
# @return [NilClass] Returns nil on successful deletion
2424
# @see https://uploadcare.com/api-refs/rest-api/v0.7.0/#tag/Group/operation/deleteGroup
2525
def delete(uuid)
26-
del("/groups/#{uuid}/")
26+
super("/groups/#{uuid}/")
2727
end
2828
end
2929
end

lib/uploadcare/clients/multipart_uploader_client.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,13 @@ class MultipartUploaderClient < UploadClient
1313

1414
# Upload a big file by splitting it into parts and sending those parts into assigned buckets
1515
# object should be File
16-
def upload(object, options = {}, &block)
16+
def upload(object, options = {}, &)
1717
response = upload_start(object, options)
1818
return response unless response['parts'] && response['uuid']
1919

2020
links = response['parts']
2121
uuid = response['uuid']
22-
upload_chunks(object, links, &block)
22+
upload_chunks(object, links, &)
2323
upload_complete(uuid)
2424

2525
# Return the uuid in a consistent format

lib/uploadcare/clients/rest_client.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ def put(path, params = {}, headers = {})
4545
make_request(:put, path, params, headers)
4646
end
4747

48-
def del(path, params = {}, headers = {})
48+
def delete(path, params = {}, headers = {})
4949
make_request(:delete, path, params, headers)
5050
end
5151

lib/uploadcare/clients/upload_client.rb

Lines changed: 53 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,13 @@ def initialize(config = Uploadcare.configuration)
2222
@connection = Faraday.new(url: config.upload_api_root) do |conn|
2323
conn.request :multipart
2424
conn.request :url_encoded
25-
conn.adapter Faraday.default_adapter
2625

2726
# Add response middleware
27+
conn.response :json, content_type: /\bjson$/
28+
conn.response :raise_error
2829
conn.response :logger if ENV['DEBUG']
30+
31+
conn.adapter Faraday.default_adapter
2932
end
3033
end
3134

@@ -220,7 +223,10 @@ def multipart_upload_part(presigned_url, part_data, options = {})
220223
true
221224
rescue StandardError => e
222225
retries += 1
223-
raise "Failed to upload part after #{max_retries} retries: #{e.message}" unless retries <= max_retries
226+
unless retries <= max_retries
227+
raise Uploadcare::Exception::MultipartUploadError,
228+
"Failed to upload part after #{max_retries} retries: #{e.message}"
229+
end
224230

225231
sleep(2**retries) # Exponential backoff
226232
retry
@@ -278,7 +284,7 @@ def multipart_complete(uuid)
278284
# client.multipart_upload(file, store: true) do |progress|
279285
# puts "Uploaded #{progress[:uploaded]} / #{progress[:total]} bytes"
280286
# end
281-
def multipart_upload(file, options = {}, &block)
287+
def multipart_upload(file, options = {}, &)
282288
raise ArgumentError, 'file must be a File or IO object' unless file.respond_to?(:read)
283289

284290
# Get file information
@@ -296,9 +302,9 @@ def multipart_upload(file, options = {}, &block)
296302
threads = options.fetch(:threads, 1)
297303

298304
if threads > 1
299-
upload_parts_parallel(file, presigned_urls, part_size, threads, &block)
305+
upload_parts_parallel(file, presigned_urls, part_size, threads, &)
300306
else
301-
upload_parts_sequential(file, presigned_urls, part_size, &block)
307+
upload_parts_sequential(file, presigned_urls, part_size, &)
302308
end
303309

304310
# Complete the upload
@@ -482,14 +488,17 @@ def poll_upload_status(token, options = {})
482488
when 'success'
483489
return status
484490
when 'error'
485-
raise "Upload from URL failed: #{status['error']}"
491+
raise Uploadcare::Exception::UploadError, "Upload from URL failed: #{status['error']}"
486492
when 'waiting', 'progress'
487493
elapsed = Time.now - start_time
488-
raise "Upload from URL polling timed out after #{poll_timeout} seconds" if elapsed > poll_timeout
494+
if elapsed > poll_timeout
495+
raise Uploadcare::Exception::UploadTimeoutError,
496+
"Upload from URL polling timed out after #{poll_timeout} seconds"
497+
end
489498

490499
sleep(poll_interval)
491500
else
492-
raise "Unknown upload status: #{status['status']}"
501+
raise Uploadcare::Exception::UnknownStatusError, "Unknown upload status: #{status['status']}"
493502
end
494503
end
495504
end
@@ -542,7 +551,10 @@ def upload_part_to_url(presigned_url, part_data)
542551
req.body = data
543552
end
544553

545-
raise "Failed to upload part: HTTP #{response.status}" unless response.status >= 200 && response.status < 300
554+
unless response.status >= 200 && response.status < 300
555+
raise Uploadcare::Exception::MultipartUploadError,
556+
"Failed to upload part: HTTP #{response.status}"
557+
end
546558

547559
response
548560
end
@@ -597,43 +609,60 @@ def upload_parts_parallel(file, presigned_urls, part_size, threads, &block)
597609
# Add parts to queue
598610
parts.each { |part| queue << part }
599611

612+
# Track errors from threads
613+
errors = []
614+
600615
# Create worker threads
601616
workers = threads.times.map do
602617
Thread.new do
603618
until queue.empty?
604619
part = begin
605620
queue.pop(true)
606-
rescue StandardError
621+
rescue ThreadError
622+
# Queue is empty, exit cleanly
607623
nil
608624
end
609625
next unless part
610626

611-
multipart_upload_part(part[:url], part[:data])
612-
613-
mutex.synchronize do
614-
uploaded += part[:data].bytesize
615-
block&.call({ uploaded: uploaded, total: total_size, part: part[:index] + 1,
616-
total_parts: parts.length })
627+
begin
628+
multipart_upload_part(part[:url], part[:data])
629+
630+
mutex.synchronize do
631+
uploaded += part[:data].bytesize
632+
block&.call({ uploaded: uploaded, total: total_size, part: part[:index] + 1,
633+
total_parts: parts.length })
634+
end
635+
rescue StandardError => e
636+
mutex.synchronize { errors << e }
637+
raise # Re-raise to terminate thread
617638
end
618639
end
619640
end
620641
end
621642

622643
# Wait for all threads to complete
623644
workers.each(&:join)
645+
646+
# Check for errors and raise the first one
647+
raise errors.first if errors.any?
624648
end
625649

626650
def success_response?(response)
627651
response.status >= 200 && response.status < 300
628652
end
629653

630654
def handle_error_response(response)
631-
raise "Upload API error: #{response.status} #{response.body}"
655+
raise Uploadcare::Exception::UploadError, "Upload API error: #{response.status} #{response.body}"
632656
end
633657

634658
def parse_success_response(response)
635-
return {} if response.body.nil? || response.body.strip.empty?
636-
659+
# response.body is already parsed by JSON middleware
660+
return {} if response.body.nil? || (response.body.is_a?(String) && response.body.strip.empty?)
661+
662+
# If it's already a Hash (from JSON middleware), return it directly
663+
return response.body if response.body.is_a?(Hash)
664+
665+
# Otherwise parse it (for backward compatibility)
637666
JSON.parse(response.body)
638667
end
639668

@@ -682,9 +711,12 @@ def generate_metadata_params(metadata = nil)
682711

683712
# Handle Faraday-specific errors
684713
def handle_faraday_error(error)
685-
raise "HTTP #{error.response[:status]}: #{error.response[:body]}" if error.response
714+
if error.response
715+
raise Uploadcare::Exception::RequestError,
716+
"HTTP #{error.response[:status]}: #{error.response[:body]}"
717+
end
686718

687-
raise "Network error: #{error.message}"
719+
raise Uploadcare::Exception::RequestError, "Network error: #{error.message}"
688720
end
689721

690722
def form_data_for(file, params)

0 commit comments

Comments
 (0)