Fix remaining review follow-ups

vipulnsward · vipulnsward · commit 3e1d35fa92f8 · 2026-03-16T18:04:10.000+05:30
diff --git a/.snyk b/.snyk
diff --git a/api_examples/support/example_helper.rb b/api_examples/support/example_helper.rb
@@ -121,6 +121,7 @@ def with_multipart_session
     yield file, response
   ensure
     file&.close
+    safe_delete_file(response['completed_uuid']) if response.is_a?(Hash) && response['completed_uuid']
   end
 
   def upload_multipart_part(file:, session:, index:)
@@ -132,11 +133,15 @@ def upload_multipart_part(file:, session:, index:)
     data.bytesize
   end
 
-  def finish_multipart_upload(file:, session:)
+  def finish_multipart_upload(file:, session:, skip_indices: [])
     session.fetch('parts').each_index do |index|
+      next if skip_indices.include?(index)
+
       upload_multipart_part(file: file, session: session, index: index)
     end
-    unwrap(client.api.upload.files.multipart_complete(uuid: session.fetch('uuid')))
+    result = unwrap(client.api.upload.files.multipart_complete(uuid: session.fetch('uuid')))
+    session['completed_uuid'] = result['uuid']
+    result
   end
 
   def uploaded_uuid_from_base_response(response)
@@ -171,9 +176,16 @@ def safe_delete_file(file)
   end
 
   def safe_delete_group(group)
-    return unless group&.id
-
-    client.api.rest.groups.delete(uuid: group.id)
+    group_id = if group.respond_to?(:id)
+                 group.id
+               elsif group.is_a?(Hash)
+                 group['id'] || group[:id]
+               else
+                 group
+               end
+    return if group_id.to_s.empty?
+
+    client.api.rest.groups.delete(uuid: group_id)
   rescue StandardError
     nil
   end
diff --git a/api_examples/support/run_rest_example.rb b/api_examples/support/run_rest_example.rb
@@ -30,7 +30,10 @@ def call
         end
       when 'post_files_local_copy.rb'
         ApiExamples::ExampleHelper.with_uploaded_file do |file|
-          client.files.copy_to_local(source: file.uuid, options: { store: true })
+          copied = client.files.copy_to_local(source: file.uuid, options: { store: true })
+          copied
+        ensure
+          ApiExamples::ExampleHelper.safe_delete_file(copied)
         end
       when 'post_files_remote_copy.rb'
         target = ENV.fetch('UPLOADCARE_REMOTE_STORAGE', nil)
diff --git a/api_examples/support/run_upload_example.rb b/api_examples/support/run_upload_example.rb
@@ -27,7 +27,10 @@ def call
         end
       when 'post_group.rb'
         ApiExamples::ExampleHelper.with_uploaded_files do |files|
-          ApiExamples::ExampleHelper.unwrap(client.api.upload.groups.create(files: files.map(&:uuid)))
+          group = ApiExamples::ExampleHelper.unwrap(client.api.upload.groups.create(files: files.map(&:uuid)))
+          group
+        ensure
+          ApiExamples::ExampleHelper.safe_delete_group(group)
         end
       when 'get_group_info.rb'
         ApiExamples::ExampleHelper.with_uploaded_group do |group, _files|
@@ -45,7 +48,11 @@ def call
       when 'put_multipart_part.rb'
         ApiExamples::ExampleHelper.with_multipart_session do |file, session|
           uploaded = ApiExamples::ExampleHelper.upload_multipart_part(file: file, session: session, index: 0)
