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.

Sign up for GitHub

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

Jump to bottom

feat(schema): set minItems and maxItems in JSON schema for tuples #2497

Merged
merged 5 commits into from Dec 5, 2021
Merged

feat(schema): set minItems and maxItems in JSON schema for tuples #2497

merged 5 commits into from Dec 5, 2021

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Mar 8, 2021 •
edited

Change Summary

We didn't enforce the length of the tuple in the JSON schema. We could hence set smaller or longer tuples.
Also handles case of empty tuple by not setting items at all to fix #2496

Related issue number

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)

@PrettyWood PrettyWood changed the title feat(schema): enforce length in generated JSON schema for tuple type feat(schema): set minItems and maxItems in JSON schema for tuples Mar 8, 2021
@PrettyWood PrettyWood mentioned this pull request Mar 8, 2021
Closed
3 tasks
@codecov
Copy link

codecov bot commented Mar 8, 2021 •
edited

Codecov Report

Merging #2497 (9439e26) into master (7b7e705) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 9439e26 differs from pull request most recent head da4fad0. Consider uploading reports for the commit da4fad0 to get more accurate results

@@            Coverage Diff            @@
##            master     #2497   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5109      5113    +4     
  Branches      1050      1051    +1     
=========================================
+ Hits          5109      5113    +4
Impacted Files Coverage Δ
pydantic/schema.py 100.00% <100.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 93aed51...da4fad0. Read the comment docs.

ghost
ghost reviewed Mar 8, 2021
View reviewed changes
tests/test_schema.py
),
(Tuple[str], {'type': 'string'}),
(Tuple[str], {'items': {'type': 'string'}, 'minItems': 1, 'maxItems': 1}),
(Tuple[()], {'maxItems': 0, 'minItems': 0}),
Copy link

Choose a reason for hiding this comment

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

Great! 👍

@PrettyWood PrettyWood mentioned this pull request Apr 10, 2021
Closed
9 tasks
samuelcolvin
samuelcolvin reviewed May 9, 2021
View reviewed changes
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.

This seems good but it will conflict with #2375 which I know @maximberg has been working on for a long time.

If someone can explain to my small brain what #2375 does, I think it would be fair to merge that first.

pydantic/schema.py Outdated

f_schema = {'type': 'array', 'minItems': tuple_len, 'maxItems': tuple_len}
if tuple_len == 1:
f_schema['items'] = sub_schema[0]
Copy link
Member

Choose a reason for hiding this comment

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

this looks weird, are you sure it's correct as per JSONSchema.

Surely even if there's only one item, items should be an array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both are valid but I agree it's easier to just do like this!

@samuelcolvin
Copy link
Member

please update

@PrettyWood
Copy link
Member Author

I'll wait for #2375 before updating this PR. If I should update it before please tell me

@vincentsarago vincentsarago mentioned this pull request Aug 24, 2021
Closed
@PrettyWood
Copy link
Member Author

please review 🙏

@samuelcolvin samuelcolvin merged commit a35cde9 into pydantic:master Dec 5, 2021
@samuelcolvin
Copy link
Member

Thank you.

@PrettyWood PrettyWood deleted the tuple-schema branch December 5, 2021 16:05
PrettyWood added a commit to PrettyWood/pydantic that referenced this pull request Dec 6, 2021
@PrettyWood
feat(schema): set minItems and maxItems in JSON schema for tuples (…
0f77032 
…pydantic#2497)

* feat(schema): enforce length in generated JSON schema for tuple type

* docs: add change file

* docs: update documentation

* simplify a bit

* always set array
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

Assignees

@samuelcolvin samuelcolvin

@PrettyWood PrettyWood

Labels
ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Empty tuple type does not generate valid JSON schema
2 participants
@PrettyWood @samuelcolvin