Correctly merge signature and expire params in UploadParamsGenerator #184

geeosh · 2025-12-17T13:19:05Z

Fix: Correctly merge signature and expire params in UploadParamsGenerator

Problem

When sign_uploads is enabled, UploadParamsGenerator incorrectly assigned the entire signature hash to the 'signature' key instead of merging both signature and expire into the params hash.

Before (buggy code):

{
  'UPLOADCARE_PUB_KEY' => Uploadcare.config.public_key,
  'UPLOADCARE_STORE' => store_value(options[:store]),
  'signature' => (Upload::SignatureGenerator.call if Uploadcare.config.sign_uploads)
}.merge(metadata(options)).compact

This resulted in:

{
  'UPLOADCARE_PUB_KEY' => '...',
  'UPLOADCARE_STORE' => '...',
  'signature' => { signature: '...', expire: 1234567890 }
}

Instead of:

{
  'UPLOADCARE_PUB_KEY' => '...',
  'UPLOADCARE_STORE' => '...',
  'signature' => '...',
  'expire' => 1234567890
}

Solution

Changed the code to merge the signature hash returned by SignatureGenerator.call:

After (fixed code):

{
  'UPLOADCARE_PUB_KEY' => Uploadcare.config.public_key,
  'UPLOADCARE_STORE' => store_value(options[:store])
}.merge(
  Uploadcare.config.sign_uploads ? Upload::SignatureGenerator.call : {}
).merge(metadata(options)).compact

This ensures both signature and expire are merged into the params hash when sign_uploads is enabled.

Additional Fix

Also updated SignatureGenerator to return string keys ('signature' and 'expire') instead of symbol keys to match the rest of the params hash.

Testing

Updated test in upload_params_generator_spec.rb to verify both signature and expire are present when sign_uploads is enabled
All existing tests pass

Checklist

Tests (if applicable)
Documentation (if applicable)
Changelog stub (or use conventional commit messages)

Summary by CodeRabbit

Bug Fixes
- Improved upload signature generation and validation to ensure consistent formatting and proper handling.
Tests
- Added test coverage for upload signature functionality verification.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-17T13:19:16Z

Walkthrough

The changes modify the signature generation and upload parameter handling: SignatureGenerator.call now returns a hash with string keys instead of symbol keys, and UploadParamsGenerator refactors signature inclusion to use a merge pattern. A test is added to verify signature values are included when signing is enabled.

Changes

Cohort / File(s)	Summary
Signature Generation `lib/uploadcare/param/upload/signature_generator.rb`, `lib/uploadcare/param/upload/upload_params_generator.rb`	`SignatureGenerator.call` return value keys changed from symbols (`:signature`, `:expire`) to strings (`'signature'`, `'expire'`). `UploadParamsGenerator` refactored to merge signature conditionally as a separate expression instead of inline, adapting to the new key structure.
Tests `spec/uploadcare/param/upload/upload_params_generator_spec.rb`	New test case added to verify that upload params include signature and expire values when `sign_uploads` is enabled.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–15 minutes

Verify the merge logic in UploadParamsGenerator produces equivalent results and maintains correct behavior when sign_uploads is disabled (empty hash case).
Confirm the hash key type change from symbols to strings does not break any callers downstream expecting symbol keys.
Ensure the new test case adequately covers the signature inclusion scenario.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the main change: fixing how signature and expire parameters are merged in UploadParamsGenerator. It directly reflects the bug fix described in the PR objectives and the code changes.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)

JIRA integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between ca841a7 and 44415b4.

📒 Files selected for processing (3)

