Empty string is a valid JSON-key

[SergeyTsaplin]

Change Summary

An empty string is a valid JSON attribute name, so aliases checking cannot be just like bool(alias) it should be strictly compared with None.

Related issue number

fix #4253

Checklist

• Unit tests for the changes exist
• Tests pass on CI and coverage remains at 100%
• Documentation reflects the changes where applicable
• changes/<pull request or issue id>-<github username>.md file added describing change
  (see changes/README.md for details)
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

[samuelcolvin]

Thanks for the contribution.

While I think you're technically correct. I'm not sure I would support changing this at this late stage for pydantic V1.

I believe this is fixed in pydantic-core (the basis for pydantic V2) - if you're willing, it would be very useful to add unit tests for this to pydantic-core?

The plan is to get v1.10 released with mostly cosmetic changes and some new features that having been hanging around for a long time, then move ahead with V2.

What do others think? @PrettyWood @hramezani

[hramezani]

Thanks @SergeyTsaplin for the patch 👍 and Thanks @samuelcolvin for explaining and asking my opinion.

I'm not sure I would support changing this at this late stage for pydantic V1.

Why would be the problem? seems we don't need a big change to support it?

it would be very useful to add unit tests for this to pydantic-core

👍

[samuelcolvin]

ye, I agree it's a small change. Just there's a lot in fields.py that scares me, its pretty complicated. My worry is subtle side-effects not covered tests, but maybe I'm being a wimp. ✋

Let's wait and see what @PrettyWood thinks.

[SergeyTsaplin]

@samuelcolvin I don't think that the change will break some existed code, it can break only when a user uses Field(alias='') for not "aliased" fields (I have no idea why to do it:)), but of course, the final decision is up to you. For me, it's not a problem to wait for 1.11 if you accidentally decide to merge it to the V1.

About the unit-test to pydantic-core - I can do it, but not soon. It may take me a few days to find a time for it.

[samuelcolvin]

otherwise LGTM

Let's go ahead with this.

[samuelcolvin]

About the unit-test to pydantic-core - I can do it, but not soon. It may take me a few days to find a time for it.

No problem with a wait, this is a marathon, not a sprint.

[SergeyTsaplin]

By the way, I found one more place where I missed the correct condition. Should I squash all the commits to a single one?

[samuelcolvin]

no, just keep committing, we squash on merge.

[PrettyWood]

Looks good! I don't see any risk for this in v1.10

[SergeyTsaplin]

So, I believe it's absolutely ready, so please review.

[hramezani]

Let's wait for @samuelcolvin for final review and probably merge.

[samuelcolvin]

thanks so much.

[samuelcolvin]

Well (while I still think this is okay to be included in v1.10), my paranoia was well placed, tis was breaking starlite, see litestar-org/litestar#402.

Basically with pydantic v1's massive public (or semi-public) API, basically every change can break something. Bring on V2.