Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: use of an async request framework (as replacement for requests) #306

Open
cglacet opened this issue Jul 13, 2023 · 3 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@cglacet
Copy link

cglacet commented Jul 13, 2023

Is your feature request related to a problem? Please describe.
The issue is that many (most?) modern python web frameworks are working in an asynchronous fashion (ASGI). The current API you are offering is purely synchronous which make it
"impossible" to use with such frameworks.

Describe the solution you'd like
Most of the time I'm using aiohttp, its interface is very close to requests which makes it a good candidate.

Describe alternatives you've considered
I'm currently re-writing it for myself.

Remark

Also, that's a bit related so I will answer this remark from your code:

# duffel_api/client.py
class Duffel:
    """Client to the entire API"""

    def __init__(self, **kwargs):
        # TODO(nlopes): I really don't like how I've built this- we shouldn't keep
        # instantiating the HttpClient through class inheritance.
        # We should (maybe!) have a singleton pattern on HttpClient and use
        # composition in all of these instead.

        # Keep this as we use it when doing the lazy-evaluation of the different
        # clients
        self._kwargs = kwargs

I think it's not too much of a problem, the real problem is underlying. You are creating one session per init (and therefore per inherited class), the code looks a bit like this:

# duffel_api/http_client.py
class HttpClient:
    def __init__(self, access_token=None, api_url=None, api_version=None, **settings):
        self.http_session = Session()

I think you should spawn a single HTTP session for all APIs (unless there is a good reason not to). Then, you could use that unique session in all subclasses (passing it as argument to the HTTpClient). You don't even need to have a singleton for that an can simply use a lazy property directly in Duffel client, like so (the code is already using aiohttp because I've already rewrote this part):

# duffel_api/client.py
class Duffel:
    _session: Optional[aiohttp.ClientSession] = None

    def __init__(self, **kwargs):
        self._kwargs = kwargs
        self._session = None

    @property
    def http_session(self):
        if self._session is None:
            self._session = aiohttp.ClientSession()
        return self._session

    @lazy_property
    def aircraft(self):
        """Aircraft API - /air/aircraft"""
        return AircraftClient(**self._kwargs, session=self.http_session)
# duffel_api/http_client.py
class HttpClient:
    _session: aiohttp.ClientSession

    URL = "https://api.duffel.com"
    VERSION = "v1"

    def __init__(
        self,
        *,
        session: aiohttp.ClientSession,
        access_token: str = None,
        api_url: str = None,
        api_version: str = None,
        **settings,
    ):
        self._api_url = api_url or HttpClient.URL
        self._api_version = api_version or HttpClient.VERSION

        if not access_token:
            access_token = os.getenv("DUFFEL_ACCESS_TOKEN")
            if not access_token:
                raise ClientError("must set DUFFEL_ACCESS_TOKEN")

        self._headers = {
            "Authorization": f"Bearer {access_token}",
            "User-Agent": f"Duffel/{self._api_version} duffel_api_python/{version()}",
            "Accept": "application/json",
            "Duffel-Version": self._api_version,
        }
        self._settings = settings
        self.http_session = session

    async def _http_call(self, endpoint, method, query_params=None, body=None):
        request_url = self._api_url + endpoint
        request_args = dict(params=query_params, json=body, headers=self._headers, **self._settings)
        async with self.http_session.request(method, request_url, **request_args) as response:
            if response.status_code in {http_codes.ok, http_codes.created}:
                try:
                    return response.json()
                # etc.

This way, all HTTP requests will use the same session (single TCP handshake).

You could also directly offer the enduser the opportunity to start the session on its own (for example when the server starts).

@cglacet
Copy link
Author

cglacet commented Jul 13, 2023

Here is how it would look like:

client = Duffel(access_token="duffel_test_Utbb-kmsFGL44mi0Ixz8-jOpQzS7OmlkGuxfxX-F4rQ")
client.start() # TCP handshake, once and for all

offer_requests = client.offer_requests.list()
async for offer_request in offer_requests:
    print(offer_request.id)

And with a more complete example (translating the one linked in the documentation):

destination = "CDG"
origin = "JFK"
departure_date = "2023-12-01"

slices = [
    {
        "origin": origin,
        "destination": destination,
        "departure_date": departure_date,
    },
]
offer_request = await (
    client.offer_requests.create()
    .passengers([{"type": "adult"}])
    .slices(slices)
    .return_offers()
    .execute()
)
offers = offer_request.offers

for idx, offer in enumerate(offers[:5]):
    print(
        f"{idx + 1}. {offer.owner.name} flight departing at "
        + f"{offer.slices[0].segments[0].departing_at} "
        + f"{offer.total_amount} {offer.total_currency}"
    )

offer_index = 0

given_name = "Christian"
family_name = "Glacet"
dob = "1986-07-31"
title = "mr"
gender = "m"
phone_number = "+33624000097"
email = "xxxx@gmail.com"

print(f"\nHang tight! Booking offer {offer_index}...")

selected_offer = offers[int(offer_index) - 1]
payments = [
    {
        "currency": selected_offer.total_currency,
        "amount": selected_offer.total_amount,
        "type": "balance",
    }
]
passengers = [
    {
        "phone_number": phone_number,
        "email": email,
        "title": title,
        "gender": gender,
        "family_name": family_name,
        "given_name": given_name,
        "born_on": dob,
        "id": offer_request.passengers[0].id,
    }
]

order = await (
    client.orders.create()
    .payments(payments)
    .passengers(passengers)
    .selected_offers([selected_offer.id])
    .execute()
)

print("\n🎉 Flight booked. Congrats! You can start packing your (duffel?) bags")
print(f"Booking reference: {order.booking_reference}")

@nlopes
Copy link
Contributor

nlopes commented Jul 18, 2023

Hey @cglacet,

Thank you for the suggestion. I think a lot of the criticism is valid.
We won't be taking this right this moment but definitely something we'll consider.

@nlopes nlopes added the enhancement New feature or request label Jul 18, 2023
@nlopes nlopes self-assigned this Jul 18, 2023
@cglacet
Copy link
Author

cglacet commented Jul 18, 2023

Hi @nlopes, don't hesitate to tag me here in case you need help. Depending on the timing I could help you find a good solution to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants