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

schema ref_template #1480

Merged
merged 17 commits into from Oct 25, 2020
Merged

schema ref_template #1480

merged 17 commits into from Oct 25, 2020

Conversation

Kilo59
Copy link
Contributor

@Kilo59 Kilo59 commented May 5, 2020

Change Summary

Adds a ref_template option to schema generation that takes a string.Template and substitutes the model_name when generating $refs to other schemas.

my_model.schema(ref_template=string.Template("/foo/${model_name}.json#/bar"))

Edit:

Updated to just use a regular .format() template.
#1479 (comment)

my_model.schema(ref_template="/foo/${model}.json#/bar")

Related issue number

#1479

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)
  • change to regular string.format() template

@Kilo59 Kilo59 changed the title Ref template schema ref_template May 5, 2020
@Kilo59 Kilo59 force-pushed the ref_template branch 2 times, most recently from 41e3eb5 to de3e038 Compare May 5, 2020 22:42
@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #1480 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1480   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files          21       21           
  Lines        4000     4003    +3     
  Branches      798      799    +1     
=======================================
+ Hits         3996     3999    +3     
  Misses          3        3           
  Partials        1        1           
Impacted Files Coverage Δ
pydantic/schema.py 100.00% <100.00%> (ø)
pydantic/typing.py 96.85% <0.00%> (ø)
pydantic/datetime_parse.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a43c2d2...1a3f30e. Read the comment docs.

Comment on lines 149 to 154
if ref_template:
m_schema = {'$ref': ref_template.substitute(model_name=model_name)}
else:
m_schema = {'$ref': ref_prefix + model_name}
Copy link
Contributor Author

@Kilo59 Kilo59 May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably be done more elegantly.

Some additional things I could do.

  1. regular ref_prefixs could also be resolved using the same string.Template method. "#/components/schemas/${model_name}". Could avoid this if else conditional.
  2. better error message if a bad template is passed.
  3. coerce a regular string into a template instead of taking an actual string.Template object.

@@ -69,6 +70,7 @@ def schema(
title: Optional[str] = None,
description: Optional[str] = None,
ref_prefix: Optional[str] = None,
ref_template: Optional[string.Template] = None,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With no extra handling...

  1. If str is passed instead of a string.Template an AttributeError will be raised.
    AttributeError: 'str' object has no attribute 'substitute'
  2. If string.Template is passed that doesn't have the $model_name identifier a KeyError will be raised.
    KeyError: 'foo_bar'

https://docs.python.org/3/library/string.html#template-strings

@Kilo59 Kilo59 marked this pull request as ready for review May 5, 2020 23:00
Comment on lines 1092 to 1169
def test_schema_with_ref_template():
class Foo(BaseModel):
a: str

class Bar(BaseModel):
b: Foo

class Baz(BaseModel):
c: Bar

model_schema = schema([Bar, Baz], ref_template=string.Template('/schemas/${model_name}.json#/'))
assert model_schema == {
'definitions': {
'Baz': {
'title': 'Baz',
'type': 'object',
'properties': {'c': {'$ref': '/schemas/Bar.json#/'}},
'required': ['c'],
},
'Bar': {
'title': 'Bar',
'type': 'object',
'properties': {'b': {'$ref': '/schemas/Foo.json#/'}},
'required': ['b'],
},
'Foo': {
'title': 'Foo',
'type': 'object',
'properties': {'a': {'title': 'A', 'type': 'string'}},
'required': ['a'],
},
}
}


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to add more tests.

@samuelcolvin
Copy link
Member

sorry for the slow reply, as I mentioned on the issue #1479, I think a normal str should be used not Template.

It's not necessary, and Template is very rarely used so will confuse many people.

@Kilo59
Copy link
Contributor Author

Kilo59 commented Jun 27, 2020

sorry for the slow reply, as I mentioned on the issue #1479, I think a normal str should be used not Template.

It's not necessary, and Template is very rarely used so will confuse many people.

Makes sense, and thanks for the reminder. I'll try to update this over the weekend.

@Kilo59 Kilo59 force-pushed the ref_template branch 3 times, most recently from 0fc49f4 to 82631c8 Compare July 4, 2020 18:29
@@ -76,6 +77,7 @@ def schema(
title: Optional[str] = None,
description: Optional[str] = None,
ref_prefix: Optional[str] = None,
ref_template: Optional[str] = None,
Copy link
Contributor Author

@Kilo59 Kilo59 Jul 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samuelcolvin I was just following the existing pattern here.
But should I set the default_template as the default argument for ref_template?

 ref_template: Optional[str] = '#/definitions/{model}',

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better to switch to ref_template: str = '#/definitions/{model}',, the others are only optional because they really can be optional.

The only advantage of making this optional would be that ref_template could take priority over ref_prefix if it's not None.

Copy link
Contributor Author

@Kilo59 Kilo59 Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
I don't know if it would have been better to keep a REF_TEMPLATE constant and use that as the default for all the ref_template defaults, but it looked like doing that prevented the editor help text from showing what the actual value was.

@Kilo59 Kilo59 force-pushed the ref_template branch 5 times, most recently from 50378db to cf5154e Compare July 4, 2020 19:45
@Kilo59
Copy link
Contributor Author

Kilo59 commented Jul 4, 2020

Getting a failure on test_constrained_set_default, same failure that is on masterthat I don't understand.
https://github.com/samuelcolvin/pydantic/runs/837388454

>           raise validation_error
E           pydantic.error_wrappers.ValidationError: 1 validation error for ConSetModelMax
E           v
E             value is not a valid set (type=type_error.set)

pydantic/main.py:346: ValidationError

@samuelcolvin
Copy link
Member

See #1682

@Kilo59
Copy link
Contributor Author

Kilo59 commented Jul 4, 2020

@samuelcolvin I still need to update the schema-customization section docs.
https://pydantic-docs.helpmanual.io/usage/schema/#schema-customization

I'll do that tonight or tomorrow.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need documentation and the tweak discussed here, otherwise looks good.

@@ -76,6 +77,7 @@ def schema(
title: Optional[str] = None,
description: Optional[str] = None,
ref_prefix: Optional[str] = None,
ref_template: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better to switch to ref_template: str = '#/definitions/{model}',, the others are only optional because they really can be optional.

The only advantage of making this optional would be that ref_template could take priority over ref_prefix if it's not None.

use a string.Template instead of a ref_prefix to allow for more varied`$ref`s to be created.
Template string is expected to have $model_name `identifier `
@samuelcolvin
Copy link
Member

This is looking great, but you still need to:

  • fix coverage (there are two cases) which are currently not covered
  • add something to docs.

@Kilo59
Copy link
Contributor Author

Kilo59 commented Oct 10, 2020

This is looking great, but you still need to:

  • fix coverage (there are two cases) which are currently not covered

What are the cases that need to be covered?

@samuelcolvin
Copy link
Member

look at the codecov report above.

@samuelcolvin samuelcolvin merged commit d14731f into pydantic:master Oct 25, 2020
@samuelcolvin
Copy link
Member

thanks so much.

@Kilo59 Kilo59 deleted the ref_template branch June 10, 2022 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants