Description

nitram509

opened on Oct 3, 2024

Issue body actions

Feature request for Cloudinary Java SDK

General idea: making the settings of http client library to be used easier for consumers of the SDK,
by enabling configuration via new Cloudinary(...) constructor.

Explain your use case

I want to use plain java http client, instead of Apache one.

Describe the problem you’re trying to solve

In Cloudinary.loadStrategies() there's a fixed list of strings with classnames, which a consumer of the SDK can't change.
This means even if I provide my own strategy, this will not loaded and I still being forced to add dependency for apache http.
(hint: technically: when the class om.cloudinary.http5.ApiStrategy is not found, an UnknownError is thrown, which is bad)

Also this brings me to another point: the constructor of Cloudinary should better accept Configuration object, instead of Map config. It's very annoying for me as an SDK user, to provide a Map with configs.
I get no auto-completion on Maps and must find details in either source code or documentation (which is just cumbersome).
So I don't know immediately what config options are available, that I could e.g. provide HTTP proxy address.

(Sorry, if I mix two problems here ... I came to these two, when I thought about a solution proposal, below)

Do you have a proposed solution?

YES.
The constructor of Cloudinary.class (or better the Configuration.class) could have some new field apiStrategy of type class<AbstractApiStrategy>. And another one for the UploadStrategy.
Then, the loadStragies() method will not load by name (which can contain typos), but rather by class(type) directly.

On the second problem, adding a new (overloaded) constructor to Cloudinary is easy to add, but that's not sufficient.
Ideally, the Configuration class will provide a convenient Builder function, so users of the SDK are better guided,
and plausibility checks can easier be implemented.

I'm happy to provide a PR, if this resonates with you.

Activity

1. ManishS6 commented on Oct 4, 2024

   

   ManishS6

   on Oct 4, 2024

   More actions

   Hi @nitram509 , I would like to work on this. Please assign.
2. nitram509 commented on Oct 4, 2024

   

   nitram509

   on Oct 4, 2024

   Author

   More actions

   @ManishS6 I'm not a maintainer ... just reporting issues here as well and also would like to work on it ;)
3. ManishS6 commented on Oct 15, 2024

   

   ManishS6

   on Oct 15, 2024

   More actions

   oh my bad @nitram509
   good luck
4. campbellbartlett commented on Jan 15, 2025

   

   campbellbartlett

   on Jan 15, 2025

   More actions

   I agree that this is a significant issue with the SDK, particularly now that the recent change has broken integration with HTTP Proxies (#348).

   If the SDK allowed us to supply our own HttpClient, we could easily work around #348. However, the unusual decision to rely on the StrategyLoader pattern prevents SDK users from customizing the HTTP client.
5. campbellbartlett commented on Jan 15, 2025

   

   campbellbartlett

   on Jan 15, 2025

   More actions

   I've had a crack at getting something to work that addresses this issue, you can view the changes in this PR.

   Assuming the maintainers at Cloudinary are happy with my approach, once they've had a look and I've addressed any comments they have, I'll open a PR against the master branch in this fork of the repo so it can me merged and released.
6. campbellbartlett commented on Jan 16, 2025

   

   campbellbartlett

   on Jan 16, 2025

   More actions

   @adimiz1 I see two new PRs have been opened in this repository with some fixes for this issue.
   My 2c on continuing down the path of using the StrategyLoader to instantiate the API/Uploader strategies using reflection:

   From my experience working with the SDK, the current StrategyLoader approach has several significant drawbacks:

   1. Type-unsafe Strategy References
      The requirement to specify fully qualified class names as strings is error-prone and bypasses compile-time checks. Typos or incorrect class paths can only be detected at runtime, violating type safety principles.

   2. Constructor Constraints
      Forcing strategies to use no-arg constructors is particularly problematic in modern applications that rely on dependency injection. For instance, we can't properly inject an HTTP client through the constructor, which is standard practice for managing dependencies.

   3. Implementation Complexity
      While the current reflection-based loading mirrors patterns seen in legacy Spring XML configurations, it creates unnecessary complexity for SDK users. Modern Java developers expect straightforward constructor-based initialization rather than having to understand internal reflection mechanics.

   My Recommendation would be to consider moving to a constructor-based approach that:

   • Leverages compile-time type checking
   • Supports proper dependency injection
   • Provides a more intuitive API for developers new to the SDK

   This would align better with current Java best practices and improve the developer experience.

   You can see an example of how this might be achieved here: campbellbartlett#1. This introduces no new dependencies and continues to allow for different Strategies between Android and non-Android clients. There is one small breaking change in that the class that users of the SDK instantiate is no longer just called Cloudinary but is now called CloudinaryClient, however if this is a major issue we can rename to avoid this problem.

Issue actions