Include contextual metadata when using url_for_direct_upload

jaydorsey commented 7 months ago

Brief Summary of Changes

Cloudinary has support for contextual metadata but it doesn't appear to be supported as an option during ActiveStorage::Service#url_for_direct_upload.

I want to send additional context to cloudinary when I generate the URL for direct upload, using active storage.

Example:

object.service.url_for_direct_upload(
  object.key,
  ...,
  extra_context: { company: "Rando Co." }
)

Creates this on cloudinary:

What does this PR address?

[ ] GitHub issue (Add reference - #XX)
[ ] Refactoring
[X] New feature
[ ] Bug fix
[ ] Adds more tests

Are tests included?

[X] Yes
[ ] No

Reviewer, please note:

I picked the extra_context key so it wouldn't collide with anything that I might have overlooked. We might be able to just use context with a reverse_merge of the active storage key and remove the extra_context option/key
Rails 7 has support for custom_metadata via a custom key in the metadata field.
- I briefly looked at this but I wasn't comfortable making the larger change to be forward compatible without discussion.
- Being able to support both sending structured metadata values & contextual metadata key/value pairs could be very useful.
- Storing them in #metadata[:custom][:structured] and #metadata[:custom][:contextual] makes sense in my head
- This would allow the local blob metadata to be recorded, but also the remote (at least on create)

Checklist:

I didn't run the full test suite, but tested the file I modified

[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[ ] I ran the full test suite before pushing the changes and all the tests pass.

const-cloudinary commented 6 months ago

@jaydorsey thank you for contribution!

Regarding the first bullet: extra_context is a bit redundant and hard to discover for users. It would be much better to just use context with a reverse_merge as you wrote. I don't see any danger in that.

Regarding storing structured and contextual metadata on both Cloudinary and Rails would lead to syncing issues (when it is updated on either side and that leads us to an unknown/invalid state). It's up to customer to implement the desired logic.

If I miss something please let me know.

Thanks!

jaydorsey commented 6 months ago

@jaydorsey thank you for contribution!

Regarding the first bullet: extra_context is a bit redundant and hard to discover for users. It would be much better to just use context with a reverse_merge as you wrote. I don't see any danger in that.

I agree, this does sound better.

Regarding storing structured and contextual metadata on both Cloudinary and Rails would lead to syncing issues (when it is updated on either side and that leads us to an unknown/invalid state). It's up to customer to implement the desired logic.

This also makes sense.

I updated my PR to try and reflect these changes. I also setup my system to be able to run the full test suite and the entire suite is green

const-cloudinary commented 6 months ago

@jaydorsey thank you for contribution!

cloudinary / cloudinary_gem