declarativex icon indicating copy to clipboard operation
declarativex copied to clipboard

BUG: Client instances share same kwarg values of base_url, default_headers, auth etc.

Open Apxdono opened this issue 1 year ago • 1 comments

Description

If one tries to create multiple instances of same declarativex class and tries to supply different values to base_url, auth, default_headers etc. as kwargs, at runtime values from first created instance will be used to make requests within same client class.

Steps to Reproduce

Below is an example code one can run to assure that both requests will fire using client1 values in headers, query parameters. URL Resolution will not fail as well and 2nd request will be made to postman-echo as well.

# issue.py
from pprint import pprint

from declarativex import BaseClient, http, BasicAuth

PMAN_URL = "https://postman-echo.com"
INVALID_URL = "https://invalid.com"


class EchoClient(BaseClient):

    @http("get", "/get")
    def sample_get(self) -> dict:
        ...


def main():
    auth1 = BasicAuth("client1", "secret1")
    client1 = EchoClient(base_url=PMAN_URL, default_headers={
        "x-request-origin": "client1"
    }, default_query_params={
        "x-my-param": "client1-param"
    }, auth=auth1)

    result1 = client1.sample_get()

    print("Request for client1")
    pprint(result1)

    auth2 = BasicAuth("client2", "secret2")
    client2 = EchoClient(base_url=INVALID_URL, default_headers={
        "x-request-origin": "client2"
    }, default_query_params={
        "x-my-param": "client2-param"
    }, auth=auth2)

    result2 = client2.sample_get()

    print("Request for client2")
    pprint(result2)


if __name__ == "__main__":
    main()

Sample result when running above code:

$ python3 issue.py
Request for client1
{'args': {'x-my-param': 'client1-param'},
 'headers': {'authorization': 'Basic Y2xpZW50MTpzZWNyZXQx',
             'host': 'postman-echo.com',
             'x-amzn-trace-id': 'Root=1-6633c90b-6ac1e1ea74279abb77e33d6f',
             'x-forwarded-port': '443',
             'x-forwarded-proto': 'https',
             'x-request-origin': 'client1'},
 'url': 'https://postman-echo.com/get?x-my-param=client1-param'}
Request for client2
{'args': {'x-my-param': 'client1-param'},
 'headers': {'authorization': 'Basic Y2xpZW50MTpzZWNyZXQx',
             'host': 'postman-echo.com',
             'x-amzn-trace-id': 'Root=1-6633c90c-6970530d7c003a9d52c9bed2',
             'x-forwarded-port': '443',
             'x-forwarded-proto': 'https',
             'x-request-origin': 'client1'},
 'url': 'https://postman-echo.com/get?x-my-param=client1-param'}

Expected Behavior

In supplied example the 2nd request should've failed with name resolution error since "https://invalid.com" is an invalid domain.

If we substitute back the postman echo url, we should see that x-request-origin header value should be client2 and x-my-param = client2-param. Auth header values would also be different.

Actual Behavior

The blame is on models.py merge function of ClientConfiguration class.

    def merge(self, other: "ClientConfiguration") -> "ClientConfiguration":
        """
        Merge two configurations. The values of the other configuration
        take precedence over the values of this configuration.
        """
        return ClientConfiguration(
            base_url=self.base_url if self.base_url else other.base_url,
            auth=self.auth if self.auth else other.auth,
            default_query_params={
                **other.default_query_params,
                **self.default_query_params,
            },
            default_headers={**other.default_headers, **self.default_headers},
            middlewares=[*other.middlewares, *self.middlewares],
            error_mappings={**other.error_mappings, **self.error_mappings},
            proxies=merge_proxies(other.proxies, self.proxies),
        )

The code itself contradicts the comment statement in multiple ways:

  1. base_url and auth pefer current value rather than an incoming one.
  2. default_headers, default_query_params, error_mappings merge incorrectly as well. If one wants the value of other to dominate when similar keys are merged, the order of dictionary unpacking must be reversed.

On top of that it's unclear whether middlewares need to be merged at all. I'd instead provide exhaustive set inside each of the clients, even if it meant duplicating some code, but at least that way I'd leave it up with developers to decide on how they want/need to merge middlewares.

merge_proxies is a mystery as well, so will not comment on that.

Environment

  • Python version: 3.11.1+
  • DeclarativeX version: 1.6.6

P.S

@floydya I'd like to hear your thoughts. If you're ok with my arguments and suggestions, I can file a PR and fix the problem.

Apxdono avatar May 02 '24 17:05 Apxdono

Can also confirm I have recently encountered this issue. FWIW and without digging too deeply into the detail myself, @Apxdono I agree with your proposal

jcalcutt avatar Jun 17 '24 13:06 jcalcutt