Fix circular schema generation, remove None checking hack

[tiangolo]

Change Summary

This is a continuation/refactor of #613 by @wongpat. It should solve #601

The original PR to support circular references (by me) used a "hack" here: https://github.com/samuelcolvin/pydantic/pull/572/files#diff-7d11a8c77c4167b2958bcdb27b18cf2aR754

definitions[model_name] = None

By setting a definition to None it was only marking it to be updated later: https://github.com/samuelcolvin/pydantic/pull/572/files#diff-7d11a8c77c4167b2958bcdb27b18cf2aR228

But it has a bug, it makes it remove other references to the same sub-model when declared in the same parent model, e.g.

class Dep(BaseModel):
    number: int

class Model(BaseModel):
    dep: Dep
    dep_l: List[Dep]

Here, by having more than one field referencing Dep, the definition is created with dep and then it's removed with dep_l (setting it to None).

#613 fixes the original hack, updating it. Instead of marking a definition as declared in the future with None in the definition itself, it creates another key (improved hack) _nested, then it is removed from the definitions and used.

This PR re-implements the same idea from #613 , but being explicit about what's it's doing. It's more verbose (with an extra variable nested_models), but presumably less "hacky", by not re-using definitions for something it is not intended to be used for, as was done in my original implementation and in the fix at #613 .

As the changes are considerable, I ended up writing this new PR. But I don't know how to include the commit/attribution to @wongpat.

I think we can start by discussing the implementation. @samuelcolvin what do you think?

Related issue number

#601

Checklist

• Unit tests for the changes exist
• Tests pass on CI and coverage remains at 100%
• Documentation reflects the changes where applicable
• HISTORY.rst has been updated
  • if this is the first change since a release, please add a new section
  • include the issue number or this pull request number #<number>
  • include your github username @<whomever>

[samuelcolvin]

LGTM from a brief look. Please update history.

@wongpat is this okay with you?

[codecov]

Codecov Report

Merging #621 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #621   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2616   2628   +12     
  Branches      516    516           
=====================================
+ Hits         2616   2628   +12

[wongpat]

@tiangolo Looks good to me a couple lines that could be a bit optimized. Definitely a better solution than using a magical key in an existing object.

I'd say if we need any more metadata than this as we process the schemas then it could be worth using a namedtuple

(Also means that the accompanying PR in FastAPI is unnecessary)

[tiangolo]

Thanks for the feedback @wongpat ! I applied your suggestions and replicated them in all the other places.

[samuelcolvin]

great, thank you.

[tiangolo]

Thanks! 🚀

[wongpat]

Thank you both!

[jorgecarleitao]

Any chance to release this as a patch? Got hit by this, and seems a bug on the latest version (0.29 as of writing).

[samuelcolvin]

released, sorry for the delay.