-          completed = ApiExamples::ExampleHelper.finish_multipart_upload(file: file, session: session)
+          completed = ApiExamples::ExampleHelper.finish_multipart_upload(
+            file: file,
+            session: session,
+            skip_indices: [0]
+          )
           {
             'uuid' => session.fetch('uuid'),
             'uploaded_bytes' => uploaded,
diff --git a/lib/uploadcare.rb b/lib/uploadcare.rb
@@ -17,6 +17,7 @@ class << self
     # @yieldparam config [Uploadcare::Configuration] The configuration object
     def configure
       yield configuration if block_given?
+    ensure
       @client = nil
     end
 
diff --git a/lib/uploadcare/api/upload/files.rb b/lib/uploadcare/api/upload/files.rb
@@ -203,8 +203,8 @@ def build_from_url_params(source_url, options)
     store = store_value(options[:store])
     params['store'] = store unless store.nil?
 
-    params['check_URL_duplicates'] = options[:check_URL_duplicates].to_s if options[:check_URL_duplicates]
-    params['save_URL_duplicates'] = options[:save_URL_duplicates].to_s if options[:save_URL_duplicates]
+    params['check_URL_duplicates'] = options[:check_URL_duplicates].to_s if options.key?(:check_URL_duplicates)
+    params['save_URL_duplicates'] = options[:save_URL_duplicates].to_s if options.key?(:save_URL_duplicates)
 
     metadata_params = generate_metadata_params(options[:metadata])
     params.merge!(metadata_params) if metadata_params.any?
diff --git a/lib/uploadcare/internal/error_handler.rb b/lib/uploadcare/internal/error_handler.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'json'
+require 'time'
 
 # Handles API errors and converts them to appropriate exceptions.
 #
@@ -85,7 +86,16 @@ def catch_upload_errors(response)
   def raise_throttle_error(response, message)
     headers = response.is_a?(Hash) ? response[:headers] : nil
     retry_after = headers && (headers['retry-after'] || headers['Retry-After'])
-    timeout = retry_after.to_f
+    timeout =
+      if retry_after.to_s.match?(/\A\d+(\.\d+)?\z/)
+        retry_after.to_f
+      else
+        begin
+          [Time.httpdate(retry_after.to_s) - Time.now, 0].max
+        rescue ArgumentError
+          0
+        end
+      end
     timeout = 10.0 if timeout <= 0
     raise Uploadcare::Exception::ThrottleError.new(message, timeout: timeout)
   end
diff --git a/lib/uploadcare/operations/multipart_upload.rb b/lib/uploadcare/operations/multipart_upload.rb
@@ -42,6 +42,7 @@ def upload(file:, request_options: {}, **options, &block)
       file_size = prepared_file.size
       filename = prepared_file.original_filename
       content_type = MIME::Types.type_for(prepared_file.path).first&.content_type || 'application/octet-stream'
+      part_size, threads = normalize_upload_options(options)
 
       start_response = Uploadcare::Result.unwrap(
         upload_client.files.multipart_start(
@@ -56,9 +57,6 @@ def upload(file:, request_options: {}, **options, &block)
       uuid = start_response['uuid']
       presigned_urls = start_response['parts']
 
-      part_size = options.fetch(:part_size, config.multipart_chunk_size)
-      threads = options.fetch(:threads, 1)
-
       if threads > 1
         upload_parts_parallel(prepared_file, presigned_urls, part_size, threads, &block)
       else
@@ -77,6 +75,16 @@ def upload(file:, request_options: {}, **options, &block)
 
   private
 
+  def normalize_upload_options(options)
+    part_size = Integer(options.fetch(:part_size, config.multipart_chunk_size || CHUNK_SIZE))
+    threads = Integer(options.fetch(:threads, 1))
+
+    raise ArgumentError, 'part_size must be > 0' if part_size <= 0
+    raise ArgumentError, 'threads must be >= 1' if threads < 1
+
+    [part_size, threads]
+  end
+
   def upload_parts_sequential(file, presigned_urls, part_size, &block)
     total_size = file.respond_to?(:size) ? file.size : ::File.size(file.path)
     uploaded = 0
diff --git a/lib/uploadcare/resources/file.rb b/lib/uploadcare/resources/file.rb
@@ -307,6 +307,7 @@ def uuid
   def cdn_url
     return @url if uploadcare_cdn_url?(@url)
     return @original_file_url if uploadcare_cdn_url?(@original_file_url)
+    return nil unless uuid
 
     "#{config.cdn_base}#{uuid}/"
   end
diff --git a/spec/uploadcare/api/upload/files_spec.rb b/spec/uploadcare/api/upload/files_spec.rb
@@ -136,6 +136,30 @@
 
       expect(stub).to have_been_requested
     end
+
+    it 'preserves explicit false duplicate flags in the request params' do
+      stub = stub_request(:post, 'https://upload.uploadcare.com/from_url/')
+             .with(
+               body: hash_including(
+                 'check_URL_duplicates' => 'false',
+                 'save_URL_duplicates' => 'false'
+               )
+             )
+             .to_return(
+               status: 200,
+               body: { token: 'upload-token' }.to_json,
+               headers: { 'Content-Type' => 'application/json' }
+             )
+
+      files.from_url(
+        source_url: source_url,
+        async: true,
+        check_URL_duplicates: false,
+        save_URL_duplicates: false
+      )
+
+      expect(stub).to have_been_requested
+    end
   end
 
   describe '#from_url_status' do
diff --git a/spec/uploadcare/internal/error_handler_spec.rb b/spec/uploadcare/internal/error_handler_spec.rb
@@ -99,6 +99,21 @@ def faraday_error(status:, body:, headers: {})
           expect(e.timeout).to eq(3.0)
         end
       end
+
+      it 'parses Retry-After as an HTTP-date' do
+        future_time = (Time.now + 5).httpdate
+        error = faraday_error(
+          status: 429,
+          body: '{"detail":"Throttled"}',
+          headers: { 'Retry-After' => future_time }
+        )
+
+        begin
+          handler.handle_error(error)
+        rescue Uploadcare::Exception::ThrottleError => e
+          expect(e.timeout).to be_between(1.0, 5.5)
+        end
+      end
     end
 
     context 'with other HTTP status codes' do
diff --git a/spec/uploadcare/operations/multipart_upload_spec.rb b/spec/uploadcare/operations/multipart_upload_spec.rb
@@ -90,6 +90,24 @@
       end
     end
 
+    context 'when upload options are invalid' do
+      it 'returns a failure Result when threads is less than 1' do
+        result = uploader.upload(file: tempfile, threads: 0)
+
+        expect(result.failure?).to be(true)
+        expect(result.error).to be_a(ArgumentError)
+        expect(result.error.message).to eq('threads must be >= 1')
+      end
+
+      it 'returns a failure Result when part_size is not positive' do
+        result = uploader.upload(file: tempfile, part_size: 0)
+
+        expect(result.failure?).to be(true)
+        expect(result.error).to be_a(ArgumentError)
+        expect(result.error.message).to eq('part_size must be > 0')
+      end
+    end
+
     context 'when performing sequential upload (threads <= 1)' do
       before do
         allow(upload_files_api).to receive(:multipart_start)
diff --git a/spec/uploadcare/resources/file_spec.rb b/spec/uploadcare/resources/file_spec.rb
@@ -447,5 +447,11 @@
       )
       expect(file.cdn_url).to eq("https://ucarecdn.com/#{file_uuid}/")
     end
+
+    it 'returns nil when no CDN url or uuid is available' do
+      file = described_class.new({ 'original_file_url' => 'https://example.com/photo.jpg' }, client)
+
+      expect(file.cdn_url).to be_nil
+    end
   end
 end
diff --git a/spec/uploadcare_spec.rb b/spec/uploadcare_spec.rb
@@ -24,6 +24,21 @@
       client2 = described_class.client
       expect(client1).not_to equal(client2)
     end
+
+    it 'resets the memoized client even when the block raises' do
+      client1 = described_class.client
+
+      expect do
+        described_class.configure do |config|
+          config.public_key = 'broken'
+          raise 'boom'
+        end
+      end.to raise_error(RuntimeError, 'boom')
+
+      client2 = described_class.client
+      expect(client1).not_to equal(client2)
+      expect(client2.config.public_key).to eq('broken')
+    end
   end
 
   describe '.configuration' do
