Centralized use of requests

[jblawatt]

Hi, it would be very helpful if all API calls which are made via requests would be routed through a central extendable method. With this it would be possible to inject individual authentication or other recurring options into the call. Since these calls are currently being made in many different methods of the EngineClient and the ExternalTaskClient, this is difficult to achieve.

If you think this suggestion is a good idea, I would participate in the implementation or make a pull request. I just want to make sure the idea has a change to become accepted.

Best regards and thanks for your work! Jens

[yogeshrnaik]

@jblawatt As per my understanding you are suggesting to write a class that will wrap all the outgoing calls to Camunda REST API that are currently being done through EngineClient and ExternalTaskClient. We can add generic method send_request in EngineClient and make all existing methods call this send_request method. This way we can control all common logic in send_request method.

[jblawatt]

@yogeshrnaik Exactly. We need something to wrap all outgoing calls to the Camundas REST API. I would prefer a dedicated transport class over a send_request method. This gives us more flexibility and satisfies the seperation of concerns.

Here are some python-like examples of what i have in mind:

Extend the EngineClient and ... with the backwards compatible ability to use a transport class.

class EngineClient:

    def __init__(self, engine_base_url=ENGINE_LOCAL_BASE_URL, transport_class=RequestsTransport, **kwargs):
        self.engine_base_url = engine_base_url
        self.transport = transport_class(**kwargs)

Let the Transport class do the oridnary request and repsonse / error-handling. The client is just expecting a dictionary.

    def get_process_instance(self, process_key=None, variables={}, tenant_ids={}):
        url = f"{self.engine_base_url}/process-instance"
        url_params = self.__get_process_instance_url_params(process_key, tenant_ids, variables)
        return self.transport.get(url, headers=self._get_headers(), params=url_params)

An idea of a lightweight transport class implementation:

class RequestsTransport:

    def get(self, path, **args, **kwargs) -> Dict:
        resp = self.requests.get(path, *args, **kwargs)
        raise_exception_if_not_ok(resp)
        return resp.json()

I borrowed this approach from the elasticsearch python api. This could be the template for the implementation. https://github.com/elastic/elasticsearch-py/blob/master/elasticsearch/client/init.py#L202

[jblawatt]

I created a draft pull request. Looking forward to your feedback.

[fliot]

I may have not understood your purpose, sorry, probably my bad, But reviewing the code of your merge request, I do not see anything that would support use of basic authentication on with engine-rest service., And I was thinking it was your target... I'm pretty sure I missed something, please explain. Thanks in advance.

[jblawatt]

Hey @fliot. Thanks for your feedback!

The purpose of my implementation was to get an extensable/changeable layer between the logic of handling external task thgrough EngineClient and ExternalTaskClient and the resulting HTTP call executed by utilizing requests.

This seperates the concerns and provides an opportunity to implement custom transport:

• adding custom headers without subclassing oder monkeypatching the EngineClient or ExternalTaskClient
• add a different way to authenticate (not just Basic Auth)
• disable verifiing the server TLS certificate
• add custom certificat validation
• not have to add basic auth credentials to the url
• use requests session
• a.s.o

Essentially, getting more control of the way to execute the request. (https://docs.python-requests.org/en/latest/api/#requests.request)

I tried to add this layer without breaking backwards compatibility.

To acieve these i implemented a tansport class which handles the execution and serializers which transforms the result into the expected value (JSON, Boolean, usw.).

To hopefully not break the compatibility with older verions, there is provided a transport_class Argument which is used per default. Otherwise you could now add a custom instance or class which will be used.

I hope this helps understanding my target.

Looking forward to your reply. Greets

[mstuebner]

@yogeshrnaik After reading this thread I also see the benefit of this implementation, but this thread seems to be lost?

[jblawatt]

@yogeshrnaik @mstuebner

I'm still interesseted in having this feature. I closed my merge request because it got a litte outdated. But upcoming projects of mine could need this. So i would like to contribute it instead of overriding or monkeypatching it all.

So if you are also interesset in it i will refactor the change and go for another MR.

@mstuebner Did my first attempt fit all your needs or is there some further change in your mind?

Greets

[mstuebner]

@jblawatt Hi Jens, I do not have a current need for that, but the idea is very useful and elegant, thats why I would vote for it.

br Matthias

[yogeshrnaik]

@jblawatt Please go ahead and raise the PR with centralised use of requests. If possible if you can make small PRs inn following order that would be good.

1. Create new class first with centralised requests usage
2. Use the new class in existing code and start refactoring one method at a time from EngineClient class to use the new class