Is a HTTPClient appropriate?

[darrelmiller]

Having had some time to review this code and also the Python requests library I am wondering if it is wise to create our own _HTTPClient class. The Python requests library exposes a module, not a class, so the behavior we are creating is potentially not consistent with how users expect requests to work.

One of primary goals is to simplify retrofitting existing applications that are calling the Graph using requests with our library.

Looking at the example code from PR #1 our calling pattern looks like:

    from msgraph_sdk_python import HTTPClientFactory
    requests = HTTPClientFactory.with_graph_middlewares(middlewares)
    response = requests.get('/me')

If an existing application already has import requests, what happens when we call requests.get ?

From what I can tell the requests object now derives from Session, but has some of the requests api added. If we keep this approach we would need to implement them all to not break existing calls to requests.

Is there an advantage to sub-classing Session? Could the "with_graph_middlewares" not just mount the custom HttpAdapter? Perhaps it could return the session if the consumer decides they want further configure it? Perhaps it should accept an existing session if other configuration has already been done to it?

While our goal is to have similar architectural concepts across languages, the role of HttpClientFactory is to configure the native library to make HTTP requests in a way that is most suited for calling Microsoft Graph. It may not actually need to construct an instance of a HttpClient. That would be an example of us taking our foreign C# pattern and applying it to a different platform.

There is something not quite right about using a variable name to pretend that our class instance is the requests module, but then it actually be a Session and therefore you need to know to close requests, which is something a Python dev has never had to do before. There is something "uncanny valley" about this.

Thoughts? /cc @jobala @ddyett @MIchaelMainer

[ddyett]

I think what I'm reading you say is that instead of wrapping the session we just provide you our httpadapter and allow you to mount it onto your existing session object if you wish.

import requests;
from .middleware_pipeline import MiddlewarePipeline
sessions = request.Session();

sessions.mount('/me', middlewareAdapter)
session.get('/me')

[darrelmiller]

Something in that direction. We should encapsulate the graph related configuration of the Session. I think we might be able to allow making calls either from the session instance, or directly from the global requests module.

import requests
session = configure_graph_session()

requests.get('/me')
# or
session.get('/me')

[jobala]

I don't have stong opinions about how we expose our API.

Is there an advantage to sub-classing Session?
Subclassing Session allows to extend the requests module with a Graph specific use case
Ie pythonic implementation of middleware options

requests.get('/me', scopes=[...])

Instead of

request.get('/me').with_scopes([...])

I think subclassing Session is the right thing to do.

the behavior we are creating is potentially not consistent with how users expect requests to work.

The Session class exposes the same API as the requests module. Docs, A Session object has all the methods of the main Requests API.
The behaviour we are creating preserves requests' behaviour and extends it with our own behaviour like supporting middleware options.

There is something not quite right about using a variable name to pretend that our class instance is the requests module

The requests module and Session object expose the same API, they are the same thing.

[darrelmiller]

The requests module and Session object expose the same API, they are the same thing.

The requests module exposes a subset of the Session object and it auto-closes the session after every request. If closing a Session closes the TCP/IP connection, then I would be happy to make the argument to customers that they should avoid using the requests module directly so for performance reasonss.

A Session object has all the methods of the main Requests API.

This is a good justification for using the Session object as the primary native interface. I'm not sure we should have examples that create a variable called requests though. I'm prepared to defend the position that customers should use the Graph customized Session as the native interface.

Subclassing Session allows to extend the requests module with a Graph specific use case

Your example is convincing, we definitely don't want the .withXXX pattern. What do you think about calling it a GraphSession?

[jobala]

It makes sense to me. How do you want us to expose the api? I was thinking of something like

session = GraphSession(auth_provider, auth_provider_options)

[ddyett]

How do you pass in the middleware? Auth options I think is just scopes so I feel like it'd be preferable to just pass in a list of scopes.

[jobala]

We could pass middlewares through the constructor.

[MIchaelMainer]

// default MSGraph middleware with customer provided tokenCredential set on a GraphSession(Session) object. I assume we will support the same authentication options as the Azure SDK.
graphSession = MSGraphClientFactory.with_default_middleware(tokenCredential) 
// overwrite default middleware with custom middleware and customer provided tokenCredential set on a GraphSession(Session) object.
graphSession = MSGraphClientFactory.with_custom_middleware(tokenCredential, customMiddleWare) 

I like the GraphSession although I think we should include a default constructor (no args) for GraphSession and encourage the use of the client factory to create and configure the GraphSession with middleware. This provides us a way to air our opinion over the middleware pipeline so we can enforce rules like order of middleware (including final handler), determine and express the existence of custom middleware (for debugging purposes), and validate/manage base URLs. Being that this is Python, customers could go their own way reassign the middleware with their own unvalidated middleware.

Also, can remove the 's' from "middlewares" as middleware is already plural.

[jobala]

@MIchaelMainer we are avoiding the .with_x_middleware pattern.

We can achieve the same thing with

# Uses default middleware
graph_session = GraphSession(token_credential)

and

# Uses customer middleware
graph_session = GraphSession(token_credential, middleware)

[MIchaelMainer]

I take it that it means that we get rid of the factory, is that correct?

[jobala]

Yes.