Correctly merge signature and expire params in UploadParamsGenerator

[geeosh]

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]

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

• X
• Mastodon
• Reddit
• LinkedIn

_{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]

⚠️ 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.