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

Stricter typing for request parameters. #3176

Open
2 of 13 tasks
karpetrosyan opened this issue Apr 21, 2024 · 12 comments
Open
2 of 13 tasks

Stricter typing for request parameters. #3176

karpetrosyan opened this issue Apr 21, 2024 · 12 comments
Labels
user-experience Ensuring that users have a good experience using the library

Comments

@karpetrosyan
Copy link
Member

karpetrosyan commented Apr 21, 2024

Ref: encode/starlette#2534 (comment)

This issue was opened to resolve all kinds of problems where third-party packages may need to use private imports. We should definitely expose them to avoid problems like #3130 (comment).

I will include the package name along with its private imports (only starlette for now), so we can clearly identify what needs to be exposed.

Note that:

  • -> Not yet publicly exposed
  • -> Publicly exposed

We can also add other packages as well to track all of them in this issue.

Starlette

@tomchristie
Copy link
Member

tomchristie commented Apr 21, 2024

Thanks @karpetrosyan ☺️

We should definitely expose them

So... I'm not necessarily(?) convinced that's the outcome I'd like to see here.
Mostly I think these types end up obscuring the actual API and are code smells that should be squished.

For example, switching to the following* seems clearer to me...

   url : httpx.URL | str,
   headers: httpx.Header | Mapping[str, str] | Sequence[Tuple[str, str]],
   params: httpx.QueryParams | Mapping[str, str] | Sequence[Tuple[str, str]],
   ...

* useful starting point for discussion

@T-256
Copy link
Contributor

T-256 commented Apr 24, 2024

I'm curios why not just expose _types module instead?

from httpx.types import HeaderTypes, QueryParamsTypes, URLTypes

...

   url : URLTypes | str,
   headers: HeaderTypes | Mapping[str, str] | Sequence[Tuple[str, str]],
   params: QueryParamsTypes | Mapping[str, str] | Sequence[Tuple[str, str]],
   ...

@tomchristie
Copy link
Member

I'm curios why not just expose _types module instead?

Mostly I think these types end up obscuring the actual API and are code smells that should be squished.

@rbagd
Copy link

rbagd commented May 31, 2024

This is not necessarily a request for a change in public API, more of a note to add to the list. opentelemetry-instrumentation-httpx is now relying on private httpx._api.Client to instrument method calls which do not go through httpx.Client, e.g. httpx.get.

@tomchristie
Copy link
Member

Okay. That looks like a bad idea, I wouldn't do that.

@rbagd
Copy link

rbagd commented Jun 4, 2024

@tomchristie, would you have an alternative proposal on how to do it efficiently? Other than patching each one of those methods, there didn't seem to be an obvious way to work around that.

@tomchristie
Copy link
Member

I suppose the point of clarity is to be aware that we might break that.

on how to do it efficiently

I suppose it depends on exactly what you're instrumenting. The trace extension provides lots of visibility into the request/response flow, might be relevant? (Tho perhaps raise a new discussion if you'd like to talk about this further.)

@karpetrosyan
Copy link
Member Author

How else can we resolve this problem?
We are getting blocked by Starlette because it is using our private API, and we can't change it for some internal refactoring :(

@rafalkrupinski
Copy link

rafalkrupinski commented Jul 13, 2024

In my opinion types like UseClientDefault should be public as they're part of the public API and there's no way around it. I'd like my library (that wraps httpx) to be able to annotate functions properly.

Convenience types like HeaderTypes should either be public or not used in the public API. Right now they are public API - if you change them, you're changing the public API`. I find the current setup confusing.

@tomchristie
Copy link
Member

Thoughts for slimming down the type interface a little, so that we no longer use type synonyms...

HTTPX 1.0 becomes a little stricter on typing that previous versions, in these areas;

  • auth = httpx.Auth | None

    The auth = (str, str) shortcut is deprecated. Use an explicit httpx.BasicAuth(username, password).
    The auth = callable(request, response) shortcut is deprecated. Use an explicit httpx.Auth subclass.

  • timeout = httpx.Timeout | None

    The timeout = float and timeout = (float, float, float, float) shortcuts are deprecated.
    Use timeout = httpx.Timeout(60.0) for single value configurations.
    Use timeout = httpx.Timeout('inf') to disable timeouts completely.
    Use timeout = httpx.Timeout(connect=3.0, socket=3.0, pool=10.0, complete=60.0) for fine-grained configuration.

  • ssl = ssl.SSLContext | None

    The cert and verify arguments are deprecated in favour of an explicit ssl context.
    Use ssl = httpx.SSLContext(verify=..., cert=...).

  • cookies = httpx.Cookies | Dict[str, str] | CookieJar | None

  • headers = httpx.Headers | Dict[str, str] | None

  • params = httpx.QueryParams | Dict[str, str] | None

  • data = httpx.FormData | Dict[str, str] | None

  • files = httpx.FormFiles | Dict[str, httpx.UploadFile] | None

    Type coercion for non-string values is no longer supported, and will raise a type error.
    For cases requiring multiple values for a single key, use the explicit type...

    params = httpx.QueryParams([
        ('filter', 'blue'),
        ('filter', 'green'),
        ('filter', 'red')
    ])
    
  • content = httpx.UploadContent | str | bytes | None

    Iterable content should use httpx.UploadContent(iterable=..., content_length=None)
    Async iterable content should use httpx.UploadContent(aiterable=..., content_length=None)

  • proxy = httpx.Proxy | None

    The URL and str shortcuts are deprecated.
    Use an explicit proxy = httpx.Proxy(...) argument instead.


Here's an example of how the Client.post method annotations would look...

    def post(
        self,
        url: URL | str,
        *,
        content: httpx.UploadContent | str | bytes | None = None,
        data: httpx.FormData | Dict[str, str] | None = None,
        files: httpx.FormFile | Dict[str, str] | None = None,
        json: typing.Any | None = None,
        params: httpx.QueryParams | Dict[str, str] | None = None,
        headers: httpx.Headers | Dict[str, str] | None = None,
        cookies: httpx.Cookies | Dict[str, str] | CookieJar | None = None,
        auth: httpx.Auth | None = None,
        follow_redirects: bool | None = None,
        timeout: httpx.Timeout | None = None,
        extensions: Dict[str, Any] | None = None
     )

@tomchristie tomchristie added the user-experience Ensuring that users have a good experience using the library label Sep 27, 2024
@tomchristie tomchristie changed the title Requests for new public APIs Stricter typing for request parameters. Sep 27, 2024
@tomchristie
Copy link
Member

I've retitled this to better reflect our design intent.

@trim21
Copy link
Contributor

trim21 commented Sep 27, 2024

This was original posted in issue 3170 #3170 (comment) for typing of data/params

I'm ok if data params do not allow int/bool type, but it at least it should support bytes.

Data/params are encoded as application/x-www-form-urlencoded and it support non-string or string in any encoding. Disallow bytes will make it impossible to send non-utf8 string unless users manually build url or form content.

And even user manually build url, it may still conflict with client arguments if httpx assume all params value is valid utf8 string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user-experience Ensuring that users have a good experience using the library
Projects
None yet
Development

No branches or pull requests

6 participants