Support Enum query params #2515

Kludex · 2022-12-26T08:31:48Z

Kludex · 2022-12-26T08:32:02Z

httpx/_urls.py

@@ -554,7 +554,6 @@ def __init__(

        value = args[0] if args else kwargs

-        items: typing.Sequence[typing.Tuple[str, PrimitiveData]]


This was unused.

tomchristie · 2022-12-30T16:15:55Z

Should we support auto-casting enums here, or should we raise a type error if you pass anything other than a boolean/int/float/None?

Option 1.

Explicit type error

import enum
import httpx

class Color(enum.Enum):
    RED = 1
    GREEN = 2
    BLUE = 3

u = httpx.URL("https://example.com", params={"color": Color.RED.value})
print(u)  # https://example.com?color=1

u = httpx.URL("https://example.com", params={"color": Color.RED})
# TypeError: Invalid type for params value. Expected str, bool, int, or float, got <enum 'Color'>: <Color.RED: 1>

Option 2.

Silent type coercion

import enum
import httpx

class Color(enum.Enum):
    RED = 1
    GREEN = 2
    BLUE = 3

u = httpx.URL("https://example.com", params={"color": Color.RED.value})
print(u)  # https://example.com?color=1

u = httpx.URL("https://example.com", params={"color": Color.RED})
print(u)  # https://example.com?color=1

tomchristie · 2022-12-30T16:17:30Z

httpx/_utils.py

+    elif isinstance(value, enum.Enum):
+        return str(value.value)


Does this imply that the PrimitiveData type ought to change to include enum.Enum?

Yeah, I miss that.

Do you think that enum.Enum should not be considered a primitive data, and hence not be in this function?

Honest answer?
My preference would be for us to raise nice tight TypeError cases here.

For a similar example...

import json json.dumps{"value": Color.RED} # TypeError: Object of type Color is not JSON serializable json.dumps{"value": Color.RED.value} # '{"value": 1}'

The thing is... I think the following should raise an error:

class Color(enum.Enum): RED = 1 GREEN = 2 BLUE = 3

But not the following:

class Color(int, enum.Enum): RED = 1 GREEN = 2 BLUE = 3

Because the int there means that each element is an integer, analogous to other types.

Hrm. That's a bit awkward...

class Color(str, enum.Enum): # heeeeyyyyyy I'm just a normal str RED = "red" GREEN = "green" BLUE = "blue" c = Color.RED isinstance(c, str) # True str(c) # 'Color.RED' yeah but you ain't

The upshot is even if you're type-checking the user still needs to correctly pass Color.RED.value, unless we special-case for enum.

Maybe the upshot is we should have strict type checking here and that we should also expand the primitive types to include enum.Enum. (Just because we're friendly like that?)

(Or maybe the upshot is that users should pass the enum value, because we're cold hard calculating types, and we like precision)

The upshot is even if you're type-checking the user still needs to correctly pass Color.RED.value, unless we special-case for enum.

If I understood correctly what you said, this is not the case. Type checkers understand that each member of a (str, Enum) is a str, and they don't raise errors.

I don't think requests has the same behavior as httpx here, does it? Because the Testclient changed, so I assume requests can do the "right" thing on (str, Enum) - I'm on my phone.

tomchristie · 2022-12-30T16:19:25Z

Currently both requests and httpx will perform silent type coercion to str here, but neither special-cases the enum type, so both give an awkward behaviour...

>>> r = requests.get("https://www.example.com", params={"a": Color.RED})
>>> r.request.url
'https://www.example.com/?a=Color.RED'

Edit: Ah okay, the behaviour there depends on if you're using Color(enum.Enum) or Color(str, enum.Enum).

Kludex · 2022-12-30T16:22:54Z

Should we support auto-casting enums here, or should we raise a type error if you pass anything other than a boolean/int/float/None?

Hmm... Maybe this would be better:

    if isinstance(value, enum.Enum):
        return primitive_value_to_str(value.value)

Then it would support all those cases. 🤔

tomchristie · 2022-12-30T18:08:51Z

Okay, I've some reluctance on this, which I'm more clear now on how to phrase.

It's about consistency.

class SiteURLs(str, enum.Enum):
    staging = "https://localhost"
    production = "https://www.example.com"

httpx.URL(SiteURLs.staging)
# Nope, not going to do what you want.
# You meant to pass `SiteURLs.staging.value`.

class ContentType(str, enum.Enum):
    json = "application/json"
    form = "application/x-www-form-urlencoded"

httpx.Headers({"Content-Type": ContentType.json})
# Nope, not going to do what you want.
# You meant to pass `ContentType.json.value`.

Same with, say json.dumps({...}) and other APIs.

It's not obvious to me that we should be special casing Enum in QueryParams, when the less ambiguous API usage is for the user to pass the .value from enum instances.

On the other hand... I can see that the less verbose style feels nicer. It's just that it's less precise and in other cases you might well end up with the __str__ representation.

Because the Testclient changed

I saw something along those lines mentioned in the referenced ticket... but I wasn't clear what it meant, and I couldn't replicated with requests. For example:

>>> r = requests.get("https://www.example.com", params={"a": Color.RED})
>>> r.request.url
'https://www.example.com/?a=Color.RED'  # Ooops you meant `Color.RED.value`. Same as with `httpx`.

Kludex · 2023-01-06T12:25:20Z

@tomchristie did your thoughts regarding this changed or should I close it?

tomchristie · 2023-01-06T14:22:50Z

It's really not obvious what behaviour you should expect when using a StrEnum if you don't access .value. For example...

class CustomEnum(str, enum.Enum):
    A = "a"
    B = "b"

"%s" % CustomEnum.A
'CustomEnum.A'
"".join([CustomEnum.A])
'a'

😬

Anyways we probably just need to suck this weirdness up and deal with it...

def primitive_value_to_str(value: "PrimitiveData") -> str:
    """
    Coerce a primitive data type into a string value.
    Note that we prefer JSON-style 'true'/'false' for boolean values here.
    """
    if value is True:
        return "true"
    elif value is False:
        return "false"
    elif value is None:
        return ""
    elif isinstance(value, (str, int)):
        if isinstance(value, enum.Enum):
            # Typed enums have some pretty inconsistent
            # behaviour around expectations when passed directly.
            #
            # For example, see...
            # https://github.com/encode/httpx/pull/2515#issuecomment-1373705563
            #
            # We could either raise an exception here and force
            # developers to pass `.value` directly, or just coerce it.
            # We're being soft on ya here...
            return str(value.value)
        return str(value)

    # Passing other types here isn't type-correct against our API,
    # but we do currently automatically coerce them.
    #
    # There has been some discussion about removing this in favour
    # stricter typing. See this reverted pull request for more details...
    # https://github.com/encode/httpx/pull/2523
    return str(value)

Perhaps this is overkill on the commenting, but at least makes clear where the nasty edges are.

Kludex · 2023-01-07T16:03:17Z

Why only int and str?

euri10 · 2023-01-07T17:34:02Z

note also 3.11 brings backward breaking changes if used with a mixin int or str and the format funciton, might not faciitate the maintenance and deal with all cases

see https://blog.pecar.me/python-enum for a certainly better explanation

florimondmanca · 2023-02-04T22:17:42Z

httpx/_utils.py

@@ -67,6 +68,8 @@ def primitive_value_to_str(value: "PrimitiveData") -> str:
        return "false"
    elif value is None:
        return ""
+    elif isinstance(value, enum.Enum):
+        return str(value.value)
    return str(value)


I think I'd be happier if HTTPX had an extra branch if isinstance(value, str): return value here, and then raised an explicit TypeError for all remaining cases.

For Enum, we could assume that people want value.value to be used, but why assume and potentially cause unexpected behavior when we can make users provide exactly what they want with a few keystrokes?

Is my thinking, at least

The if isinstance(value, str): return value solves the issue that I want to solve here.

Should I proceed? Aren't we going to have some side effects?

It was kind of what happened on #2523, and we had to revert.

tomchristie · 2024-03-01T15:31:05Z

Closing as stale

Support Enum based on query params

4c5a884

Kludex commented Dec 26, 2022

View reviewed changes

tomchristie reviewed Dec 30, 2022

View reviewed changes

Merge branch 'master' into feat/query-params-enum

b583641

tomchristie mentioned this pull request Dec 30, 2022

Raise TypeError on invalid query params. #2523

Merged

tomchristie added the enhancement New feature or request label Jan 4, 2023

tomchristie added 2 commits January 4, 2023 10:00

Merge branch 'master' into feat/query-params-enum

98e5ad7

Merge branch 'master' into feat/query-params-enum

7a33454

florimondmanca reviewed Feb 4, 2023

View reviewed changes

tomchristie closed this Mar 1, 2024

tomchristie deleted the feat/query-params-enum branch March 1, 2024 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Enum query params #2515

Support Enum query params #2515

Kludex commented Dec 26, 2022

Kludex Dec 26, 2022

tomchristie commented Dec 30, 2022

tomchristie Dec 30, 2022

Kludex Dec 30, 2022

tomchristie Dec 30, 2022 •

edited

tomchristie Dec 30, 2022

Kludex Dec 30, 2022

tomchristie Dec 30, 2022 •

edited

Kludex Dec 30, 2022

Kludex Dec 30, 2022

tomchristie commented Dec 30, 2022 •

edited

Kludex commented Dec 30, 2022

tomchristie commented Dec 30, 2022 •

edited

Kludex commented Jan 6, 2023

tomchristie commented Jan 6, 2023 •

edited

Kludex commented Jan 7, 2023

euri10 commented Jan 7, 2023

florimondmanca Feb 4, 2023

Kludex Feb 13, 2023

Kludex Feb 15, 2023

tomchristie commented Mar 1, 2024

		@@ -554,7 +554,6 @@ def __init__(

		value = args[0] if args else kwargs

		items: typing.Sequence[typing.Tuple[str, PrimitiveData]]

Support Enum query params #2515

Support Enum query params #2515

Conversation

Kludex commented Dec 26, 2022

Choose a reason for hiding this comment

tomchristie commented Dec 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie Dec 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie Dec 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie commented Dec 30, 2022 • edited

Kludex commented Dec 30, 2022

tomchristie commented Dec 30, 2022 • edited

Kludex commented Jan 6, 2023

tomchristie commented Jan 6, 2023 • edited

Kludex commented Jan 7, 2023

euri10 commented Jan 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie commented Mar 1, 2024

tomchristie Dec 30, 2022 •

edited

tomchristie Dec 30, 2022 •

edited

tomchristie commented Dec 30, 2022 •

edited

tomchristie commented Dec 30, 2022 •

edited

tomchristie commented Jan 6, 2023 •

edited