A third batch of Pydantic validation for rest/client/account.py

[DMRobertson]

More of #13183.

Should be commit-by-commit reviewable.

[reivilibre]

This is basically all good; just a few questions but even if you merged it as-is I'd say it's solid.

[reivilibre]

can you be specific about what advice you're applying / what this is commenting on?
The constants being lowercased perhaps (main thing that strikes me as weird)?

[DMRobertson]

Sure. My first thought was to say medium: StrictStr but I wanted to see if there was a way to say that "medium should be one of these specific strings". The link seems to be the blessed way to do this.

What I was suspicious of is the multiple inheritance from str and Enum. These ThreepidMedium types will get passed through to the rest of the application, where I want them to be treated like any other str. I'm not fully convinced that the rest of the application will be happy with that tbh... but I don't have anything more concrete than a bad feeling about this.

Having said that, looking here it might be possible to write medium: Literal["email", "msisdn"]. If it works, that seems a bit less magic.

[DMRobertson]

Having said that, looking here it might be possible to write medium: Literal["email", "msisdn"]. If it works, that seems a bit less magic.

This seems to work pretty well:

class ThreepidMediumEnumTestCase(stdlib_unittest.TestCase):
    class Model(BaseModel):
        medium: Literal["email", "msisdn"]

    def test_accepts_valid_medium_string(self) -> None:
        """Sanity check that Pydantic behaves sensibly with an enum-of-str

        This is arguably more of a test of a class that inherits from str and Enum
        simultaneously.
        """
        model = self.Model.parse_obj({"medium": "email"})
        self.assertIsInstance(model.medium, str)
        self.assertEqual(model.medium, "email")

    def test_rejects_invalid_medium_value(self) -> None:
        with self.assertRaises(ValidationError) as ctx:
            self.Model.parse_obj({"medium": "interpretive_dance"})
        print(ctx.exception)

    def test_rejects_invalid_medium_type(self) -> None:
        with self.assertRaises(ValidationError) as ctx:
            self.Model.parse_obj({"medium": 123})
        print(ctx.exception)

The error messages generated are

Ran 3 tests in 0.002s

PASSED (successes=3)

Process finished with exit code 0
1 validation error for Model
medium
  unexpected value; permitted: 'email', 'msisdn' (type=value_error.const; given=123; permitted=('email', 'msisdn'))
1 validation error for Model
medium
  unexpected value; permitted: 'email', 'msisdn' (type=value_error.const; given=interpretive_dance; permitted=('email', 'msisdn'))

(the errors are a little technical, but see #13337)

[reivilibre]

I was actually totally fine with the enum, but enum constants usually have uppercase names — that's what I was mostly wondering about (alongside a slightly underspecified comment).

Inheriting from both str and Enum and supplying a proper __str__ is, as far as I remember, the sensible way to do it.

[DMRobertson]

(In passing, I noticed pydantic/pydantic#4505)

[DMRobertson]

Oh I see. That's fair.

I really want to like stdlib enums, but I've been bitten by their sharp corners in the past. We've even seen it ourselves with IntEnum a few times I think? In that light, I think I prefer the Literal approach. But I can also see that a proper Enum can be passed throughout the rest of the application.

Maybe we should dwell on this for a bit. (I'd be interested in other opinions too!)

[reivilibre]

Wonder if we should roll our own StrEnum that also does __repr__ and __str__ in a less surprising way (i.e. by calling it on the string value)?
I suppose it ultimately doesn't matter for the sake of Pydantic

[DMRobertson]

Note that 3.11 will have a StrEnum in the stdlib: https://docs.python.org/3.11/library/enum.html?highlight=strenum#enum.StrEnum

There's a backport here. If we do this I'd prefer to import from stdlib and fallback to the backport.

[reivilibre]

absolutely fine with that idea, as long as we have confidence that the usual pitfall around __str__ is addressed

[DMRobertson]

I think I'm going to opt for Literal[...,...] as a baby step and save StrEnum etc for a later PR.

[reivilibre]

I think I'm going to opt for Literal[...,...] as a baby step and save StrEnum etc for a later PR.

fair enough, thanks for thinking through it!

[reivilibre]

Thanks!

[reivilibre]

tiny tiny question: would calling this ClientSecretStr make more sense so that the class is more readable without looking up the definition of this? I don't think it matters either way but it may be a good convention to have, in which case we would better consider that now

[DMRobertson]

Good idea, I like it. So what's the rule of thumb: if you need an alias for a constr, conint or confloat, call it SomethingSomethingStr, FooBarInt or PotatoFloat rather than BlahBlahType?

Perhaps you'd also like to see this for conlist (JSON arrays) and certain mapping types too?

[reivilibre]

yeah, sounds good to me; I wouldn't do it for pydantic models though, so you wouldn't call e.g. PostBody PostBodyMap.