lib/uploadcare/param/upload/signature_generator.rb (1 hunks)
lib/uploadcare/param/upload/upload_params_generator.rb (1 hunks)
spec/uploadcare/param/upload/upload_params_generator_spec.rb (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

spec/uploadcare/param/upload/upload_params_generator_spec.rb (4)

lib/uploadcare/param/upload/signature_generator.rb (2)

call (9-21)

call (12-20)

lib/uploadcare/param/upload/upload_params_generator.rb (2)

call (9-39)

call (12-19)

lib/uploadcare/param/authentication_header.rb (2)

call (11-39)

call (13-23)

lib/uploadcare/param/user_agent.rb (2)

call (8-19)

call (13-18)

lib/uploadcare/param/upload/signature_generator.rb (1)

lib/uploadcare/param/secure_auth_header.rb (1)

signature (26-31)

🔇 Additional comments (2)

lib/uploadcare/param/upload/upload_params_generator.rb (1)

15-18: LGTM! Merge pattern correctly fixes the signature nesting bug.

The refactored merge pattern properly flattens the signature and expire keys into the params hash instead of nesting them under a 'signature' key. This aligns with the Uploadcare API requirements and maintains consistency with other string-keyed parameters.

lib/uploadcare/param/upload/signature_generator.rb (1)

17-18: LGTM! String keys ensure consistency with the params structure.

Changing from symbol keys to string keys aligns with the other parameters in UploadParamsGenerator (which uses 'UPLOADCARE_PUB_KEY', 'UPLOADCARE_STORE', etc.) and enables proper merging into the params hash.

coderabbitai · 2025-12-17T13:22:39Z

spec/uploadcare/param/upload/upload_params_generator_spec.rb

+        it 'generates upload params with signature if sign_uploads is true' do
+          Uploadcare.config.sign_uploads = true
+          params = subject.call
+          expect(params['signature']).not_to be_nil
+          expect(params['expire']).not_to be_nil
+        end


⚠️ Potential issue | 🟡 Minor

Restore config state after test to prevent test pollution.

The test modifies the global Uploadcare.config.sign_uploads setting but doesn't restore it afterward, which could affect other tests running in the same suite.

Apply this diff to ensure the config is restored:

it 'generates upload params with signature if sign_uploads is true' do + original_value = Uploadcare.config.sign_uploads Uploadcare.config.sign_uploads = true params = subject.call expect(params['signature']).not_to be_nil expect(params['expire']).not_to be_nil +ensure + Uploadcare.config.sign_uploads = original_value 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.

Suggested change

it 'generates upload params with signature if sign_uploads is true' do

Uploadcare.config.sign_uploads = true

params = subject.call

expect(params['signature']).not_to be_nil

expect(params['expire']).not_to be_nil

end

it 'generates upload params with signature if sign_uploads is true' do

begin

original_value = Uploadcare.config.sign_uploads

Uploadcare.config.sign_uploads = true

params = subject.call

expect(params['signature']).not_to be_nil

expect(params['expire']).not_to be_nil

ensure

Uploadcare.config.sign_uploads = original_value

end

end

🤖 Prompt for AI Agents

In spec/uploadcare/param/upload/upload_params_generator_spec.rb around lines 20 to 25, the test mutates the global Uploadcare.config.sign_uploads without restoring it; wrap the change so the original value is saved before setting sign_uploads = true and restored after the example (use an ensure block inside the example or an RSpec around/after hook) so other tests are not polluted — save original = Uploadcare.config.sign_uploads, set Uploadcare.config.sign_uploads = true, run the subject.call expectations, then ensure Uploadcare.config.sign_uploads = original.

Copilot

Pull request overview

This PR fixes a bug in UploadParamsGenerator where the signature hash was being incorrectly assigned to a single 'signature' key instead of being merged into the params hash. The fix ensures that when sign_uploads is enabled, both 'signature' and 'expire' keys are properly included in the params hash.

Key changes:

Modified UploadParamsGenerator to merge the signature hash instead of assigning it to a single key
Updated SignatureGenerator to return string keys instead of symbol keys for consistency
Added test coverage to verify signature and expire params are present when sign_uploads is enabled

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
lib/uploadcare/param/upload/upload_params_generator.rb	Fixed the merging of signature hash into params by using conditional merge instead of assigning to 'signature' key
lib/uploadcare/param/upload/signature_generator.rb	Changed hash keys from symbols to strings ('signature', 'expire') for consistency with other params
spec/uploadcare/param/upload/upload_params_generator_spec.rb	Added test to verify signature and expire params are present when sign_uploads is enabled

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-06T13:20:05Z

lib/uploadcare/param/upload/signature_generator.rb

+            'signature' => signature,
+            'expire' => expires_at


The test for SignatureGenerator in spec/uploadcare/param/upload/signature_generator_spec.rb still expects symbol keys (:signature and :expire), but this change returns string keys. The test at line 13 defines expected_result with symbol keys and will fail with this implementation. The expected result in the test should be updated to use string keys to match this change.

Suggested change

'signature' => signature,

'expire' => expires_at

signature: signature,

expire: expires_at

Copilot · 2026-01-06T13:20:06Z

spec/uploadcare/param/upload/upload_params_generator_spec.rb

+          expect(params['signature']).not_to be_nil
+          expect(params['expire']).not_to be_nil


The test should verify that 'signature' and 'expire' have the correct types and reasonable values, not just that they are not nil. Consider adding expectations like expect(params['signature']).to be_a(String) and expect(params['expire']).to be_a(Integer) to ensure the values are of the expected types.

Copilot · 2026-01-06T13:20:06Z

spec/uploadcare/param/upload/upload_params_generator_spec.rb

+          params = subject.call
+          expect(params['signature']).not_to be_nil
+          expect(params['expire']).not_to be_nil
+        end


Consider adding a test case to verify that 'signature' and 'expire' are NOT present in the params when sign_uploads is false. This would ensure the conditional logic works correctly in both directions and prevent regression if the default behavior changes.

Fix merging of signature + expiry into params

44415b4

coderabbitai bot reviewed Dec 17, 2025

View reviewed changes

vipulnsward requested a review from Copilot January 6, 2026 13:17

Copilot started reviewing on behalf of vipulnsward January 6, 2026 13:17 View session

Copilot AI reviewed Jan 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Correctly merge signature and expire params in UploadParamsGenerator #184

Correctly merge signature and expire params in UploadParamsGenerator #184

Uh oh!

geeosh commented Dec 17, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 17, 2025 •

edited

Loading

Review ran into problems

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Dec 17, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Copilot AI Jan 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		expect(params['signature']).not_to be_nil
		expect(params['expire']).not_to be_nil

Correctly merge signature and expire params in UploadParamsGenerator #184

Are you sure you want to change the base?

Correctly merge signature and expire params in UploadParamsGenerator #184

Uh oh!

Conversation

geeosh commented Dec 17, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix: Correctly merge signature and expire params in UploadParamsGenerator

Problem

Solution

Additional Fix

Testing

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Review ran into problems

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

geeosh commented Dec 17, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 17, 2025 •

edited

Loading